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

SLE Service timesyncd configured rule #10670

Conversation

teacup-on-rockingchair
Copy link
Contributor

Description:

  • Add rule check/andsible and bash remediations and tests for systemd timesyncd configured

Rationale:

  • Add definition of service_timesyncd_configured rule
  • Bash and Ansible remediations
  • Add rule service_timesyncd_configured to CIS SLE profiles

@teacup-on-rockingchair teacup-on-rockingchair added Ansible Ansible remediation update. Bash Bash remediation update. SLES SUSE Linux Enterprise Server product related. CIS CIS Benchmark related. labels Jun 2, 2023
@teacup-on-rockingchair teacup-on-rockingchair requested a review from a team as a code owner June 2, 2023 05:11
@github-actions
Copy link

github-actions bot commented Jun 2, 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

@jan-cerny
Copy link
Collaborator

@teacup-on-rockingchair The CI fails look valid. You should add your new rule ID at least to components/systemd.yml.

@jan-cerny
Copy link
Collaborator

@teacup-on-rockingchair you have some shellcheck warnings in the OpenSUSE CI job.

The fail on Fedora Rawhide is caused by OpenSCAP/openscap#1995 and isn't related to the contents of this PR.

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

The Automatus fail is expected because the rule is marked by a prodtype key as SLES only and therefore isn't part of RHEL content.

@jan-cerny jan-cerny added this to the 0.1.69 milestone Jul 10, 2023
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.

The tests folder is inside bash folder. I assume it was a mistake. Could you move it to the rule root folder, please?

@vojtapolasek vojtapolasek modified the milestones: 0.1.69, 0.1.70 Jul 18, 2023
<definition class="compliance" id="{{{ rule_id }}}" version="1">
{{{ oval_metadata("Ensure that timesyncd RootDistanceMaxSec is configured") }}}
<criteria comment="Timesyncd servers are configured" operator="AND">
<extend_definition comment="service timesyncd enabled"
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it is necessary to care about service state in this rule. It should be only about parameters values, regardless of the service being enabled or disabled. Could you consider removing it completely in this rule, please? Keeping the rule as simple as possible to ensure the proper values should be ideal. Service state should be managed by other rules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have two more generic concerns in this area, and would be happy to get your opinion:

  • if the rule is checked on its own and fails or reports false positive since the service is not enabled
  • or in the case we run all the rules but for some reason, say alphabetic order of execution, this rule reports OK, while there is problem with service enabled, so this rule will report wrongly pass

Another theoretical question, going even further out of context :). not about the rules but remediations. Say we have rule about service enabling and such for service configuration. Both initially failed, so a ansible or bash script is prepared containing remediation for at least those two rules. When that script is executed and say the service enabling is performed first, then configuration, often it is the case that the follow-up check will report pass, but actually the system is not in correct state, since the configuration is not applied, because the service was not restarted for example. What do you think is the best way to approach this scenario?

Copy link
Member

Choose a reason for hiding this comment

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

I have two more generic concerns in this area, and would be happy to get your opinion:

  • if the rule is checked on its own and fails or reports false positive since the service is not enabled

I don't see this situation as a false positive. In general it is better to have more specific and as granular as possible rules. If the rule is checking a configuration value, it will only pass if the configuration value is compliant. Another rule will check if the service is enabled and will only pass if the service is indeed enabled. It is even simpler to maintain the rule description, OVAL and remediation when we have simpler and specific rules. In a context of a profile, where both rules in this example are present, the report will clearly and specifically show what is missing or compliant. That is the idea of a profile, where many rules together creates a context and may complement each other to satisfy a requirement. O don't think it would make so much sense to include only one of these rules in a profile. On the other hand, if we have rules doing more than necessary it could be confusing to realize the reason it is failing.

  • or in the case we run all the rules but for some reason, say alphabetic order of execution, this rule reports OK, while there is problem with service enabled, so this rule will report wrongly pass

There are many cases like this. For example, when a rule is only applicable if a package is present, if the package is not initially installed, the check will report not applicable but then the package is installed by another requirement. In a second round the not applicable requirement may became a fail or pass. If it became fail, the remediation would be necessary. See OpenSCAP/openscap#1880 for other similar cases. Currently, the simplest solution in this case is to run the scan/remediation once more. Maybe it is not the ideal experience but it is not unreasonable too. Also, it would be a relatively complex case to be solved on the scanner side.

Another theoretical question, going even further out of context :). not about the rules but remediations. Say we have rule about service enabling and such for service configuration.

service enabling and service configuration should report not applicable if the service package is not present.
Otherwise, both should be checked and, if necessary, remediated.

Both initially failed, so a ansible or bash script is prepared containing remediation for at least those two rules. When that script is executed and say the service enabling is performed first, then configuration, often it is the case that the follow-up check will report pass, but actually the system is not in correct state, since the configuration is not applied, because the service was not restarted for example. What do you think is the best way to approach this scenario?

If a service needs to be restarted after configuration changes, it would make sense to ensure the service restart or reload in the respective remediation.

@marcusburghardt
Copy link
Member

@teacup-on-rockingchair , there are some pending suggestions about platform and it should be good to be merged. Could you take a look, please?

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.

Maybe it the future would be interesting to implement a variable for service_timesyncd_root_distance_configured if the policies become to ask any specific value. But for now it is doesn't look the case. Thanks for the rules.

@marcusburghardt
Copy link
Member

@teacup-on-rockingchair , could you resolve the conflict, please?

teacup-on-rockingchair and others added 21 commits July 27, 2023 18:17
…le/shared.yml

Co-authored-by: Marcus Burghardt <2074099+marcusburghardt@users.noreply.github.com>
…shared.sh

Co-authored-by: Marcus Burghardt <2074099+marcusburghardt@users.noreply.github.com>
…nfigured/tests/no_distance_config.fail.sh

Co-authored-by: Marcus Burghardt <2074099+marcusburghardt@users.noreply.github.com>
…nfigured/tests/timesyncd_config.pass.sh

Co-authored-by: Marcus Burghardt <2074099+marcusburghardt@users.noreply.github.com>
…/dropin_config.pass.sh

Co-authored-by: Marcus Burghardt <2074099+marcusburghardt@users.noreply.github.com>
Thanks to @marcusburghart 🙇
…shared.xml

Co-authored-by: Marcus Burghardt <2074099+marcusburghardt@users.noreply.github.com>
…nfigured/oval/shared.xml

Co-authored-by: Marcus Burghardt <2074099+marcusburghardt@users.noreply.github.com>
…/empty_config.fail.sh

Co-authored-by: Marcus Burghardt <2074099+marcusburghardt@users.noreply.github.com>
…/no_fallback.fail.sh

Co-authored-by: Marcus Burghardt <2074099+marcusburghardt@users.noreply.github.com>
…/timesyncd_config.pass.sh

Co-authored-by: Marcus Burghardt <2074099+marcusburghardt@users.noreply.github.com>
…nfigured/ansible/shared.yml

Co-authored-by: Marcus Burghardt <2074099+marcusburghardt@users.noreply.github.com>
…nfigured/bash/shared.sh

Co-authored-by: Marcus Burghardt <2074099+marcusburghardt@users.noreply.github.com>
…nfigured/oval/shared.xml

Co-authored-by: Marcus Burghardt <2074099+marcusburghardt@users.noreply.github.com>
…nfigured/oval/shared.xml

Co-authored-by: Marcus Burghardt <2074099+marcusburghardt@users.noreply.github.com>
…nfigured/tests/dropin_config.pass.sh

Co-authored-by: Marcus Burghardt <2074099+marcusburghardt@users.noreply.github.com>
…nfigured/tests/empty_config.fail.sh

Co-authored-by: Marcus Burghardt <2074099+marcusburghardt@users.noreply.github.com>
@codeclimate
Copy link

codeclimate bot commented Jul 27, 2023

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

View more on Code Climate.

@teacup-on-rockingchair
Copy link
Contributor Author

@jan-cerny, @marcusburghardt can you please overwrite the rule for suse-maintainers, since I am the only member of the group and cannot approve my own PRs

@marcusburghardt
Copy link
Member

/packit retest-failed

2 similar comments
@jan-cerny
Copy link
Collaborator

/packit retest-failed

@marcusburghardt
Copy link
Member

/packit retest-failed

@marcusburghardt
Copy link
Member

/packit build

@marcusburghardt
Copy link
Member

Overriding CODEOWNERS as @teacup-on-rockingchair can't approve his own PR.

@marcusburghardt marcusburghardt merged commit 9618f19 into ComplianceAsCode:master Aug 2, 2023
10 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. Bash Bash remediation update. CIS CIS Benchmark related. SLES SUSE Linux Enterprise Server product related.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants