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

Fix crony.d config directory in Ansible in rule chronyd_or_ntpd_set_maxpoll #11958

Merged
merged 9 commits into from
May 28, 2024

Conversation

jan-cerny
Copy link
Collaborator

@jan-cerny jan-cerny commented May 7, 2024

The Ansible remediation manipulates unrelated files in /etc because the support for chrony.d configuration directory was implemented wrong. This patch reworks the support for chrony.d configuration directory.

For more details, please read commit messages of all commits.

@jan-cerny jan-cerny added the productization-issue Issue found in upstream stabilization process. label May 7, 2024
Copy link

github-actions bot commented May 7, 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 May 7, 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_chronyd_or_ntpd_set_maxpoll'.
--- xccdf_org.ssgproject.content_rule_chronyd_or_ntpd_set_maxpoll
+++ xccdf_org.ssgproject.content_rule_chronyd_or_ntpd_set_maxpoll
@@ -5,15 +5,13 @@
 [description]:
 The maxpoll should be configured to
 'xccdf_org.ssgproject.content_value_var_time_service_set_maxpoll' in /etc/ntp.conf or
-/etc/chrony.conf to continuously poll time servers. To configure
-maxpoll in /etc/ntp.conf or /etc/chrony.conf
-add the following after each `server`, `pool` or `peer` entry:
+/etc/chrony.conf (or /etc/chrony.d/) to continuously poll time servers. To configure
+maxpoll in /etc/ntp.conf or /etc/chrony.conf (or /etc/chrony.d/)
+add the following after each server, pool or peer entry:
 maxpoll 'xccdf_org.ssgproject.content_value_var_time_service_set_maxpoll'
        
-to server directives. If using chrony any pool directives
+to server directives. If using chrony, any pool directives
 should be configured too.
-If no server or pool directives are configured, the rule evaluates
-to pass.
 
 [reference]:
 1

OVAL for rule 'xccdf_org.ssgproject.content_rule_chronyd_or_ntpd_set_maxpoll' differs.
--- oval:ssg-chronyd_or_ntpd_set_maxpoll:def:1
+++ oval:ssg-chronyd_or_ntpd_set_maxpoll:def:1
@@ -1,11 +1,7 @@
-criteria AND
 criteria OR
-criterion oval:ssg-test_ntp_no_server:tst:1
 criteria AND
 criterion oval:ssg-test_ntp_set_maxpoll:tst:1
 criterion oval:ssg-test_ntp_all_server_has_maxpoll:tst:1
-criteria OR
-criterion oval:ssg-test_chrony_no_server_nor_pool:tst:1
 criteria AND
 criterion oval:ssg-test_chrony_set_maxpoll:tst:1
 criterion oval:ssg-test_chrony_all_server_has_maxpoll:tst:1

OCIL for rule 'xccdf_org.ssgproject.content_rule_chronyd_or_ntpd_set_maxpoll' differs.
--- ocil:ssg-chronyd_or_ntpd_set_maxpoll_ocil:questionnaire:1
+++ ocil:ssg-chronyd_or_ntpd_set_maxpoll_ocil:questionnaire:1
@@ -1,5 +1,5 @@
 Verify Red Hat Enterprise Linux 8 is securely comparing internal information system clocks at a regular interval with an NTP server with the following command:
-$ sudo grep maxpoll /etc/ntp.conf /etc/chrony.conf
+$ sudo grep maxpoll /etc/ntp.conf /etc/chrony.conf /etc/chrony.d/
 server [ntp.server.name] iburst maxpoll .
       Is it the case that "maxpoll" has not been set to the value of "<sub idref="var_time_service_set_maxpoll" />", is commented out, or is missing?
       
bash remediation for rule 'xccdf_org.ssgproject.content_rule_chronyd_or_ntpd_set_maxpoll' differs.
--- xccdf_org.ssgproject.content_rule_chronyd_or_ntpd_set_maxpoll
+++ xccdf_org.ssgproject.content_rule_chronyd_or_ntpd_set_maxpoll
@@ -11,22 +11,19 @@
 
 CONFIG_FILES="/etc/ntp.conf"
 $pof ntpd || {
-    CHRONY_NAME=/etc/chrony.conf
-    CHRONY_PATH=${CHRONY_NAME%%.*}
-    CONFIG_FILES=$(find ${CHRONY_PATH}.* -type f -name '*.conf')
+    CHRONY_D_PATH=/etc/chrony.d/
+    mapfile -t CONFIG_FILES < <(find ${CHRONY_D_PATH}.* -type f -name '*.conf')
+    CONFIG_FILES+=(/etc/chrony.conf)
 }
 
 # get list of ntp files
 
-for config_file in $CONFIG_FILES; do
+for config_file in "${CONFIG_FILES[@]}" ; do
     # Set maxpoll values to var_time_service_set_maxpoll
     sed -i "s/^\(\(server\|pool\|peer\).*maxpoll\) [0-9][0-9]*\(.*\)$/\1 $var_time_service_set_maxpoll \3/" "$config_file"
 done
 
-
-
-
-for config_file in $CONFIG_FILES; do
+for config_file in "${CONFIG_FILES[@]}" ; do
     # Add maxpoll to server, pool or peer entries without maxpoll
     grep "^\(server\|pool\|peer\)" "$config_file" | grep -v maxpoll | while read -r line ; do
         sed -i "s/$line/& maxpoll $var_time_service_set_maxpoll/" "$config_file"

ansible remediation for rule 'xccdf_org.ssgproject.content_rule_chronyd_or_ntpd_set_maxpoll' differs.
--- xccdf_org.ssgproject.content_rule_chronyd_or_ntpd_set_maxpoll
+++ xccdf_org.ssgproject.content_rule_chronyd_or_ntpd_set_maxpoll
@@ -39,7 +39,7 @@
   - no_reboot_needed
   - restrict_strategy
 
-- name: Configure Time Service Maxpoll Interval - Update the Maxpoll Values in /etc/ntp.conf
+- name: Configure Time Service Maxpoll Interval - Update the maxpoll Values in /etc/ntp.conf
   ansible.builtin.replace:
     path: /etc/ntp.conf
     regexp: ^(server.*maxpoll)[ ]+[0-9]+(.*)$
@@ -61,7 +61,7 @@
   - no_reboot_needed
   - restrict_strategy
 
-- name: Configure Time Service Maxpoll Interval - Set the Maxpoll Values in /etc/ntp.conf
+- name: Configure Time Service Maxpoll Interval - Set the maxpoll Values in /etc/ntp.conf
   ansible.builtin.replace:
     path: /etc/ntp.conf
     regexp: (^server\s+((?!maxpoll).)*)$
@@ -103,90 +103,114 @@
   - no_reboot_needed
   - restrict_strategy
 
-- name: Configure Time Service Maxpoll Interval - Set Chrony Path Facts
-  ansible.builtin.set_fact:
-    chrony_path: /etc/chrony.conf
-  when:
-  - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
-  - ( "chrony" in ansible_facts.packages or "ntp" in ansible_facts.packages )
-  tags:
-  - CCE-84059-5
-  - DISA-STIG-RHEL-08-030740
-  - NIST-800-53-AU-12(1)
-  - NIST-800-53-AU-8(1)(b)
-  - NIST-800-53-CM-6(a)
-  - chronyd_or_ntpd_set_maxpoll
-  - low_complexity
-  - low_disruption
-  - medium_severity
-  - no_reboot_needed
-  - restrict_strategy
-
-- name: Configure Time Service Maxpoll Interval - Get Conf Files from {{ chrony_path
-    | dirname }}
+- name: Configure Time Service Maxpoll Interval - Update the maxpoll Values in /etc/chrony.conf
+  ansible.builtin.replace:
+    path: /etc/chrony.conf
+    regexp: ^((?:server|pool|peer).*maxpoll)[ ]+[0-9]+(.*)$
+    replace: \1 {{ var_time_service_set_maxpoll }}\2
+  when:
+  - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
+  - ( "chrony" in ansible_facts.packages or "ntp" in ansible_facts.packages )
+  - chrony_conf_exist_result.stat.exists
+  tags:
+  - CCE-84059-5
+  - DISA-STIG-RHEL-08-030740
+  - NIST-800-53-AU-12(1)
+  - NIST-800-53-AU-8(1)(b)
+  - NIST-800-53-CM-6(a)
+  - chronyd_or_ntpd_set_maxpoll
+  - low_complexity
+  - low_disruption
+  - medium_severity
+  - no_reboot_needed
+  - restrict_strategy
+
+- name: Configure Time Service Maxpoll Interval - Set the maxpoll Values in /etc/chrony.conf
+  ansible.builtin.replace:
+    path: /etc/chrony.conf
+    regexp: (^(?:server|pool|peer)\s+((?!maxpoll).)*)$
+    replace: \1 maxpoll {{ var_time_service_set_maxpoll }}\n
+  when:
+  - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
+  - ( "chrony" in ansible_facts.packages or "ntp" in ansible_facts.packages )
+  - chrony_conf_exist_result.stat.exists
+  tags:
+  - CCE-84059-5
+  - DISA-STIG-RHEL-08-030740
+  - NIST-800-53-AU-12(1)
+  - NIST-800-53-AU-8(1)(b)
+  - NIST-800-53-CM-6(a)
+  - chronyd_or_ntpd_set_maxpoll
+  - low_complexity
+  - low_disruption
+  - medium_severity
+  - no_reboot_needed
+  - restrict_strategy
+
+- name: Configure Time Service Maxpoll Interval - Get Conf Files from /etc/chrony.d/
   ansible.builtin.find:
-    path: '{{ chrony_path | dirname }}'
+    path: /etc/chrony.d/
     patterns: '*.conf'
     file_type: file
-  register: chrony_conf_files
-  when:
-  - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
-  - ( "chrony" in ansible_facts.packages or "ntp" in ansible_facts.packages )
-  tags:
-  - CCE-84059-5
-  - DISA-STIG-RHEL-08-030740
-  - NIST-800-53-AU-12(1)
-  - NIST-800-53-AU-8(1)(b)
-  - NIST-800-53-CM-6(a)
-  - chronyd_or_ntpd_set_maxpoll
-  - low_complexity
-  - low_disruption
-  - medium_severity
-  - no_reboot_needed
-  - restrict_strategy
-
-- name: Configure Time Service Maxpoll Interval - Update the Maxpoll Values in /etc/chrony.conf
+  register: chrony_d_conf_files
+  when:
+  - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
+  - ( "chrony" in ansible_facts.packages or "ntp" in ansible_facts.packages )
+  tags:
+  - CCE-84059-5
+  - DISA-STIG-RHEL-08-030740
+  - NIST-800-53-AU-12(1)
+  - NIST-800-53-AU-8(1)(b)
+  - NIST-800-53-CM-6(a)
+  - chronyd_or_ntpd_set_maxpoll
+  - low_complexity
+  - low_disruption
+  - medium_severity
+  - no_reboot_needed
+  - restrict_strategy
+
+- name: Configure Time Service Maxpoll Interval - Update the maxpoll Values in /etc/chrony.d/
   ansible.builtin.replace:
     path: '{{ item.path }}'
     regexp: ^((?:server|pool|peer).*maxpoll)[ ]+[0-9]+(.*)$
     replace: \1 {{ var_time_service_set_maxpoll }}\2
-  loop: '{{ chrony_conf_files.files }}'
-  when:
-  - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
-  - ( "chrony" in ansible_facts.packages or "ntp" in ansible_facts.packages )
-  - chrony_conf_files.matched
-  tags:
-  - CCE-84059-5
-  - DISA-STIG-RHEL-08-030740
-  - NIST-800-53-AU-12(1)
-  - NIST-800-53-AU-8(1)(b)
-  - NIST-800-53-CM-6(a)
-  - chronyd_or_ntpd_set_maxpoll
-  - low_complexity
-  - low_disruption
-  - medium_severity
-  - no_reboot_needed
-  - restrict_strategy
-
-- name: Configure Time Service Maxpoll Interval - Set the Maxpoll Values in /etc/chrony.conf
+  loop: '{{ chrony_d_conf_files.files }}'
+  when:
+  - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
+  - ( "chrony" in ansible_facts.packages or "ntp" in ansible_facts.packages )
+  - chrony_d_conf_files.matched
+  tags:
+  - CCE-84059-5
+  - DISA-STIG-RHEL-08-030740
+  - NIST-800-53-AU-12(1)
+  - NIST-800-53-AU-8(1)(b)
+  - NIST-800-53-CM-6(a)
+  - chronyd_or_ntpd_set_maxpoll
+  - low_complexity
+  - low_disruption
+  - medium_severity
+  - no_reboot_needed
+  - restrict_strategy
+
+- name: Configure Time Service Maxpoll Interval - Set the maxpoll Values in /etc/chrony.d/
   ansible.builtin.replace:
     path: '{{ item.path }}'
     regexp: (^(?:server|pool|peer)\s+((?!maxpoll).)*)$
     replace: \1 maxpoll {{ var_time_service_set_maxpoll }}\n
-  loop: '{{ chrony_conf_files.files }}'
-  when:
-  - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
-  - ( "chrony" in ansible_facts.packages or "ntp" in ansible_facts.packages )
-  - chrony_conf_files.matched
-  tags:
-  - CCE-84059-5
-  - DISA-STIG-RHEL-08-030740
-  - NIST-800-53-AU-12(1)
-  - NIST-800-53-AU-8(1)(b)
-  - NIST-800-53-CM-6(a)
-  - chronyd_or_ntpd_set_maxpoll
-  - low_complexity
-  - low_disruption
-  - medium_severity
-  - no_reboot_needed
-  - restrict_strategy
+  loop: '{{ chrony_d_conf_files.files }}'
+  when:
+  - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
+  - ( "chrony" in ansible_facts.packages or "ntp" in ansible_facts.packages )
+  - chrony_d_conf_files.matched
+  tags:
+  - CCE-84059-5
+  - DISA-STIG-RHEL-08-030740
+  - NIST-800-53-AU-12(1)
+  - NIST-800-53-AU-8(1)(b)
+  - NIST-800-53-CM-6(a)
+  - chronyd_or_ntpd_set_maxpoll
+  - low_complexity
+  - low_disruption
+  - medium_severity
+  - no_reboot_needed
+  - restrict_strategy

Copy link

github-actions bot commented May 7, 2024

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

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

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

@jan-cerny jan-cerny requested review from a team as code owners May 7, 2024 13:12
@jan-cerny jan-cerny added this to the 0.1.74 milestone May 7, 2024
@Mab879 Mab879 marked this pull request as draft May 7, 2024 13:51
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Used by openshift-ci bot. label May 7, 2024
@jan-cerny jan-cerny removed the productization-issue Issue found in upstream stabilization process. label May 7, 2024
The Ansible Tasks in this rule manipulate with many unrelated files such
as /etc/sestatus.conf or /etc/krb5.conf.  The reason is that they
wrongly use the chrony_conf_path variable. They get the name of the
parent directory and look for all .conf files there. This probably works
nicely for Ubuntu products, where chrony_conf_path is set to
/etc/chrony/chrony.conf, but creates harm in all other products where
chrony_conf_path is set to /etc/chrony.conf, so the dirname is /etc and
the search matches all *.conf files in /etc.

We will fix this problem by explicit using the chrony_d_path content
variable which defines path to the chrony configuration directory.
Adds scenarios testing the configuration in the .d directory.
@jan-cerny
Copy link
Collaborator Author

This PR has been originally intended as a fix for #11934. However, it turned out that only the order of the rules is the cause of the reported issue. To fix the issue, it isn't necessary to fix the the support for chrony.d configuration directory. To fix the issue we only need to reorder the rules. As this PR needs a review from broader community, the ordering change has been extracted out to a new separate PR: #11960

@jan-cerny jan-cerny added the Ansible Ansible remediation update. label May 7, 2024
@jan-cerny jan-cerny changed the title Fix rule chronyd_or_ntpd_set_maxpoll Fix crony.d config directory in Ansible in rule chronyd_or_ntpd_set_maxpoll May 7, 2024
@jan-cerny jan-cerny marked this pull request as ready for review May 7, 2024 16:21
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Used by openshift-ci bot. label May 7, 2024
@marcusburghardt marcusburghardt self-assigned this May 8, 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.

LGTM. Thanks

@marcusburghardt marcusburghardt removed their assignment May 8, 2024
@marcusburghardt
Copy link
Member

FYI @ComplianceAsCode/ubuntu-maintainers @ComplianceAsCode/suse-maintainers @ComplianceAsCode/oracle-maintainers

Copy link
Contributor

@mpurg mpurg left a comment

Choose a reason for hiding this comment

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

This could be a good time to fix the hardcoded (and thus broken) paths in OVAL (/etc/chrony.conf, /etc/chrony.d/*conf) and bash (/etc/chrony/chrony.conf, /etc/chrony/chrony.*/*.conf on Ubuntu) , though it could be done in a followup PR as well.

# Remove all pool options
sed -i "/^pool.*/d" {{{ chrony_d_path }}}/10-servers.conf

if ! grep "^server.*maxpoll 10" {{{ chrony_d_path }}}/10-servers.conf ; then
Copy link
Contributor

Choose a reason for hiding this comment

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

These configs seem platform specific. Maybe the tests should be made platform specific?

{{{ bash_package_remove("ntp") }}}

# Remove all server or pool options
sed -i "/^\(server\|pool\).*/d" {{{ chrony_d_path }}}/20-pools.conf
Copy link
Contributor

Choose a reason for hiding this comment

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

These configs seem platform specific. Maybe the tests should be made platform specific?

Copy link
Contributor

Choose a reason for hiding this comment

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

I tend to agree with the above comment, instead of making it platform specific we can replace 20-pools.conf with wildcard

@mpurg
Copy link
Contributor

mpurg commented May 9, 2024

Slightly unrelated, but after looking at the OVAL, it seems that it passes when 'server/poll' directives are missing in the config files (or the config files are not present at those hardcoded paths). According to the Ubuntu/RHEL STIGs, the directives should be explicitly defined (and not in ntp configs).

- consistent support for the configuration directory
- always use product properties `chrony_conf_path` and `chrony_d_path`
  for the configuration file and configuration directory instead of
  using hard-coded path
- do not pass if no server is set, this align the behavior with RHEL
  and Ubuntu STIGs
@jan-cerny
Copy link
Collaborator Author

I have updated in the rule chronyd_or_ntpd_set_maxpoll:

  • consistent support for the configuration directory
  • always use product properties chrony_conf_path and chrony_d_path
    for the configuration file and configuration directory instead of
    using hard-coded path
  • do not pass if no server is set, this align the behavior with RHEL
    and Ubuntu STIGs

Use the "variables" keyword in the test scenario headers instead of the
"profiles" keyword. These test scenarios aren't a regression test
special to STIG. The profile was set in order to set the XCCDF Value
value. This is legacy usage of the "profiles" keyword and we recommend
using "variables" for this situation.
@jan-cerny
Copy link
Collaborator Author

I have changed the test scenarios to use variables instead of profiles key.

Copy link
Contributor

@dodys dodys left a comment

Choose a reason for hiding this comment

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

lgtm

@jan-cerny
Copy link
Collaborator Author

@teacup-on-rockingchair PTAL

@teacup-on-rockingchair
Copy link
Contributor

@teacup-on-rockingchair PTAL

I guess you missed @mpurg and mine comments on the tests above? #11958 (comment)

@jan-cerny
Copy link
Collaborator Author

I have marked platform specific scenarios.

Copy link

codeclimate bot commented May 23, 2024

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

View more on Code Climate.

Copy link
Contributor

@teacup-on-rockingchair teacup-on-rockingchair left a comment

Choose a reason for hiding this comment

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

Nice one 🙇

@marcusburghardt
Copy link
Member

@Xeicker , could you also review, please? It also needs approval from @ComplianceAsCode/oracle-maintainers .

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.

Also reviewed the updates after my first review. Thanks @jan-cerny

@marcusburghardt marcusburghardt merged commit a2f912a into ComplianceAsCode:master May 28, 2024
112 of 113 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants