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

Remove groupmems command from ensure_pam_wheel_group_empty rule #11210

Merged
merged 3 commits into from
Oct 20, 2023

Conversation

marcusburghardt
Copy link
Member

@marcusburghardt marcusburghardt commented Oct 18, 2023

Description:

It was noticed that groupmems command, which initially seemed like a handy command, does not work on all distros or behaves differently among distros.
It is better to avoid it and use a more generic approach that works for all distros.

The issue was discovered after #11192

Rationale:

Review Hints:

automatus tests should be enough to test the change.

It was noticed that groupmems command, which initially seemed like a
handy command, does not work on all distros or behaves differently
among distros. It is better to avoid it and use a more generic approach
that works for all distros.
@marcusburghardt marcusburghardt added bugfix Fixes to reported bugs. Bash Bash remediation update. labels Oct 18, 2023
@marcusburghardt marcusburghardt added this to the 0.1.71 milestone Oct 18, 2023
@github-actions
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

@github-actions
Copy link

github-actions bot commented Oct 18, 2023

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_ensure_pam_wheel_group_empty' differs.
--- xccdf_org.ssgproject.content_rule_ensure_pam_wheel_group_empty
+++ xccdf_org.ssgproject.content_rule_ensure_pam_wheel_group_empty
@@ -9,7 +9,7 @@
 fi
 
 # group must be empty
-groupmems -g ${var_pam_wheel_group_for_su} -p
+gpasswd -M '' ${var_pam_wheel_group_for_su}
 
 else
     >&2 echo 'Remediation is not applicable, nothing was done'

@@ -7,4 +7,4 @@ if ! grep -q "^${var_pam_wheel_group_for_su}:[^:]*:[^:]*:[^:]*" /etc/group; then
fi

# group must be empty
groupmems -g ${var_pam_wheel_group_for_su} -p
sed -i -E "s/^(${var_pam_wheel_group_for_su}:[^:]*:[^:]*:)[^:]*/\1/g" /etc/group
Copy link
Contributor

Choose a reason for hiding this comment

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

What is your opinion on using gpasswd instead of sed directly to edit the group file, say:

gpasswd -M '' ${var_pam_wheel_group_for_su}

Copy link
Member Author

Choose a reason for hiding this comment

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

It should work fine for rhel. It is fine for Ubuntu too @dodys ? If so I would prefer to use the command suggested by @teacup-on-rockingchair instead of directly editing the file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @teacup-on-rockingchair @marcusburghardt
I checked and it works fine on Ubuntu 20.04 and 22.04.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great. I will update it now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Thanks @teacup-on-rockingchair and @mpurg

Thanks @teacup-on-rockingchair for suggesting this command.
Also used the command in group_without_members.pass.sh test scenario.
It is intentional to not update the Ansible remediation since it would
demand to use the command module. But the gpasswd command doesn't have
different return codes to know when members were removed or not from the
group. It would make the Ansible task to be reported as changed always
or never, which may cause confusion.
@codeclimate
Copy link

codeclimate bot commented Oct 19, 2023

Code Climate has analyzed commit 6450fa7 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 57.0%.

View more on Code Climate.

@mpurg
Copy link
Contributor

mpurg commented Oct 19, 2023

Thanks for the fix @marcusburghardt and @teacup-on-rockingchair.
I can confirm that profile and rule tests passed for Ubuntu.

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!
and thanks @mpurg for testing it!

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.

Good stuff 🙇

Copy link
Member

@Mab879 Mab879 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.

@Mab879
Copy link
Member

Mab879 commented Oct 20, 2023

The SLES15 failure is not related to this PR, waving.

@Mab879 Mab879 merged commit a7f0de6 into ComplianceAsCode:master Oct 20, 2023
33 of 34 checks passed
@marcusburghardt marcusburghardt deleted the groupmems_alternative branch October 20, 2023 06:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bash Bash remediation update. bugfix Fixes to reported bugs.
Projects
None yet
5 participants