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 auditd rule to watch apparmor instead of selinux on Ubuntu #12790

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mpurg
Copy link
Contributor

@mpurg mpurg commented Jan 8, 2025

Description:

  • Fix auditd rule audit_rules_mac_modifications to watch apparmor dirs /etc/apparmor and /etc/apparmor.d instead of /etc/selinux on Ubuntu

@openshift-ci openshift-ci bot added the needs-ok-to-test Used by openshift-ci bot. label Jan 8, 2025
Copy link

openshift-ci bot commented Jan 8, 2025

Hi @mpurg. Thanks for your PR.

I'm waiting for a ComplianceAsCode member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

Copy link

github-actions bot commented Jan 8, 2025

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

rhel8 (from CTF) Environment (using Fedora as testing environment)
Open in Gitpod

Fedora Testing Environment
Open in Gitpod

Oracle Linux 8 Environment
Open in Gitpod

Copy link

github-actions bot commented Jan 8, 2025

This datastream diff is auto generated by the check Compare DS/Generate Diff

Click here to see the full diff
New content has different text for rule 'xccdf_org.ssgproject.content_rule_audit_rules_mac_modification'.
--- xccdf_org.ssgproject.content_rule_audit_rules_mac_modification
+++ xccdf_org.ssgproject.content_rule_audit_rules_mac_modification
@@ -7,10 +7,13 @@
 augenrules program to read audit rules during daemon startup (the
 default), add the following line to a file with suffix .rules in the
 directory /etc/audit/rules.d:
+
 -w /etc/selinux/ -p wa -k MAC-policy
+
 If the auditd daemon is configured to use the auditctl
 utility to read audit rules during daemon startup, add the following line to
 /etc/audit/audit.rules file:
+
 -w /etc/selinux/ -p wa -k MAC-policy
 
 [reference]:
@@ -371,7 +374,7 @@
 10.3
 
 [rationale]:
-The system's mandatory access policy (SELinux) should not be
+The system's mandatory access policy (SELinux or Apparmor) should not be
 arbitrarily changed by anything other than administrator action. All changes to
 MAC policy should be audited.
 

bash remediation for rule 'xccdf_org.ssgproject.content_rule_audit_rules_mac_modification' differs.
--- xccdf_org.ssgproject.content_rule_audit_rules_mac_modification
+++ xccdf_org.ssgproject.content_rule_audit_rules_mac_modification
@@ -2,6 +2,7 @@
 if rpm --quiet -q audit && rpm --quiet -q kernel; then
 
 # Perform the remediation for both possible tools: 'auditctl' and 'augenrules'
+
 # Create a list of audit *.rules files that should be inspected for presence and correctness
 # of a particular audit rule. The scheme is as follows:
 #

Copy link

codeclimate bot commented Jan 8, 2025

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

View more on Code Climate.

@vojtapolasek vojtapolasek self-assigned this Jan 10, 2025
@vojtapolasek
Copy link
Collaborator

/ok-to-test

@openshift-ci openshift-ci bot added ok-to-test Used by openshift-ci bot. and removed needs-ok-to-test Used by openshift-ci bot. labels Jan 10, 2025
@vojtapolasek
Copy link
Collaborator

@mpurg thank you for the PR. I reviewed it and I understand its purpose. However, I wonder if this is an optimal way.
The PR introduces lots of conditions into the rule. I think that it would be easier to create two new rules, one checking for /etc/apparmor and another checking for /etc/apparmor.d. Both rules could be templated, therefore minimizing chance of error. You could use audit_rules_watch template for this. What do you think about this suggestion?

@mpurg
Copy link
Contributor Author

mpurg commented Jan 11, 2025

@vojtapolasek thanks for reviewing. I agree that your solution is cleaner. To clarify, the reason I adapted the existing rule was to avoid introducing a breaking change downstream when using customized tailoring files. I.e. if a user disabled this rule using a tailoring file, and the rule was now renamed, the rule would no longer be disabled with the same tailoring file. Additionally, I assumed it was ok to modify the existing rule since the rule id contains mac_modifications and not selinux_modifications. Do you think that these rationales could justify the less clean code? Alternatively, I can implement your cleaner solution upstream and implement the current patch downstream only.

@dodys dodys added this to the 0.1.76 milestone Jan 14, 2025
@dodys dodys added the Ubuntu Ubuntu product related. label Jan 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test Used by openshift-ci bot. Ubuntu Ubuntu product related.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants