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

Fix v1.24 Cluster Scan Failures #92

Merged
merged 4 commits into from
Jul 22, 2022

Conversation

rayandas
Copy link
Contributor

After QA validation, they found that the --insecure-port check is failing on every cluster. So this PR fixes them all.
check_cafile_permissions.sh script is also failing (on K8s 1.24 cluster) for RKE1 permissive profile scans, this PR fixes that as well.
Related:
rancher/cis-operator#135
rancher/cis-operator#153
Note: All the fixes are tested on respective clusters.

@rayandas rayandas requested a review from macedogm July 22, 2022 12:55
@rayandas
Copy link
Contributor Author

merging the PR.

@rayandas rayandas merged commit 4538276 into rancher:master Jul 22, 2022
@rancher-max
Copy link
Member

I don't believe that setting an or condition to check "whether the insecure-port flag is either set to 0 or not set at all" is sufficient. The flag is removed as part of 1.24+ (see rancher/cis-operator#158), but is still present for all previous releases and has a default value of 8080.

The change in this PR could therefore result in falsely passing scans (both permissive and hardened) for any minor release prior to 1.24.

@mitulshah-suse
Copy link
Contributor

Referring to the kubernetes/kubernetes#91506 (comment) it looks like this flag was removed in 1.21 itself and only allows it to be set to 0 if at all it is used.

This flag was deprecated back in 1.10 kubernetes/kubernetes#91506 (comment)

Also, none of the kubernetes documentation from 1.20+ has this flag as an option for apiserver.
https://kubernetes.io/docs/reference/command-line-tools-reference/kube-apiserver/
https://v1-23.docs.kubernetes.io/docs/reference/command-line-tools-reference/kube-apiserver/
https://v1-22.docs.kubernetes.io/docs/reference/command-line-tools-reference/kube-apiserver/
https://v1-21.docs.kubernetes.io/docs/reference/command-line-tools-reference/kube-apiserver/
https://v1-20.docs.kubernetes.io/docs/reference/command-line-tools-reference/kube-apiserver/

So, I think we are fine for versions 1.21+
Are we expecting different scan conditions for different kubernetes versions? On clusters below 1.21 the flag should mandatorily be set to 0 and on versions above the or condition currently defined will suffice?
@rancher-max Please advise

@rancher-max
Copy link
Member

You're right that the flag itself was semi-removed in 1.21. My previous message was therefore somewhat incorrect. The problem then is similar though: "The change in this PR could therefore result in falsely passing scans for any minor release prior to 1.21.

From what I understand, our profiles conform to the CIS Benchmark guides. The starred one in the below screenshot is our default:
image

I don't think it's correct to necessarily change what our checks are doing to be different from these. Rather we should properly to document to users that "if they are using kubernetes v1.23+, they should use the 1.23 scan profile". I obtained the following information from these PDFs:

  • The 1.20 guide DOES include a check for the insecure-port arg:

image

  • The 1.23 guide does NOT include a check for the insecure-port arg

  • The default 1.6 guide DOES include a check for the insecure-port arg:

image


To me, it's almost more correct if we don't have this OR condition that was added as part of the PR, and instead actually keep the passing scans.

@mitulshah-suse
Copy link
Contributor

So please confirm if we should do the below changes

  • For 1.20 and 1.6, we have a mandatory check for insecure port flag without the OR condition
  • For 1.23 we remove the scan itself, since it it not part of the guide.

Please note that this will show Failed scans for 1.20 and 1.6 on clusters where this flag is missing. So please confirm that this is the expected result and we are fine with those failures.

@rancher-max
Copy link
Member

I think that's the correct approach, but I am not the right person to make the call. Adding @prachidamle @brandonsuse @samkulkarni20 @MKlimuszka for visibility/prioritization.

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