Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Check sssd conf.d files and fix bash remediation for sssd_enable_pam_services #6014

Merged

Conversation

ggbecker
Copy link
Member

@ggbecker ggbecker commented Aug 19, 2020

Description:

Rule sssd_enable_pam_services:

Rationale:

Help needed:

- Since /etc/sssd/conf.d/ can be empty, we need a way to skip the check in this case. Using existence=any_exist does not help because if a given file exists and it doesn't comply with the requirement it will evaluate as True.
- Since we are using this multi line regex, I didn't find a way to combine this with sed to use in the remediation. Right now it will look for the line services = and append pam when needed.
- Maybe we don't need to be strict in many ways so we could drop the check of /etc/sssd/conf.d/, but the remediation fix part is definitely something that needs to be fixed because the it is right now, it will always append services = pam to the file.

Note: to test this it is needed to have sssd installed. This is something to be added to test scenarios yet.

@ggbecker ggbecker added this to the 0.1.52 milestone Aug 19, 2020
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Used by openshift-ci bot. label Aug 19, 2020
@openshift-ci-robot
Copy link
Collaborator

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@jan-cerny
Copy link
Collaborator

Since /etc/sssd/conf.d/ can be empty, we need a way to skip the check in this case. Using existence=any_exist does not help because if a given file exists and it doesn't comply with the requirement it will evaluate as True.

A single regex which describes all possible locations together with check_existence="at_least_one_exists". Can that work?

@ggbecker
Copy link
Member Author

Since /etc/sssd/conf.d/ can be empty, we need a way to skip the check in this case. Using existence=any_exist does not help because if a given file exists and it doesn't comply with the requirement it will evaluate as True.

A single regex which describes all possible locations together with check_existence="at_least_one_exists". Can that work?

That might improve the situation, but I'm not sure if it would entirely solve it. If I understood correctly, if there is no services = something at all, the check should pass (according to STIG), so in this case it would be check_existence="any_exist", but then if it happens that there is a services = nss (without pam) line in one file for example, it would pass even if it doesn't comply with the check because of the any_exist, is that a correct assumption?

I think we should split the check in two then and make one to check if there is any services = something line evaluating to true if no entry is found and the other one checking for the actual values of services = something. And combine then with OR operation.

Copy link
Member

@yuumasato yuumasato left a comment

Choose a reason for hiding this comment

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

* Since `/etc/sssd/conf.d/` can be empty, we need a way to skip the check in this case. Using `existence=any_exist` does not help because if a given file exists and it doesn't comply with the requirement it will evaluate as `True`.

I think the problem is that the test is already looking for the object with the desired state. If the test has an explicit state it can work.

For example, if the object is the services line, and we capture the services list:
<ind:pattern operation="pattern match">^[\s]*\[sssd]([^\n\[\]]*\n+)+?[\s]*services[\s]*=[\s]*(.*)$</ind:pattern>
And then state makes sure that pam is one of the captured services:

    <ind:subexpression operation="pattern_match">.*pam.*</ind:subexpression>
  </ind:textfilecontent54_state>

Edit: added explanation below.

This would work with check_existence="any_exist", any number of services = line can be found.
And with check="all", all objects need to conform to the state, i.e. any services = line found need to contain pam.

@ggbecker
Copy link
Member Author

<ind:subexpression operation="pattern_match">.pam.</ind:subexpression>

Thanks for the thoroughly explanation, I'll run some tests based on feedback and then I'll submit new changes.

@ggbecker ggbecker force-pushed the update-stig-RHEL-07-041002 branch from 004b320 to e4f85f1 Compare August 20, 2020 14:19
@ggbecker
Copy link
Member Author

I have pushed new changes but the OVAL is not working properly, I can't make the check pass, even with sssd.conf containing correct content. @yuumasato can you take a look at the OVAL check if you can find any discrepancies? Thank you.

@ggbecker ggbecker force-pushed the update-stig-RHEL-07-041002 branch from e4f85f1 to 169e26c Compare August 24, 2020 16:01
@mildas
Copy link
Contributor

mildas commented Aug 24, 2020

Changes identified:
Rule sssd_enable_pam_services:
 Found change in bash remediation.
 New node inserted to OVAL check.
 Node moved within OVAL check.
 Attribute value changed in OVAL check.
 Text changed in OVAL check.
 Node deleted from OVAL check.
 Text added outsite tags in OVAL check.

Recommended tests to execute:
 build_product ol7
 tests/test_suite.py rule --libvirt qemu:///system test-suite-vm --remediate-using bash --datastream build/ssg-ol7-ds.xml sssd_enable_pam_services

@ggbecker ggbecker marked this pull request as ready for review August 24, 2020 16:03
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Used by openshift-ci bot. label Aug 24, 2020
@ggbecker ggbecker requested a review from yuumasato August 24, 2020 16:08
@ggbecker
Copy link
Member Author

/retest

@openshift-ci-robot
Copy link
Collaborator

@ggbecker: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-aws-rhcos4-moderate 169e26c link /test e2e-aws-rhcos4-moderate

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@yuumasato yuumasato self-assigned this Aug 25, 2020
Copy link
Member

@yuumasato yuumasato left a comment

Choose a reason for hiding this comment

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

Nice work.

@yuumasato yuumasato merged commit 326cb98 into ComplianceAsCode:master Aug 26, 2020
@marcusburghardt marcusburghardt added RHEL7 Red Hat Enterprise Linux 7 product related. STIG STIG Benchmark related. labels Jun 23, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
RHEL7 Red Hat Enterprise Linux 7 product related. STIG STIG Benchmark related.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants