Skip to content

Fix sssd_enable_smartcards case sensitivity#8930

Merged
jan-cerny merged 1 commit intoComplianceAsCode:masterfrom
hjones2199:fix-sssd-smartcard
Jul 8, 2022
Merged

Fix sssd_enable_smartcards case sensitivity#8930
jan-cerny merged 1 commit intoComplianceAsCode:masterfrom
hjones2199:fix-sssd-smartcard

Conversation

@hjones2199
Copy link
Copy Markdown
Contributor

@hjones2199 hjones2199 commented Jun 11, 2022

Description:

Fix case-sensitivity issue in sssd_enable_smartcards

Rationale:

All sssd documentation I can find seems to imply that the canonical format for booleans in sssd.conf is 'True/False' not 'true/false'.
sssd_enable_smartcards is currently failing even when 'pam_cert_auth = True' is set, and it insists that 'pam_cert_auth = true' is the correct value.

I'm not sure if the currently checked for lowercase version is valid according to SSSD, but since the manpages, docs, etc use camel-case 'True/False' that should be considered valid here, and should be the remediated format.

@openshift-ci
Copy link
Copy Markdown

openshift-ci Bot commented Jun 11, 2022

Hi @hjones2199. 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.

Details

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.

@openshift-ci openshift-ci Bot added the needs-ok-to-test Used by openshift-ci bot. label Jun 11, 2022
@github-actions
Copy link
Copy Markdown

Start a new ephemeral environment with changes proposed in this pull request:

Open in Gitpod

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 11, 2022

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

Click here to see the full diff
OCIL for rule 'xccdf_org.ssgproject.content_rule_sssd_enable_smartcards' differs:
--- old datastream
+++ new datastream
@@ -1,7 +1,7 @@
 To verify that smart cards are enabled in SSSD, run the following command:
 $ sudo grep pam_cert_auth /etc/sssd/sssd.conf
 If configured properly, output should be
-pam_cert_auth = true
+pam_cert_auth = True
 
 
 $ sudo grep cert_auth /etc/sssd/sssd.conf /etc/pam.d/*

bash remediation for rule 'xccdf_org.ssgproject.content_rule_sssd_enable_smartcards' differs:
--- old datastream
+++ new datastream
@@ -11,12 +11,12 @@
 
 # find key in section and change value
 if grep -qzosP "[[:space:]]*\[pam\]([^\n\[]*\n+)+?[[:space:]]*pam_cert_auth" "$f"; then
- sed -i "s/pam_cert_auth[^(\n)]*/pam_cert_auth = true/" "$f"
+ sed -i "s/pam_cert_auth[^(\n)]*/pam_cert_auth = True/" "$f"
 found=true
 
 # find section and add key = value to it
 elif grep -qs "[[:space:]]*\[pam\]" "$f"; then
- sed -i "/[[:space:]]*\[pam\]/a pam_cert_auth = true" "$f"
+ sed -i "/[[:space:]]*\[pam\]/a pam_cert_auth = True" "$f"
 found=true
 fi
 done
@@ -25,7 +25,7 @@
 if ! $found ; then
 file=$(echo "/etc/sssd/sssd.conf" | cut -f1 -d' ')
 mkdir -p "$(dirname "$file")"
- echo -e "[pam]\npam_cert_auth = true" >> "$file"
+ echo -e "[pam]\npam_cert_auth = True" >> "$file"
 fi
 
 

ansible remediation for rule 'xccdf_org.ssgproject.content_rule_sssd_enable_smartcards' differs:
--- old datastream
+++ new datastream
@@ -65,7 +65,7 @@
 dest: /etc/sssd/sssd.conf
 section: pam
 option: pam_cert_auth
- value: 'true'
+ value: 'True'
 create: true
 mode: 384
 when:

@jan-cerny
Copy link
Copy Markdown
Collaborator

/ok-to-test

@openshift-ci openshift-ci Bot added ok-to-test Used by openshift-ci bot. and removed needs-ok-to-test Used by openshift-ci bot. labels Jun 13, 2022
Copy link
Copy Markdown
Collaborator

@jan-cerny jan-cerny left a comment

Choose a reason for hiding this comment

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

AFAIK sssd.conf is case insensitive so both True and true should work

<ind:textfilecontent54_object id="obj_sssd_enable_smartcards" version="1">
<ind:filepath>/etc/sssd/sssd.conf</ind:filepath>
<ind:pattern operation="pattern match">^[\s]*\[pam](?:[^\n\[]*\n+)+?[\s]*pam_cert_auth[\s]*=[\s]*(?i)true\s*$</ind:pattern>
<ind:pattern operation="pattern match">^[\s]*\[pam](?:[^\n\[]*\n+)+?[\s]*pam_cert_auth[\s]*=[\s]*(?i)[Tt]rue\s*$</ind:pattern>
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The (?i) in the regular expression turns on case insensivity for the remainder of the pattern, therefore, you don't need to use [Tt] in the pattern, both lowercase and uppercase variant will be matched anyway.

@jan-cerny jan-cerny self-assigned this Jun 14, 2022
@jan-cerny jan-cerny added this to the 0.1.63 milestone Jun 14, 2022
@marcusburghardt
Copy link
Copy Markdown
Member

It would also be good a pass test scenario script for each case: True and true.

@hjones2199 hjones2199 force-pushed the fix-sssd-smartcard branch from 5d06522 to ab3bea9 Compare June 18, 2022 15:04
@hjones2199 hjones2199 force-pushed the fix-sssd-smartcard branch from ab3bea9 to 7c675b7 Compare June 18, 2022 15:11
@hjones2199
Copy link
Copy Markdown
Contributor Author

Removed the misguided regex modification. Now I'm not sure what the issue is causing the check to fail.

@qlty-cloud-legacy
Copy link
Copy Markdown

Code Climate has analyzed commit 7c675b7 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 42.6% (0.0% change).

View more on Code Climate.

Copy link
Copy Markdown
Collaborator

@jan-cerny jan-cerny left a comment

Choose a reason for hiding this comment

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

@hjones2199 The test suite fail is fine. It's caused by the fact that on GitHub we use a container as a backend and in that container we don't run the sssd service. If I run the tests with a virtual machine backend, everything passes:

[jcerny@thinkpad scap-security-guide{pr/8930}]$ python3 tests/test_suite.py rule --libvirt qemu:///system ssgts_rhel8 sssd_enable_smartcards
WARNING - You call Automatus using the legacy 'test_suite.py' script, use the 'automatus.py' instead

Setting console output to log level INFO
INFO - The base image option has not been specified, choosing libvirt-based test environment.
INFO - Logging into /home/jcerny/work/git/scap-security-guide/logs/rule-custom-2022-06-21-0924/test_suite.log
INFO - xccdf_org.ssgproject.content_rule_sssd_enable_smartcards
INFO - Script correct_value.pass.sh using profile (all) OK
INFO - Script pamd_argument_missing.fail.sh using profile (all) OK
INFO - Script pamd_argument_missing_authselect.fail.sh using profile (all) OK
INFO - Script value_missing.fail.sh using profile (all) OK
INFO - Script wrong_value.fail.sh using profile (all) OK

WARNING - You call Automatus using the legacy 'test_suite.py' script, use the 'automatus.py' instead
[jcerny@thinkpad scap-security-guide{pr/8930}]$ python3 tests/test_suite.py rule --remediate-using ansible --libvirt qemu:///system ssgts_rhel8 sssd_enable_smartcards
WARNING - You call Automatus using the legacy 'test_suite.py' script, use the 'automatus.py' instead

Setting console output to log level INFO
INFO - The base image option has not been specified, choosing libvirt-based test environment.
INFO - Logging into /home/jcerny/work/git/scap-security-guide/logs/rule-custom-2022-06-21-0928/test_suite.log
INFO - xccdf_org.ssgproject.content_rule_sssd_enable_smartcards
INFO - Script correct_value.pass.sh using profile (all) OK
INFO - Script pamd_argument_missing.fail.sh using profile (all) OK
INFO - Script pamd_argument_missing_authselect.fail.sh using profile (all) OK
INFO - Script value_missing.fail.sh using profile (all) OK
INFO - Script wrong_value.fail.sh using profile (all) OK

WARNING - You call Automatus using the legacy 'test_suite.py' script, use the 'automatus.py' instead

So you don't need to do anything regarding that.

But, what is your opinion on adding a new test scenario covering the case insensitive case that @marcusburghardt suggested? Can you add it?

@jhrozek
Copy link
Copy Markdown
Collaborator

jhrozek commented Jun 22, 2022

/retest

@marcusburghardt
Copy link
Copy Markdown
Member

@jan-cerny , I am preparing another PR to improve the sssd_enable_smartcards rule. I should send it soon, probably on Monday and I am happy to include some additional test scenarios for this case insensitive situation. So, unless you have any other point for this PR, feel free to merge it.

Copy link
Copy Markdown
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.

I can create these test scenarios since I am already working in some improvements for the sssd_enable_smartcards rules.

@jan-cerny
Copy link
Copy Markdown
Collaborator

@marcusburghardt That's great news!

@jan-cerny jan-cerny merged commit 745eb3e into ComplianceAsCode:master Jul 8, 2022
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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

content_rule_sssd_enable_smartcards rule is not passing after remediation

4 participants