Update var_password_pam_remember_control_flag to allow multiple values in OL8

[Xeicker]

Description:

• Update var_password_pam_remember_control_flag to allow multiple values
• Update ansible, bash and OVAL to work with this new var_password_pam_remember_control_flag
• Update bash to use the bash_ensure_pam_module_options to cover all possible scenarios
• Update ansible regex to fix any control flag
• Update tests

Rationale:

• This makes the rule to accept multiple compliant scenarios
• The updates in fixes increase the scenarios which they can handle

[openshift-ci]

Hi @Xeicker. 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

OCIL for rule 'xccdf_org.ssgproject.content_rule_accounts_password_pam_pwhistory_remember_password_auth' differs:
--- old datastream
+++ new datastream
@@ -1,10 +1,10 @@
 Check that the operating system prohibits the reuse of a password for
 a minimum of generations with the following command:
 # grep pam_pwhistory.so /etc/pam.d/password-auth
-password pam_pwhistory.so remember= use_authtok
-If the command does not return a result, or the returned line is commented
-out, has a second column value different from , does not contain
-"remember" value, or the value is less than
-, this is a finding.
+password control_flag pam_pwhistory.so remember= use_authtok
+If the command does not return a result, the returned line is commented out,
+the line does not contain "remember" value, the value is less than , the control_flag value isn't one of
+the next: this is a
+finding.
 Is it the case that the value of remember is not set equal to or greater than <sub idref="var_password_pam_remember" />?
 
bash remediation for rule 'xccdf_org.ssgproject.content_rule_accounts_password_pam_pwhistory_remember_password_auth' differs:
--- old datastream
+++ new datastream
@@ -4,6 +4,8 @@
 var_password_pam_remember=''
 var_password_pam_remember_control_flag=''
 
+
+var_password_pam_remember_control_flag="$(echo $var_password_pam_remember_control_flag | cut -d \, -f 1)"
 
 if [ -e "/etc/pam.d/password-auth" ] ; then
 PAM_FILE_PATH="/etc/pam.d/password-auth"

ansible remediation for rule 'xccdf_org.ssgproject.content_rule_accounts_password_pam_pwhistory_remember_password_auth' differs:
--- old datastream
+++ new datastream
@@ -194,7 +194,8 @@
 is present in {{ pam_file_path }}'
 ansible.builtin.lineinfile:
 path: '{{ pam_file_path }}'
- regexp: ^\s*password\s+{{ var_password_pam_remember_control_flag }}\s+pam_pwhistory.so\s*.*
+ regexp: ^\s*password\s+{{ var_password_pam_remember_control_flag.split(",")[0]
+ }}\s+pam_pwhistory.so\s*.*
 state: absent
 check_mode: true
 changed_when: false
@@ -219,7 +220,7 @@
 ansible.builtin.replace:
 dest: '{{ pam_file_path }}'
 regexp: ^(\s*password\s+).*(\bpam_pwhistory.so.*)
- replace: \1{{ var_password_pam_remember_control_flag }} \2
+ replace: \1{{ var_password_pam_remember_control_flag.split(",")[0] }} \2
 register: result_pam_module_edit
 when:
 - result_pam_line_other_control_present.found == 1
@@ -229,7 +230,8 @@
 ansible.builtin.lineinfile:
 dest: '{{ pam_file_path }}'
 insertafter: ^password.*requisite.*pam_pwquality.so
- line: password {{ var_password_pam_remember_control_flag }} pam_pwhistory.so
+ line: password {{ var_password_pam_remember_control_flag.split(",")[0]
+ }} pam_pwhistory.so
 register: result_pam_module_add
 when:
 - result_pam_line_other_control_present.found == 0 or result_pam_line_other_control_present.found
@@ -248,7 +250,8 @@
 option is present in {{ pam_file_path }}'
 ansible.builtin.lineinfile:
 path: '{{ pam_file_path }}'
- regexp: ^\s*password\s+{{ var_password_pam_remember_control_flag }}\s+pam_pwhistory.so\s*.*\sremember\b
+ regexp: ^\s*password\s+{{ var_password_pam_remember_control_flag.split(",")[0]
+ }}\s+pam_pwhistory.so\s*.*\sremember\b
 state: absent
 check_mode: true
 changed_when: false
@@ -259,7 +262,8 @@
 ansible.builtin.lineinfile:
 path: '{{ pam_file_path }}'
 backrefs: true
- regexp: ^(\s*password\s+{{ var_password_pam_remember_control_flag }}\s+pam_pwhistory.so.*)
+ regexp: ^(\s*password\s+{{ var_password_pam_remember_control_flag.split(",")[0]
+ }}\s+pam_pwhistory.so.*)
 line: \1 remember={{ var_password_pam_remember }}
 state: present
 register: result_pam_remember_add
@@ -271,7 +275,8 @@
 ansible.builtin.lineinfile:
 path: '{{ pam_file_path }}'
 backrefs: true
- regexp: ^(\s*password\s+{{ var_password_pam_remember_control_flag }}\s+pam_pwhistory.so\s+.*)(remember)=[0-9a-zA-Z]+\s*(.*)
+ regexp: ^(\s*password\s+{{ var_password_pam_remember_control_flag.split(",")[0]
+ }}\s+pam_pwhistory.so\s+.*)(remember)=[0-9a-zA-Z]+\s*(.*)
 line: \1\2={{ var_password_pam_remember }} \3
 register: result_pam_remember_edit
 when:

OCIL for rule 'xccdf_org.ssgproject.content_rule_accounts_password_pam_pwhistory_remember_system_auth' differs:
--- old datastream
+++ new datastream
@@ -1,10 +1,10 @@
 Check that the operating system prohibits the reuse of a password for a minimum of
 generations with the following command:
 # grep pam_pwhistory.so /etc/pam.d/system-auth
-password pam_pwhistory.so remember= use_authtok
-If the command does not return a result, or the returned line is commented out, has a second
-column value different from ,
-does not contain "remember" value, or the value is less than ,
-this is a finding.
+password control_flag pam_pwhistory.so remember= use_authtok
+If the command does not return a result, the returned line is commented out,
+the line does not contain "remember" value, the value is less than , the control_flag value isn't one of
+the next: this is a
+finding.
 Is it the case that the value of remember is not set equal to or greater than <sub idref="var_password_pam_remember" />?
 
bash remediation for rule 'xccdf_org.ssgproject.content_rule_accounts_password_pam_pwhistory_remember_system_auth' differs:
--- old datastream
+++ new datastream
@@ -4,6 +4,8 @@
 var_password_pam_remember=''
 var_password_pam_remember_control_flag=''
 
+
+var_password_pam_remember_control_flag="$(echo $var_password_pam_remember_control_flag | cut -d \, -f 1)"
 
 if [ -e "/etc/pam.d/system-auth" ] ; then
 PAM_FILE_PATH="/etc/pam.d/system-auth"

ansible remediation for rule 'xccdf_org.ssgproject.content_rule_accounts_password_pam_pwhistory_remember_system_auth' differs:
--- old datastream
+++ new datastream
@@ -194,7 +194,8 @@
 present in {{ pam_file_path }}'
 ansible.builtin.lineinfile:
 path: '{{ pam_file_path }}'
- regexp: ^\s*password\s+{{ var_password_pam_remember_control_flag }}\s+pam_pwhistory.so\s*.*
+ regexp: ^\s*password\s+{{ var_password_pam_remember_control_flag.split(",")[0]
+ }}\s+pam_pwhistory.so\s*.*
 state: absent
 check_mode: true
 changed_when: false
@@ -219,7 +220,7 @@
 ansible.builtin.replace:
 dest: '{{ pam_file_path }}'
 regexp: ^(\s*password\s+).*(\bpam_pwhistory.so.*)
- replace: \1{{ var_password_pam_remember_control_flag }} \2
+ replace: \1{{ var_password_pam_remember_control_flag.split(",")[0] }} \2
 register: result_pam_module_edit
 when:
 - result_pam_line_other_control_present.found == 1
@@ -229,7 +230,8 @@
 ansible.builtin.lineinfile:
 dest: '{{ pam_file_path }}'
 insertafter: ^password.*requisite.*pam_pwquality.so
- line: password {{ var_password_pam_remember_control_flag }} pam_pwhistory.so
+ line: password {{ var_password_pam_remember_control_flag.split(",")[0]
+ }} pam_pwhistory.so
 register: result_pam_module_add
 when:
 - result_pam_line_other_control_present.found == 0 or result_pam_line_other_control_present.found
@@ -248,7 +250,8 @@
 is present in {{ pam_file_path }}'
 ansible.builtin.lineinfile:
 path: '{{ pam_file_path }}'
- regexp: ^\s*password\s+{{ var_password_pam_remember_control_flag }}\s+pam_pwhistory.so\s*.*\sremember\b
+ regexp: ^\s*password\s+{{ var_password_pam_remember_control_flag.split(",")[0]
+ }}\s+pam_pwhistory.so\s*.*\sremember\b
 state: absent
 check_mode: true
 changed_when: false
@@ -259,7 +262,8 @@
 ansible.builtin.lineinfile:
 path: '{{ pam_file_path }}'
 backrefs: true
- regexp: ^(\s*password\s+{{ var_password_pam_remember_control_flag }}\s+pam_pwhistory.so.*)
+ regexp: ^(\s*password\s+{{ var_password_pam_remember_control_flag.split(",")[0]
+ }}\s+pam_pwhistory.so.*)
 line: \1 remember={{ var_password_pam_remember }}
 state: present
 register: result_pam_remember_add
@@ -271,7 +275,8 @@
 ansible.builtin.lineinfile:
 path: '{{ pam_file_path }}'
 backrefs: true
- regexp: ^(\s*password\s+{{ var_password_pam_remember_control_flag }}\s+pam_pwhistory.so\s+.*)(remember)=[0-9a-zA-Z]+\s*(.*)
+ regexp: ^(\s*password\s+{{ var_password_pam_remember_control_flag.split(",")[0]
+ }}\s+pam_pwhistory.so\s+.*)(remember)=[0-9a-zA-Z]+\s*(.*)
 line: \1\2={{ var_password_pam_remember }} \3
 register: result_pam_remember_edit
 when:

[marcusburghardt]

I would suggest to consider the use of macros for the remediation.

Another point I am curious is about the acceptable controls. I am fine if this is the case for ol8, but I would like to remind that requisite and required are slightly different in this context. For example, in RHEL benchmarks, only one control is acceptable but the control was changed between RHEL7 and RHEL8. I wonder if the case is similar here.

[Xeicker]

Another point I am curious is about the acceptable controls. I am fine if this is the case for ol8, but I would like to remind that requisite and required are slightly different in this context. For example, in RHEL benchmarks, only one control is acceptable but the control was changed between RHEL7 and RHEL8. I wonder if the case is similar here.

Well from DISA STIG requirements OL08-00-020221 & OL08-00-020220(and RHEL equivalents RHEL-08-020221 & RHEL-08-020220) it is not explicitly stated that any control must be there. And from man pages both required and requisite sounded reasonable. Could you explain with more detail why in RHEL8 only one of them is acceptable? may be the same applies to OL8

[marcusburghardt]

Another point I am curious is about the acceptable controls. I am fine if this is the case for ol8, but I would like to remind that requisite and required are slightly different in this context. For example, in RHEL benchmarks, only one control is acceptable but the control was changed between RHEL7 and RHEL8. I wonder if the case is similar here.

Well from DISA STIG requirements OL08-00-020221 & OL08-00-020220(and RHEL equivalents RHEL-08-020221 & RHEL-08-020220) it is not explicitly stated that any control must be there. And from man pages both required and requisite sounded reasonable. Could you explain with more detail why in RHEL8 only one of them is acceptable? may be the same applies to OL8

Unfortunately I don't have the context of this change, but in RHEL7, for example, there are documentation using required while since RHEL8 the recommendation is to use requisite.

Looking the pam.conf man page, I can see this:

required
failure of such a PAM will ultimately lead to the PAM-API returning failure but only after the
remaining stacked modules (for this service and type) have been invoked.

requisite
like required, however, in the case that such a module returns a failure, control is directly
returned to the application. The return value is that associated with the first required or requisite
module to fail. Note, this flag can be used to protect against the possibility of a user getting the
opportunity to enter a password over an unsafe medium. It is conceivable that such behavior
might inform an attacker of valid accounts on a system. This possibility should be weighed against
the not insignificant concerns of exposing a sensitive password in a hostile environment.

So, IMHO, it is better to use requisite in the context of pam_pwhistory.so in the password group because there is no much sense to continue the stack once pam_pwhistory.so returns fail. I can imagine some scenarios where this could lead to a situation where the password is changed even if pam_pwhistory.so fails, depending on which modules are combined in the stack and their respective settings. This is probably undesired.
The only probably desired case I can imagine is to log this somewhere by any reason if more modules are included after pam_pwhistory.so to do so and then finally stop the processing. But this sounds atypical. Maybe there are other cases I can't see, so this is a personal view.

In short, regardless of the specific case of pam_pwhistory.so, I can see fair points on accepting more than one control in the var_password_pam_remember_control_flag variable.

[jan-cerny]

@marcusburghardt Could you please take a look at this? It's been rebased and as you recently modified the PAM and autheselect related things you have more context than me.

[Xeicker]

@marcusburghardt I don't know why I thought the macros for ansible where also included in the PR you mentioned.
Should I include the migration of ansible remediation to macros in this PR?

[marcusburghardt]

@marcusburghardt I don't know why I thought the macros for ansible where also included in the PR you mentioned. Should I include the migration of ansible remediation to macros in this PR?

Hi @Xeicker , yes. Please, use the new macros for the remediations. This way we can keep the same standard for all PAM related rules and make them easier to be maintained. The Ansible macros are very new (merged today). They would make our lives much easier, I hope. : )

[marcusburghardt]

@marcusburghardt Could you please take a look at this? It's been rebased and as you recently modified the PAM and autheselect related things you have more context than me.

Sure @jan-cerny , I am assign this PR to myself. Thanks

[marcusburghardt]

@Xeicker , could you rabase the PR and solve the conflicts, please?

[marcusburghardt]

I have some considerations regarding the test scenarios, to make them more independent from profiles. Also, the Ansible remediation is failing in some cases, but is due to an already fixed issue (#9141). If you rebase the PR, this should also be fixed. Thanks.

[marcusburghardt]

For this test scenario, I would recommend to define the variable value to be testes instead of the profile where it is set. This way we make sure the test scenario is not impacted by any change in the profile. For example:
variables = var_password_pam_remember=5,var_password_pam_remember_control_flag=ol8

[Xeicker]

I wanted to do something similar, but it doesn't work in this way, if I set var_password_pam_remember_control_flag=ol8 the value is literally "ol8" it doesn't take the value from the .var file

[marcusburghardt]

I see. I am not sure now if it is intentional or there are any technical limitation for this, but I think it is worth to report an issue so we can investigate and try to improve this.

[marcusburghardt]

I had to read the description twice. Maybe we could subtly improve the reading. For example:

    'Specify the control flag required for password remember requirement. If multiple
    values are allowed, write them separated by commas as in "required,requisite".
    In case of multiple values, the first value will be taken for remediations.'

[marcusburghardt]

I have successfully tested all the impacted rules. There are only some comments about wording where I think we can improve the experience for the readers. Also it would be good to rebase this PR so we can run all the tests considering the updates and fixes (#9141) on relevant macros. I wouldn't say these points are blockers to merge this PR, but I also think is a good moment to tackle them. @Xeicker , could you take a look and share your thoughts, please?

[marcusburghardt]

I see. I am not sure now if it is intentional or there are any technical limitation for this, but I think it is worth to report an issue so we can investigate and try to improve this.

[codeclimate]

Code Climate has analyzed commit 64e2ad9 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 42.7% (0.0% change).

View more on Code Climate.

[marcusburghardt]

@jan-cerny , there are changes requested by you which seems to be already addressed in this PR. Could you update your review, please?

All reviewer comments have been addressed.