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

Review and fix sssd rules in ANSSI control file #12378

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

marcusburghardt
Copy link
Member

@marcusburghardt marcusburghardt commented Sep 13, 2024

Description:

When reviewing #12351 I noticed some issues with sssd related rules.
This PR:

  • Remove unnecessary steps in sssd_enable_pam_services bash remediation
  • Updates the id_provider value and fix the sssd package name in some test scenarios
  • Simplify and align the test scenarios logic
  • Create applicability to check if SSSD configuration files are present
  • Use the applicability in sssd_enable_pam_services for better reporting

Rationale:

Better test scenarios and remediation for sssd related rules used in ANSSI control file.

Review Hints:

./build_product rhel9
./tests/automatus.py rule --libvirt qemu:///session rhel9 --datastream build/ssg-rhel9-ds.xml --dontclean --remediate-using bash --debug service_sssd_enabled
./tests/automatus.py rule --libvirt qemu:///session rhel9 --datastream build/ssg-rhel9-ds.xml --dontclean --remediate-using bash --debug sssd_enable_pam_services

@marcusburghardt marcusburghardt added bugfix Fixes to reported bugs. Test Suite Update in Test Suite. Bash Bash remediation update. labels Sep 13, 2024
@marcusburghardt marcusburghardt added this to the 0.1.75 milestone Sep 13, 2024
Copy link

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 Sep 13, 2024

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

Click here to see the full diff
New content has different text for rule 'xccdf_org.ssgproject.content_rule_sssd_enable_pam_services'.
--- xccdf_org.ssgproject.content_rule_sssd_enable_pam_services
+++ xccdf_org.ssgproject.content_rule_sssd_enable_pam_services
@@ -9,6 +9,9 @@
 /etc/sssd/sssd.conf. For example:
 [sssd]
 services = sudo, autofs, pam
+
+[warning]:
+This rule will report as "notapplicable" if there is no SSSD configuration file present in the system. The SSSD configuration might be different for each site and therefore a new configuration file is not automatically created.
 
 [reference]:
 1

bash remediation for rule 'xccdf_org.ssgproject.content_rule_sssd_enable_pam_services' differs.
--- xccdf_org.ssgproject.content_rule_sssd_enable_pam_services
+++ xccdf_org.ssgproject.content_rule_sssd_enable_pam_services
@@ -1,53 +1,43 @@
 # Remediation is applicable only in certain platforms
 if rpm --quiet -q sssd-common; then
-
-# sssd configuration files must be created with 600 permissions if they don't exist
-# otherwise the sssd module fails to start
-OLD_UMASK=$(umask)
-umask u=rw,go=
 
 SSSD_CONF="/etc/sssd/sssd.conf"
 SSSD_CONF_DIR="/etc/sssd/conf.d/*.conf"
 
 if [ ! -f "$SSSD_CONF" ] && [ ! -f "$SSSD_CONF_DIR" ]; then
-    mkdir -p /etc/sssd
-    touch "$SSSD_CONF"
-fi
+    echo "
+    sssd configuration files not found. Ensure a valid configuration is present.
+    Manual modification of configuration files may be necessary to align with site policies."
+else
+    # Flag to check if there is already services with pam
+    service_already_exist=false
+    for f in $SSSD_CONF $SSSD_CONF_DIR; do
+        if [ ! -e "$f" ]; then
+            continue
+        fi
+        # finds all services entries under [sssd] configuration category, get a unique list so it doesn't add redundant fix
+        services_list=$( awk '/^\s*\[/{f=0} /^\s*\[sssd\]/{f=1}f' $f | grep -P '^services[ \t]*=' | uniq )
+        if [ -z "$services_list" ]; then
+            continue
+        fi
 
-# Flag to check if there is already services with pam
-service_already_exist=false
-for f in $SSSD_CONF $SSSD_CONF_DIR; do
-	if [ ! -e "$f" ]; then
-		continue
-	fi
-	# finds all services entries under [sssd] configuration category, get a unique list so it doesn't add redundant fix
-	services_list=$( awk '/^\s*\[/{f=0} /^\s*\[sssd\]/{f=1}f' $f | grep -P '^services[ \t]*=' | uniq )
-    if [ -z "$services_list" ]; then
-        continue
-    fi
+        while IFS= read -r services; do
+            if [[ ! $services =~ "pam" ]]; then
+                sed -i "s/$services$/&, pam/" $f
+            fi
+            # Either pam service was already there or got added now
+            service_already_exist=true
+        done <<< "$services_list"
+    done
 
-	while IFS= read -r services; do
-		if [[ ! $services =~ "pam" ]]; then
-			sed -i "s/$services$/&, pam/" $f
-		fi
-        # Either pam service was already there or got added now
-        service_already_exist=true
-	done <<< "$services_list"
-
-done
-
-# If there was no service in [sssd], add it to first config
-if [ "$service_already_exist" = false ]; then
-    for f in $SSSD_CONF $SSSD_CONF_DIR; do
-        cat << EOF >> "$f"
+    # If there was no service in [sssd], add it to first config
+    if [ "$service_already_exist" = false ]; then
+        cat << EOF >> "$SSSD_CONF"
 [sssd]
 services = pam
 EOF
-        break
-    done
+    fi
 fi
-
-umask $OLD_UMASK
 
 else
     >&2 echo 'Remediation is not applicable, nothing was done'

ansible remediation for rule 'xccdf_org.ssgproject.content_rule_sssd_enable_pam_services' differs.
--- xccdf_org.ssgproject.content_rule_sssd_enable_pam_services
+++ xccdf_org.ssgproject.content_rule_sssd_enable_pam_services
@@ -12,7 +12,7 @@
   - no_reboot_needed
   - sssd_enable_pam_services
 
-- name: Configure PAM in SSSD Services - Find all the conf files inside the /etc/sssd/conf.d/
+- name: Configure PAM in SSSD Services - Find all conf files inside the /etc/sssd/conf.d/
     directory
   ansible.builtin.find:
     paths:
@@ -31,7 +31,7 @@
   - no_reboot_needed
   - sssd_enable_pam_services
 
-- name: Configure PAM in SSSD Services - Modify lines in files in the /etc/sssd/conf.d/
+- name: Configure PAM in SSSD Services - Modify lines in files found in the /etc/sssd/conf.d/
     directory
   ansible.builtin.replace:
     path: '{{ item }}'
@@ -53,7 +53,7 @@
   - no_reboot_needed
   - sssd_enable_pam_services
 
-- name: Configure PAM in SSSD Services - Find /etc/sssd/sssd.conf
+- name: Configure PAM in SSSD Services - Check existence of /etc/sssd/sssd.conf
   ansible.builtin.stat:
     path: /etc/sssd/sssd.conf
   register: sssd_conf_file
@@ -89,16 +89,17 @@
   - no_reboot_needed
   - sssd_enable_pam_services
 
-- name: Configure PAM in SSSD Services - Find services key in /etc/sssd/sssd.conf
-  ansible.builtin.replace:
+- name: Configure PAM in SSSD Services - Ensure services entry in sssd section of
+    /etc/sssd/sssd.conf
+  ini_file:
     path: /etc/sssd/sssd.conf
-    regexp: ^\s*\[sssd\][^\[\]]*?(?:\n(?!\[)[^\n]*?services\s*=)+
-    replace: ''
-  changed_when: false
-  check_mode: true
-  register: sssd_conf_file_services
+    section: sssd
+    option: services
+    value: pam
   when:
   - '"sssd-common" in ansible_facts.packages'
+  - not modify_lines_sssd_conf_d_files.changed
+  - not modify_lines_sssd_conf_file.changed
   - sssd_conf_file.stat.exists
   tags:
   - CCE-82446-6
@@ -110,26 +111,3 @@
   - medium_severity
   - no_reboot_needed
   - sssd_enable_pam_services
-
-- name: Configure PAM in SSSD Services - Insert entry to /etc/sssd/sssd.conf
-  ini_file:
-    path: /etc/sssd/sssd.conf
-    section: sssd
-    option: services
-    value: pam
-  when:
-  - '"sssd-common" in ansible_facts.packages'
-  - not modify_lines_sssd_conf_d_files.changed
-  - not modify_lines_sssd_conf_file.changed
-  - (sssd_conf_file_services.msg is defined and "replacements" not in sssd_conf_file_services.msg)
-    or not sssd_conf_file.stat.exists
-  tags:
-  - CCE-82446-6
-  - NIST-800-53-CM-6(a)
-  - NIST-800-53-IA-2(1)
-  - configure_strategy
-  - low_complexity
-  - medium_disruption
-  - medium_severity
-  - no_reboot_needed
-  - sssd_enable_pam_services

Platform has been changed for rule 'xccdf_org.ssgproject.content_rule_sssd_enable_pam_services'
--- xccdf_org.ssgproject.content_rule_sssd_enable_pam_services
+++ xccdf_org.ssgproject.content_rule_sssd_enable_pam_services
@@ -1 +1 @@
-
+oval:ssg-sssd_conf_files_present:def:1

Copy link

github-actions bot commented Sep 13, 2024

🤖 A k8s content image for this PR is available at:
ghcr.io/complianceascode/k8scontent:12378
This image was built from commit: 319c8aa

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:12378

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

@marcusburghardt
Copy link
Member Author

CI tests for ANSSI are returning error with rules service_sssd_enabled and sssd_enable_pam_services.
The remediation of sssd_enable_pam_services is not supposed to create sssd conf files, as it was incorrectly doing before the PR. It should only update existing files.

Now, since the VM doesn't have sssd conf files created by default, the rules keep failing after remediation.
It is expected, but we need to figure out how to treat this.

@marcusburghardt marcusburghardt marked this pull request as draft September 13, 2024 13:18
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Used by openshift-ci bot. label Sep 13, 2024
@marcusburghardt marcusburghardt changed the title Review and fix sssd rules Review and fix sssd rules in ANSSI Sep 17, 2024
@marcusburghardt marcusburghardt changed the title Review and fix sssd rules in ANSSI Review and fix sssd rules in ANSSI control file Sep 17, 2024
@marcusburghardt marcusburghardt added the CPE-AL CPE Applicability Language label Sep 17, 2024
@marcusburghardt
Copy link
Member Author

CI tests for ANSSI are returning error with rules service_sssd_enabled and sssd_enable_pam_services. The remediation of sssd_enable_pam_services is not supposed to create sssd conf files, as it was incorrectly doing before the PR. It should only update existing files.

Now, since the VM doesn't have sssd conf files created by default, the rules keep failing after remediation. It is expected, but we need to figure out how to treat this.

I created a new applicability to check if any SSSD configuration file is already present in the system. If so, the rule will check and, if necessary, remediate. If not, the rule will report notapplicable. It was also included warning in the rule description informing this behavior.

@marcusburghardt
Copy link
Member Author

I saw that other SSSD related rules could benefit of the applicability introduced here. I have plan to check these rules in another opportunity. Each rule uses a different approach. Ideally we should create a template (maybe some macros are enough) for SSSD and align all the SSSD related rules to the same approach. Unfortunately, this would demand more time than I have right now. So, for this PR I focused only on rules used in ANSSI control file.

@Mab879
Copy link
Member

Mab879 commented Nov 4, 2024

@marcusburghardt Do you plan on workig on this? How can we move this along?

Updated the test scenarios for a valid option.

Signed-off-by: Marcus Burghardt <[email protected]>
This is a templated rule but the test scenarios are overridden in order
to create a sssd.conf file for testing purposes. The package name in the
custom test scenarios was not correct.

Signed-off-by: Marcus Burghardt <[email protected]>
sssd remediation should not create sssd configuration files if they were
not previous created by sys admins. The reason the sssd configuration
files are not created by default is because its parameters may differ
for each site policy, therefore manual intervention is necessary to
ensure the sssd parameters are compliant. The bash remediation was
creating a new file only to satisfy a parameter. This creates
incosistent sssd configuration and makes the sssd service to fail.

Signed-off-by: Marcus Burghardt <[email protected]>
Signed-off-by: Marcus Burghardt <[email protected]>
The sssd.conf should only modified if already present. It is not created
by this remediation.

Signed-off-by: Marcus Burghardt <[email protected]>
Some SSSD related rules are only applicable when there is already SSSD
configuration files present in the system.

Signed-off-by: Marcus Burghardt <[email protected]>
This rule is only applicable when SSSD is already in use and
consequently configured.

Signed-off-by: Marcus Burghardt <[email protected]>
Copy link

codeclimate bot commented Nov 6, 2024

Code Climate has analyzed commit 6e3a497 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.

@Mab879 Mab879 removed this from the 0.1.75 milestone Nov 6, 2024
@Mab879 Mab879 added this to the 0.1.76 milestone Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bash Bash remediation update. bugfix Fixes to reported bugs. CPE-AL CPE Applicability Language do-not-merge/work-in-progress Used by openshift-ci bot. Test Suite Update in Test Suite.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants