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

FIXING RKE2-CIS-1.24 CHECKS #1686

Closed

Conversation

sm171190
Copy link
Contributor

@sm171190 sm171190 commented Sep 19, 2024

. MASTER:
a. Checks 1.1.10,1.1.20 are manual according to https://docs.rke2.io/security/cis_self_assessment124#1110-ensure-that-the-container-network-interface-file-ownership-is-set-to-root-manual and https://docs.rke2.io/security/cis_self_assessment124#1110-ensure-that-the-container-network-interface-file-ownership-is-set-to-root-manual respectively.
b. Check 1.3.6 is not relevant to an RKE2 cluster as RKE2 rotates TLS certificates internally - rancher/dashboard#4485. We will skip it and not score it
2. NODE:
a. Check 4.2.12 is the node-level equivalent of the master-level check 1.3.6 and is treated the same way.
@afdesk request review please.

. MASTER:
            a. Checks 1.1.10,1.1.20 are manual according to https://docs.rke2.io/security/cis_self_assessment124#1110-ensure-that-the-container-network-interface-file-ownership-is-set-to-root-manual and https://docs.rke2.io/security/cis_self_assessment124#1110-ensure-that-the-container-network-interface-file-ownership-is-set-to-root-manual respectively.
            b. Check 1.3.6 is not relevant to an RKE2 cluster as RKE2 rotates TLS certificates internally - rancher/dashboard#4485. We will skip it and not score it

    2. NODE:
            a. Check 4.2.12 is the node-level equivalent of the master-level check 1.3.6 and is treated the same way.
@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.

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

afdesk commented Sep 19, 2024

Contributor License Agreement is not signed yet.

@@ -979,7 +981,7 @@ groups:
Edit the Controller Manager pod specification file $controllermanagerconf
on the control plane node and set the --feature-gates parameter to include RotateKubeletServerCertificate=true.
--feature-gates=RotateKubeletServerCertificate=true
scored: true
scored: false
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm still not sure about this. but if you're - we can merge it

my concerns are next:

but we want to ignore it.

What did I miss? thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

These are two separate features, and based on this, As I see it we should not be skipping the check.

@sm171190 The --feature-gates option has been removed. You can verify this at https://kubernetes.io/docs/reference/command-line-tools-reference/feature-gates-removed/ to see if it helps clarify the issue.

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 Based on the reference shared by @ManojShastha https://kubernetes.io/docs/reference/command-line-tools-reference/feature-gates-removed/ the RotateKubeletServerCertificate feature gate was in GA from v1.19 to v1.21. SInce we're dealing with checks on k8s 1.24 I think we can skip it. Whatsay?

@sm171190
Copy link
Contributor Author

Contributor License Agreement is not signed yet.

Have signed it already :)

@afdesk
Copy link
Collaborator

afdesk commented Sep 20, 2024

Contributor License Agreement is not signed yet.

Have signed it already :)

Github still doesn't detect it. Could you recheck?

@@ -440,6 +440,7 @@ groups:
systemctl daemon-reload
systemctl restart kubelet.service
scored: false
type: skip
Copy link
Contributor

Choose a reason for hiding this comment

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

also here.

The --feature-gates option has been removed. You can verify this at https://kubernetes.io/docs/reference/command-line-tools-reference/feature-gates-removed/ to see if it helps clarify the issue.

@@ -154,6 +154,7 @@ groups:
tests:
test_items:
- flag: "root:root"
type: manual
Copy link
Contributor

Choose a reason for hiding this comment

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

From the provided link, the check is labeled "manual." What’s the rationale behind setting the type to "manual"?

We shouldn't classify checks titled "manual" as type "manual," as they have different meanings.

The "manual" type indicates tests that cannot be automated. These tests always yield a WARN state and require user intervention.

CIS’s tool, CAT, has shifted from "score/not scored" to "Automated/Manual" in the 1.6 CIS benchmark to align with its functionality. We follow CIS benchmarks, which is why we use these titles.

Copy link
Contributor Author

@sm171190 sm171190 Sep 20, 2024

Choose a reason for hiding this comment

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

@mjshastha That is precisely why we want to keep type as manual. Someone using Kube-Bench recently complained about false positives regarding checks that CIS mentioned as Manual. The issue was that these users were seeing a PASS/FAIL for this check and wanted to cross-check. Setting type as manual will mark this check as WARN and not lead to someone thinking that Kube-Bench has incorrectly marked an audit as PAss/FAIL. WARN will, in some sense, force them to run the audit manually to verify - which is the very purpose of a Manual test to begin with ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

If users have previously complained about false positives, it would be more beneficial to verify the check and adjust its implementation accordingly, rather than marking it as type: manual.

Marking it as manual does not address the root cause of the issue and may lead to confusion about the check's validity. We should only set the type to manual if it is truly a non-automatable check.

@@ -313,6 +314,7 @@ groups:
op: bitmask
value: "600"
set: true
type: manual
Copy link
Contributor

Choose a reason for hiding this comment

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

also here.

From the provided link, the check is labeled "manual." What’s the rationale behind setting the type to "manual"?

We shouldn't classify checks titled "manual" as type "manual," as they have different meanings.

The "manual" type indicates tests that cannot be automated. These tests always yield a WARN state and require user intervention.

CIS’s tool, CAT, has shifted from "score/not scored" to "Automated/Manual" in the 1.6 CIS benchmark to align with its functionality. We follow CIS benchmarks, which is why we use these titles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mjshastha That is precisely why we want to keep type as manual. Someone using Kube-Bench recently complained about false positives regarding checks that CIS mentioned as Manual. The issue was that these users were seeing a PASS?FAIL fo rthis check and wanted to cross-check. Setting type as manual will mark this check as WARN and not lead to someone thinking that Kube-Bench has incorrectly marked an audit as PAss/FAIL. WARN will, in some sense, force them to run the audit manually to verify - which is the very purpose of a Manual test to begin with ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

If users have previously complained about false positives, it would be more beneficial to verify the check and adjust its implementation accordingly, rather than marking it as type: manual.

Marking it as manual does not address the root cause of the issue and may lead to confusion about the check's validity. We should only set the type to manual if it is truly a non-automatable check.

@afdesk
Copy link
Collaborator

afdesk commented Sep 20, 2024

HI guys!
@sm171190 @mjshastha could you confirm the status of this PR?
thanks

@andypitcher
Copy link
Contributor

andypitcher commented Sep 20, 2024

Adding @dereknola (RKE2/K3s) for review, the RKE2/K3s profiles have been recently improved to be more accurate.

@sm171190 sm171190 closed this by deleting the head repository Sep 23, 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.

5 participants