-
Notifications
You must be signed in to change notification settings - Fork 671
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
CPE AL: Introduce version specifiers support #9945
CPE AL: Introduce version specifiers support #9945
Conversation
@@ -3,9 +3,18 @@ | |||
version="1"> | |||
{{{ oval_metadata("The " + pkg_system|upper + " package " + PKGNAME + " should be installed.", affected_platforms=["multi_platform_all"]) }}} | |||
<criteria> | |||
<criterion comment="Package {{{ PKGNAME }}} is installed" | |||
{{% for spec in VER_SPECS %}} |
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 a question: Will we do similar changes also in the Bash and Ansible code for the package template platform? My understanding is that Bash is too difficult so we put it out of scope of this year, but what will we do about Ansible?
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.
Ah, yeah. There was some code for Ansible. I'll adopt it for package
CPE. Added to TODO.
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 ansible.template
for package
platform.
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.
Removed ansible.template
and all new code that supports it.
ssg/boolean_expression.py
Outdated
res['ver_title'] = ' and '.join(['{0} {1}'.format( | ||
ssg.utils.comparison_to_oval(op), ver) | ||
for op, ver in self.ver_specs]) | ||
|
||
res['ver_cpe'] = ':'.join(['{0}:{1}'.format( | ||
ssg.utils.escape_comparison(op), ver) | ||
for op, ver in self.ver_specs]) |
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.
Where are the variables ver_title
and ver_cpe
used? Also, is it a duplicate information from the contents of ver_specs
dictionary?
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.
Let's take foo>1.0,<2.0
:
The ver_specs
is a list of individual specs: op: >, ver: 1.0, etc...
, op: <, ver: 2.0, etc...
, whereas ver_cpe
in that case is a composition cpe:/a:foo:lt:1.0:gt:2.0
(yes, CPE name does not make much sense in this case but we are going to get rid of dictionaries anyway). The ver_title
would be greater than 1.0 and less than 2.0
.
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.
OK, and where are they used?
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 ver_cpe
was designed to be used to update name
for CPEs. I just missed that part of code because scanner does not care about uniqueness of CPE ID in dictionaries. The title was designed for things like <criteria>
titles. I'll add it to the 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.
Done.
@@ -663,7 +663,7 @@ Generates the :code:`<affected>` tag for OVAL check using correct product platfo | |||
</linux:rpminfo_object> | |||
{{% if evr %}} | |||
<linux:rpminfo_state id="ste_{{{ test_id }}}" version="1"> | |||
<linux:evr datatype="evr_string" operation="greater than or equal">{{{ evr }}}</linux:evr> | |||
<linux:evr datatype="evr_string" operation="{{{ evr_op }}}">{{{ evr }}}</linux:evr> |
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 will the equality operation work with RPM packages? For example, I want to check firefox
. I have firefox-107.0-4.fc37.x86_64
installed. I have platform: package[firefox]==107.0-4
in rule.yml. The result is notapplicable. I can't define platform: package[firefox]==107.0-4.fc37
because that causes a build error. The collected items in ARF contain <lin-sys:evr datatype="evr_string">0:107.0-4.fc37</lin-sys:evr>
. It seems to me that I can't have ==
operator for RPM packages in platform definitions. How does that work?
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 also the limitation of distutils as it does not support extra letters in versions. We can work around for now with package[firefox]>=107.0-4,<107.0-5
if we absolutely have to.
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 general we might need to think about it and amend OVAL/OpenSCAP in some way because package[firefox]==107.0-4.fc37
makes platform distro-version specific, which we kinda trying to avoid in the content.
Add functions that would handle versions in platform definition. Sample implementation in in package platform's OVAL template. Version notation currently does not support epoch. Supported format: "maj.min.micro-release".
Directly import utils module from ssg.
Extract logic for parsing requirements and versions into separate requirement_specs.py module. Migrate formatting logic to utils.py. New VersionSpecifier and VersionSpecifierSet classes are responsible for representation of requirements and versions specs independently of parsing module. The boolean_expression.py now only deals with expressions logic.
CPEItem now uses its id_ as check_id for OVAL templates. The check_id property is no longer required, but it will override id_ if provided. The title property is inherited from XCCDFEntity.KEYS.
086c8a2
to
32338af
Compare
you are also affected by ComplianceAsCode/content-test-filtering#38 |
I have checked it again and I think you should move the Ansible code and the code that supports the templated Bash and Ansible conditional to a separate PR. Keep here only the OVAL checks so that we will have narrow scope in this PR and we can focus only on the actual "versioning feature". |
Will do. |
035b6d1
to
ab1ef0a
Compare
/retest |
I'm sorry for not being clear, when I talked about "the code that supports the templated Bash and Ansible conditional" I meant also the commit CPE/AL Add support for templated Ansible and Bash conditionals . Because I understood that if we don't have the templated Ansible and Bash conditional in this rule we don't need this code urgently and the support for it can be complex and also it has an alternative solution proposed by Vojta. What do you think? Is that a possible way? |
That's exactly how I got it. I'll remove that commit. I was fixing problems with versions in our CI yesterday and haven't had time to get rid of it. |
…ecs.py At around v40.8 the `post` attribute of `Version` wasn't available.
ab1ef0a
to
25d442a
Compare
@jan-cerny And it's gone. |
@evgenyz I have a problem when I try to use the versions in some rules I have created a new package platform for coconut package and then I made rule selinux_state applicable on coconut package and the rule accounts_tmout also on coconut but only on a version greater than 4.5. Here is the full diff of my changes:
Now I build the |
Use VER_SPECS_TITLE in OVAL template. Use ver_specs_cpe in applicability/package.yml Wrap confusing get_symbols in CPEItem. Cosmetic fixes.
25d442a
to
88e4ae4
Compare
Removed a bit too much, huh. Should be fixed now. |
/retest |
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 that now the code climate issues remain to be solved.
} | ||
|
||
if self.requirement.has_version_specs(): | ||
for ver_spec in sorted(self.requirement.ver_specs): |
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 it should be self.ver_specs
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.
Nah, Symbol.ver_specs
doesn't make sense.
test_ref="platform_test_{{{ _RULE_ID }}}_installed" /> | ||
{{% for spec in VER_SPECS %}} | ||
<criterion comment="Platform package {{{ PKGNAME }}} of version {{{ spec.evr_op }}} {{{ spec.ver }}} is installed" | ||
test_ref="inventory_test_{{{ _RULE_ID }}}_{{{ spec.id }}}_installed" /> |
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.
Why do we add the spec.id
to the test ID? It's already a part of the rule_id. That leads to a weird ID like "oval:ssg-inventory_test_package_coconut_gt_or_eq_4_5_gt_or_eq_4_5_installed:tst:1"
.
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.
In general it's for package[coconut]>=4.5,<5
.
Code Climate is happy now. |
@jan-cerny |
@evgenyz I hope we're OK with that, let's see how the documentation will look like. |
Code Climate has analyzed commit 82a174f and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 94.3% (50% is the threshold). This pull request will bring the total coverage in the repository to 49.8% (1.0% change). View more on Code Climate. |
/retest |
…t them The 'versioned' property of a CPE entity indicates if it can process version specifier in a sensible way. It is False by default.
/retest |
1 similar comment
/retest |
@evgenyz: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Description:
Add functions that would handle versions in platform definition. Sample implementation in in package platform's OVAL template.
Version notation currently does not support epoch. Supported format:
maj.min.micro[.nano...]-release
.Rationale:
os_linux[rhel]>=8.2
,package[grub]>=2.0
).Review Hints:
package[ntp]<=1.99.5-1,>1.1
):