-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[WIP] 🌱 Change crt permissions in KCP to 0600 #12648
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Stefan Büringer [email protected]
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@@ -393,7 +393,7 @@ func (c *Certificate) AsFiles() []bootstrapv1.File { | |||
out = append(out, bootstrapv1.File{ | |||
Path: c.CertFile, | |||
Owner: rootOwnerValue, | |||
Permissions: "0640", | |||
Permissions: "0600", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Context:
- the latest CIS 1.10 profile suggests to use 0600 for these certificates: https://github.com/aquasecurity/kube-bench/blob/main/cfg/cis-1.10/master.yaml#L292-L306
- kubeadm uses 0644 today: https://github.com/kubernetes/kubernetes/blob/88dfa51b6003c90e8f0a0508939a1d79950a40df/staging/src/k8s.io/client-go/util/cert/io.go#L68-L67
I wonder if 0600 leads to problems with: kubernetes/kubeadm#2473 (comment)
@neolit123 What do you think?
Is there a way that I can verify this works with userns? Do I only have to use Kubernetes >= 1.33?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if i understand how userns works, this might be fine. you can test with any new release that has userns support. i don't think we ever tested kubeadm with userns yet, fwiw.
they are still fixing bugs and updating docs for the feature and we need it to be ga.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, cis has been annoying since it doesn't consider permissions of the parent dir. distributions on top of kubeadm and capi ca choose to generate their own certs, but that's extra work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering if "userns is being enabled by default in 1.33" (kubernetes/kubeadm#2473 (comment)) means that it's also used for static Pods per default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's enabled in core k8s, but pod users must opt-in; kubeadm hasn't yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, thx!
/test pull-cluster-api-e2e-main |
Signed-off-by: Stefan Büringer [email protected]
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #