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

ING RKE2-CIS-1.24 CHECKS #1685

Closed

Conversation

sm171190
Copy link
Contributor

@sm171190 sm171190 commented Sep 19, 2024

In this change we are making 2 changes:

  1. adding the check Type as manual for some manual checks for which the type was missing or incorrect - RKe2-cis-1.24 -(1.1.10,1.1.20,1.1.21)
    Some checks are being skipped(1.3.6,4.2.12)as they are not applicable for an RKE2 cluster
    checks 1.3.6 and 4.2.12 check whether the cluster has been configured with the flags to rotate TLS certificates. However, RKE Internally handles certificate rotation : Rotate certificates for RKE2 provisioned clusters rancher/dashboard#4485

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Saurabh Misra seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@sm171190 sm171190 mentioned this pull request Sep 19, 2024
@itaysk itaysk requested a review from afdesk September 19, 2024 08:07
@afdesk
Copy link
Collaborator

afdesk commented Sep 19, 2024

Hi guys!
give me an hour to review it.
I have some notes.
thanks!

Copy link
Collaborator

@afdesk afdesk left a comment

Choose a reason for hiding this comment

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

@sm171190
I left some comments. Please feel free to correct me. thanks

@@ -318,6 +319,7 @@ groups:
For example,
chmod -R 600 /var/lib/rancher/rke2/server/tls/*.crt
scored: false
type: manual
Copy link
Collaborator

Choose a reason for hiding this comment

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

it seems this check (1.1.20) is automated by the docs.
and text field contains a mistake.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@afdesk I was referring to https://docs.rke2.io/security/cis_self_assessment124#1110-ensure-that-the-container-network-interface-file-ownership-is-set-to-root-manual. Here 1.1.10,1.1.20 and 1.1.21 are mentioned as manual
Would you like me to revert the change?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@@ -159,6 +159,7 @@ groups:
For example,
chown root:root <path/to/cni/files>
scored: false
type: manual
Copy link
Collaborator

Choose a reason for hiding this comment

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

it's a bit strange. because the same test for RKE1 is automated.
and audit shows commands for check...

Copy link
Collaborator

Choose a reason for hiding this comment

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

but it doesn't matter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cfg/rke2-cis-1.24/node.yaml Outdated Show resolved Hide resolved
@@ -432,14 +432,14 @@ groups:
- flag: RotateKubeletServerCertificate
path: '{.featureGates.RotateKubeletServerCertificate}'
set: false
type: skip
Copy link
Collaborator

Choose a reason for hiding this comment

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

I can see that check 1.3.6 isn't applicable for RKE2: the docs

but 4.2.12 is manual check and should pass: 4.2.12 Verify that the RotateKubeletServerCertificate argument is set to true (Manual)

maybe we shouldn't skip it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

@afdesk
Copy link
Collaborator

afdesk commented Sep 19, 2024

@sm171190 could you sign CLA for tests?

In this change we are making 2 changes:
1. Seom CIS-1.24 checks(1.3.6 , 4.2.12 are not relevant for RKE clusters). So we will skip them.
2. SomeCIS-1.24 tests(1.1.10,1.1.20) are manual checks yet they don't have the type correctly set causing KB to run them with all automated tests.
@sm171190
Copy link
Contributor Author

@sm171190 I left some comments. Please feel free to correct me. thanks
@afdesk Can you please the PR now? Thanks

@afdesk afdesk self-requested a review September 19, 2024 11:14
Copy link
Collaborator

@afdesk afdesk left a comment

Choose a reason for hiding this comment

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

@sm171190 I'd like your point about this diffs?
what is the reason of these updates? thanks!

@@ -335,6 +337,7 @@ groups:
For example,
chmod -R 600 /var/lib/rancher/rke2/server/tls/*.key
scored: false
type: manual
Copy link
Collaborator

Choose a reason for hiding this comment

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

@@ -432,6 +432,7 @@ groups:
- flag: RotateKubeletServerCertificate
path: '{.featureGates.RotateKubeletServerCertificate}'
set: false
type: manual
Copy link
Collaborator

Choose a reason for hiding this comment

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

@afdesk
Copy link
Collaborator

afdesk commented Sep 19, 2024

and tests can't be passed without CLA

@afdesk
Copy link
Collaborator

afdesk commented Sep 19, 2024

also it makes sense to update the PR description, and add a link to the docs there.
for future.

@afdesk
Copy link
Collaborator

afdesk commented Sep 19, 2024

just note
@sm171190 you're right, we should keep https://docs.rke2.io/,
because https://ranchermanager.docs.rancher.com/ looks old:

Last updated on Oct 12, 2023

@sm171190 sm171190 closed this by deleting the head repository Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants