Skip to content
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

Fix Rule CPE Name inheritance #6943

Merged

Conversation

yuumasato
Copy link
Member

Description:

  • Conversion of platforms to cpe_name was not done when a rule inherited a platform from a parent group.

Rationale:

This ensures that rules inheriting platforms from groups have them
converted to CPE names.
@openscap-ci
Copy link
Collaborator

openscap-ci commented May 3, 2021

Changes identified:
Others:
 Changes in Python files.

Show details

Others:
 Python abstract syntax tree change found in ssg/build_yaml.py.
 Python abstract syntax tree change found in tests/test_machine_only_rules.py.

Recommended tests to execute:
 (cd build && cmake ../ && ctest -j4)

@JAORMX
Copy link
Contributor

JAORMX commented May 4, 2021

/retest

@JAORMX
Copy link
Contributor

JAORMX commented May 4, 2021

I'd really like to see e2e-aws-ocp4-cis-node passing, we do use an overridden platform in that scenario (ocp4-master), so it's relevant to this change IMO.

Copy link
Contributor

@mildas mildas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rerun of the test that found #6938 passed on this change. Looks good to me.

@jan-cerny
Copy link
Collaborator

It would be amazing to have a test for this.

@yuumasato
Copy link
Member Author

@jan-cerny @vojtapolasek Do you have suggestions how to test this "implicit inheritance"?

(A) I would write a test that checks if a specific rule whose parent group.yml has CPE machine gets built with a platform cpe:/a:machine. But the rule applicability could change and the test start to fail.

(B) Ideally, we need a test content tree to build and include into a test product so that we a have stable test from content tree to DS, but that seem too much work for this PR.

I think it would be good to have this test content tree, and actually a lot of tests could be built on top of that.
But I'd go for the approach (A). Given that the implicit inheritance may go away after #6772 is unblocked and the PCI-DSS benchmark generation is fixed with "squashed" platforms (with use of Applicability Language)
Thoughts?

@vojtapolasek
Copy link
Collaborator

I think the proposal A is good for this case. This state should be only temporary anyway. How about checking the datastream like this:
if group has some platform specified:
for all rules within this group:
check if the rule contains the same platform

I am not sure what you mean by "But the rule applicability could change and the test start to fail.".

@pep8speaks
Copy link

pep8speaks commented May 4, 2021

Hello @yuumasato! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-05-05 10:26:29 UTC

@yuumasato yuumasato added this to the 0.1.56 milestone May 5, 2021
@yuumasato
Copy link
Member Author

/retest

The machine_only_rules.py test already filtered for rules
with machine platform in the yml files but never checked the
rules in the built DS.
Iterates over all available DS and print all groups and rules missing
machine platform.

Also better formats the error messages.
@yuumasato
Copy link
Member Author

@vojtapolasek Enhanced the test per feedback. Now it goes through all available data streams and prints every error it finds.

@vojtapolasek
Copy link
Collaborator

/retest

@vojtapolasek vojtapolasek merged commit 821b133 into ComplianceAsCode:master May 5, 2021
@yuumasato yuumasato deleted the fix_cpe_name_inheritance branch May 6, 2021 06:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RHEL7 - rules that are not applicable to containers are evaluated in container scan
7 participants