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

Add new rule socket_systemd-journal-remote_disabled #10210

Merged
merged 16 commits into from
Mar 3, 2023

Conversation

dodys
Copy link
Contributor

@dodys dodys commented Feb 14, 2023

Description:

  • Rule to disable systemd-journal-remote.socket

Rationale:

  • This rule is needed on CIS for both Ubuntu and RHEL

@dodys dodys requested a review from a team as a code owner February 14, 2023 12:20
@github-actions
Copy link

github-actions bot commented Feb 14, 2023

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

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

Fedora Testing Environment
Open in Gitpod

Oracle Linux 8 Environment
Open in Gitpod

@marcusburghardt marcusburghardt added this to the 0.1.67 milestone Feb 14, 2023
@marcusburghardt marcusburghardt added New Rule Issues or pull requests related to new Rules. CIS CIS Benchmark related. labels Feb 14, 2023
@vojtapolasek vojtapolasek self-assigned this Feb 15, 2023
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, thank you for the new rule. See my comments. But here is the main question:
Are you aiming at disabling ONLY the socket as stated in CIS benchmark?
If yes, the you should know that themplate you are using disables the service unit as well as the socket unit.

@dodys dodys changed the title Add new rule service_systemd-journal-remote_disabled Add new rule socket_systemd-journal-remote_disabled Feb 17, 2023
@dodys
Copy link
Contributor Author

dodys commented Feb 21, 2023

/retest

@dodys
Copy link
Contributor Author

dodys commented Feb 22, 2023

/retest

@vojtapolasek
Copy link
Collaborator

@dodys are you planing to add test scenarios as well? How about Ansible remediation?

@dodys
Copy link
Contributor Author

dodys commented Feb 23, 2023

@dodys are you planing to add test scenarios as well? How about Ansible remediation?

Just pushed tests for it.
Regarding ansible I won't be pushing anything for it, as we don't support it and have no way to test it properly

@github-actions
Copy link

github-actions bot commented Feb 23, 2023

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

Click here to see the full diff
bash remediation for rule 'xccdf_org.ssgproject.content_rule_smartcard_auth' differs.
--- xccdf_org.ssgproject.content_rule_smartcard_auth
+++ xccdf_org.ssgproject.content_rule_smartcard_auth
@@ -15,7 +15,9 @@
 # The service may not be running because it has been started and failed,
 # so let's reset the state so OVAL checks pass.
 # Service should be 'inactive', not 'failed' after reboot though.
-/usr/bin/systemctl reset-failed "pcscd.socket"
+if /usr/bin/systemctl --failed | grep -q "pcscd.socket"; then
+ /usr/bin/systemctl reset-failed "pcscd.socket"
+fi
 
 # Configure the expected /etc/pam.d/system-auth{,-ac} settings directly
 #

bash remediation for rule 'xccdf_org.ssgproject.content_rule_service_chronyd_or_ntpd_enabled' differs.
--- xccdf_org.ssgproject.content_rule_service_chronyd_or_ntpd_enabled
+++ xccdf_org.ssgproject.content_rule_service_chronyd_or_ntpd_enabled
@@ -8,7 +8,9 @@
 # The service may not be running because it has been started and failed,
 # so let's reset the state so OVAL checks pass.
 # Service should be 'inactive', not 'failed' after reboot though.
- /usr/bin/systemctl reset-failed "chronyd"
+ if /usr/bin/systemctl --failed | grep -q "chronyd"; then
+ /usr/bin/systemctl reset-failed "chronyd"
+ fi
 fi
 elif rpm --quiet -q "ntp" ; then
 /usr/bin/systemctl enable "ntpd"
@@ -16,7 +18,9 @@
 # The service may not be running because it has been started and failed,
 # so let's reset the state so OVAL checks pass.
 # Service should be 'inactive', not 'failed' after reboot though.
- /usr/bin/systemctl reset-failed "ntpd"
+ if /usr/bin/systemctl --failed | grep -q "ntpd"; then
+ /usr/bin/systemctl reset-failed "ntpd"
+ fi
 else
 if ! rpm -q --quiet "chrony" ; then
 yum install -y "chrony"
@@ -26,7 +30,9 @@
 # The service may not be running because it has been started and failed,
 # so let's reset the state so OVAL checks pass.
 # Service should be 'inactive', not 'failed' after reboot though.
- /usr/bin/systemctl reset-failed "chronyd"
+ if /usr/bin/systemctl --failed | grep -q "chronyd"; then
+ /usr/bin/systemctl reset-failed "chronyd"
+ fi
 fi
 
 else

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,
thank you for updates. It wil need few more changes, see comments.

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, thank you for improvements. Just two more things and I think we are ready to merge it.

@dodys dodys requested a review from vojtapolasek March 1, 2023 10:26
@codeclimate
Copy link

codeclimate bot commented Mar 1, 2023

Code Climate has analyzed commit 1ca3238 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.

@openshift-ci
Copy link

openshift-ci bot commented Mar 1, 2023

@dodys: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-aws-ocp4-moderate-node 1ca3238 link true /test e2e-aws-ocp4-moderate-node

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

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.

Looks good now.
automatus tests are failing because during the PR development, there were added test scenarios (which passed), but later it was decided to restrict the rule to Ubuntu2204 prodtype. I think it does not make sense to remove those tests - it is possible that the rule will be eventually reused in RHEL since it is from CIS profile.

@vojtapolasek
Copy link
Collaborator

@dodys are you able to approve this PR? I can't merge it because @ComplianceAsCode/ubuntu-maintainers is required to approve the PR and you are the only member.@dodys are you planing to add test scenarios as well? How about Ansible remediation?

@dodys
Copy link
Contributor Author

dodys commented Mar 2, 2023

@dodys are you able to approve this PR? I can't merge it because @ComplianceAsCode/ubuntu-maintainers is required to approve the PR and you are the only member.@dodys are you planing to add test scenarios as well? How about Ansible remediation?

@vojtapolasek no I cannot approve PRs that I'm the author. @marcusburghardt can you help us here?
also @vojtapolasek regarding the test and ansible I've mentioned before:
"Just pushed tests for it.
Regarding ansible I won't be pushing anything for it, as we don't support it and have no way to test it properly"

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.

Thanks for this rule. It will also be useful for other distros. I will be happy to include the Ansible remediation for it.

@marcusburghardt
Copy link
Member

Automatus CS8, CS9 and Fedora are failing because the rule is restricted to ubuntu2204 in its prodtype. Waived.
ci/prow/e2e-aws-ocp4-moderate-node failed due to test environment issue but can safely be waived too since the rule is specific for ubuntu2204.

@marcusburghardt
Copy link
Member

Overriding CODEOWNERS since @dodys can't approve his own PR.

@marcusburghardt marcusburghardt merged commit 8d93b2b into ComplianceAsCode:master Mar 3, 2023
@dodys dodys deleted the systemd-journal-remote branch April 17, 2023 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CIS CIS Benchmark related. New Rule Issues or pull requests related to new Rules.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants