Fix in sudo_require_reauthentication #10216
Conversation
|
Hi @rumch-se. 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 Once the patch is verified, the new status will be reflected by the 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/test-infra repository. |
|
Start a new ephemeral environment with changes proposed in this pull request: rhel8 (from CTF) Environment (using Fedora as testing environment) Fedora Testing Environment Oracle Linux 8 Environment |
|
This datastream diff is auto generated by the check Click here to see the full diffOVAL for rule 'xccdf_org.ssgproject.content_rule_sudo_require_reauthentication' differs.
--- oval:ssg-sudo_require_reauthentication:def:1
+++ oval:ssg-sudo_require_reauthentication:def:1
@@ -1,2 +1,3 @@
-criteria None
+criteria AND
criterion oval:ssg-test_sudo_timestamp_timeout:tst:1
+criterion oval:ssg-test_sudo_timestamp_timeout_no_signs:tst:1
bash remediation for rule 'xccdf_org.ssgproject.content_rule_sudo_require_reauthentication' differs.
--- xccdf_org.ssgproject.content_rule_sudo_require_reauthentication
+++ xccdf_org.ssgproject.content_rule_sudo_require_reauthentication
@@ -1,4 +1,5 @@
-
+# Remediation is applicable only in certain platforms
+if rpm --quiet -q sudo; then
var_sudo_timestamp_timeout=''
@@ -32,3 +33,7 @@
echo "Skipping remediation, /etc/sudoers failed to validate"
false
fi
+
+else
+ >&2 echo 'Remediation is not applicable, nothing was done'
+fi
ansible remediation for rule 'xccdf_org.ssgproject.content_rule_sudo_require_reauthentication' differs.
--- xccdf_org.ssgproject.content_rule_sudo_require_reauthentication
+++ xccdf_org.ssgproject.content_rule_sudo_require_reauthentication
@@ -1,3 +1,16 @@
+- name: Gather the package facts
+ package_facts:
+ manager: auto
+ tags:
+ - CCE-87838-9
+ - DISA-STIG-RHEL-08-010384
+ - NIST-800-53-IA-11
+ - low_complexity
+ - low_disruption
+ - medium_severity
+ - no_reboot_needed
+ - restrict_strategy
+ - sudo_require_reauthentication
- name: XCCDF Value var_sudo_timestamp_timeout # promote to variable
set_fact:
var_sudo_timestamp_timeout: !!str
@@ -11,6 +24,7 @@
patterns: '*'
contains: ^[\s]*Defaults\s.*\btimestamp_timeout[\s]*=.*
register: sudoers_d_defaults_timestamp_timeout
+ when: '"sudo" in ansible_facts.packages'
tags:
- CCE-87838-9
- DISA-STIG-RHEL-08-010384
@@ -29,6 +43,7 @@
regexp: ^[\s]*Defaults\s.*\btimestamp_timeout[\s]*=.*
state: absent
with_items: '{{ sudoers_d_defaults_timestamp_timeout.files }}'
+ when: '"sudo" in ansible_facts.packages'
tags:
- CCE-87838-9
- DISA-STIG-RHEL-08-010384
@@ -48,6 +63,7 @@
validate: /usr/sbin/visudo -cf %s
backrefs: true
register: edit_sudoers_timestamp_timeout_option
+ when: '"sudo" in ansible_facts.packages'
tags:
- CCE-87838-9
- DISA-STIG-RHEL-08-010384
@@ -64,7 +80,9 @@
path: /etc/sudoers
line: Defaults timestamp_timeout={{ var_sudo_timestamp_timeout }}
validate: /usr/sbin/visudo -cf %s
- when: edit_sudoers_timestamp_timeout_option is defined and not edit_sudoers_timestamp_timeout_option.changed
+ when:
+ - '"sudo" in ansible_facts.packages'
+ - edit_sudoers_timestamp_timeout_option is defined and not edit_sudoers_timestamp_timeout_option.changed
tags:
- CCE-87838-9
- DISA-STIG-RHEL-08-010384
Platform has been changed for rule 'xccdf_org.ssgproject.content_rule_sudo_require_reauthentication'
--- xccdf_org.ssgproject.content_rule_sudo_require_reauthentication
+++ xccdf_org.ssgproject.content_rule_sudo_require_reauthentication
@@ -1 +1 @@
-
+cpe:/a:sudo: |
|
another way to achieve the same goal is to use platforms instead. But I guess a CPE platform for sudo would need to be created. |
|
Hello @ggbecker |
|
I think that a simple: diff --git a/linux_os/guide/system/software/sudo/sudo_require_authentication/rule.yml b/linux_os/guide/system/software/sudo/sudo_require_authentication/rule.yml
index 0828312ba5..4a6d7d4dc9 100644
--- a/linux_os/guide/system/software/sudo/sudo_require_authentication/rule.yml
+++ b/linux_os/guide/system/software/sudo/sudo_require_authentication/rule.yml
@@ -25,6 +25,8 @@ identifiers:
cce@rhel9: CCE-83543-9
cce@sle15: CCE-85673-2
+platform: package[sudo]
+
references:
cis-csc: 1,12,15,16,5
cis@rhel8: 5.3.4would do the trick. And this would ensure that the remediation also takes the platform in consideration for example. As described in https://complianceascode.readthedocs.io/en/latest/manual/developer/06_contributing_with_content.html#rules in the |
|
We have a few examples of |
|
Good morning @ggbecker |
| <ind:pattern operation="pattern match">^[\s]*Defaults[\s]+timestamp_timeout[\s]*=[\s]*([-]?[\d]+)$</ind:pattern> | ||
| <ind:instance datatype="int" operation="greater than or equal">1</ind:instance> | ||
| <ind:filepath operation="pattern match">^/etc/sudoers(\.d/.*)?$</ind:filepath> | ||
| <ind:pattern operation="pattern match">^[\s]*Defaults[\s]*timestamp_timeout[\s]*=\s*[0-9]+$</ind:pattern> |
There was a problem hiding this comment.
| <ind:pattern operation="pattern match">^[\s]*Defaults[\s]*timestamp_timeout[\s]*=\s*[0-9]+$</ind:pattern> | |
| <ind:pattern operation="pattern match">^[\s]*Defaults[\s]+timestamp_timeout[\s]*=\s*[0-9]+$</ind:pattern> |
I think that there should be at least a space between the Defaults and timestamp_timeout
There was a problem hiding this comment.
ERROR - Script multiple_conflicting_value.fail.sh using profile (all) found issue:
ERROR - Rule evaluation resulted in pass, instead of expected fail during initial stage
ERROR - The initial scan failed for rule 'xccdf_org.ssgproject.content_rule_sudo_require_reauthentication'.
This test scenario is failing. We should investigate why before proceeding.
|
Hello @ggbecker |
| <ind:filepath operation="pattern match">^/etc/sudoers(\.d/.*)?$</ind:filepath> | ||
| <ind:pattern operation="pattern match">^[\s]*Defaults[\s]+timestamp_timeout[\s]*=[\s]*([-]?[\d]+)$</ind:pattern> | ||
| <ind:instance datatype="int" operation="greater than or equal">1</ind:instance> | ||
| <ind:filepath operation="pattern match">^\/etc\/(sudoers|sudoers\.d\/*)$</ind:filepath> |
There was a problem hiding this comment.
| <ind:filepath operation="pattern match">^\/etc\/(sudoers|sudoers\.d\/*)$</ind:filepath> | |
| <ind:filepath operation="pattern match">^\/etc\/(sudoers|sudoers\.d\/.*)$</ind:filepath> |
There was a problem hiding this comment.
It looks like it's missing the proper capture all element here
|
Hello @ggbecker === Test cases === /etc/sudoers | /etc/sudoers.d/00-complianceascode-test.conf | test Have a nice day |
|
Hi @rumch-se I'm not seeing the new test you mention in the changes section. Maybe you forgot to add the new file in the commit. |
|
Hello @ggbecker There is no new test. I used the problematic test as a reference and changed values manually. Like that I wanted to be sure that the correction will cover all possible cases. Have a nice day |
Understood. I think some of these scenarios are worthwhile to have a new automated test added. You can simply copy and paste existing files within |
|
Hello @ggbecker |
|
Hello @ggbecker |
|
Code Climate has analyzed commit bd8ea99 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 51.7% (0.0% change). View more on Code Climate. |
We have to take a look at the results and fix them if possible (maybe some package is missing or some specific directory is missing in the testing environment): https://github.com/ComplianceAsCode/content/actions/runs/4261303411/jobs/7415506767 logs: https://github.com/ComplianceAsCode/content/suites/11180141093/artifacts/571245791 |
|
@rumch-se whenever you have time can you look at my comment above. |
|
Hello @ggbecker I need some clarification here. Would be possible you to execute locally in the following order some of problematic test(s)
My last commit adds the test wrong_value_2.fail.sh Have a nice day |
|
I've identified that having +/- signs does not generate a parsing problem for the following command
which is the precondition for the remediation to be run: I didn't dig deeper in the ansible remediation, but it seems that having one sign is ok, but having two or more is a problem for the sudoers file parsing. What is the reason behind blocking usage of one sign in the value from the OVAL perspective? |
|
Hello @ggbecker, When I worked on the OVAL, I decided to focus on cases when we have a value which is a Whole number i.e. according math definition we have: Have a nice day |
I understand that this reference you mention doesn't accept some of the values, but the parsing of the sudoers file seems to be a different and they accept those values as valid. We should align with what the software accepts in this case unfortunately, otherwise valid configurations will be interpreted as invalid. Additionally, the remediation probably needs to be reworked to be smarter when there is an invalid value set, maybe it should try to remediate and then run the validation again having a backup file that will revert in case the validation ( |
|
Hello @ggbecker After this change +0 / +1 / +2.5 / 2.5 are valid settings. Have a nice day |
Added an additional tests for - sign
|
The test scenario called |
|
Hello @ggbecker |
jan-cerny
added
the
Update Rule
Description:
Rationale: