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

Fail build if profiles or controls contain invalid rule selections #11135

Merged
merged 20 commits into from
Sep 26, 2023

Conversation

jan-cerny
Copy link
Collaborator

Description:

This PR adds checks which will cause to raise an exception and terminate the build:

  • if a profile selects a rule and this rule isn't available in the currently build product
  • if a control file selects a rule that doesn't exist at all

This change revealed some invalid selections in our profiles and controls, these bugs are fixed in this PR as well.

For more details, please read commit messages of all commits.

Rationale:

This will ensure integrity of profiles and avoid invalid rule IDs.

Fixes: #8870

Review Hints:

  1. Add an invalid rule ID to selections in your favorite profile, build the product and watch if it terminates with an error message.
  2. Add an invalid rule ID to rules section in a control file, build the product and watch if it terminates with an error message.

@openshift-ci
Copy link

openshift-ci bot commented Sep 22, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Used by openshift-ci bot. label Sep 22, 2023
@github-actions
Copy link

Start a new ephemeral environment with changes proposed in this pull request:

Fedora Environment
Open in Gitpod

Oracle Linux 8 Environment
Open in Gitpod

The correct rule ID is `install_PAE_kernel_on_x86-32`.
The rule service_zebra_disabled doesn't apply to RHEL 9 because of
its `prodtype`, that means this rule shouldn't be a part of the
RHEL 9 profile.
The rule service_rexec_disabled doesn't apply to RHEL 9 because of
its `prodtype`, that means this rule shouldn't be a part of the
RHEL 9 profile.
The rule service_zebra_disabled doesn't apply to RHEL 9 because of
its `prodtype`, that means this rule shouldn't be a part of the
RHEL 9 profile.
`locking_out_password_attempts` is an invalid rule ID, this
rule doesn't exist, it's a group instead.
The rule `package_iptables_installed` is a part of multiple
profiles in products but doesn't have prodtypes in these products.
These rules don't have the sle15 prodtype and therefore
they don't exist in the sle15 benchmark.
This rule isn't a part of Fedora according to the rule `prodtype`.
This rule is a part of the RHEL 8 OSPP profile, but the rule
doesn't have ol8 in prodtype. I think we can enable this rule on
OL 8 because the rule exists also in RHEL 8.
This rule is selected by OL 9 OSPP profile but it doesn't have
the prodtype.
This rule is selected by the E8 profile for OL 9 but it doesn't
have OL 9 in the prodtype
firefox_preferences-dod_root_certificate is a group
the correct rule ID is firefox_preferences-dod_root_certificate_installed
@jan-cerny jan-cerny added the Infrastructure Our content build system label Sep 25, 2023
@jan-cerny jan-cerny added this to the 0.1.70 milestone Sep 25, 2023
The rules changed by this commit are used by the OpenEmbedded
standard profile but don't have the product ID in the prodtype
key.
The rule accounts_users_own_home_directories is a part of the
Ubuntu 22.04 Standard profile, but the rule doesn't have this
product in its prodtype.
First, rename the function `resolve_selections_with_rules` to
`apply_filter` to better express the purpose of this function.

Second, move the call of `apply_filter` to a correct place.
Previously, the function was checking if a rule ID is a rule that
exists and is applicable to the currently built product. It removed
the selections that selected the rules that don't exist. However,
this operation shadowed a check in `resolve` that served to raise
an exception if a rule isn't available. This exception
could never be raised because at the moment of the check guarding
the exception the selections already contained only existing rules.
This flaw is fixed by moving the `apply_filter` call after
the exception and removing the duplicate check.
We will raise an exception and terminate the build if a control
selects a rule that doesn't exist.

To do that, we need to get a list of all existing rules in the project.
Unfortunately, we can't reuse a list of the rules available in the
currently built product because control files can contain all rules from
all benchmarks from all products. Control files are product agnostic and
benchmark agnostic.
Add a simple unit test that verifies if the `Control.load()`
method raises an exception if a control selects an invalid rule
ID.
@codeclimate
Copy link

codeclimate bot commented Sep 25, 2023

Code Climate has analyzed commit 70cbe76 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 56.9%.

View more on Code Climate.

@jan-cerny jan-cerny marked this pull request as ready for review September 25, 2023 10:00
@jan-cerny jan-cerny requested review from a team as code owners September 25, 2023 10:00
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Used by openshift-ci bot. label Sep 25, 2023
@jan-cerny
Copy link
Collaborator Author

I have made this PR ready for review

@Mab879 Mab879 self-assigned this Sep 25, 2023
Copy link
Member

@Mab879 Mab879 left a comment

Choose a reason for hiding this comment

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

Thanks for this great improvement @jan-cerny !

@ComplianceAsCode/suse-maintainers and @ComplianceAsCode/oracle-maintainers please take a look as well.

@freddieRv
Copy link
Contributor

@jan-cerny Great feature! Thank you for fixing the missing OL prodtypes and sorry we missed them!

Copy link
Contributor

@teacup-on-rockingchair teacup-on-rockingchair left a comment

Choose a reason for hiding this comment

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

Nice stuff indeed 🙇

@Mab879 Mab879 merged commit e6e408d into ComplianceAsCode:master Sep 26, 2023
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Infrastructure Our content build system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Profiles and Controls with invalid rule IDs build successfuly when they shouldn't
4 participants