A fix into ansible part of the rule audit_rules_suid_privilege_function

[rumch-se]

Description:

• Small fix in the rule audit_rules_suid_privilege_function

Rationale:

• In the current version ansible remediation does not work

[openshift-ci]

Hi @rumch-se. 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/test-infra repository.

[github-actions]

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

rhel8 (from CTF) Environment (using Fedora as testing environment)

Fedora Testing Environment

Oracle Linux 8 Environment

[github-actions]

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

Click here to see the full diff

ansible remediation for rule 'xccdf_org.ssgproject.content_rule_audit_rules_suid_privilege_function' differs.
--- xccdf_org.ssgproject.content_rule_audit_rules_suid_privilege_function
+++ xccdf_org.ssgproject.content_rule_audit_rules_suid_privilege_function
@@ -19,30 +19,6 @@
 
 - name: Service facts
   ansible.builtin.service_facts: null
-  when:
-  - '"audit" in ansible_facts.packages'
-  - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
-  tags:
-  - CCE-83556-1
-  - DISA-STIG-RHEL-08-030000
-  - NIST-800-53-AC-6(9)
-  - NIST-800-53-AU-12(3)
-  - NIST-800-53-AU-7(a)
-  - NIST-800-53-AU-7(b)
-  - NIST-800-53-AU-8(b)
-  - NIST-800-53-CM-5(1)
-  - audit_rules_suid_privilege_function
-  - low_complexity
-  - low_disruption
-  - medium_severity
-  - no_reboot_needed
-  - restrict_strategy
-
-- name: Check the rules script being used
-  ansible.builtin.command: grep '^ExecStartPost' /usr/lib/systemd/system/auditd.service
-  register: check_rules_scripts_result
-  changed_when: false
-  failed_when: false
   when:
   - '"audit" in ansible_facts.packages'
   - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
@@ -101,8 +77,7 @@
   when:
   - '"audit" in ansible_facts.packages'
   - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
-  - '"auditd.service" in ansible_facts.services'
-  - '"augenrules" in check_rules_scripts_result.stdout'
+  - ('"auditd.service" in ansible_facts.services' or '"augenrules.service" in ansible_facts.services')
   register: augenrules_audit_rules_privilege_function_update_result
   with_items: '{{ suid_audit_rules }}'
   tags:
@@ -130,8 +105,7 @@
   when:
   - '"audit" in ansible_facts.packages'
   - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
-  - '"auditd.service" in ansible_facts.services'
-  - '"auditctl" in check_rules_scripts_result.stdout'
+  - ('"auditd.service" in ansible_facts.services' or '"augenrules.service" in ansible_facts.services')
   register: auditctl_audit_rules_privilege_function_update_result
   with_items: '{{ suid_audit_rules }}'
   tags:

[vojtapolasek]

Hello @rumch-se and thank you for this improvement. You are improving the part which decides if /etc/audit/audit.rules or /etc/audit/rules.d/privileged.rules should be considered. But there are test scenarios only for the second case. Could you please also add additional test scenarios so that we can make sure that this decision is performed correctly?
I think you don't need to duplicate all test scenarios, just a few.
Thank you.

[rumch-se]

Hello @vojtapolasek

The tests were added

Have a nice day
Rumen

[vojtapolasek]

Hello @rumch-se and thank you for tests.
Before I get to my feedback about them, I have a question: What are you actually trying to accomplish with this change?
Was the previous way of detection if auditctl or augen-rules is used not working on SLE?
If yes, what was the problem? I am asking because this detection of auditctl vs augen-rules is used in large number of Audit-related rules. I would like to know the answer before we proceed further.
Thank you,
Vojta

[rumch-se]

Hello @vojtapolasek
The rule did not work on SLE. The execution of ansible remediation did not add the audit rules i.e. we did not have fix and the check was fail. I did some investigation about ansible and I found that when we have a complex logical condition based on some facts (but these facts do not exist) - ansible skips execution. I faced this way of working of ansible, because after the execution of this part of the code

• name: Check the rules script being used
  ansible.builtin.command:
  grep '^ExecStartPost' /usr/lib/systemd/system/auditd.service
  register: check_rules_scripts_result
  changed_when: false
  failed_when: false

on my test machine I did not have anything in check_rules_scripts_result . The command grep returned nothing. And this varibale is a part of the condition to make changes (to add audit rules) after.

Have a nice day
Rumen

[jan-cerny]

/packit retest-failed

[rumch-se]

Hello @teacup-on-rockingchair and @vojtapolasek

Would be possible this PR to be merged into the master

Have a nice day
Rumen

[vojtapolasek]

Hello @rumch-se , I apologize for such a long delay, I was out for two weeks.
I have doupts that the solution you propose is the correct one. There are two tasks; one puts rules into /etc/audit/audit.rules and the second one puts rule into /etc/audit/rules.d/privileged.rules. Original when clauses contained two conditions specified as a list, that puts implicit AND between them. Your changes put explicit OR between them. Before your changes, it was decided which task to execute based on contents of the auditd.service definition file. Now as you put the OR between clauses, those tasks will execute no matter what is the content of the auditd.service file. On RHEL, we actually do not have any augenrules.service file, the choice is made by commenting / uncommenting the desired line within auditd.service.
Could you please send here contents of the /usr/lib/systemd/system/auditd.service file from your test machine? I think the problem lies somewhere in this file.
Best regards,
Vojtech Polasek

[rumch-se]

Hello @vojtapolasek

The attached file was taken from SLE 15 SP5 after fresh installation. I put .txt to be able to attach it.

Have a nice day
Rumen

auditd.service.txt

[vojtapolasek]

Hello @rumch-se so I see the problem now.
This is the relevant section from the file you submitted:

## To not use augenrules: copy this file to /etc/systemd/system/auditd.service,
## uncomment the next line, and comment the Requires=augenrules.service above.
#ExecStartPost=-/sbin/auditctl -R /etc/audit/audit.rules

Where as the relevant part for RHEL and latest Ubuntu is this:

## To not use augenrules, copy this file to /etc/systemd/system/auditd.service
## and comment/delete the next line and uncomment the auditctl line.
## NOTE: augenrules expect any rules to be added to /etc/audit/rules.d/
ExecStartPost=-/sbin/augenrules --load
#ExecStartPost=-/sbin/auditctl -R /etc/audit/audit.rules

That explains the misunderstanding. SLE has extra Systemd service for Augenrules, while some other distros don't.
I think the easiest solution would be to either create whole new remediation specific for SLE, or to use Jinja conditionals to check for different conditions than rest of distros. What do you think?

[rumch-se]

Hello @vojtapolasek

I made a new commit, the proposed solution probably is not the most elegant one, but I think is the most robust and it will cover different cases.

The proposed by you "if" is too complex to be done, because when the problematic ansible block was created it worked at that point of time for SLE 12 SP5 and SLE 15 SP2, but now it does not work for SLE 15 SP5. I do expect that other vendors could have also changes because of improvements of their products via service packs/or something else ... and in this case it will be too complex to adapt the code to these different changes of the products. From the other side ansible has a benefit to make a check and after to execute if it is necessary.

Have a nice day
Rumen

[vojtapolasek]

Hello @rumch-se and thank you for changes. I think we are getting to the end of this story.
I have some remarks though:
Please see my specific comments.
Please rename test scenarios, names are not descriptive enough. I suggest following:

1. correct_value.audit.pass.sh -> correct_value_auditctl.pass.sh
2. correct_value.privileged.pass.sh -> correct_value_augenrules.pass.sh

I think this makes it obvious what is the difference between tests.
Also note, that the scenario names should look like scenario_name.fail.sh or scenario_name.pass.sh, that means there should not be any extra dots in the name except for those two.

Next, please ensure that you are correctly modifying the /usr/lib/systemd/system/auditd.service file at the begining of every test scenario.
Notice that none of two files mentioned above modify this file, but different content of that file is basically the reason for their existence - they test if correct conditionals are applied.

Lastly, I suggest you also add test scenario which tests two new tasks you have added for the case that there is no uncommented ExecStartPost line in the auditd.service.
Thank you.

[rumch-se]

Hello @vojtapolasek

Please check the last commit

Have a nice day
Rumen

[vojtapolasek]

Hello @rumch-se and thank you for updating the playbook and test scenarios.
Unfortunately I found another challenge we need to solve.
The problem is that you modified Ansible, but you did not modify the Bash. Therefore, your new test scenarios commented_execstartpost.fail.sh and commented_execstartpost.pass.sh don't pass.
I still have feeling that these configuration differences you are trying to embrace in this PR are SLE specific... how about copying the shared.yml with your modifications to sle12.yml and sle15.yml and then discarding changes to the original shared.yml? In this case, I think everyone will be satisfied. You can then make your special test scenarios mentioned above SLE specific.
Additionally, scenarios still need a bit of work.
Note that Github actions which run tests needed to approve this PR are running on either Fedora or Centos streams.
This is important because you will need to modify test scenarios so that they pass on these operating systems.
The relevant part of the auditd.service file is described in this comment above: #11170 (comment)
Now, let's go through categories opf test scenarios touched in this PR.
Scenarios containing augenrules in their name do not need to modify /usr/lib/systemd/system/auditd.service when run on RHEL or Fedora because default configuration of these operating systems uses augenrules. Please remove the conditional containing sed command from them.
Scenarios containing auditctl in their name need to modify the auditd.service file, probably in the way similar to this one

sed -i "s%^ExecStartPost=.*%ExecStartPost=-/sbin/auditctl%" /usr/lib/systemd/system/auditd.service

[rumch-se]

Hello @vojtapolasek

1/I afraid that your proposal "...how about copying the shared.yml with your modifications to sle12.yml and sle15.yml" will not work because SUSE has Service Packs - and in the past the original code worked on SLE , but the code is not working now. That means because of evolution of the product via service packs - for some service pack the problematic line is commented for other is not.

2/From the design point of view the ansible is idempotent - which will work as a "filter" during the execution - i.e. only the necessary block will be executed. You can see that when you run the playbook more that one time.

3/A General Question - which should be leading - the bash / or ansible. I thought that bash is leading. In bash we don't have the check created for ansible, but the problematic check in ansible - breaks the execution of the ansible playbook with a FATAL error. Did you check this on a system different than SLE? Because in bash we don't have this check - I decided to remove the problematic part and redesigned the code to use "or". After your 1st feedback - I returned back the problematic part of the code and made some modifications to cover all cases. In other words Which part is the correct one. I am asking because the bash part was not designed by SUSE. The macro bash_fix_audit_syscall_rule does not check internally for the comment of the ExecStartPost

Have a nice day
Rumen

[vojtapolasek]

Hello @rumch-se ,

1/I afraid that your proposal "...how about copying the shared.yml with your modifications to sle12.yml and sle15.yml" will not work because SUSE has Service Packs - and in the past the original code worked on SLE , but the code is not working now. That means because of evolution of the product via service packs - for some service pack the problematic line is commented for other is not.

Does the implementation which you NOW propose in your PR work with all SLE releases you need?

[rumch-se]

Hello @vojtapolasek

It should work because I am covering 2 cases when ExecStartPos is commented, and 2 cases when ExecStartPos is not commented.

Have a nice day
Rumen

[vojtapolasek]

Hello @rumch-se, great. So my idea was to take the state of shared.yml as it is now and make two copies of it - sle12.yml and sle15.yml. And then leave the shared.yml in tact as it was before you introduced these changes. I am suggesting this because it seems that this specific auditd.service configuration which is handled in this PR is so far specific for SLE.
This would bring two benefits:

1. the shared.yml would not be so complicated
2. you could add test scenarios for this configuration and mark them with SLE platform. Also you won't need to modify Bash remediation because your changes will be SLE specific and Bash remediation is not SLE specific.
   What do you think?

[rumch-se]

Hello @vojtapolasek

1/I don't understand your logic here, because we at SUSE are trying to have 100% coverage of ansible and bash remediation. When I did my 1st commit to this PR, I made a change which does ansible to work more closely to the bash - I have excluded a part which was added by SUSE's developer. I am talking about this part

name: Check the rules script being used
ansible.builtin.command:
grep '^ExecStartPost' /usr/lib/systemd/system/auditd.service
register: check_rules_scripts_result
changed_when: false
failed_when: false

which other OSes (products) need this part?

2/ Despite of the fact how you do remediation the results should be the same. In case of bash - because we don't have this check related to auditd.service - we will have records auditctl and augenrules, but in case of ansible the decision about this records will be taken according to the ExecStartPost - why?

Seems we don't have consistency here....

Have a nice day
Rumen

[vojtapolasek]

Hello @rumch-se and thank you for this last comment. It explains a lot. I am really sorry that we had to took such detour. But from reading description and rationale, I did not deduce that you plan to align Ansible with Bash remediation by removing that conditional. I think it would help it this was stated there.
In this case, I think we can revert to 9921054.
And then I will try to merge it.

[vojtapolasek]

Hello @rumch-se are you going to work on this PR? I think that after agreeing on its purpose, the work should be fairly easy - just revert to one of its previous states.

[rumch-se]

Hello @vojtapolasek

Sorry for my delay, but I was not at work previous week. I did a revert expected.

Have a nice day
Rumen

[codeclimate]

Code Climate has analyzed commit c1b87fd 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 58.5%.

View more on Code Climate.

[rumch-se]

Hello @vojtapolasek

Will this PR be included into the planed new release ?
Thank you for your feedback.

Rumen

[vojtapolasek]

Hello @rumch-se , the PR will be included into release 0.1.72, which will happen in February 2024. Unfortunately we are already one week into stabilization phase of 0.1.71. But I am merging the PR as all checks pass.