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

Update Ubuntu STIG-20-010072 and fix faillock rules #11355

Merged
merged 9 commits into from
Dec 7, 2023

Conversation

mpurg
Copy link
Contributor

@mpurg mpurg commented Dec 6, 2023

Description:

  1. Rewrite of PR Fix rule ubtu 20 010072 #11074
  2. Add missing platform stanzas to existing tests and add Ubuntu-specific tests
  3. Fix logic in Ubuntu OVALs in accounts_passwords_pam_faillock_...
  4. Refactor Ubuntu OVALs in accounts_passwords_pam_faillock_... to use jinja parameters for parameter name, regex, etc.
  5. Update Ubuntu STIG-20-010072 to use pam_faillock instead of pam_tally2

Rationale:

  1. Rewritten primarily to keep Ubuntu OVALs separate from shared OVALs for sake of readability
  2. Existing tests are not applicable to Ubuntu thus need to be restricted to other platforms
  3. The OVAL check passed when the parameter was not defined in either pam.d or faillock.conf
  4. This makes the OVAL easily reusable across different rules. TODO: switch to template.
  5. Part of STIG update v1r1 -> v1r9

Rules affected:
- accounts_passwords_pam_faillock_unlock_time
- accounts_passwords_pam_faillock_deny
- accounts_passwords_pam_faillock_interval
The ubuntu OVAL was incorrectly checking for the condition:
- unlock_time not present in pam.d/common-auth
OR
- unlock_time present in /etc/security/faillock.conf

This means that the OVAL passed when unlock_time was not
present in either of them.

The fix is analogous to the implementation in shared.xml,
where the OVAL checks for presence of unlock_time in either
one of pam.d/common-auth or faillock.conf, but not both.

The fix results in passing test ubuntu_empty_faillock_conf.fail.sh
Abstract out the parameter name and regex into Jinja variables
to make the OVAL more generic.

Reason is that an almost identical OVAL is used in several rules,
with the main difference being the parameter name and regex.

The change allows for better comparibility/transferability across
those rules, although ideally they should be using a single template.
Analogous to changes in rule faillock_unlock_time
@mpurg mpurg requested a review from a team as a code owner December 6, 2023 14:37
@openshift-ci openshift-ci bot added the needs-ok-to-test Used by openshift-ci bot. label Dec 6, 2023
Copy link

openshift-ci bot commented Dec 6, 2023

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 /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.

@dodys dodys self-assigned this Dec 6, 2023
@dodys dodys added Ubuntu Ubuntu product related. ok-to-test Used by openshift-ci bot. STIG STIG Benchmark related. and removed needs-ok-to-test Used by openshift-ci bot. labels Dec 6, 2023
Copy link

github-actions bot commented Dec 6, 2023

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 Dec 6, 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_accounts_passwords_pam_faillock_silent' differs.
--- xccdf_org.ssgproject.content_rule_accounts_passwords_pam_faillock_silent
+++ xccdf_org.ssgproject.content_rule_accounts_passwords_pam_faillock_silent
@@ -28,6 +28,7 @@
 
 fi
 
+
 AUTH_FILES=("/etc/pam.d/system-auth" "/etc/pam.d/password-auth")
 FAILLOCK_CONF="/etc/security/faillock.conf"
 if [ -f $FAILLOCK_CONF ]; then

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.

overall lgtm, I think there's just these two small changes needed

@@ -1,3 +1,4 @@
# platform = multi_platform_rhel,multi_platform_fedora,multi_platform_ol,multi_platform_rhv,multi_platform_sle
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we don't need to add this here, since you added to the tests and you source this file from the tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch, don't know how that got there :)

@@ -1,3 +1,4 @@
# platform = multi_platform_rhel,multi_platform_fedora,multi_platform_ol,multi_platform_rhv,multi_platform_sle
Copy link
Contributor

Choose a reason for hiding this comment

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

same as the other, I think you won't need to add platform here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will remove

@dodys dodys mentioned this pull request Dec 7, 2023
Copy link

codeclimate bot commented Dec 7, 2023

Code Climate has analyzed commit dc98f2b 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.5%.

View more on Code Climate.

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!

@dodys dodys merged commit bfe4953 into ComplianceAsCode:master Dec 7, 2023
36 of 37 checks passed
@Mab879 Mab879 added this to the 0.1.72 milestone Dec 7, 2023
@Mab879 Mab879 added the Update Rule Issues or pull requests related to Rules updates. label Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test Used by openshift-ci bot. STIG STIG Benchmark related. Ubuntu Ubuntu product related. Update Rule Issues or pull requests related to Rules updates.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants