-
Notifications
You must be signed in to change notification settings - Fork 685
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce templated platforms (CPEs) #9906
Introduce templated platforms (CPEs) #9906
Conversation
42715ff
to
0923c0a
Compare
@evgenyz Thanks for opening this PR. I haven't reviewed the details yet, but I think that the PR needs a better description explaining what actually changes and why. Also, the rationale would be better to be reword and make it more generic and provide more context. Also, we should document this new feature in the documentation. |
is this somehow related to this previous attempt: #7635 (comment) |
assert cpe_item.check_id == "installed_env_has_test_package" | ||
|
||
|
||
assert cpe_item.name == "cpe:/a:ntp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add an assert also for the CPE ID, not only CPE name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
shared/applicability/package.yml
Outdated
title: "Package {arg} is installed" | ||
check_id: cond_package_{arg} | ||
template: | ||
name: cond_package |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder why is the template called cond_package
. I think that it could have a better name, eg. platform_package
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it is a "conditional" test. We use this term everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does a "conditional" test mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It means the same thing as in CPE entry fields bash_conditional
and ansible_conditional
:
bash_conditional: '[ ! -f /.dockerenv ] && [ ! -f /run/.containerenv ]' |
Except template can potentially contain any of them: OVAL, Bash, Ansible, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Jan, and BTW the machine.yml
also shows this: bash_conditional
is a variable that stores a Bash conditional statement, so the naming is accurate - you can store many things in Bash variables, not only conditional statements. Whereas an OVAL check ID is always some sort of test or condition, so in the machine yml
, it is stored under check_id
, and the name installed_env_is_a_machine
describes what the check is about. Therefore, why not to pick a check ID {arg}_package_installed
or something similar?
Also I would factor in the installed
either in the template name or in template arguments, as it is likely that we will get platform checks checking on absence of packages in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The final form for applicability entity in cpe_versioning
implementation is:
name: 'cpe:/a:{arg}:{ver_cpe}'
title: 'Package {arg} {ver_str} is installed'
template:
name: cond_package
args:
chronytest:
title: "Chrony Server Test"
pkgname: chrony
pkgname@debian10: chrony
ntptest:
title: "NTP Server Test"
pkgname: ntp
or
name: cpe:/a:container
title: Container
conditional:
bash: '[ ! -f /.dockerenv ] && [ ! -f /run/.containerenv ]'
oval_id: installed_env_is_a_container
In the first case everything is generated by the template (including the check_id, because template knows how to id itself). And in the second case everything is defined without a template (using static OVAL check file, hence the renamed oval_id).
The combination of templated and direct definitions was also possible, with direct definitions (inside conditional
) taking over template content on individual lang basis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But in the documentation that is part of this PR it looks different, there is no conditional
key and the data in this PR are also different from this, so it isn't the final form?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This requires mass edit of like 100 files and I'm not sure if it even makes any sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This requires mass edit of like 100 files and I'm not sure if it even makes any sense.
I don't want to rename 100 files. Instead, I try to point out that the comment above is misleading because the actual contents of the file(s) is different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, platform_package
it is.
ssg/build_cpe.py
Outdated
parameters = cpe.args[self.arg] | ||
parameters.update(self.as_dict()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here people can be confused because we mix parameters, args, arg and self.as_dict so it isn't clear what contains what. Are the "parameters" here the template arguments? In our example,
pkgname: ntp
title: NTP daemon and utilities
or does it contain something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here they are meta-template arguments, yes. The name follows the name of the function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can improve the method by returning None if there are is no self.arg
.
However, the method does a lot of things, not only "passing parameters". In fact, the mechanics of creating a new item with "parameters" is understandable, but the rest of those things not so much.
As Jan mentioned, the line parameters = cpe.args[self.arg]
is the most striking example of confusing naming.
Then, I see that a new "associated CPE item" is constructed using parameters and interestingly, those parameters are used twice. That has to do something with that two-step templating, but there is no hint of what's going on.
Finally, the newly created item is stashed somewhere, and we take part of it. Why, for whom?
I believe that those questions can be answered by the code if it is extracted into short private methods that indicate what's going on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @vojtapolasek might be able to provide some information on why it was implemented this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change the code so it explains itself - I believe that renaming and extraction can do the trick. Code or PR comments are an emergency measure how to handle hopeless cases s.a. creation of difficult regular expressions, and this is not the right place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code has been changed.
ssg/build_cpe.py
Outdated
new_associated_cpe_item_as_dict["id_"] = self.as_id() | ||
if cpe.is_templated(): | ||
new_associated_cpe_item_as_dict["template"]["vars"] = parameters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have checked the build
directory and I found that the compiled CPE, eg. build/rhel9/cpe_items/package_ntp.yml
looks ugly:
name: cpe:/a:ntp
check_id: cond_package_ntp
bash_conditional: ''
ansible_conditional: ''
is_product_cpe: false
args:
ntp:
pkgname: ntp
title: NTP daemon and utilities
id: package_ntp
name: package
arg: ntp
title: Package ntp is installed
definition_location: /home/jcerny/work/git/scap-security-guide/shared/applicability/package.yml
template:
name: cond_package
vars:
pkgname: ntp
title: NTP daemon and utilities
id: package_ntp
name: package
arg: ntp
documentation_complete: true
This is a resolved CPE Item so the args
key probably doesn't make sense there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem here is that there is no distinction between resolved and unresolved CPEe, they all are stored in the same product_cpes.cpes_as_id
dictionary.
Anyhow, this is the best place to fill template variables, IMHO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is again a design smell and this byproduct is, unlike other byproducts, confusing. First of all, some arguments repeated twice (or is it a coincidence?), and second of all, they are already partially substituted, as Jan has noticed. Finally, how come that e.g. bash_conditional
is there and it is empty? Is the template incomplete?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
, how come that e.g. bash_conditional is there and it is empty? Is the template incomplete?
Because the snippet that I pasted is from build/rhel9/cpe_items/package_ntp.yml which is an interim file that is created before the templates are resolved. The platforms are stored in build/<product>/platforms
instead. But there the bash conditional is empty as well because the template doesn't contain any bash check. And that leads me to the question to @evgenyz: Why the template submitted in this PR doesn't contain any Bash check? Can we add it? Will that need some additional Python code to be implemented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is empty because shared/applicability/package.yml
does not have bash_conditional
at this moment. Yes, this is another set of changes that is pending.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aha
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason that bash_conditional
is empty then? Is the template incomplete, or do they get populated at some point later? If it is the case of the incomplete template, then please add those bits, as reviewing an incomplete template doesn't feel reasonable, as changes to this part will affect the integration of the incoming part as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added missing *_conditional
s to the package
CPE.
The args
element is now empty in the resolved entity.
It is a different approach to the problem to some degree. |
@dodys This is a part of series of PRs developing the CPE applicability language that should give us more flexibility in defining platform expressions and using it to specify applicability of our rules more flexibly. However, it will all be resolved at build time. We don't plan to make the platforms parametrized using XCCDF values at scan time. |
got it, thanks! |
0923c0a
to
0be2177
Compare
Add the 'package' CPE and 'cond_package' template for it. Add platforms support to the templates.Builder.
0be2177
to
aff595e
Compare
/retest |
@@ -13,6 +13,7 @@ | |||
ssg_root = os.path.dirname(os.path.dirname(os.path.dirname(os.path.dirname(__file__)))) | |||
DATADIR = os.path.abspath(os.path.join(os.path.dirname(__file__), "data")) | |||
templates_dir = os.path.join(DATADIR, "templates") | |||
platforms_dir = os.path.join(DATADIR) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't have to join if you have only a single item.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find some of the new code very difficult to understand due to leakage of inappropriate levels of abstractions both in naming of things, and in the code. At the same time, I find the code to be easily fixable by extracting it to the same objects, or by delegating the functionality to other objects.
ssg/templates.py
Outdated
for symbol in platform.test.get_symbols(): | ||
platform.test.pass_parameters(self.product_cpes) | ||
cpe = self.product_cpes.get_cpe(symbol.as_id()) | ||
if cpe.is_templated(): | ||
for lang in self.get_resolved_langs_to_generate(cpe): | ||
self.build_lang_for_templatable(cpe, lang) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of this code shouldn't be in the template class, because it interacts with low-level details of Platform.test
and CPEItem
. And you call platform.test.pass_parameters(self.product_cpes)
the same way for each symbol, is it intended?
You probably want to "pass parameters" to the platform as a whole, and then get list of templated CPEs from it without having to know which buttons to push exactly in order to get those.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. The template builder does not know a thing about that from now on.
Move cpe_id_is_parametrized and is_cpe_name methods to the appropriate classes in order to support encapsulation of Symbol and Function.
Move the function to the ProductCPEs class, where it would collect resolved CPEs into the internal dictionary. Extract parts of the function that create a resolved CPE into the CPEItem class in form of a clone factory. The function is now a part of Platform initialization routine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's improving
I have tried to use the whole thing, so I decided to define my own platform and use it in a rule. But I have discovered a suspicious problem.
I have done the following changes in my repository on the top of this PR:
[jcerny@thinkpad build{pr/9906}]$ git diff
diff --git a/linux_os/guide/system/accounts/accounts-session/accounts_tmout/rule.yml b/linux_os/guide/system/accounts/accounts-session/accounts_tmout/rule.yml
index 1a9fe55638..06aa0fa4fc 100644
--- a/linux_os/guide/system/accounts/accounts-session/accounts_tmout/rule.yml
+++ b/linux_os/guide/system/accounts/accounts-session/accounts_tmout/rule.yml
@@ -83,7 +83,7 @@ ocil: |-
export TMOUT
{{% endif %}}
-platform: machine
+platform: package[carrot]
fixtext: |-
Configure {{{ full_name }}} to terminate user sessions after {{{ xccdf_value("var_accounts_tmout") }}} seconds of inactivity.
diff --git a/shared/applicability/package.yml b/shared/applicability/package.yml
index 501e0b855e..f06a4bfeea 100644
--- a/shared/applicability/package.yml
+++ b/shared/applicability/package.yml
@@ -9,3 +9,6 @@ args:
ntp:
pkgname: ntp
title: NTP daemon and utilities
+ carrot:
+ pkgname: carrot
+ title: healthy package
[jcerny@thinkpad build{pr/9906}]$
The I build the RHEL 9 content.
But in the generated ssg-rhel9-ds.xml
data stream, I have found 2 different OVAL definitions with ID "oval:ssg-cond_package_carrot:def:1"
. I would expect 1 there. The first is in the ssg-rhel9-oval.xml
component, the second is in the ssg-rhel9-cpe-oval.xml
component. I think that there should exist only the component in the ssg-rhel9-cpe-oval.xml
.
The second entry defines a CPE for GDM. | ||
By setting the `platform` to `gdm`, the rule will have its applicability | ||
restricted to only environments which have `gdm` package installed. | ||
The second entry defines a CPE for NTP. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this line and instead describe the second file.
For example:
The second file defines a parametrized CPE. This allows us to define multiple similar CPEs that differ in their argument. In our example, we define the package
CPE. Within the args
key we define a set of its possible arguments and their values. In our example, the possible values are ntp
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
shared/applicability/package.yml
Outdated
title: "Package {arg} is installed" | ||
check_id: cond_package_{arg} | ||
template: | ||
name: cond_package |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But in the documentation that is part of this PR it looks different, there is no conditional
key and the data in this PR are also different from this, so it isn't the final form?
ssg/build_cpe.py
Outdated
for symbol in platform.test.get_symbols(): | ||
if symbol.arg: | ||
cpe = self.get_cpe(symbol.cpe_name) | ||
new_cpe = cpe.create_resolved_cpe_item_for_fact_ref(symbol) | ||
self.add_cpe_item(new_cpe) | ||
symbol.cpe_name = new_cpe.name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's much better than it used to be.
But I still think that we're bad at naming things. We should at least be consistent. Notice that the expression member is called symbol
here but suddenly it becomes a fact_ref
. You have create_resolved_cpe_item_for_fact_ref
but you pass a symbol to it and inside it's a fact_ref
. It won't be clear to people that these 2 things are the same thing.
Then, we have a class CPEALFactRef
which inhertis from Symbol
. So do we pass an instance of CPEALFactRef
or instance of Symbol
to create_resolved_cpe_item_for_fact_ref
? The name of the method suggests that we pass CPEALFactRef
to it but is it really the case? Perhaps making the names consistent would make it more clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not deal with naked Symbol
s anywhere outside boolean.py. So any function that receives or returns a Symbol
(or Function
) in fact operates with CPEALFactRef
or CPEALLogicalTest
.
I've renamed remaining variables.
@staticmethod | ||
def cpe_id_is_parametrized(cpe_id): | ||
return Symbol.is_parametrized(cpe_id) | ||
|
||
@staticmethod | ||
def get_base_name_of_parametrized_cpe_id(cpe_id): | ||
return Symbol.get_base_of_parametrized_name(cpe_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing!
The |
915ef27
to
11c1a07
Compare
Code Climate has analyzed commit 11c1a07 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 81.5% (50% is the threshold). This pull request will bring the total coverage in the repository to 48.8% (0.1% change). View more on Code Climate. |
/retest |
Thanks, since this issue is also present in master, I have reported it as an upstream issue in #9920 and I consider it out of scope of this PR |
/packit retest-failed |
Description:
Add the 'package' CPE and 'cond_package' template for it.
Add platforms support to the templates.Builder.
Some refactoring (see individual commits).
Rationale:
This change set would allow to use
package[ntp]
in platform expressions.Example templated platform CPE
/shared/applicability/package.yml
:Review Hints:
The feature is not yet used in the content. In order to test it in real build process one would have to replace
ntp
platfrom with
package[ntp]
in an active rule.