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

[Ubuntu2404] Fix remediation of rule accounts_user_dot_user_ownership #13026

Conversation

alanmcanonical
Copy link
Contributor

Description:

  • Use find instead globbing inside awk

Rationale:

  • According to man chown, -f only suppress error msg. So replace the -f with -v (verbose) to debug
  • The result of awk -F':' '{ if ($3 >= 1000 && $3 != 65534) system("chown -v " $3" "$6"/.[^\.]?*") }' /etc/passwd is:
root@sec-noble-amd64:/home/am# awk -F':' '{ if ($3 >= 1000 && $3 != 65534) system("chown -v " $3" "$6"/.[^\.]?*") }' /etc/passwd
chown: cannot access '/home/am/.[^.]?*': No such file or directory
failed to change ownership of '/home/am/.[^.]?*' to 1000

@openshift-ci openshift-ci bot added the needs-ok-to-test Used by openshift-ci bot. label Feb 11, 2025
Copy link

openshift-ci bot commented Feb 11, 2025

Hi @alanmcanonical. 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-sigs/prow repository.

@alanmcanonical
Copy link
Contributor Author

@dodys Please have a look.

Copy link

github-actions bot commented Feb 11, 2025

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_user_dot_user_ownership' differs.
--- xccdf_org.ssgproject.content_rule_accounts_user_dot_user_ownership
+++ xccdf_org.ssgproject.content_rule_accounts_user_dot_user_ownership
@@ -1,2 +1,2 @@
 
-awk -F':' '{ if ($3 >= 1000 && $3 != 65534) system("chown -f " $3" "$6"/.[^\.]?*") }' /etc/passwd
+awk -F: '{if ($3 >= 1000 && $3 != 65534) print $3":"$6}' /etc/passwd | while IFS=: read -r uid home; do find "$home" -maxdepth 1 -name "\.[^.]*" -exec chown -f $uid "{}" \;; done

ansible remediation for rule 'xccdf_org.ssgproject.content_rule_accounts_user_dot_user_ownership' differs.
--- xccdf_org.ssgproject.content_rule_accounts_user_dot_user_ownership
+++ xccdf_org.ssgproject.content_rule_accounts_user_dot_user_ownership
@@ -1,8 +1,9 @@
 - name: Ensure interactive local users are the owners of their respective initialization
     files
-  ansible.builtin.command:
-    cmd: awk -F':' '{ if ($3 >= 1000 && $3 != 65534) system("chown -f " $3" "$6"/.[^\.]?*")
-      }' /etc/passwd
+  ansible.builtin.shell:
+    cmd: 'awk -F: ''{if ($3 >= 1000 && $3 != 65534) print $3":"$6}'' /etc/passwd |
+      while IFS=: read -r uid home; do find "$home" -maxdepth 1 -name "\.[^.]*" -exec
+      chown -f $uid "{}" \;; done'
   tags:
   - CCE-86316-7
   - accounts_user_dot_user_ownership

Copy link

Change in Ansible shell module found.

Please consider using more suitable Ansible module than shell if possible.

@@ -4,4 +4,4 @@
# complexity = low
# disruption = low

awk -F':' '{ if ($3 >= {{{ uid_min }}} && $3 != {{{ nobody_uid }}}) system("chown -f " $3" "$6"/.[^\.]?*") }' /etc/passwd
awk -F':' '{ if ($3 >= {{{ uid_min }}} && $3 != {{{ nobody_uid }}}) system("find "$6" -maxdepth 1 -name \.[^.]?* -exec chown -f "$3" {} \;") }' /etc/passwd
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason for the ? in the regex? I think it should be \.[^.]*

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe a mistake made by previous author

Copy link
Contributor Author

@alanmcanonical alanmcanonical Feb 12, 2025

Choose a reason for hiding this comment

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

If we want to exclude double dot files we should use \.[^.]*

@dodys dodys self-assigned this Feb 12, 2025
@dodys dodys added Ubuntu Ubuntu product related. CIS CIS Benchmark related. labels Feb 12, 2025
@dodys dodys added this to the 0.1.77 milestone Feb 12, 2025
Copy link

Change in Ansible shell module found.

Please consider using more suitable Ansible module than shell if possible.

@alanmcanonical alanmcanonical force-pushed the fix_accounts_user_dot_user_ownership branch from c2289bd to a1eb65f Compare February 12, 2025 14:42
@alanmcanonical alanmcanonical force-pushed the fix_accounts_user_dot_user_ownership branch from a1eb65f to a993c12 Compare February 12, 2025 14:45
Copy link

Change in Ansible shell module found.

Please consider using more suitable Ansible module than shell if possible.

@alanmcanonical
Copy link
Contributor Author

@mpurg Please review the latest changes

Copy link

Change in Ansible shell module found.

Please consider using more suitable Ansible module than shell if possible.

Copy link

Change in Ansible shell module found.

Please consider using more suitable Ansible module than shell if possible.

@alanmcanonical
Copy link
Contributor Author

alanmcanonical commented Feb 12, 2025

ansible.builtin.(shell|command) is /bin/sh under the hood and it's soft link to dash in Ubuntu which doesn't support process substitution syntax. Step to prove:
Run cat <(ls) on clean Ubuntu on /bin/sh, the result is:

/bin/sh: 1: Syntax error: "(" unexpected

Copy link

codeclimate bot commented Feb 12, 2025

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

View more on Code Climate.

@dodys
Copy link
Contributor

dodys commented Feb 13, 2025

@Mab879 @vojtapolasek could you take a look on this PR as it will also affect you, specifically the ansible.

Copy link
Contributor

@dodys dodys left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

@dodys dodys merged commit 752795a into ComplianceAsCode:master Feb 14, 2025
101 of 102 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CIS CIS Benchmark related. needs-ok-to-test Used by openshift-ci bot. Ubuntu Ubuntu product related.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants