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

Sysctl template remediations do not modify package files #10881

Conversation

teacup-on-rockingchair
Copy link
Contributor

@teacup-on-rockingchair teacup-on-rockingchair commented Jul 20, 2023

Description:

  • Remediations should not coment settings in /usr/lib/sysctl.d/

Rationale:

Those files belong to the packages and as such this is considered as modification of the system package.
SYSCTL.D(5) manual page states that priority of sysctl configuration file is:

   /etc/sysctl.d/*.conf
   /run/sysctl.d/*.conf
   /usr/lib/sysctl.d/*.conf

So if there is conflict between a setting in /usr/lib/sysctl.d/ and /etc/sysctl.d/, the latter will win
In this context current remediation code that modifies only /etc/sysctl.d/ files is good enough since even, if we do not comment out setting in /usr/lib/sysctl.d/ it will be ignored by the system

Review Hints:

@freddieRv you might want to consider if for your platform the changes are valid also

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

openshift-ci bot commented Jul 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

@teacup-on-rockingchair teacup-on-rockingchair added OVAL OVAL update. Related to the systems assessments. SLES SUSE Linux Enterprise Server product related. and removed do-not-merge/work-in-progress Used by openshift-ci bot. labels Jul 20, 2023
@teacup-on-rockingchair teacup-on-rockingchair marked this pull request as ready for review July 20, 2023 08:48
@github-actions
Copy link

github-actions bot commented Jul 20, 2023

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

Fedora Environment
Open in Gitpod

Oracle Linux 8 Environment
Open in Gitpod

@jan-cerny
Copy link
Collaborator

@teacup-on-rockingchair Usually, problems like that should be solved in the RPM package that owns these files by marking them as config files in the spec file.

Also, I don't like the change of the STIG file in this PR, it's unrelated to the topic of this PR. Please submit the change of STIG file in a separate PR.

@teacup-on-rockingchair teacup-on-rockingchair force-pushed the fix_rpm_verify_hashes_ignore_cfg_dir_sle branch from 2dbda51 to 5c300e0 Compare July 22, 2023 05:31
@teacup-on-rockingchair teacup-on-rockingchair marked this pull request as draft July 23, 2023 10:21
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Used by openshift-ci bot. label Jul 23, 2023
@teacup-on-rockingchair teacup-on-rockingchair force-pushed the fix_rpm_verify_hashes_ignore_cfg_dir_sle branch from 5c300e0 to a11e01f Compare August 29, 2023 17:33
Those files belong to the packages and as such this is considered as modification of the system package.
SYSCTL.D(5) manual page states that priority of sysctl configuration file is:

/etc/sysctl.d/*.conf
/run/sysctl.d/*.conf
/usr/lib/sysctl.d/*.conf

So if there is conflict between a setting in /usr/lib/sysctl.d/ and /etc/sysctl.d/, the latter will win
In this context current remediation code that modifies only /etc/sysctl.d/ files is good enough since even,
if we do not comment out setting in /usr/lib/sysctl.d/ it will be ignored by the system
@teacup-on-rockingchair teacup-on-rockingchair changed the title Fix rpm verify hashes ignore SLE specifc configuration directory Sysctl template remediations do not modify package files Aug 29, 2023
@teacup-on-rockingchair teacup-on-rockingchair marked this pull request as ready for review August 29, 2023 18:12
@codeclimate
Copy link

codeclimate bot commented Aug 29, 2023

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

View more on Code Climate.

@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Used by openshift-ci bot. label Aug 29, 2023
Copy link
Contributor

@freddieRv freddieRv left a comment

Choose a reason for hiding this comment

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

Thank you, @teacup-on-rockingchair. I think these changes rather make SLE and OL products behave the same with respect to the files you are mentioning.

@jan-cerny jan-cerny self-assigned this Aug 30, 2023
@jan-cerny jan-cerny added this to the 0.1.70 milestone Aug 30, 2023
@jan-cerny jan-cerny merged commit f743946 into ComplianceAsCode:master Aug 30, 2023
55 of 57 checks passed
@@ -6,7 +6,7 @@

# Comment out any occurrences of {{{ SYSCTLVAR }}} from /etc/sysctl.d/*.conf files
{{% if product in [ "sle12", "sle15"] %}}
for f in /run/sysctl.d/*.conf /etc/sysctl.d/*.conf /usr/local/lib/sysctl.d/*.conf /usr/lib/sysctl.d/*.conf /lib/sysctl.d/*.conf; do
for f in /etc/sysctl.d/*.conf /run/sysctl.d/*.conf /usr/local/lib/sysctl.d/*.conf /lib/sysctl.d/*.conf; do
Copy link
Member

Choose a reason for hiding this comment

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

/lib directory is a symlink to /usr/lib at least for Fedora/RHEL-alike distributions if I'm not mistaken. So this probably needs to be removed as well.

Copy link
Member

Choose a reason for hiding this comment

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

Surprisingly, Debian has issues with this as of 2022, but this is indeed the case for everybody else for some time (apparently including Ubuntu): https://lwn.net/Articles/890219/

Copy link
Member

@ggbecker ggbecker Sep 1, 2023

Choose a reason for hiding this comment

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

This can be potentially merged with the elif statement as they will become very similar in case /lib and /usr/lib are dropped from both cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OVAL OVAL update. Related to the systems assessments. SLES SUSE Linux Enterprise Server product related.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants