Changes in bash remediation for accounts_password_set_max_life_existi…

[rumch-se]

…ng and accounts_password_set_min_life_existing

Description:

• Correction in bash remediation in accounts_password_set_max_life_existing and accounts_password_set_min_life_existing

Rationale:

• Existing version does not manage correctly all user. For example when user account has minimum age 0

Notes: Please address my message to Marcus Burghardt as developer of OVAL and Ansible parts for both rules.
1/
I think that both parts (Ansible and OVAL for both rules) need corrections, because ansible parts do not apply min/max age for all users.
On my test host they changed parameters for root and vagrant users, but not for users which have empty definitions for the age of their accounts.
When I applied ansible remediation for min_life - audit check had status pass - despite of the fact that only of two users min_age parameter was changed - i.e. OVAL part is not correct
When I applied ansible remediation for max_life - audit check had status fail
2/
The usage of awk - makes the code shorter, but in some cases I had issue to find users which have min_age 0 and to correct this value to the new one.

[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

[codeclimate]

Code Climate has analyzed commit 155c51b 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 51.7% (0.0% change).

View more on Code Climate.

[rumch-se]

Hello @marcusburghardt
May you see my notes about this PR?
Have a nice day
Rumen

[marcusburghardt]

@rumch-se , the only difference between the approach used by sle and other products (jinja else) is the command used to update the password policy. SUSE prefers passwd while other products would use chage. However, the approach to collect these accounts should be the same.

So, I propose to simplify this remediation by collecting the accounts just once and using the jinja conditional only for the passwd and chage command. In that case, I would personally prefer the approach used in else. The awk can be extended if necessary to test more conditions.

This would make the remediation simpler to maintain and read. Could you consider that, please?

[rumch-se]

Hello @marcusburghardt
1/
I tried to use chage - but I had issues related to PAM (i.e. PAM blocked me to apply this change). With passwd I did not have these issues. I think that RedHat is not protected from this issue also.
2/
The second issue which I had was to apply the change via awk. When I did cat /etc/shadow - I had an account which had to be changed, but it remained unchanged i.e. awk did not take it in consideration. Now with the current approach I was able to apply changes for all accounts.
Have a nice day
Rumen

[marcusburghardt]

Hello @marcusburghardt 1/ I tried to use chage - but I had issues related to PAM (i.e. PAM blocked me to apply this change). With passwd I did not have these issues. I think that RedHat is not protected from this issue also.

It is fine to use passwd for sle products. My point was to put only this inside jinja conditionals since the list of users can be shared among other distros.

2/ The second issue which I had was to apply the change via awk. When I did cat /etc/shadow - I had an account which had to be changed, but it remained unchanged i.e. awk did not take it in consideration. Now with the current approach I was able to apply changes for all accounts. Have a nice day Rumen

Could you provide an example, please? I believe the awk command can be easily extended to cover more cases.

[rumch-se]

Hello @marcusburghardt
The awk approach is not working correctly. Please see my message with attached files. The test_max_with_passwd.txt cover the case where we have a loop based on awk, and we do change with passwd. Only two accounts were changed!
With my approach I scan /etc/shadow ones (I do not collect accounts) and if during the scan there is a need of a change I do it.

[marcusburghardt]

@rumch-se , could you also take a look in the Project Style Guide related to commit messages, please?
https://complianceascode.readthedocs.io/en/latest/manual/developer/04_style_guide.html#commit-messages

[rumch-se]

Hello @marcusburghardt
I did the following

1. I built master to support rhel8
2. I used bash remediation for rhel8 (based on awk) an executed them on sle 15 sp2
3. Case 1. test_max_with_passwd.txt - in this case I used original bash for rhel8, but replaced in it chage with passwd. In the results you can see that only 2 from 25 accounts were changed. But for unchanged accounts we had empty string!
4. Case 2. test_min_with_passwd.txt - in this case I used original bash for rhel8, but replaced in it chage with passwd. In the results you can see that only 2 from 25 accounts were changed. But for unchanged accounts we had empty string!
5. Case 3. test_max_with_chage.txt - in this case I kept the original bash script. In this case - only one account was changed and I had a message on the screen "You are required to change your password immediately (password expired)
   chage: PAM: Authentication token is no longer valid; new one required"
6. Case 4. test_min_with_chage.txt - in this case I kept the original bash script. Only 2 from 25 accounts were changed. But for unchanged accounts we had empty string!

With my version I change ALL accounts and I did not have the message about PAM.
I think that OVAL (the audit) at the moment is not working correctly also

Have a nice day
Rumen

test_max_with_passwd.txt
test_min_with_passwd.txt
test_max_with_chage.txt
test_min_with_chage.txt

[marcusburghardt]

Hello @marcusburghardt I did the following

1. I built master to support rhel8
2. I used bash remediation for rhel8 (based on awk) an executed them on sle 15 sp2
3. Case 1. test_max_with_passwd.txt - in this case I used original bash for rhel8, but replaced in it chage with passwd. In the results you can see that only 2 from 25 accounts were changed. But for unchanged accounts we had empty string!
4. Case 2. test_min_with_passwd.txt - in this case I used original bash for rhel8, but replaced in it chage with passwd. In the results you can see that only 2 from 25 accounts were changed. But for unchanged accounts we had empty string!
5. Case 3. test_max_with_chage.txt - in this case I kept the original bash script. In this case - only one account was changed and I had a message on the screen "You are required to change your password immediately (password expired)
   chage: PAM: Authentication token is no longer valid; new one required"
6. Case 4. test_min_with_chage.txt - in this case I kept the original bash script. Only 2 from 25 accounts were changed. But for unchanged accounts we had empty string!

With my version I change ALL accounts and I did not have the message about PAM. I think that OVAL (the audit) at the moment is not working correctly also

Have a nice day Rumen

test_max_with_passwd.txt test_min_with_passwd.txt test_max_with_chage.txt test_min_with_chage.txt

@rumch-se , it is intentional to not change all accounts but only the accounts with a password set. It is useless to set max and min password age for accounts without a password even set. So, the awk approach is correct, efficient and working as expected. If you are sure the SUSE approach is indeed intending to set max and min password age for all accounts, regardless of having a password set or not, I am fine with this new conditional since it does not affect other products. Just let me know if you are sure about that.

[github-actions]

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

Click here to see the full diff

bash remediation for rule 'xccdf_org.ssgproject.content_rule_accounts_password_set_max_life_existing' differs.
--- xccdf_org.ssgproject.content_rule_accounts_password_set_max_life_existing
+++ xccdf_org.ssgproject.content_rule_accounts_password_set_max_life_existing
@@ -2,7 +2,8 @@
 var_accounts_maximum_age_login_defs=''
 
 
+while IFS= read -r i; do
+ 
+ chage -M $var_accounts_maximum_age_login_defs $i
 
-while IFS= read -r i; do
- chage -M $var_accounts_maximum_age_login_defs $i
 done < <(awk -v var="$var_accounts_maximum_age_login_defs" -F: '(/^[^:]+:[^!*]/ && ($5 > var || $5 == "")) {print $1}' /etc/shadow)

bash remediation for rule 'xccdf_org.ssgproject.content_rule_accounts_password_set_min_life_existing' differs.
--- xccdf_org.ssgproject.content_rule_accounts_password_set_min_life_existing
+++ xccdf_org.ssgproject.content_rule_accounts_password_set_min_life_existing
@@ -2,7 +2,8 @@
 var_accounts_minimum_age_login_defs=''
 
 
+while IFS= read -r i; do
+ 
+ chage -m $var_accounts_minimum_age_login_defs $i
 
-while IFS= read -r i; do
- chage -m $var_accounts_minimum_age_login_defs $i
 done < <(awk -v var="$var_accounts_minimum_age_login_defs" -F: '(/^[^:]+:[^!*]/ && ($4 < var || $4 == "")) {print $1}' /etc/shadow)

[rumch-se]

Hello @marcusburghardt
Thank you for your feedback. I did a research on this topic, and now I know better the main idea behind this rule. According to the research we can have accounts (usually used to run a single command) without passwords in unix/linux, but the remote access via ssh has to be forbidden for them (see attached pictures), because they usually a subject of an attack.

In my last commit I kept the usage of awk, but for SLE product I used passwd instead of chage.
Have a nice day
Rumen

[marcusburghardt]

Hello @marcusburghardt Thank you for your feedback. I did a research on this topic, and now I know better the main idea behind this rule. According to the research we can have accounts (usually used to run a single command) without passwords in unix/linux, but the remote access via ssh has to be forbidden for them (see attached pictures), because they usually a subject of an attack.

In my last commit I kept the usage of awk, but for SLE product I used passwd instead of chage. Have a nice day Rumen

Correct @rumch-se . I am glad the it was helpful. I will do a final review, but it seems ready to be merged. Thanks.