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

Skip users with ID above UID MAX on accounts_user_interactive_home_directory_defined #12527

Conversation

Mab879
Copy link
Member

@Mab879 Mab879 commented Oct 22, 2024

Description:

To skip systemd dynamic users.
Since accounts_user_interactive_home_directory_defined only works on local users this should be fine.

Since bash remediation accesses /etc/passwd directly and the systemd dynamic users do not show up in that file, the bash remediation was not updated.

Rationale:

Fix Ansible playbook failures.

@Mab879 Mab879 added the Ansible Ansible remediation update. label Oct 22, 2024
@Mab879 Mab879 added this to the 0.1.75 milestone Oct 22, 2024
Copy link

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 Oct 22, 2024

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_user_interactive_home_directory_defined' differs.
--- xccdf_org.ssgproject.content_rule_accounts_user_interactive_home_directory_defined
+++ xccdf_org.ssgproject.content_rule_accounts_user_interactive_home_directory_defined
@@ -34,6 +34,7 @@
   when:
   - item.value[2]|int >= 1000
   - item.value[2]|int != 65534
+  - item.value[2]|int < 61184 or item.value[2]|int > 65519
   - not item.value[4] | regex_search('^\/\w*\/\w{1,}')
   tags:
   - CCE-84036-3

@Mab879 Mab879 requested review from a team as code owners October 22, 2024 21:42
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!
Ubuntu CI failures is because this rule is not present in our benchmarks

@vojtapolasek vojtapolasek self-assigned this Oct 25, 2024
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.

Looks good. See my specific comment.
out of curiosity, why in the past it did not matter that test files for some distros in tests/data/product_stability do not exist? It makes this PR quite big.

@@ -5,3 +5,4 @@ default:
nobody_gid: 65534
nobody_uid: 65534
auid: 1000
uid_max: 60000
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe the number can be this one, per http://0pointer.net/blog/dynamic-users-with-systemd.html, you can see it few lines under the heading called "Introducing dynamic users".

Suggested change
uid_max: 60000
uid_max: 60183

@Mab879 Mab879 force-pushed the fix_systemd_dyn_users_interactive_home branch from 651fc7e to d8f7855 Compare October 25, 2024 16:30
Copy link
Contributor

@Xeicker Xeicker left a comment

Choose a reason for hiding this comment

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

changes LGTM

@Mab879 Mab879 marked this pull request as draft October 25, 2024 20:59
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Used by openshift-ci bot. label Oct 25, 2024
@Mab879 Mab879 marked this pull request as ready for review October 29, 2024 18:28
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Used by openshift-ci bot. label Oct 29, 2024
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.

Nice catch with the whole dynamic uid range. But I think this needs to be fixed.

@Mab879
Copy link
Member Author

Mab879 commented Nov 1, 2024

/packit build

@Mab879 Mab879 force-pushed the fix_systemd_dyn_users_interactive_home branch from d7e008e to 708fc41 Compare November 4, 2024 13:10
Copy link

codeclimate bot commented Nov 4, 2024

Code Climate has analyzed commit 708fc41 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 60.9% (0.0% change).

View more on Code Climate.

Copy link
Contributor

@teacup-on-rockingchair teacup-on-rockingchair left a comment

Choose a reason for hiding this comment

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

👍

@vojtapolasek
Copy link
Collaborator

@Mab879 There is a failing Automatus test on SLE which seems valid.

@Mab879
Copy link
Member Author

Mab879 commented Nov 4, 2024

@Mab879 There is a failing Automatus test on SLE which seems valid.

This isn't a new issue, it is also failing on master.

At least testing with OpenSUSE.

@vojtapolasek
Copy link
Collaborator

OK @Mab879 merging. Thank you.

@vojtapolasek
Copy link
Collaborator

/packit retest-failed

@vojtapolasek vojtapolasek merged commit 06cc241 into ComplianceAsCode:master Nov 5, 2024
101 of 105 checks passed
@Mab879 Mab879 deleted the fix_systemd_dyn_users_interactive_home branch November 5, 2024 15:10
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.

6 participants