-
Notifications
You must be signed in to change notification settings - Fork 39.8k
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
Auto approve kubelet server certificate signing requests. #46884
Auto approve kubelet server certificate signing requests. #46884
Conversation
capi.UsageServerAuth, | ||
} | ||
|
||
func isSelfNodeServerCert(csr *capi.CertificateSigningRequest, x509cr *x509.CertificateRequest) bool { | ||
if !reflect.DeepEqual([]string{"system:nodes"}, x509cr.Subject.Organization) { |
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.
This doesn't check it is from a node
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 missing a check like https://github.com/kubernetes/kubernetes/pull/46884/files#diff-05476437bf221fbe4cab028bf7ffbb74R172
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.
Added.
fb0968a
to
0aebbe3
Compare
0aebbe3
to
1e4a7ef
Compare
@@ -187,14 +187,25 @@ func isSelfNodeClientCert(csr *capi.CertificateSigningRequest, x509cr *x509.Cert | |||
|
|||
var kubeletServerUsages = []capi.KeyUsage{ | |||
capi.UsageKeyEncipherment, | |||
capi.UsageDigitalSignature, | |||
capi.UsageSigning, | |||
capi.UsageServerAuth, | |||
} | |||
|
|||
func isSelfNodeServerCert(csr *capi.CertificateSigningRequest, x509cr *x509.CertificateRequest) bool { |
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.
Feature flag this so it returns false if the kubelet server cert rotation feature is off.
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.
Done.
You need this: diff --git a/cluster/addons/rbac/kubelet-certificate-management.yaml b/cluster/addons/rbac/kubelet-certificate-management.yaml
index 4b4b3d7381..83f1187d57 100644
--- a/cluster/addons/rbac/kubelet-certificate-management.yaml
+++ b/cluster/addons/rbac/kubelet-certificate-management.yaml
@@ -57,5 +57,6 @@ rules:
- "certificates.k8s.io"
resources:
- certificatesigningrequests/selfnodeclient
+ - certificatesigningrequests/selfnodeserver
verbs:
- "create" |
1e4a7ef
to
993a783
Compare
capi.UsageServerAuth, | ||
} | ||
|
||
func isSelfNodeServerCert(csr *capi.CertificateSigningRequest, x509cr *x509.CertificateRequest) bool { | ||
if !hasExactUsages(csr, kubeletServerUsages) { | ||
if utilfeature.DefaultFeatureGate.Enabled(features.RotateKubeletServerCertificate) { |
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.
Invert this. If feature not enabled return false.
993a783
to
b34dc6e
Compare
/lgtm |
/approve |
pkg/kubelet/kubelet.go
Outdated
// | ||
// Signing allows the certificate to be used to verify | ||
// digital signatures used during TLS negotiation. | ||
certificates.UsageSigning, |
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.
what key usage or ext key usage does this correspond to?
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.
These are the same thing, I wish we had noticed that earlier.
https://github.com/cloudflare/cfssl/blob/437c6213e569dde639fd116272c27557fd38eaf9/config/config.go#L553
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.
oh, digital signature...
"signing": x509.KeyUsageDigitalSignature,
"digital signature": x509.KeyUsageDigitalSignature,
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.
can we actually change the kubelet to use the usage that corresponds to the RFC? "signing" is pretty ambiguous
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.
Changed to UsageDigitalSignature is both locations.
@@ -187,14 +189,28 @@ func isSelfNodeClientCert(csr *capi.CertificateSigningRequest, x509cr *x509.Cert | |||
|
|||
var kubeletServerUsages = []capi.KeyUsage{ | |||
capi.UsageKeyEncipherment, | |||
capi.UsageDigitalSignature, | |||
capi.UsageSigning, |
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'd prefer this to keep the usage that matches the term used in the RFC
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.
Changed back to UsageDigitalSignature.
b34dc6e
to
dacfede
Compare
dacfede
to
d2f9643
Compare
/lgtm |
d2f9643
to
0b4f825
Compare
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.
/lgtm
/approve |
/unassign @derekwaynecarr |
@@ -1124,16 +1124,25 @@ func initializeServerCertificateManager(kubeClient clientset.Interface, kubeCfg | |||
CertificateSigningRequestClient: certSigningRequestClient, | |||
Template: &x509.CertificateRequest{ | |||
Subject: pkix.Name{ | |||
CommonName: string(nodeName), | |||
CommonName: fmt.Sprintf("system:node:%s", nodeName), |
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'd really like to see this change make 1.7. I'd prefer not to have a mix of CSR formats to recognize unnecessarily
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.
Working on it.
@dchen1107 this fixes the alpha e2e tests and only touches alpha codepaths. Please add 1.7 milestone |
We need this to fix broken e2es. |
) | ||
|
||
func init() { | ||
utilfeature.DefaultFeatureGate.Set(string(features.RotateKubeletServerCertificate) + "=true") |
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 wasn't expecting this... the approver is permission based... I only expected the kubelet rotation to be gated by the flag. Also, setting this one way in an init() function means it is always enabled when testing, right?
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 is only enabled in this package test, yes.
@@ -192,9 +194,23 @@ var kubeletServerUsages = []capi.KeyUsage{ | |||
} | |||
|
|||
func isSelfNodeServerCert(csr *capi.CertificateSigningRequest, x509cr *x509.CertificateRequest) bool { | |||
if !utilfeature.DefaultFeatureGate.Enabled(features.RotateKubeletServerCertificate) { |
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 didn't expect the approver to be flag gated this way, only the kubelet function
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 you want to gate it in the apiserver, gate adding isSelfNodeServerCert in recognizers()
, and you can unit test isSelfNodeServerCert without messing with feature flag enablement in init()
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 recommended to @jcbsmpsn that we do this. I really don't want to be tied to the behavior of isSelfNodeServer until we have thought through kubelets self reporting their SANs. What we currently allow is too weak, and this is an API.
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.
ok, then move the gate to recognizers(), and remove the gate-twiddling from the init()
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.
that sounds good to me.
I am fine with kubelet change, which is trivial anyway. But I am not comfortable with the changes in pkg/controller/certificates/approver simply due to the lack of the knowledge from my side. I will leave @liggitt to make the final call. |
0b4f825
to
e1b05e0
Compare
|
e1b05e0
to
334de1c
Compare
/lgtm |
/lgtm function is gated and off by default, and this helps fix the alpha features CI |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jcbsmpsn, liggitt, mikedanese, timstclair Associated issue: 47208 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
Automatic merge from submit-queue (batch tested with PRs 46884, 47557) |
Fixes #47208
Release note: