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

Improve template pam_account_password_faillock #12687

Merged
merged 5 commits into from
Jan 6, 2025

Conversation

mpurg
Copy link
Contributor

@mpurg mpurg commented Dec 9, 2024

Description:

  • Improve template pam_account_password_faillock
  • Add documentation for template
  • Define requirements for variables in template.py:
    • ext_variable must be defined since it is used in the remediation
    • bounding variables must be 'use_ext_variable', (int), or undefined (if undefined, bounding variables are initialized to None)
  • Modify OVAL logic:
    • fix conditionals to consistently use inclusive comparisons instead of inclusive for ext_variable, and exclusive for numbers (this requires changing the lower boundary for _deny rule from 0 to 1)
    • remove conditionals which compare to var_ref="{{{ VARIABLE_*_BOUND}}}" as these variables don't exist in the OVAL
    • modify check for undefined variable to compare to jinja test none
  • Fix tests
    • tests were generalized and are no longer specific to _deny rule
    • tests check the different logic flows defined by template parameters and external variables
    • a new macro was created to initialize the variables
    • tests from the rules that use the template were removed
    • fix for missing profile "minimal" in fedora
  • Fix platform in ansible remediation to match remediations in original rules

@openshift-ci openshift-ci bot added the needs-ok-to-test Used by openshift-ci bot. label Dec 9, 2024
Copy link

openshift-ci bot commented Dec 9, 2024

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-sigs/prow repository.

Copy link

github-actions bot commented Dec 9, 2024

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

@mpurg mpurg marked this pull request as draft December 9, 2024 12:37
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Used by openshift-ci bot. label Dec 9, 2024
@mpurg mpurg force-pushed the faillock_template_fix branch from a058c3d to e1ea42b Compare December 9, 2024 12:37
@mpurg
Copy link
Contributor Author

mpurg commented Dec 9, 2024

@Mab879 @jan-cerny I see that one fedora test (conflicting_settings_authselect.fail.sh) is failing with

+ authselect create-profile testingProfile --base-on minimal
[error] Unable to read base profile [minimal] [2]: No such file or directory
[error] Unable to create profile [2]: No such file or directory
Unable to create new profile [2]: No such file or directory
+ CUSTOM_PROFILE_DIR=/etc/authselect/custom/testingProfile
+ authselect select --force custom/testingProfile
[error] Unable to get profile information [2]: No such file or directory

Should the profile be based on local or sssd maybe?

Update: I added a fallback command based on the local profile which seems to fix the issue: 2507ad8

Update2: The ansible remediations in fedora automatus tests are failing because of missing python3-rpm

@dodys dodys requested a review from a team December 10, 2024 10:49
@dodys dodys self-assigned this Dec 10, 2024
@dodys dodys added this to the 0.1.76 milestone Dec 10, 2024
Added template to docs.

Defined requirements for variables in template.py:
- ext_variable must be defined since it is used in the remediation
- bounding variables must be 'use_ext_variable', (int), or undefined
  (if undefined, bounding variables are initialized to None)

Cleaned up the OVAL:
- fix conditionals to consistently use inclusive comparisons instead of
  inclusive for ext_variable, and exclusive for numbers
- remove conditionals which compare to `var_ref="{{{ VARIABLE_*_BOUND}}}"`
  as these variables don't exist in the OVAL
- modify check for undefined variable to compare to jinja test none
Fixed to work with new OVAL logic in template (inclusive comparison).
- tests were generalized and are no longer specific to `_deny` rule
- tests check the different logic flows defined by template parameters
  and external variables
- a new macro was created to initialize the variables
- tests from the rules that use the template were removed
@mpurg mpurg force-pushed the faillock_template_fix branch from e1ea42b to 2507ad8 Compare December 11, 2024 11:14
The ansible remediation in `pam_account_password_faillock` was
fixed to be applicable on same platforms as the remediations in original rules
(accounts_passwords_pam_faillock_deny/unlock_time/interval).
@mpurg mpurg marked this pull request as ready for review December 11, 2024 20:13
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Used by openshift-ci bot. label Dec 11, 2024
Copy link

codeclimate bot commented Dec 11, 2024

Code Climate has analyzed commit 8055c39 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 60.9% (0.0% change).

View more on Code Climate.

@dodys dodys requested review from a team December 16, 2024 09:23
@dodys dodys added the Update Template Issues or pull requests related to Templates updates. label Dec 16, 2024
Copy link
Contributor

@Xeicker Xeicker left a comment

Choose a reason for hiding this comment

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

LGTM

@dodys
Copy link
Contributor

dodys commented Dec 19, 2024

@Mab879 could you please take a look on this one?

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.

Thanks!

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 d82db2b into ComplianceAsCode:master Jan 6, 2025
94 of 98 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ok-to-test Used by openshift-ci bot. Update Template Issues or pull requests related to Templates updates.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants