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

auth-select options variable not used #156

Conversation

ipruteanu-sie
Copy link
Contributor

Overall Review of Changes:
Other OS flavors have required extra-options for authselect profiles.
Currently, it seems this is not needed.

Issue Fixes:
#155

Enhancements:
Please list any enhancements/features that are not open issue tickets

How has this been tested?:
N/A

bgro and others added 19 commits October 12, 2023 12:56
…l which governs if extra params will be configured

Signed-off-by: Ionut Pruteanu <[email protected]>
Correction to "when":  1_3_3

Signed-off-by: Joachim la Poutré <[email protected]>
Corrected tag rule_1.8.10

Signed-off-by: Joachim la Poutré <[email protected]>
Corrected tag: rule_5.6.1.1

Signed-off-by: Joachim la Poutré <[email protected]>
Corrected tag: rule_5.6.1.5

Signed-off-by: Joachim la Poutré <[email protected]>
Corrected tags: rule_6.1.8 & rule_6.1.12

Signed-off-by: Joachim la Poutré <[email protected]>
Corrected tag: rule_6.2.3

Signed-off-by: Joachim la Poutré <[email protected]>
Signed-off-by: Joshua Hemmings <[email protected]>
…Therefore var-attr is not needed anymore.

Signed-off-by: Ionut Pruteanu <[email protected]>
…erse_path_filtering_3_3_7

Adding missing lines to usr: sysctl.d/50-default.conf
updates:
- [github.com/ansible-community/ansible-lint: v6.22.1 → v6.22.2](ansible/ansible-lint@v6.22.1...v6.22.2)
…ure_default_umask_027_5_6_5

Adding new entry in /etc/pam.d/system-auth
…itVarsRefactoring

Siemens/feat/audit vars refactoring
Remove trailing comma to align with other roles
@uk-bolly
Copy link
Member

hi @ipruteanu-sie

We are seeing errors in a merge conflict for this PR. If you can resync to resolve this happy to look at this further.

Many thanks

uk-bolly

@uk-bolly uk-bolly self-assigned this Jan 26, 2024
uk-bolly and others added 4 commits January 26, 2024 12:50
…mit-ci-update-config

[pre-commit.ci] pre-commit autoupdate
…Therefore var-attr is not needed anymore.

Signed-off-by: Ionut Pruteanu <[email protected]>
Use the proper sub-task name when authselect custom profile is selected.

Signed-off-by: Ionut Pruteanu <[email protected]>
…m:infosec-pss-gov/security-crafter-baseline-automations/ansible-lockdown/rhel9-cis into siemens/feat/bUSE_authSelectOptions
@ipruteanu-sie
Copy link
Contributor Author

@uk-bolly:
I fixed the conflict for this rule, but I'm planning to close this PR.
What I noticed in a CIS report:

  1. In a CIS report, one can notice the remediation of 5.4.2 specifies:
# authselect enable-feature with-faillock
# authselect apply-changes

However, current impl here is different.

Even more, there's NO real check from their part on other authselect options(other than with-faillock), even if we're currently using them:

  • with-sudo
  • without-nullok.

The rule name says it all, rule deals with faillock:

5.4.2 Ensure authselect includes with-faillock

  1. There's indeed an example in 5.4.1, about a custom-profile selection, which includes the other two extra-options, so probably the two extra ones won't hurt:
    image

Copy link
Member

Choose a reason for hiding this comment

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

Trying to understand why this is added?
This is already as as per the guidelines to a sysctl.d file in rule 3.3.7

Copy link
Member

Choose a reason for hiding this comment

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

Can see the confusion on this one,
the first method pam has been started but the control ends with writing it the second option.
Shall we just remove the pam method and have the second one via shell methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only purpose of this PR was to highlight that:

  • there are some extra authselect options, not explicitly requested by CIS, present in 5.4.2
  • but encountered within the CIS report, as an impl-example for 5.4.1.
    Considering the extra-options will not hurt, please feel free to close this PR, in case is does not help.

@uk-bolly
Copy link
Member

uk-bolly commented Mar 6, 2024

hi @ipruteanu-sie

I can see we still have a large number of PRs open from you, but they all seem to have a huge number of commits assigned for a change to one or two files and is getting very confusing to read when trying to review what is actually changing. NOt sure why we see this number as it doesn't seem right.
I have incorporated a lot of issues and changes in to the latest devel branch, could i suggest that we close the current ones and work through the issues to see what is left outstanding. Happy to work with you to resolve the rest that is outstanding.

Kindest regards

uk-bolly

@uk-bolly uk-bolly mentioned this pull request Mar 6, 2024
@ipruteanu-sie
Copy link
Contributor Author

Hi @uk-bolly , I think I was incorrectly performing some rebase-commands. Thanks for your message and sorry for the confusion.

For current PR, despite current devel does not include the approach suggested in issue #155, we can put it aside(since CIS is still happy, even if we add the extra-options):
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants