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

Updates of the rule use_pam_wheel_group_for_su #10714

Conversation

rumch-se
Copy link
Contributor

Description:

  • Added ansible part to the rule extend_use_pam_wheel_group_for_su and added logic for a creation of an empty group

Rationale:

  • According to the requirement /etc/pam.d/su has to include a line auth required pam_wheel.so use_uid group=sugroup, where sugroup is empty. The new changes fix this and add ansible part.

@rumch-se rumch-se requested a review from a team as a code owner June 14, 2023 08:49
@openshift-ci openshift-ci bot added the needs-ok-to-test Used by openshift-ci bot. label Jun 14, 2023
@openshift-ci
Copy link

openshift-ci bot commented Jun 14, 2023

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
Copy link

Start a new ephemeral environment with changes proposed in this pull request:

sle12 (from CTF) Environment (using Fedora as testing environment)
Open in Gitpod

Fedora Testing Environment
Open in Gitpod

Oracle Linux 8 Environment
Open in Gitpod

@Mab879 Mab879 added Bash Bash remediation update. SLES SUSE Linux Enterprise Server product related. CIS CIS Benchmark related. labels Jun 15, 2023
@openshift-merge-robot openshift-merge-robot added the needs-rebase Used by openshift-ci bot. label Jun 16, 2023
@rumch-se rumch-se force-pushed the extend_use_pam_wheel_group_for_su branch from fc3b154 to 90378e1 Compare June 19, 2023 07:36
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Used by openshift-ci bot. label Jun 19, 2023
@marcusburghardt marcusburghardt requested a review from a team June 21, 2023 09:36
@marcusburghardt marcusburghardt added the Ubuntu Ubuntu product related. label Jun 21, 2023
@@ -1,6 +1,6 @@
documentation_complete: true

prodtype: ubuntu2004,ubuntu2204
prodtype: ubuntu2004,ubuntu2204,sle12,sle15
Copy link
Collaborator

Choose a reason for hiding this comment

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

The prodtype isn't sorted which triggers the CI test.

@openshift-merge-robot openshift-merge-robot added needs-rebase Used by openshift-ci bot. and removed needs-rebase Used by openshift-ci bot. labels Jun 23, 2023
@jan-cerny
Copy link
Collaborator

@rumch-se Please remove the "Merge" commit from this PR.

@rumch-se
Copy link
Contributor Author

Hello @teacup-on-rockingchair
May you remove "Merge" commit from this PR, because there is a request for that.

Have a nice day
Rumen

@jan-cerny
Copy link
Collaborator

Why would @teacup-on-rockingchair do that? This PR is from a branch in @rumch-se 's fork.

@rumch-se
Copy link
Contributor Author

Hello @jan-cerny,

There are 4 commits (3 of them done by me and the last one done by @teacup-on-rockingchair). The last one has "Merge" in its description. Are you talking about the last commit, aren't you?

Have a nice day
Rumen

@jan-cerny
Copy link
Collaborator

@rumch-se Yes, exactly! I'm talking about fca182e, please remove it from your PR. Pull request that contain merge commits can't be accepted, they would mess the git history.

@rumch-se rumch-se force-pushed the extend_use_pam_wheel_group_for_su branch from fca182e to c2cb156 Compare June 27, 2023 07:40
@openshift-merge-robot openshift-merge-robot added the needs-rebase Used by openshift-ci bot. label Jun 27, 2023
@rumch-se rumch-se force-pushed the extend_use_pam_wheel_group_for_su branch from c2cb156 to b3e29ad Compare June 27, 2023 07:47
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Used by openshift-ci bot. label Jun 27, 2023
@rumch-se
Copy link
Contributor Author

Hello @jan-cerny and @teacup-on-rockingchair
I have removed the last commit.

Have a nice day
Rumen

Comment on lines 6 to 12
if [ $(getent group ${var_pam_wheel_group_for_su}) ]; then
# group exists
groupdel -f ${var_pam_wheel_group_for_su}
fi
groupadd -f ${var_pam_wheel_group_for_su}


Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this handled by ensure_pam_wheel_group_empty already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @wokis

1/The rule https://github.com/ComplianceAsCode/content/tree/master/linux_os/guide/system/accounts/accounts-restrictions/root_logins/ensure_pam_wheel_group_empty - does not have ansible part.
According to this PR I am covering this part.

2/The rule is a part of ubuntu profile, and I am contribute for SLE 12/15.

Have a nice day
Rumen

Copy link
Contributor

Choose a reason for hiding this comment

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

Hello @wokis

1/The rule https://github.com/ComplianceAsCode/content/tree/master/linux_os/guide/system/accounts/accounts-restrictions/root_logins/ensure_pam_wheel_group_empty - does not have ansible part. According to this PR I am covering this part.

2/The rule is a part of ubuntu profile, and I am contribute for SLE 12/15.

As wokis mentioned, you should use the rule ensure_pam_wheel_group_empty together with use_pam_wheel_group_for_su otherwise you won't be checking through oval if the group is empty and the things you are adding to bash and ansible won't be needed here.

Comment on lines 9 to 17
- name: {{{ rule_title }}} - Ensure group {{ var_pam_wheel_group_for_su }} is removed
group:
name: "{{ var_pam_wheel_group_for_su }}"
state: absent

- name: {{{ rule_title }}} - Ensure group {{ var_pam_wheel_group_for_su }} exist
group:
name: "{{ var_pam_wheel_group_for_su }}"
state: present
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this handled by ensure_pam_wheel_group_empty already?

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.

use ensure_pam_wheel_group_empty together with use_pam_wheel_group_for_su

@rumch-se
Copy link
Contributor Author

Hello @dodys
I have modified both rules use_pam_wheel_group_for_su and ensure_pam_wheel_group_empty according to your recommendations.

Have a nice day
Rumen

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 for addressing our comments!
I will leave to @teacup-on-rockingchair to do the final review and merge it.

@codeclimate
Copy link

codeclimate bot commented Jun 29, 2023

Code Climate has analyzed commit a6b6843 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 53.4% (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.

LGTM

@teacup-on-rockingchair teacup-on-rockingchair merged commit 90f35be into ComplianceAsCode:master Jul 5, 2023
30 of 33 checks passed
@Mab879 Mab879 added this to the 0.1.69 milestone Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bash Bash remediation update. CIS CIS Benchmark related. needs-ok-to-test Used by openshift-ci bot. SLES SUSE Linux Enterprise Server product related. Ubuntu Ubuntu product related.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants