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

Sle15 fix ansible pci-dss remediations in check mode #11263

Conversation

teacup-on-rockingchair
Copy link
Contributor

Description:

  • When run in check mode ansible remediation scripts should be more isolated and need extra checks instead of assumptions

Rationale:

  • Make sure required ansible steps are not skipped

@teacup-on-rockingchair teacup-on-rockingchair added the Ansible Ansible remediation update. label Nov 9, 2023
Copy link

github-actions bot commented Nov 9, 2023

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 Nov 9, 2023

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_ensure_gpgcheck_never_disabled' differs.
--- xccdf_org.ssgproject.content_rule_ensure_gpgcheck_never_disabled
+++ xccdf_org.ssgproject.content_rule_ensure_gpgcheck_never_disabled
@@ -35,8 +35,9 @@
     option: gpgcheck
     value: '1'
     no_extra_spaces: true
-  loop: '{{ repo_grep_results.stdout | regex_findall( ''(.+\.repo):\[(.+)\]\n?'' )
-    }}'
+  loop: '{{ repo_grep_results.stdout |regex_findall( ''(.+\.repo):\[(.+)\]\n?'' )
+    if repo_grep_results is not skipped else []}}'
+  when: repo_grep_results is not skipped
   tags:
   - CCE-80792-5
   - CJIS-5.10.4.1

ansible remediation for rule 'xccdf_org.ssgproject.content_rule_file_permissions_var_log_audit' differs.
--- xccdf_org.ssgproject.content_rule_file_permissions_var_log_audit
+++ xccdf_org.ssgproject.content_rule_file_permissions_var_log_audit
@@ -48,7 +48,7 @@
   when:
   - '"audit" in ansible_facts.packages'
   - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
-  - (log_file_exists.stdout | length > 0)
+  - log_file_exists is not skipped and (log_file_exists.stdout | length > 0)
   tags:
   - CCE-80819-6
   - CJIS-5.4.1.1
@@ -72,7 +72,8 @@
   when:
   - '"audit" in ansible_facts.packages'
   - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
-  - (log_file_exists is undefined) or (log_file_exists.stdout | length == 0)
+  - (log_file_exists is skipped) or (log_file_exists is undefined) or (log_file_exists.stdout
+    | length == 0)
   tags:
   - CCE-80819-6
   - CJIS-5.4.1.1
@@ -96,7 +97,8 @@
   when:
   - '"audit" in ansible_facts.packages'
   - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
-  - (log_file_line.stdout is defined) and (log_file_line.stdout | length > 0)
+  - (log_file_exists is not skipped) and (log_file_line.stdout is defined) and (log_file_line.stdout
+    | length > 0)
   tags:
   - CCE-80819-6
   - CJIS-5.4.1.1

ansible remediation for rule 'xccdf_org.ssgproject.content_rule_audit_rules_privileged_commands' differs.
--- xccdf_org.ssgproject.content_rule_audit_rules_privileged_commands
+++ xccdf_org.ssgproject.content_rule_audit_rules_privileged_commands
@@ -75,11 +75,13 @@
 - name: Ensure auditd Collects Information on the Use of Privileged Commands - Set
     List of Privileged Commands Found in Eligible Mount Points
   ansible.builtin.set_fact:
-    privileged_commands: '{{( result_privileged_commands_search.results | map(attribute=''stdout_lines'')
-      | select() | list ) | sum(start=[]) }}'
+    privileged_commands: '{{ privileged_commands | default([]) + item.stdout_lines
+      }}'
+  loop: '{{ result_privileged_commands_search.results }}'
   when:
   - '"audit" in ansible_facts.packages'
   - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
+  - item is not skipped
   tags:
   - CCE-80724-8
   - CJIS-5.4.1.1

ansible remediation for rule 'xccdf_org.ssgproject.content_rule_rsyslog_files_groupownership' differs.
--- xccdf_org.ssgproject.content_rule_rsyslog_files_groupownership
+++ xccdf_org.ssgproject.content_rule_rsyslog_files_groupownership
@@ -63,7 +63,9 @@
   ansible.builtin.set_fact:
     include_config_output: '{{ rsyslog_old_inc.stdout_lines + rsyslog_new_inc.stdout_lines
       }}'
-  when: ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
+  when:
+  - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
+  - rsyslog_old_inc is not skipped and rsyslog_new_inc is not skipped
   tags:
   - CCE-80860-0
   - NIST-800-53-AC-6(1)
@@ -85,10 +87,12 @@
     hidden: false
     follow: true
   loop: '{{ include_config_output | list + [rsyslog_etc_config] }}'
+  when:
+  - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
+  - include_config_output is defined
   register: rsyslog_config_files
   failed_when: false
   changed_when: false
-  when: ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
   tags:
   - CCE-80860-0
   - NIST-800-53-AC-6(1)
@@ -109,10 +113,12 @@
     grep -oP '^[^(\s|#|\$)]+[\s]+.*[\s]+-?(/+[^:;\s]+);*\.*$' {{ item.1.path }} | \
     awk '{print $NF}' | \
     sed -e 's/^-//' || true
-  loop: '{{ rsyslog_config_files.results | subelements(''files'') }}'
+  loop: '{{ rsyslog_config_files.results | default([]) | subelements(''files'') }}'
   register: log_files_old
   changed_when: false
-  when: ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
+  when:
+  - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
+  - rsyslog_config_files is not skipped
   tags:
   - CCE-80860-0
   - NIST-800-53-AC-6(1)
@@ -134,10 +140,12 @@
     grep -aoP "File\s*=\s*\"([/[:alnum:][:punct:]]*)\"\s*\)" | \
     grep -oE "\"([/[:alnum:][:punct:]]*)\"" | \
     tr -d "\""|| true
-  loop: '{{ rsyslog_config_files.results | subelements(''files'') }}'
+  loop: '{{ rsyslog_config_files.results | default([]) | subelements(''files'') }}'
   register: log_files_new
   changed_when: false
-  when: ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
+  when:
+  - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
+  - rsyslog_config_files is not skipped
   tags:
   - CCE-80860-0
   - NIST-800-53-AC-6(1)

ansible remediation for rule 'xccdf_org.ssgproject.content_rule_rsyslog_files_ownership' differs.
--- xccdf_org.ssgproject.content_rule_rsyslog_files_ownership
+++ xccdf_org.ssgproject.content_rule_rsyslog_files_ownership
@@ -63,7 +63,9 @@
   ansible.builtin.set_fact:
     include_config_output: '{{ rsyslog_old_inc.stdout_lines + rsyslog_new_inc.stdout_lines
       }}'
-  when: ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
+  when:
+  - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
+  - rsyslog_old_inc is not skipped and rsyslog_new_inc is not skipped
   tags:
   - CCE-80861-8
   - NIST-800-53-AC-6(1)
@@ -85,10 +87,12 @@
     hidden: false
     follow: true
   loop: '{{ include_config_output | list + [rsyslog_etc_config] }}'
+  when:
+  - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
+  - include_config_output is defined
   register: rsyslog_config_files
   failed_when: false
   changed_when: false
-  when: ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
   tags:
   - CCE-80861-8
   - NIST-800-53-AC-6(1)
@@ -109,10 +113,12 @@
     grep -oP '^[^(\s|#|\$)]+[\s]+.*[\s]+-?(/+[^:;\s]+);*\.*$' {{ item.1.path }} | \
     awk '{print $NF}' | \
     sed -e 's/^-//' || true
-  loop: '{{ rsyslog_config_files.results | subelements(''files'') }}'
+  loop: '{{ rsyslog_config_files.results | default([]) | subelements(''files'') }}'
   register: log_files_old
   changed_when: false
-  when: ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
+  when:
+  - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
+  - rsyslog_config_files is not skipped
   tags:
   - CCE-80861-8
   - NIST-800-53-AC-6(1)
@@ -134,10 +140,12 @@
     grep -aoP "File\s*=\s*\"([/[:alnum:][:punct:]]*)\"\s*\)" | \
     grep -oE "\"([/[:alnum:][:punct:]]*)\"" | \
     tr -d "\""|| true
-  loop: '{{ rsyslog_config_files.results | subelements(''files'') }}'
+  loop: '{{ rsyslog_config_files.results | default([]) | subelements(''files'') }}'
   register: log_files_new
   changed_when: false
-  when: ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
+  when:
+  - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
+  - rsyslog_config_files is not skipped
   tags:
   - CCE-80861-8
   - NIST-800-53-AC-6(1)

ansible remediation for rule 'xccdf_org.ssgproject.content_rule_rsyslog_files_permissions' differs.
--- xccdf_org.ssgproject.content_rule_rsyslog_files_permissions
+++ xccdf_org.ssgproject.content_rule_rsyslog_files_permissions
@@ -63,7 +63,9 @@
   ansible.builtin.set_fact:
     include_config_output: '{{ rsyslog_old_inc.stdout_lines + rsyslog_new_inc.stdout_lines
       }}'
-  when: ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
+  when:
+  - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
+  - rsyslog_old_inc is not skipped and rsyslog_new_inc is not skipped
   tags:
   - CCE-80862-6
   - NIST-800-53-AC-6(1)
@@ -85,10 +87,12 @@
     hidden: false
     follow: true
   loop: '{{ include_config_output | list + [rsyslog_etc_config] }}'
+  when:
+  - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
+  - include_config_output is defined
   register: rsyslog_config_files
   failed_when: false
   changed_when: false
-  when: ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
   tags:
   - CCE-80862-6
   - NIST-800-53-AC-6(1)
@@ -109,10 +113,12 @@
     grep -oP '^[^(\s|#|\$)]+[\s]+.*[\s]+-?(/+[^:;\s]+);*\.*$' {{ item.1.path }} | \
     awk '{print $NF}' | \
     sed -e 's/^-//' || true
-  loop: '{{ rsyslog_config_files.results | subelements(''files'') }}'
+  loop: '{{ rsyslog_config_files.results | default([]) | subelements(''files'') }}'
   register: log_files_old
   changed_when: false
-  when: ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
+  when:
+  - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
+  - rsyslog_config_files is not skipped
   tags:
   - CCE-80862-6
   - NIST-800-53-AC-6(1)
@@ -134,10 +140,12 @@
     grep -aoP "File\s*=\s*\"([/[:alnum:][:punct:]]*)\"\s*\)" | \
     grep -oE "\"([/[:alnum:][:punct:]]*)\"" | \
     tr -d "\""|| true
-  loop: '{{ rsyslog_config_files.results | subelements(''files'') }}'
+  loop: '{{ rsyslog_config_files.results | default([]) | subelements(''files'') }}'
   register: log_files_new
   changed_when: false
-  when: ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
+  when:
+  - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
+  - rsyslog_config_files is not skipped
   tags:
   - CCE-80862-6
   - NIST-800-53-AC-6(1)

@Mab879
Copy link
Member

Mab879 commented Nov 17, 2023

Can you please rebase this PR? We updated the Ansible lint CI job and I would like to run it on this PR.

@vojtapolasek vojtapolasek self-assigned this Nov 24, 2023
Copy link
Collaborator

@vojtapolasek vojtapolasek left a comment

Choose a reason for hiding this comment

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

Hello @teacup-on-rockingchair could you please explain what is the goal of these changes? Is there a test scenario which I could use to prove that previous Ansible did not work as expected and now it works?
Thank you.

@@ -23,4 +23,5 @@
value: '1'
no_extra_spaces: True
# regex filters grep output for files ending in .repo and matching section names.
loop: "{{ repo_grep_results.stdout | regex_findall( '(.+\\.repo):\\[(.+)\\]\\n?' ) }}"
loop: "{{ repo_grep_results.stdout |regex_findall( '(.+\\.repo):\\[(.+)\\]\\n?' ) if repo_grep_results is not skipped else []}}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this change needed in case we consider the next line with the "when" clause?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well both to answer both to this and the above question the extra checks added are handling the case when run ansible remediation is run in check mode in a similar way:

ansible-playbook -i /root/ansible_inventory.yml sle15-playbook-pci-dss.yml --check --diff

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @vojtapolasek does this PR causes any issues that I am missing, if not can you please approve it 🙏

@vojtapolasek vojtapolasek added this to the 0.1.72 milestone Nov 29, 2023
Copy link

codeclimate bot commented Dec 5, 2023

Code Climate has analyzed commit 08d6c3f 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.

@vojtapolasek
Copy link
Collaborator

Hello @teacup-on-rockingchair I would like to merge the PR, but I still don't understand its purpose. I tried to do the following with master branch checked out and later with this PR checked out:

  1. build rhel9 content
  2. run the RHEL 9 STIG playbook like
ansible-playbook -u root -i rhel9, --tags file_permissions_var_log_audit rhel9-playbook-stig.yml --check --diff

I thought it will show me different output but the output was exactly the same.
I chose the rule file_permissions_var_log_audit as an example because you modified it in this PR.
Could you please provide reliable reproducer which wil show the problem you are trying to fix in this PR?
As I said, I would like to merge this, but I still don't know what it fixes.
Thank you.

@teacup-on-rockingchair
Copy link
Contributor Author

Hello @teacup-on-rockingchair I would like to merge the PR, but I still don't understand its purpose. I tried to do the following with master branch checked out and later with this PR checked out:

1. build rhel9 content

2. run the RHEL 9 STIG playbook like
ansible-playbook -u root -i rhel9, --tags file_permissions_var_log_audit rhel9-playbook-stig.yml --check --diff

I thought it will show me different output but the output was exactly the same. I chose the rule file_permissions_var_log_audit as an example because you modified it in this PR. Could you please provide reliable reproducer which wil show the problem you are trying to fix in this PR? As I said, I would like to merge this, but I still don't know what it fixes. Thank you.

Hi @vojtapolasek , sorry for the much delayed reply but was on a long vacation. So to return to this, what I use to reproduce it is following command (which is not much different than yours):

# ansible-playbook --connection=local  --inventory 127.0.0.1,  --limit 127.0.0.1 /root/src/content/build/ansible/sle15-playbook-pci-dss-4.yml  --tags file_permissions_var_log_audit  --check --diff

PLAY [Ansible Playbook for xccdf_org.ssgproject.content_profile_pci-dss-4] *************************************************************

TASK [Gathering Facts] *****************************************************************************************************************
[WARNING]: Platform linux on host 127.0.0.1 is using the discovered Python interpreter at /usr/bin/python, but future installation of
another Python interpreter could change this. See https://docs.ansible.com/ansible/2.9/reference_appendices/interpreter_discovery.html
for more information.
ok: [127.0.0.1]

TASK [Gather the package facts] ********************************************************************************************************
ok: [127.0.0.1]

TASK [Get audit log files] *************************************************************************************************************
skipping: [127.0.0.1]

TASK [Parse log file line] *************************************************************************************************************
fatal: [127.0.0.1]: FAILED! => {"msg": "The conditional check '(log_file_exists.stdout | length > 0)' failed. The error was: error while evaluating conditional ((log_file_exists.stdout | length > 0)): 'dict object' has no attribute 'stdout'\n\nThe error appears to be in '/root/src/content/build/ansible/sle15-playbook-pci-dss-4.yml': line 10181, column 7, but may\nbe elsewhere in the file depending on the exact syntax problem.\n\nThe offending line appears to be:\n\n\n    - name: Parse log file line\n      ^ here\n"}

PLAY RECAP *****************************************************************************************************************************
127.0.0.1                  : ok=2    changed=0    unreachable=0    failed=1    skipped=1    rescued=0    ignored=0   


if I run the same command and try to get more verbose output , adding -vvv to ansible playbook params, I see the reason for the failure (skipping some output):

...
skipping: [127.0.0.1] => {
    "changed": false,
    "invocation": {
        "module_args": {
            "_raw_params": "grep -iw ^log_file /etc/audit/auditd.conf",
            "_uses_shell": false,
            "argv": null,
            "chdir": null,
            "creates": null,
            "executable": null,
            "removes": null,
            "stdin": null,
            "stdin_add_newline": true,
            "strip_empty_ends": true,
            "warn": true
        }
    },
    "msg": "skipped, running in check mode"
}

TASK [Parse log file line] *************************************************************************************************************
task path: /root/src/content/build/ansible/sle15-playbook-pci-dss-4.yml:10181
fatal: [127.0.0.1]: FAILED! => {
    "msg": "The conditional check '(log_file_exists.stdout | length > 0)' failed. The error was: error while evaluating conditional ((log_file_exists.stdout | length > 0)): 'dict object' has no attribute 'stdout'\n\nThe error appears to be in '/root/src/content/build/ansible/sle15-playbook-pci-dss-4.yml': line 10181, column 7, but may\nbe elsewhere in the file depending on the exact syntax problem.\n\nThe offending line appears to be:\n\n\n    - name: Parse log file line\n      ^ here\n"
}

PLAY RECAP *****************************************************************************************************************************
127.0.0.1                  : ok=2    changed=0    unreachable=0    failed=1    skipped=1    rescued=0    ignored=0   
...

so the source of the issue here in my understanding that the greping task is skipped in check mode, since it is command type of task and is skipped, but the next tasks rely on it, so there goes in the patch that was done.

Copy link
Collaborator

@vojtapolasek vojtapolasek left a comment

Choose a reason for hiding this comment

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

Hello, thank you for explaining it. I think I understand the changes. I did not manage to reproduce those fatal failures in my environment, but I think the changes are fine.

@vojtapolasek vojtapolasek merged commit 80b5408 into ComplianceAsCode:master Jan 10, 2024
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ansible Ansible remediation update.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants