-
Notifications
You must be signed in to change notification settings - Fork 685
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
Fix macro for extracting local interactive users #11589
Fix macro for extracting local interactive users #11589
Conversation
This macro is used to extract specific fields from /etc/passwd. Only local interactive users are considered by excluding those with shell /sbin/nologin. This fix excludes also users with following shells: - /bin/false - /usr/bin/false - /usr/sbin/nologin
Hi @mpurg. 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 Once the patch is verified, the new status will be reflected by the 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. |
e7c1d8e
to
b5d7f08
Compare
b5d7f08
to
89951fa
Compare
@jan-cerny since you wrote most of this code, would take a look at it? |
/packit retest-failed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good to me.
I have successfully run some of the tests:
jcerny@fedora:~/work/git/scap-security-guide (pr/11589)$ python3 tests/automatus.py rule --libvirt qemu:///system ssgts_rhel8 accounts_users_home_files_permissions
Setting console output to log level INFO
INFO - The base image option has not been specified, choosing libvirt-based test environment.
INFO - Logging into /home/jcerny/work/git/scap-security-guide/logs/rule-custom-2024-02-16-0924/test_suite.log
INFO - xccdf_org.ssgproject.content_rule_accounts_users_home_files_permissions
INFO - Script acceptable_permission.pass.sh using profile (all) OK
INFO - Script expected_permissions.pass.sh using profile (all) OK
INFO - Script home_dirs_absent.pass.sh using profile (all) OK
INFO - Script interactive_users_absent.pass.sh using profile (all) OK
INFO - Script lenient_permission.fail.sh using profile (all) OK
INFO - Script lenient_permission_hidden_files.pass.sh using profile (all) OK
INFO - Script lenient_permissions_directory.fail.sh using profile (all) OK
INFO - Script interactive_user_nologin_ignored.pass.sh using profile (all) OK
jcerny@fedora:~/work/git/scap-security-guide (pr/11589)$ python3 tests/automatus.py rule --libvirt qemu:///system ssgts_rhel8 accounts_user_dot_group_ownership
Setting console output to log level INFO
INFO - The base image option has not been specified, choosing libvirt-based test environment.
INFO - Logging into /home/jcerny/work/git/scap-security-guide/logs/rule-custom-2024-02-16-0930/test_suite.log
INFO - xccdf_org.ssgproject.content_rule_accounts_user_dot_group_ownership
INFO - Script expected_groupowner.pass.sh using profile (all) OK
INFO - Script home_dirs_all_absent.pass.sh using profile (all) OK
INFO - Script home_dirs_one_absent_group_ok.pass.sh using profile (all) OK
INFO - Script interactive_users_absent.pass.sh using profile (all) OK
INFO - Script no_dot_file_ignored.pass.sh using profile (all) OK
INFO - Script unexpected_groupowner_system_gid.fail.sh using profile (all) OK
INFO - Script unexpected_groupowner_unknown_gid.fail.sh using profile (all) OK
INFO - Script warning_swapped_groupowners.pass.sh using profile (all) OK
INFO - Script interactive_user_nologin_ignored.pass.sh using profile (all) OK
/packit retest-failed |
@teacup-on-rockingchair could you check the failing test results? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, maybe you can use the occasion to minimize the duplication as much as possible, say doing something like :
#!/bin/sh
per_user_shell_procedure() {
procedure=$*
for shell in "/sbin/nologin" \
"/usr/sbin/nologin" \
"/bin/false" \
"/usr/bin/false"; do
user=cac_user${shell//\//_}
useradd -m -s $shell $user
eval "${procedure}"
done
}
in a shared file and use:
per_user_shell_procedure "do_stuff \$user"
The problem with the failing SLE tests for 'file_groupownership_home_directories', 'accounts_users_home_files_groupownership' is the fact that The problem with |
Hi @teacup-on-rockingchair , thanks for the suggestion, I agree that there's too much duplicated code here. Where do you think would be the best place to store this code so that it could be reused by tests in different rules? One thing I should also point out is that not all the tests use |
Best practice as far as I saw in the project is to have those as sahred file in the common tests directory and source that. I am not sure, but I guess you can test that also to make it a jinja macro, the ones we use for bash remediations, but I am not sure if it will work. I also notice the useradd -m vs useradd -M, but I guess for the sake of using common code you can have a |
89951fa
to
535b6fa
Compare
936b025
to
ff8ec64
Compare
🤖 A k8s content image for this PR is available at: Click here to see how to deploy itIf you alread have Compliance Operator deployed: Otherwise deploy the content and operator together by checking out ComplianceAsCode/compliance-operator and: |
@teacup-on-rockingchair please check out the proposed solution |
/packit retest-failed |
LGTM 🙇 |
…ers_object The solution deduplicates most of the code to test/shared/accounts_common.sh, as proposed in PR review.
ff8ec64
to
c2d3784
Compare
Code Climate has analyzed commit c2d3784 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 58.1% (0.0% change). View more on Code Climate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thanks!
Description:
Fix regex used in OVAL macro
create_local_interactive_users_object
to include other shell paths.Rationale:
This macro is used to extract specific fields from /etc/passwd.
Only local interactive users are considered, excluding those with shell /sbin/nologin.
This fix excludes also users with following shells:
Additional information:
This macro is used in the following rules:
I adapted the test for checking ignored shells accordingly.