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

Review sshd_set_maxstartups rule #12419

Merged
merged 9 commits into from
Sep 26, 2024

Conversation

marcusburghardt
Copy link
Member

@marcusburghardt marcusburghardt commented Sep 25, 2024

Description:

The template sshd_lineinfile was recently improved (#12251) to use variables. The rule sshd_set_maxstartups is used in different profiles and could benefit from this template, but the SSH MaxStartups parameter has some particularities in its OVAL.

During the review, I concluded that is better to keep a specific OVAL for sshd_set_maxstartups since there is no other rules using a similar parameter or even similar parameters in SSH configuration. So it would bring an unnecessary complexity to the templated OVAL. But the bash, Ansible and test scenarios could be nicely used to keep a better consistency among SSH rules.

This PR:

  • Refactor the sshd_set_maxstartups OVAL logic
    • Remove incorrect assumptions about parameter duplication
    • Remove hard-coded values and use variable values instead, giving more flexibility to the rule.
  • Use the sshd_lineinfile template for remediation and tests.
  • Increase coverage of test scenarios.

Rationale:

Review Hints:

./build_product rhel9
./tests/automatus.py rule --libvirt qemu:///session rhel9 --datastream build/ssg-rhel9-ds.xml --dontclean --remediate-using bash sshd_set_maxstartups

Other CI tests will help us to test the changes in a context of a profile. But I expect everything to be green.

Improve readability and ocil. Also removed removed CIS specific mention
from the description.

Signed-off-by: Marcus Burghardt <[email protected]>
There is no change in logic but only minor updates to make it easier to
read and more aligned to project style guide.

Signed-off-by: Marcus Burghardt <[email protected]>
Signed-off-by: Marcus Burghardt <[email protected]>
The rule uses a variable but the OVAL check was not considering the
variable values. It was instead using hard-coded values. The OVAL was
updated to consume the values from the variable.

Signed-off-by: Marcus Burghardt <[email protected]>
Use directly a variable value instead of referencing a profile in test
scenarios. This way the test scenarios won't break if the profile
changes. It was also included two new test scenarios for stricter and
lenient tests.

Signed-off-by: Marcus Burghardt <[email protected]>
The OVAL logic sshd_set_maxstartups is very specific and it is not worth
to move it in the template, but we can benefit from the templated
remediation and test scenarios.

Signed-off-by: Marcus Burghardt <[email protected]>
The OVAL check would fail if the parameter was mentioned twice, for
example in the main configuration file and in a drop-in file. It is
not a problem at all if both are compliant.

Signed-off-by: Marcus Burghardt <[email protected]>
These tests are already covered by the template. Only two test scenarios
specific for this rule were kept.

Signed-off-by: Marcus Burghardt <[email protected]>
@marcusburghardt marcusburghardt added refactoring Improvement which, once completed, will enable the project to progress faster. Update Rule Issues or pull requests related to Rules updates. Update Template Issues or pull requests related to Templates updates. labels Sep 25, 2024
@marcusburghardt marcusburghardt added this to the 0.1.75 milestone Sep 25, 2024
Copy link

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

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_sshd_set_maxstartups'.
--- xccdf_org.ssgproject.content_rule_sshd_set_maxstartups
+++ xccdf_org.ssgproject.content_rule_sshd_set_maxstartups
@@ -3,16 +3,11 @@
 Ensure SSH MaxStartups is configured
 
 [description]:
-The MaxStartups parameter specifies the maximum number of concurrent
-unauthenticated connections to the SSH daemon. Additional connections will be
-dropped until authentication succeeds or the LoginGraceTime expires for a
-connection. To confgure MaxStartups, you should add or correct the following
-line in the
-/etc/ssh/sshd_config file:
+The MaxStartups parameter specifies the maximum number of concurrent unauthenticated
+connections to the SSH daemon. Additional connections will be dropped until authentication
+succeeds or the LoginGraceTime expires for a connection. To configure MaxStartups, you should
+add or edit the following line in the /etc/ssh/sshd_config file:
 MaxStartups 'xccdf_org.ssgproject.content_value_var_sshd_set_maxstartups'
-        
-CIS recommends a MaxStartups value of '10:30:60', or more restrictive where
-dictated by site policy.
 
 [reference]:
 4.2.17
@@ -24,9 +19,9 @@
 2.2
 
 [rationale]:
-To protect a system from denial of service due to a large number of pending
-authentication connection attempts, use the rate limiting function of MaxStartups
-to protect availability of sshd logins and prevent overwhelming the daemon.
+To protect a system from denial of service due to a large number of pending authentication
+connection attempts, use the rate limiting function of MaxStartups to protect availability of
+sshd logins and prevent overwhelming the daemon.
 
 [ident]:
 CCE-90718-8

OCIL for rule 'xccdf_org.ssgproject.content_rule_sshd_set_maxstartups' differs.
--- ocil:ssg-sshd_set_maxstartups_ocil:questionnaire:1
+++ ocil:ssg-sshd_set_maxstartups_ocil:questionnaire:1
@@ -1,5 +1,5 @@
 To check if MaxStartups is configured, run the following command:
-$ sudo grep MaxStartups /etc/ssh/sshd_config
+$ sudo grep -r ^[\s]*MaxStartups /etc/ssh/sshd_config*
 If configured, this command should output the configuration.
       Is it the case that maxstartups is not configured?
       
bash remediation for rule 'xccdf_org.ssgproject.content_rule_sshd_set_maxstartups' differs.
--- xccdf_org.ssgproject.content_rule_sshd_set_maxstartups
+++ xccdf_org.ssgproject.content_rule_sshd_set_maxstartups
@@ -2,6 +2,7 @@
 if [ ! -f /.dockerenv ] && [ ! -f /run/.containerenv ]; then
 
 var_sshd_set_maxstartups=''
+
 
 
 if [ -e "/etc/ssh/sshd_config" ] ; then

Copy link

github-actions bot commented Sep 25, 2024

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

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

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

@jan-cerny jan-cerny self-assigned this Sep 26, 2024
@jan-cerny
Copy link
Collaborator

The Jira link is wrong, it should be https://issues.redhat.com/browse/RHEL-38206

@marcusburghardt
Copy link
Member Author

The Jira link is wrong, it should be https://issues.redhat.com/browse/RHEL-38206

Thanks for alerting me on that. It is fixed.

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 test scenarios pass for me on both RHEL 8 and 9.

I only have a minor problem.

Signed-off-by: Marcus Burghardt <[email protected]>
Copy link

codeclimate bot commented Sep 26, 2024

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

View more on Code Climate.

@jan-cerny jan-cerny merged commit ca4d055 into ComplianceAsCode:master Sep 26, 2024
100 checks passed
@marcusburghardt marcusburghardt deleted the sshd_maxstartups branch September 26, 2024 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Improvement which, once completed, will enable the project to progress faster. Update Rule Issues or pull requests related to Rules updates. Update Template Issues or pull requests related to Templates updates.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants