Improve ansible remediation of accounts_umask_etc_login_defs #9490

ggbecker · 2022-09-07T16:58:04Z

Description:

Improve ansible remediation of accounts_umask_etc_login_defs.
- Correctly replace wrong values
- Do not attempt to change a file that already has a valid configuration

Rationale:

Regex 101 Example: https://regex101.com/r/T0xVnu/1
Fixes: UMASK is not set correctly for /etc/login.defs #9456

This PR can inspire fixes for other similar rules as well. #9456 (comment)

github-actions · 2022-09-07T16:59:07Z

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 · 2022-09-07T17:02:40Z

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_accounts_umask_etc_bashrc' differs.
--- xccdf_org.ssgproject.content_rule_accounts_umask_etc_bashrc
+++ xccdf_org.ssgproject.content_rule_accounts_umask_etc_bashrc
@@ -4,11 +4,13 @@
 tags:
 - always
 
-- name: Replace user umask in /etc/bashrc
- replace:
+- name: Check if umask in /etc/bashrc is already set
+ ansible.builtin.lineinfile:
 path: /etc/bashrc
- regexp: umask.*
- replace: umask {{ var_accounts_user_umask }}
+ regexp: ^(\s*)umask\s+.*
+ state: absent
+ check_mode: true
+ changed_when: false
 register: umask_replace
 tags:
 - CCE-81036-6
@@ -22,12 +24,12 @@
 - no_reboot_needed
 - restrict_strategy
 
-- name: Append user umask in /etc/bashrc
- lineinfile:
- create: true
+- name: Replace user umask in /etc/bashrc
+ ansible.builtin.replace:
 path: /etc/bashrc
- line: umask {{ var_accounts_user_umask }}
- when: umask_replace is not changed
+ regexp: ^(\s*)umask(\s+).*
+ replace: \g<1>umask\g<2>{{ var_accounts_user_umask }}
+ when: umask_replace.found > 0
 tags:
 - CCE-81036-6
 - DISA-STIG-RHEL-08-020353
@@ -39,3 +41,21 @@
 - medium_severity
 - no_reboot_needed
 - restrict_strategy
+
+- name: Ensure the Default umask is Appended Correctly
+ ansible.builtin.lineinfile:
+ create: true
+ path: /etc/bashrc
+ line: umask {{ var_accounts_user_umask }}
+ when: umask_replace.found == 0
+ tags:
+ - CCE-81036-6
+ - DISA-STIG-RHEL-08-020353
+ - NIST-800-53-AC-6(1)
+ - NIST-800-53-CM-6(a)
+ - accounts_umask_etc_bashrc
+ - low_complexity
+ - low_disruption
+ - medium_severity
+ - no_reboot_needed
+ - restrict_strategy

ansible remediation for rule 'xccdf_org.ssgproject.content_rule_accounts_umask_etc_csh_cshrc' differs.
--- xccdf_org.ssgproject.content_rule_accounts_umask_etc_csh_cshrc
+++ xccdf_org.ssgproject.content_rule_accounts_umask_etc_csh_cshrc
@@ -4,11 +4,13 @@
 tags:
 - always
 
-- name: Replace user umask in /etc/csh.cshrc
- replace:
+- name: Check if umask in /etc/csh.cshrc is already set
+ ansible.builtin.lineinfile:
 path: /etc/csh.cshrc
- regexp: umask.*
- replace: umask {{ var_accounts_user_umask }}
+ regexp: ^(\s*)umask\s+.*
+ state: absent
+ check_mode: true
+ changed_when: false
 register: umask_replace
 tags:
 - CCE-81037-4
@@ -22,12 +24,12 @@
 - no_reboot_needed
 - restrict_strategy
 
-- name: Append user umask in /etc/csh.cshrc
- lineinfile:
- create: true
+- name: Replace user umask in /etc/csh.cshrc
+ ansible.builtin.replace:
 path: /etc/csh.cshrc
- line: umask {{ var_accounts_user_umask }}
- when: umask_replace is not changed
+ regexp: ^(\s*)umask(\s+).*
+ replace: \g<1>umask\g<2>{{ var_accounts_user_umask }}
+ when: umask_replace.found > 0
 tags:
 - CCE-81037-4
 - DISA-STIG-RHEL-08-020353
@@ -39,3 +41,21 @@
 - medium_severity
 - no_reboot_needed
 - restrict_strategy
+
+- name: Ensure the Default umask is Appended Correctly
+ ansible.builtin.lineinfile:
+ create: true
+ path: /etc/csh.cshrc
+ line: umask {{ var_accounts_user_umask }}
+ when: umask_replace.found == 0
+ tags:
+ - CCE-81037-4
+ - DISA-STIG-RHEL-08-020353
+ - NIST-800-53-AC-6(1)
+ - NIST-800-53-CM-6(a)
+ - accounts_umask_etc_csh_cshrc
+ - low_complexity
+ - low_disruption
+ - medium_severity
+ - no_reboot_needed
+ - restrict_strategy

ansible remediation for rule 'xccdf_org.ssgproject.content_rule_accounts_umask_etc_login_defs' differs.
--- xccdf_org.ssgproject.content_rule_accounts_umask_etc_login_defs
+++ xccdf_org.ssgproject.content_rule_accounts_umask_etc_login_defs
@@ -18,12 +18,14 @@
 tags:
 - always
 
-- name: Ensure the Default UMASK is Set Correctly
- replace:
+- name: Check if UMASK is already set
+ ansible.builtin.lineinfile:
 path: /etc/login.defs
- regexp: ^UMASK
- replace: UMASK {{ var_accounts_user_umask }}
- register: umask_replace
+ regexp: ^(\s*)UMASK\s+.*
+ state: absent
+ check_mode: true
+ changed_when: false
+ register: result_umask_is_set
 when: '"shadow-utils" in ansible_facts.packages'
 tags:
 - CCE-82888-9
@@ -37,14 +39,14 @@
 - no_reboot_needed
 - restrict_strategy
 
-- name: Ensure the Default UMASK is Appended Correctly
- lineinfile:
- create: true
+- name: Replace user UMASK in /etc/login.defs
+ ansible.builtin.replace:
 path: /etc/login.defs
- line: UMASK {{ var_accounts_user_umask }}
+ regexp: ^(\s*)UMASK(\s+).*
+ replace: \g<1>UMASK\g<2>{{ var_accounts_user_umask }}
 when:
 - '"shadow-utils" in ansible_facts.packages'
- - umask_replace is not changed
+ - result_umask_is_set.found > 0
 tags:
 - CCE-82888-9
 - DISA-STIG-RHEL-08-020351
@@ -56,3 +58,23 @@
 - medium_severity
 - no_reboot_needed
 - restrict_strategy
+
+- name: Ensure the Default UMASK is Appended Correctly
+ ansible.builtin.lineinfile:
+ create: true
+ path: /etc/login.defs
+ line: UMASK {{ var_accounts_user_umask }}
+ when:
+ - '"shadow-utils" in ansible_facts.packages'
+ - result_umask_is_set.found == 0
+ tags:
+ - CCE-82888-9
+ - DISA-STIG-RHEL-08-020351
+ - NIST-800-53-AC-6(1)
+ - NIST-800-53-CM-6(a)
+ - accounts_umask_etc_login_defs
+ - low_complexity
+ - low_disruption
+ - medium_severity
+ - no_reboot_needed
+ - restrict_strategy

ggbecker · 2022-09-08T10:14:01Z

one extra fix was needed in the line (check if the variable was defined first:

when: result_umask_is_correctly_set.found is defined and result_umask_is_correctly_set.found == 0

marcusburghardt

The Ansible playbook can be simplified using only the lineinfile module.

...system/accounts/accounts-session/user_umask/accounts_umask_etc_login_defs/ansible/shared.yml

marcusburghardt · 2022-09-08T11:04:25Z

...system/accounts/accounts-session/user_umask/accounts_umask_etc_login_defs/ansible/shared.yml

+  changed_when: false
+  register: result_umask_is_set
+
+- name: Check if UMASK is already correctly set


Using the lineinfile module you can check and fix the parameter at once. You can use the combination of backrefs, line and regexp options. At this point you already know that UMASK is defined there. So, using this approach, the lineinfile module will only change the line if it is different from the desired line.

There is something similar here: https://github.com/ComplianceAsCode/content/blob/master/shared/macros/10-ansible.jinja#L914-L929

I'm struggling to make it work, maybe I need some more help here. Also, it may not align with multiple occurrences if found as stated below.

Correct. The lineinfile module only changes one single line per time. If more lines are expected to be changed, the replace module is indeed the best option. Let me test the playbook locally here.

@ggbecker , I could only check this now. So, basically the playbook can be simplified. You don't need to check of the parameter is already correct in a separate task. The replace and lineinfile modules will already do that. Here is an example of a simplified playbook:

- name: Check if UMASK is already set ansible.builtin.lineinfile: path: /etc/login.defs regexp: ^(\s*)UMASK\s+.* state: absent check_mode: true changed_when: false register: result_umask_is_set - name: Replace user UMASK in /etc/login.defs ansible.builtin.replace: path: /etc/login.defs regexp: ^(\s*)UMASK(\s+).* replace: \g<1>UMASK\g<2>{{ var_accounts_user_umask }} when: - result_umask_is_set.found > 0 - name: Ensure the Default UMASK is Appended Correctly ansible.builtin.lineinfile: create: true path: /etc/login.defs line: UMASK {{ var_accounts_user_umask }} when: - result_umask_is_set.found == 0

...system/accounts/accounts-session/user_umask/accounts_umask_etc_login_defs/ansible/shared.yml

marcusburghardt · 2022-09-09T13:13:17Z

...system/accounts/accounts-session/user_umask/accounts_umask_etc_login_defs/ansible/shared.yml

+  changed_when: false
+  register: result_umask_is_set
+
+- name: Check if UMASK is already correctly set


@ggbecker , I could only check this now. So, basically the playbook can be simplified. You don't need to check of the parameter is already correct in a separate task. The replace and lineinfile modules will already do that. Here is an example of a simplified playbook:

- name: Check if UMASK is already set ansible.builtin.lineinfile: path: /etc/login.defs regexp: ^(\s*)UMASK\s+.* state: absent check_mode: true changed_when: false register: result_umask_is_set - name: Replace user UMASK in /etc/login.defs ansible.builtin.replace: path: /etc/login.defs regexp: ^(\s*)UMASK(\s+).* replace: \g<1>UMASK\g<2>{{ var_accounts_user_umask }} when: - result_umask_is_set.found > 0 - name: Ensure the Default UMASK is Appended Correctly ansible.builtin.lineinfile: create: true path: /etc/login.defs line: UMASK {{ var_accounts_user_umask }} when: - result_umask_is_set.found == 0

@marcusburghardt

Co-authored by: @marcusburghardt

Remediation improved to consider existing data in the files and replace only when there is no configuration or misconfigured.

openshift-ci · 2022-09-15T15:44:44Z

@ggbecker: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-aws-rhcos4-high	`b2b2a8d`	link	true	`/test e2e-aws-rhcos4-high`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

codeclimate · 2022-09-15T15:55:58Z

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

View more on Code Climate.

marcusburghardt

Thanks @ggbecker

ggbecker added the Ansible Ansible remediation update. label Sep 7, 2022

ggbecker added this to the 0.1.64 milestone Sep 7, 2022

marcusburghardt self-assigned this Sep 8, 2022

ggbecker force-pushed the fix-umask-regex branch from 1b4a7ee to e13fa3b Compare September 8, 2022 10:12

marcusburghardt requested changes Sep 8, 2022

View reviewed changes

Improve ansible remediation of accounts_umask_etc_login_defs.

be2a4c8

ggbecker force-pushed the fix-umask-regex branch from e13fa3b to be2a4c8 Compare September 8, 2022 11:08

ggbecker added 2 commits September 8, 2022 13:57

Remove unnecessary negative lookahead from regex.

b8640cd

Use fully qualified name for ansible module.

bc99431

marcusburghardt requested changes Sep 9, 2022

View reviewed changes

ggbecker added 2 commits September 15, 2022 17:03

Make the ansible remediation of accounts_umask_etc_login_defs simpler.

9e6b564

Co-authored by: @marcusburghardt

Update ansible remediation of two umask rules.

b2b2a8d

Remediation improved to consider existing data in the files and replace only when there is no configuration or misconfigured.

ggbecker requested a review from marcusburghardt September 15, 2022 15:14

vojtapolasek modified the milestones: 0.1.64, 0.1.65 Sep 19, 2022

marcusburghardt approved these changes Sep 22, 2022

View reviewed changes

marcusburghardt merged commit bf063f0 into ComplianceAsCode:master Sep 22, 2022

merillr mentioned this pull request Nov 2, 2022

Reopen #9456 (umask in login.defs) #9758

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve ansible remediation of accounts_umask_etc_login_defs #9490

Improve ansible remediation of accounts_umask_etc_login_defs #9490

ggbecker commented Sep 7, 2022

github-actions bot commented Sep 7, 2022

github-actions bot commented Sep 7, 2022 •

edited

Loading

ggbecker commented Sep 8, 2022 •

edited

Loading

marcusburghardt left a comment

marcusburghardt Sep 8, 2022

ggbecker Sep 8, 2022

marcusburghardt Sep 8, 2022

marcusburghardt Sep 9, 2022

marcusburghardt Sep 9, 2022

openshift-ci bot commented Sep 15, 2022 •

edited

Loading

codeclimate bot commented Sep 15, 2022

marcusburghardt left a comment

Improve ansible remediation of accounts_umask_etc_login_defs #9490

Improve ansible remediation of accounts_umask_etc_login_defs #9490

Conversation

ggbecker commented Sep 7, 2022

Description:

Rationale:

github-actions bot commented Sep 7, 2022

github-actions bot commented Sep 7, 2022 • edited Loading

ggbecker commented Sep 8, 2022 • edited Loading

marcusburghardt left a comment

Choose a reason for hiding this comment

marcusburghardt Sep 8, 2022

Choose a reason for hiding this comment

ggbecker Sep 8, 2022

Choose a reason for hiding this comment

marcusburghardt Sep 8, 2022

Choose a reason for hiding this comment

marcusburghardt Sep 9, 2022

Choose a reason for hiding this comment

marcusburghardt Sep 9, 2022

Choose a reason for hiding this comment

openshift-ci bot commented Sep 15, 2022 • edited Loading

codeclimate bot commented Sep 15, 2022

marcusburghardt left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 7, 2022 •

edited

Loading

ggbecker commented Sep 8, 2022 •

edited

Loading

openshift-ci bot commented Sep 15, 2022 •

edited

Loading