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

Add utils/controlrefcheck.py #10096

Merged
merged 10 commits into from
Jan 26, 2023

Conversation

Mab879
Copy link
Member

@Mab879 Mab879 commented Jan 20, 2023

Description:

Add a script to test if the control id equals the reference of a rule.

Rationale:

To ensure that the control file and the rules are in sync.

Review Hints:

$ ./utils/controlrefcheck.py rhel8 cis_rhel8 cis

Example output:

Skipping reload_dconf_db as it does not match a CIS id.
Skipping enable_authselect as it does not match a CIS id.
selinux_state cis@rhel8 1.7.1.4 does not match the control id 1.6.1.5
configure_crypto_policy cis@rhel8 1.10,1.11 does not match the control id 1.1
package_xinetd_removed cis@rhel8 2.1.1 does not match the control id 2.2.1
rsyslog_files_permissions cis@rhel8 4.2.3 does not match the control id 4.2.1.4
sudo_require_reauthentication cis@rhel8 5.3.6 does not match the control id 5.3.5
accounts_password_pam_retry cis@rhel8 5.4.1 does not match the control id 5.5.1
accounts_passwords_pam_faillock_unlock_time cis@rhel8 5.4.2 does not match the control id 5.5.2
set_password_hashing_algorithm_systemauth cis@rhel8 5.4.4 does not match the control id 5.5.4
set_password_hashing_algorithm_passwordauth cis@rhel8 5.4.4 does not match the control id 5.5.4

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Used by openshift-ci bot. label Jan 20, 2023
@openshift-ci
Copy link

openshift-ci bot commented Jan 20, 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

@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

@jhrozek
Copy link
Collaborator

jhrozek commented Jan 20, 2023

This is great! I already used the script to find bugs in OCP4 PCI-DSS (PR #10098).
One suggestion: In get_rule_object, could we handle None returned from all_rules.get() to handle the situation I ran into?

@jhrozek
Copy link
Collaborator

jhrozek commented Jan 20, 2023

And a question: This seems like something that should be ran in CI after we fix the initial issues. Is that what the hunk that modifies ctest does?

@Mab879
Copy link
Member Author

Mab879 commented Jan 20, 2023

This is great! I already used the script to find bugs in OCP4 PCI-DSS (PR #10098). One suggestion: In get_rule_object, could we handle None returned from all_rules.get() to handle the situation I ran into?

Yes, I can work on that.

And a question: This seems like something that should be ran in CI after we fix the initial issues. Is that what the hunk that modifies ctest does?

That is correct.

@Mab879 Mab879 marked this pull request as ready for review January 20, 2023 19:52
@Mab879 Mab879 requested a review from a team as a code owner January 20, 2023 19:52
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Used by openshift-ci bot. label Jan 20, 2023
@Mab879 Mab879 added enhancement General enhancements to the project. Test Suite Update in Test Suite. labels Jan 20, 2023
Copy link
Collaborator

@jhrozek jhrozek left a comment

Choose a reason for hiding this comment

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

LGTM
I tried running the tool against the RHEL profiles that you fixed and verified that they are indeed fixed. Running against OCP4 profiles that have issues does produce meaningful messages.

ship it :-)

@jan-cerny jan-cerny self-assigned this Jan 24, 2023
@jan-cerny jan-cerny added this to the 0.1.67 milestone Jan 24, 2023
utils/controlrefcheck.py Show resolved Hide resolved
utils/controlrefcheck.py Outdated Show resolved Hide resolved
rule_object.references[reference].split(',')


def main():
Copy link
Collaborator

Choose a reason for hiding this comment

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

the function is too complex

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we want to lower the threshold in Code Climate, as this function passed the complexity before I changed it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

probably not because in other situations I find Codeclimate strict enough

utils/controlrefcheck.py Outdated Show resolved Hide resolved
utils/controlrefcheck.py Show resolved Hide resolved
@jan-cerny
Copy link
Collaborator

/packit test

Copy link
Collaborator

@jan-cerny jan-cerny left a comment

Choose a reason for hiding this comment

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

I have tried to run it and I also tried to break reference consistency in some rule.ymls and the script has shown the expected output. I have also executed the CTest locally and I have seen that it runs in the upstream GitHub Actions in the Fedora latest and other jobs.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Used by openshift-ci bot. label Jan 25, 2023
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Used by openshift-ci bot. label Jan 25, 2023
@codeclimate
Copy link

codeclimate bot commented Jan 25, 2023

Code Climate has analyzed commit ba3056f 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 49.5% (0.0% change).

View more on Code Climate.

@jan-cerny jan-cerny merged commit 38a9775 into ComplianceAsCode:master Jan 26, 2023
@marcusburghardt marcusburghardt added the Highlight This PR/Issue should make it to the featured changelog. label Jan 26, 2023
@Mab879 Mab879 deleted the add_control_ref_audit branch January 26, 2023 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement General enhancements to the project. Highlight This PR/Issue should make it to the featured changelog. Test Suite Update in Test Suite.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants