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

Add Ansible remediation to sssd_enable_pam_services #11796

Merged
merged 9 commits into from
Apr 15, 2024

Conversation

jan-cerny
Copy link
Collaborator

This commit adds an Ansible remediation to rule sssd_enable_pam_services.

Fixes: #11753

This commit adds an Ansible remediation to rule sssd_enable_pam_services.

Fixes: ComplianceAsCode#11753
@jan-cerny jan-cerny added bugfix Fixes to reported bugs. Ansible Ansible remediation update. labels Apr 5, 2024
@jan-cerny jan-cerny added this to the 0.1.73 milestone Apr 5, 2024
Copy link

github-actions bot commented Apr 5, 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

Copy link

github-actions bot commented Apr 5, 2024

This datastream diff is auto generated by the check Compare DS/Generate Diff

Click here to see the full diff
New data stream adds ansible remediation for rule 'xccdf_org.ssgproject.content_rule_sssd_enable_pam_services'.

Copy link

github-actions bot commented Apr 5, 2024

🤖 A k8s content image for this PR is available at:
ghcr.io/complianceascode/k8scontent:11796
This image was built from commit: 1f7b478

Click here to see how to deploy it

If you alread have Compliance Operator deployed:
utils/build_ds_container.py -i ghcr.io/complianceascode/k8scontent:11796

Otherwise deploy the content and operator together by checking out ComplianceAsCode/compliance-operator and:
CONTENT_IMAGE=ghcr.io/complianceascode/k8scontent:11796 make deploy-local

@jan-cerny
Copy link
Collaborator Author

/packit retest-failed

@marcusburghardt marcusburghardt self-assigned this Apr 9, 2024
Copy link
Member

@marcusburghardt marcusburghardt left a comment

Choose a reason for hiding this comment

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

Besides my comments in the Ansible remediation, I also saw some test scenarios confusing in this rule. For example, variables not used (services_pam_missing.fail.sh). I missed a pass test scenario using drop-in files in /etc/sssd/conf.d/. I know the test scenarios are not part of this PR but it could be valid to take a look on them.

Imrpove the regular expression so that it won't match entries
containing the pam entry. This would lead to duplication
of pam entries if the Playbook is executed twice or multiple times.
This test scenario tests a pass situation if the conf.d directory
is used for configuration.
@jan-cerny
Copy link
Collaborator Author

I have add rule titles, changed the regexes and I have add the test scenario.

@jan-cerny
Copy link
Collaborator Author

/packit retest-failed

Resolves this fail on CentOS 7:

34/44 Test #34: ansible-playbook-syntax-check-rhel7 ..............................***Failed    2.69 sec
[WARNING]: provided hosts list is empty, only localhost is available. Note that
the implicit localhost does not match 'all'
ERROR! couldn't resolve module/action 'community.general.ini_file'. This often indicates a misspelling, missing collection, or incorrect module path.
jan-cerny added a commit to jan-cerny/contest that referenced this pull request Apr 12, 2024
The problem will be fixed by PR ComplianceAsCode/content#11796
which will add an Ansible remediation for sssd_enable_pam_services.
@jan-cerny
Copy link
Collaborator Author

I have created PR RHSecurityCompliance/contest#148 in contest project.

path: /etc/sssd/sssd.conf
section: sssd
option: services
value: pam
Copy link
Member

Choose a reason for hiding this comment

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

There is an issue with this last task. It is here to ensure a case where the services line is not present in [sssd]. However in most cases, if not all cases, the services is already there and commonly would include more services instead of only pam.
For example, if the line is:
services = nss,pam
This last task will remove nss and leave the line as services = pam. This is not desired.

I believe the way to solve this is creating another task in check mode only to test if there is already a line with services = * and use the result in this last task.

Copy link
Member

Choose a reason for hiding this comment

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

If a services key exists, and contains a compliant line in sssd.conf
which also contains other services, eg. `services = nss,pam`
we shouldn't remove the other services but we should keep them.
@jan-cerny
Copy link
Collaborator Author

I have changed the Ansible code to prevent removing existing entries

@marcusburghardt
Copy link
Member

I have changed the Ansible code to prevent removing existing entries

@jan-cerny , unfortunately now it is not creating the line if it is missing in [sssd] but there is a service line in any other section. : (

@marcusburghardt
Copy link
Member

I have changed the Ansible code to prevent removing existing entries

@jan-cerny , unfortunately now it is not creating the line if it is missing in [sssd] but there is a service line in any other section. : (

It seems I can't commit in this PR, but here is an update that should resolve the issue:
7c5040c

The task was updated to ensure that last task is only executed when
there isn't already any definition of "services" in sssd section. Only
this case a new line will be included. This is to avoid removing
existing options from existing configuration.
@jan-cerny
Copy link
Collaborator Author

I have added that commit to this PR.

Copy link

codeclimate bot commented Apr 15, 2024

Code Climate has analyzed commit 1f7b478 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 59.2% (0.0% change).

View more on Code Climate.

Copy link
Member

@marcusburghardt marcusburghardt left a comment

Choose a reason for hiding this comment

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

Thanks @jan-cerny . I tested the changes also in local VMs and everything is working as expected. I am just waiting the CI tests to finish.

@jan-cerny
Copy link
Collaborator Author

/packit retest-failed

@jan-cerny jan-cerny merged commit a16d080 into ComplianceAsCode:master Apr 15, 2024
44 of 45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ansible Ansible remediation update. bugfix Fixes to reported bugs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sssd_enable_pam_services lacks Ansible remediation
2 participants