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

New rule ensure_chrony_is_configured #10353

Merged

Conversation

rumch-se
Copy link
Contributor

Description:

  • Rule covers CIS requirement 2.2.1.3 Ensure chrony is configured (Automated)

Rationale:

  • The rule has audit and remediation part which check/add remote NTP servers to /etc/chrony.conf

@rumch-se rumch-se requested a review from a team as a code owner March 23, 2023 11:23
@openshift-ci openshift-ci bot added the needs-ok-to-test Used by openshift-ci bot. label Mar 23, 2023
@openshift-ci
Copy link

openshift-ci bot commented Mar 23, 2023

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 /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.

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.

@github-actions
Copy link

github-actions bot commented Mar 23, 2023

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

sle12 (from CTF) Environment (using Fedora as testing environment)
Open in Gitpod

Fedora Testing Environment
Open in Gitpod

Oracle Linux 8 Environment
Open in Gitpod

<ind:textfilecontent54_object comment="Ensure at least one remote NTP server is set"
id="object_chrony_remote_server_configuartion" version="1">
<ind:filepath operation="pattern match">^/etc/chrony.conf$</ind:filepath>
<ind:pattern operation="pattern match">^(\bserver[[:space:]]|pool[[:space:]]\b)([0-3]\b(.fedora|.suse|.rhel)\b\b(.pool.ntp.org)\b)|([0-3]\b(.pool.ntp.org)\b)|([0-3]\b(.ntp.cloud.aliyuncs.com)\b)|([0-3]\b(.us.pool.ntp.mil)\b)$</ind:pattern>
Copy link
Contributor

@dodys dodys Mar 23, 2023

Choose a reason for hiding this comment

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

shouldn't you be checking if the server specified in var_multiple_time_servers is in /etc/chrony.conf?
That would allow anyone working with a tailoring file to set var_multiple_time_servers to the servers according to their site policy, and that might be different from this hard-coded check.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also isn't this entire rule the same as chronyd_specify_remote_server?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @dodys

At the moment regerp - covers ALL time servers defined into the variable + servers which are not defined - for example .mil. Do you know a way to generate regexp dynamically from the definition of the variable?
There is another issue - the variable defines time servers for different vendors i.e. OVAL check /audit/ has to check if /etc/chrony.conf includes servers for the selected vendor.

Have a nice day
Rumen

Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't use any regex, I would check if the /etc/chrony.conf file has the value set in var_multiple_time_servers, that way you allow users to define their own servers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also @dodys
According to your second question I will propose you to check conversation in #10278. According to me the OVAL part of the rule chronyd_specify_remote_server is not correct - because it checks for ANY word after the word server. I received a proposal to develop another rule, and because of that this PR was developed. With this PR I am trying to control better the content of /etc/chrony.conf - because the correct configuration of servers is very important for audit/ digital forensic/ how IDS/IPS are working.
Have a nice day
Rumen

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @dodys
I have a question about your proposal " I would check if the /etc/chrony.conf file has the value set in var_multiple_time_servers, that way you allow users to define their own servers.". The variable includes 4 servers for each vendor delimited by a comma. In /etc/chrony.conf file we have to have at least one line with the following format
server one_space name_of_the_time_server. How can I check that the line in .conf file is correct. Would be possible to provide me an example from another rule?

Have a nice day
Rumen

Copy link
Contributor

Choose a reason for hiding this comment

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

you can create a local_variable from this external_variable and do a regex with it
or you could have 4 local_variables and do 4 checks, I think it might be easier the previous idea.
for some example, see:
https://github.com/ComplianceAsCode/content/blob/master/linux_os/guide/system/logging/rsyslog_filecreatemode/oval/shared.xml#L24

There are other ways to do it as well, it just requires some thinking and creating variables.


<ind:textfilecontent54_object comment="Ensure at least one remote NTP server is set"
id="object_chrony_remote_server_configuartion" version="1">
<ind:filepath operation="pattern match">^/etc/chrony.conf$</ind:filepath>
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe here you might want to use: {{{ chrony_conf_path }}}
see other rules that have this variable use

@rumch-se
Copy link
Contributor Author

Hello @dodys
I did necessary changes
Have a nice day
Rumen

@dodys
Copy link
Contributor

dodys commented Mar 24, 2023

Hello @dodys I did necessary changes Have a nice day Rumen

hi @rumch-se
you've added four new variables when you already have the var_multiple_time_servers. I don't think you need that at all, you can have a local_variable in oval that is generated from the var_multiple_time_servers. No need for new variables outside of oval.
I'm doing some tests and will propose a better solution

@rumch-se
Copy link
Contributor Author

rumch-se commented Mar 24, 2023

Hello @dodys
1/
I don't know how to generate 4 local variables from the existing one and how these 4 local variables to be like a pattern which I use with this update. For example after this modification the OVAL check will pass only when we have at least correct line in the /etc/chnrony.conf file. If in /etc/chrony.conf there are records like these:
server0.suse.pool.ntp.org - will not pass because these is no space between server and 0
server 0.suse.pool.ntp.org - will not pass because we have more than one space after the word server
server 0.suse.pool.ntp.org - will not pass because we have one space before the word server
server 0.suse.pool.ntp.org - will not pass because we have space(s) before and after word server
server 0.pool.ntp.org - will not pass because we don't use the server propose by the vendor. We can play here again with spaces before and after the word server.

2/
There is another approach - to define only one variable in which we will have a pattern for each vendor. The pattern could be extended to cover IP address also. In this case in the OVAL we will have only one check - not 4, but with this check we will cover 5 cases:
For example the pattern for SUSE will be like this:
"^\bserver[[:space:]]\b[0-3]\b.suse.pool.ntp.org\b|(\b25[0-5]|\b2[0-4][0-9]|\b[01]?[0-9][0-9]?)(.(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)){3}$"

3/
I don't like the approach to have 5 variables - because risk of error is growing. I will prefer to use only one variable and from it to create inside OVAL 1+ variables which I can use for a test.

Have a nice day
Rumen

@dodys
Copy link
Contributor

dodys commented Mar 24, 2023

Hi @rumch-se, I think I got a working prototype of the OVAL.

Do note that I did on top of chronyd_specify_remote_server just because that's what we have in master right now, but you could re-use it in your new rule.

  1. Here is the diff I added to the oval in chronyd_specify_remote_server: https://pastebin.ubuntu.com/p/CV4PC9YTqc/
    1.1. To explain what it is doing, first I'm referencing the external_variable var_multiple_time_servers, which in my case here is set to the default value of 0.pool.ntp.org,1.pool.ntp.org,2.pool.ntp.org,3.pool.ntp.org
    1.2. I'm creating a local_variable that is first splitting the value of var_multiple_time_servers by the ','. From this split you will get 4 values: 0.pool.ntp.org, 1.pool.ntp.org, 2.pool.ntp.org and 3.pool.ntp.org. Then it concatenates the string ^(?:server|pool)[[:space:]] with each of the values from the split + the string $. That might be confusing but what happens in oval is de following:
I: oscap:     Variable 'oval:ssg-temp_variable_test:var:1' has values
 "^(?:server|pool)[[:space:]]0.pool.ntp.org$",
 "^(?:server|pool)[[:space: ]]1.pool.ntp.org$",
 "^(?:server|pool)[[:space:]]2.pool.ntp.org$",
 "^(?:server|pool)[[:space:]]3.pool.ntp.org$".
 [oscap(1934): oscap(7fba75ff0980):oval_variable.c:513:_dump_variable_values]
  1. Now that we have the local_variable setup, I just changed textfilecontent54_object to actually:
    2.1. use {{{ chrony_conf_path }}} just to keep consistency with the other rules
    2.2. make the pattern reference the local_variable we created, after all it is a pattern
    2.3. altered the instance check for more than one instance is a match, since we expect 4 matches.

This is the result I got:

image

You can see it correctly matched the four entries in chrony.conf

@dodys
Copy link
Contributor

dodys commented Mar 24, 2023

@vojtapolasek I wonder if we can have the OVAL change I mentioned above in chronyd_specify_remote_server and not need this new rule at all. Thoughts?

@rumch-se
Copy link
Contributor Author

Hello @dodys

I have corrected this rule, by following your approach. In addition I have added support for server and pool, because:

  • the server directive specifies an NTP server which can be used as a time source
  • the pool directive specifies a pool of NTP servers rather a single NTP server. The pool name is expected to resolve to multiple addresses which might change over time.
    I have defined a variable for pool.

Have a nice day
Rumen

dodys
dodys previously requested changes Mar 27, 2023
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.

a few minor things to fix, but overall it is looking good now

fi
done

# Check and configure pools in /etc/chorny.conf
Copy link
Contributor

Choose a reason for hiding this comment

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

typo, perhaps use the chrony_conf_path


config_file="{{{ chrony_conf_path }}}"

# Check and configigure servers in /etc/chrony.conf
Copy link
Contributor

Choose a reason for hiding this comment

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

better use chrony_conf_path

<concat>
<literal_component>^(?:pool)[[:space:]]</literal_component>
<split delimiter=",">
<variable_component var_ref="var_multiple_time_servers" />
Copy link
Contributor

Choose a reason for hiding this comment

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

wrong variable name


<ind:textfilecontent54_test check="all" check_existence="at_least_one_exists"
comment="Ensure remote NTP server is set " id="test_chrony_remote_server_configuration" version="1">
<ind:object object_ref="object_chrony_remote_server_configuartion" />
Copy link
Contributor

Choose a reason for hiding this comment

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

typo in configuration

<ind:object object_ref="object_chrony_remote_server_configuartion" />
</ind:textfilecontent54_test>

<ind:textfilecontent54_object comment="Ensure at least one NTP server is set" id="object_chrony_remote_server_configuartion" version="1">
Copy link
Contributor

Choose a reason for hiding this comment

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

typo in configuration

</local_variable>

<ind:textfilecontent54_test check="all" check_existence="at_least_one_exists"
comment="Ensure remote NTP server is set " id="test_chrony_remote_server_configuration" version="1">
Copy link
Contributor

Choose a reason for hiding this comment

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

please remove the space after "set" in comment


<ind:textfilecontent54_test check="all" check_existence="at_least_one_exists"
comment="Ensure remote NTP pool is set " id="test_chrony_remote_pool_configuration" version="1">
<ind:object object_ref="object_chrony_remote_pool_configuartion" />
Copy link
Contributor

Choose a reason for hiding this comment

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

typo in configuration

@dodys dodys requested a review from vojtapolasek March 27, 2023 16:34
@rumch-se
Copy link
Contributor Author

Hello @dodys
The necessary corrections were done.
Have a nice day
Rumen

@marcusburghardt
Copy link
Member

Related to #10278

@rumch-se rumch-se force-pushed the rule_ensure_chrony_is_configured branch from 05b8b42 to 9f31915 Compare April 6, 2023 07:34
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Used by openshift-ci bot. label Apr 6, 2023
@dodys
Copy link
Contributor

dodys commented Apr 15, 2023

Related to #10278

@marcusburghardt @vojtapolasek is there any reason for this new rule to not be integrated with the existing rule?

@mildas
Copy link
Contributor

mildas commented Apr 24, 2023

/packit test

@vojtapolasek
Copy link
Collaborator

@rumch-se I am very sorry for a long delay, I am going to review this PR asap since I know it is important for you.
@dodys the rule was not integrated into chronyd_specify_remote_server because I believe it introduces a big change to the rule from user's point of view.
I consider the case when user decices to use some time servers which are not configured in the XCCDF variable, for example their internal corporate servers. In that case the rule would start failing out of nowhere (if it was passing before). To make rule passing user needs to modify the XCCDF variable. I do not dispute that the rule in this PR is more secure and better, but I don't think we should simply replace the rule chronyd_specify_remote_server by this rule because of user experience. However, we might gradually transition to the new rule.

Copy link
Collaborator

@vojtapolasek vojtapolasek left a comment

Choose a reason for hiding this comment

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

Hello @rumch-se and thank you for this new rule. It looks challenging but I believe the goal you are trying to accomplish can be done.
See my comments please. Please fix the wrong variable name in the pci-dss profile so that the product can be built and tested by the CI.
I have two general comments:

  1. Could you consider a different rule name? This name sounds very generic - I believe time servers and pools are not the only configuration options which can be set in chrony.conf. How about chrony_configure_exact_servers... or something along the lines.
  2. I see that the rule remediation (I tried Bash so far) configures both servers and pools in the file. Is this what you want to achieve?
    Thank you once again.

<external_variable comment="remote vendor-approved time servers" datatype="string"
id="var_multiple_time_servers" version="1" />

<local_variable id="temp_variable_test_servers" datatype="string" version="1" comment="local var">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please make the variable name clearer and / or extend the comment. I think it is enough if you specify that this variable is a regexp.

<external_variable comment="remote vendor-approved pool servers" datatype="string"
id="var_multiple_time_pools" version="1" />

<local_variable id="temp_variable_test_pools" datatype="string" version="1" comment="local var">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please make the variable name clearer and / or extend the comment.

@@ -0,0 +1,25 @@
# platform = multi_platform_sle
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is no longer needed, the prodtype should ensure that the remediation is built only with specified products.

Suggested change
# platform = multi_platform_sle
# platform = multi_platform_all

@@ -0,0 +1,30 @@
# platform = multi_platform_sle
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# platform = multi_platform_sle
# platform = multi_platform_all

@@ -0,0 +1,31 @@
#!/bin/bash
# packages = chrony
# platform = multi_platform_sle
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just a note. Currently, the rule is applicable only to SLE products so this line is fine. If this rule is going to be used in more products, the test will need to be rewritten because each product will have its own correct XCCDF variable.
An example of such test scenario is here:
https://github.com/ComplianceAsCode/content/blob/9f319151147974e333bdb4e5474997a81bf97f3a/linux_os/guide/system/accounts/accounts-physical/logind_session_timeout/tests/correct.pass.sh

@@ -0,0 +1,7 @@
#!/bin/bash
# packages = chrony
# platform = multi_platform_sle
Copy link
Collaborator

Choose a reason for hiding this comment

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

Line not needed as long as the erroneous line does not appear in any value of the XCCDF variable related to the rule.

@@ -0,0 +1,7 @@
#!/bin/bash
# packages = chrony
# platform = multi_platform_sle
Copy link
Collaborator

Choose a reason for hiding this comment

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

LIne not needed as long as the incorrect line is incorrect for all applicable products.

@@ -0,0 +1,7 @@
#!/bin/bash
# packages = chrony
# platform = multi_platform_sle
Copy link
Collaborator

Choose a reason for hiding this comment

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

line not needed, see above cases of test scenarios starting with incorrect_line*.

@@ -0,0 +1,7 @@
#!/bin/bash
# packages = chrony
# platform = multi_platform_sle
Copy link
Collaborator

Choose a reason for hiding this comment

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

line probably not needed, see similar file

@@ -0,0 +1,9 @@
#!/bin/bash
# packages = chrony
# platform = multi_platform_sle
Copy link
Collaborator

Choose a reason for hiding this comment

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

See my comment on the same line in the file correct_chrony_configuration.pass.sh.

@rumch-se
Copy link
Contributor Author

Hello @vojtapolasek

I have implemented the proposed corrections.

Have a nice day
Rumen

Copy link
Collaborator

@vojtapolasek vojtapolasek left a comment

Choose a reason for hiding this comment

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

Thank you for the update. There are just minor fixes to Ansible remaining because in the current state it ofter terminates prenaturely.

- name: {{{ rule_title }}} - Add missing / update wrong records for remote time pools
ansible.builtin.lineinfile:
path: {{{ chrony_conf_path }}}
regexp: '^\s*\pool\b\s*\b{{ item }}\b$'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
regexp: '^\s*\pool\b\s*\b{{ item }}\b$'
regexp: '^\s*\bpool\b\s*\b{{ item }}\b$'

path: {{{ chrony_conf_path }}}
regexp: '^\s*\pool\b\s*\b{{ item }}\b$'
state: present
line: 'pool {{ item }}'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add create: true because if the file does not exist the task fails.

path: {{{ chrony_conf_path }}}
regexp: '^\s*\bserver\b\s*\b{{ item }}\b$'
state: present
line: 'server {{ item }}'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add create: true because if the file does not exist the task fails.

@rumch-se
Copy link
Contributor Author

Hello @vojtapolasek

The proposed changes were implemented
Have a nice day
Rumen

Copy link
Collaborator

@vojtapolasek vojtapolasek left a comment

Choose a reason for hiding this comment

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

Thank you for the update. It looks good now.
Automatus tests are failing because the rule is SLE specific and therefore Automats has nothing to test... and returns error.

@vojtapolasek vojtapolasek dismissed dodys’s stale review April 28, 2023 14:00

I did the review and I believe the PR is OK.

@vojtapolasek
Copy link
Collaborator

@teacup-on-rockingchair could you please review?

@vojtapolasek
Copy link
Collaborator

vojtapolasek commented May 2, 2023

@ComplianceAsCode/suse-maintainers please review the PR. Othervise, github does not allow merging. Thank you.

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, thanks

@vojtapolasek
Copy link
Collaborator

@rumch-se one more thing to consider... what do you think about splitting the rule into two rules? One would check for pools, one for servers. The reason why I am asking is for example this STIG item:
https://stigaview.com/products/rhel8/v1r9/RHEL-08-030740/
It does not mention pools at all, only the server directive.

@rumch-se
Copy link
Contributor Author

rumch-se commented May 4, 2023

Hello @vojtapolasek
What I know about server and pool is the following. In case of server you will have 1 source of time, in case of pool - you will have a set of servers. In other words, the pool is more robust - because if one of time servers is not active, the remaining ones will provide time. When I created the rule I used CIS recommendation where we have server and pool.
Have a nice day
Rumen

@vojtapolasek
Copy link
Collaborator

I understand. I think it would be nice to name the rule chronyd_configure_server_and_pool. We could then have separate rules (out of scope of this PR) chronyd_configure_server and chronyd_configure_pool. @yuumasato @jan-cerny ideas?

@openshift-merge-robot openshift-merge-robot added the needs-rebase Used by openshift-ci bot. label May 15, 2023
@rumch-se rumch-se force-pushed the rule_ensure_chrony_is_configured branch from 16df8c1 to 2c4a081 Compare May 16, 2023 11:03
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Used by openshift-ci bot. label May 16, 2023
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.

LGTM 👍

@vojtapolasek
Copy link
Collaborator

@rumch-se hello, please could you change the rule name? When you look at other rules in the content, they do not contain werbs in third person singular form. I still think that chronyd_configure_pool_and_server fits the rule purpose the best.

@rumch-se
Copy link
Contributor Author

Hello @vojtapolasek
I changed the name of the rule.
Have a nice day
Rumen

@codeclimate
Copy link

codeclimate bot commented May 23, 2023

Code Climate has analyzed commit 5be9450 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 52.5% (0.1% change).

View more on Code Climate.

@vojtapolasek
Copy link
Collaborator

Thank you @rumch-se, merging.

@vojtapolasek vojtapolasek merged commit 91ba1a5 into ComplianceAsCode:master May 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ok-to-test Used by openshift-ci bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants