From e6176c28b74bd85be9d3d4d04324e0d518218107 Mon Sep 17 00:00:00 2001 From: Paco Xu Date: Mon, 11 Jul 2022 09:42:37 +0800 Subject: [PATCH] modify the signing/approving controller to tolerate either set of usages for kubelet client and serving certificates Signed-off-by: Paco Xu --- pkg/apis/certificates/helpers.go | 60 ++++++++++++----- pkg/apis/certificates/v1beta1/defaults.go | 20 +++--- .../certificates/v1beta1/defaults_test.go | 67 ++++++++++++++++--- .../certificates/approver/sarapprove.go | 2 +- .../certificates/approver/sarapprove_test.go | 22 ++++++ pkg/controller/certificates/signer/signer.go | 4 +- .../certificates/signer/signer_test.go | 55 +++++++++++++++ 7 files changed, 191 insertions(+), 39 deletions(-) diff --git a/pkg/apis/certificates/helpers.go b/pkg/apis/certificates/helpers.go index ef88bf8a527..b50317df27c 100644 --- a/pkg/apis/certificates/helpers.go +++ b/pkg/apis/certificates/helpers.go @@ -50,16 +50,22 @@ var ( uriSANNotAllowedErr = fmt.Errorf("URI subjectAltNames are not allowed") ) -var kubeletServingRequiredUsages = sets.NewString( - string(UsageDigitalSignature), - string(UsageKeyEncipherment), - string(UsageServerAuth), +var ( + kubeletServingRequiredUsages = sets.NewString( + string(UsageDigitalSignature), + string(UsageKeyEncipherment), + string(UsageServerAuth), + ) + kubeletServingRequiredUsagesNoRSA = sets.NewString( + string(UsageDigitalSignature), + string(UsageServerAuth), + ) ) -func IsKubeletServingCSR(req *x509.CertificateRequest, usages sets.String) bool { - return ValidateKubeletServingCSR(req, usages) == nil +func IsKubeletServingCSR(req *x509.CertificateRequest, usages sets.String, allowOmittingUsageKeyEncipherment bool) bool { + return ValidateKubeletServingCSR(req, usages, allowOmittingUsageKeyEncipherment) == nil } -func ValidateKubeletServingCSR(req *x509.CertificateRequest, usages sets.String) error { +func ValidateKubeletServingCSR(req *x509.CertificateRequest, usages sets.String, allowOmittingUsageKeyEncipherment bool) error { if !reflect.DeepEqual([]string{"system:nodes"}, req.Subject.Organization) { return organizationNotSystemNodesErr } @@ -76,8 +82,14 @@ func ValidateKubeletServingCSR(req *x509.CertificateRequest, usages sets.String) return uriSANNotAllowedErr } - if !kubeletServingRequiredUsages.Equal(usages) { - return fmt.Errorf("usages did not match %v", kubeletServingRequiredUsages.List()) + if allowOmittingUsageKeyEncipherment { + if !kubeletServingRequiredUsages.Equal(usages) && !kubeletServingRequiredUsagesNoRSA.Equal(usages) { + return fmt.Errorf("usages did not match %v", kubeletServingRequiredUsages.List()) + } + } else { + if !kubeletServingRequiredUsages.Equal(usages) { + return fmt.Errorf("usages did not match %v", kubeletServingRequiredUsages.List()) + } } if !strings.HasPrefix(req.Subject.CommonName, "system:node:") { @@ -87,16 +99,22 @@ func ValidateKubeletServingCSR(req *x509.CertificateRequest, usages sets.String) return nil } -var kubeletClientRequiredUsages = sets.NewString( - string(UsageDigitalSignature), - string(UsageKeyEncipherment), - string(UsageClientAuth), +var ( + kubeletClientRequiredUsagesNoRSA = sets.NewString( + string(UsageDigitalSignature), + string(UsageClientAuth), + ) + kubeletClientRequiredUsages = sets.NewString( + string(UsageDigitalSignature), + string(UsageKeyEncipherment), + string(UsageClientAuth), + ) ) -func IsKubeletClientCSR(req *x509.CertificateRequest, usages sets.String) bool { - return ValidateKubeletClientCSR(req, usages) == nil +func IsKubeletClientCSR(req *x509.CertificateRequest, usages sets.String, allowOmittingUsageKeyEncipherment bool) bool { + return ValidateKubeletClientCSR(req, usages, allowOmittingUsageKeyEncipherment) == nil } -func ValidateKubeletClientCSR(req *x509.CertificateRequest, usages sets.String) error { +func ValidateKubeletClientCSR(req *x509.CertificateRequest, usages sets.String, allowOmittingUsageKeyEncipherment bool) error { if !reflect.DeepEqual([]string{"system:nodes"}, req.Subject.Organization) { return organizationNotSystemNodesErr } @@ -118,8 +136,14 @@ func ValidateKubeletClientCSR(req *x509.CertificateRequest, usages sets.String) return commonNameNotSystemNode } - if !kubeletClientRequiredUsages.Equal(usages) { - return fmt.Errorf("usages did not match %v", kubeletClientRequiredUsages.List()) + if allowOmittingUsageKeyEncipherment { + if !kubeletClientRequiredUsages.Equal(usages) && !kubeletClientRequiredUsagesNoRSA.Equal(usages) { + return fmt.Errorf("usages did not match %v", kubeletClientRequiredUsages.List()) + } + } else { + if !kubeletClientRequiredUsages.Equal(usages) { + return fmt.Errorf("usages did not match %v", kubeletClientRequiredUsages.List()) + } } return nil diff --git a/pkg/apis/certificates/v1beta1/defaults.go b/pkg/apis/certificates/v1beta1/defaults.go index 14133d9df2a..4781c8f341b 100644 --- a/pkg/apis/certificates/v1beta1/defaults.go +++ b/pkg/apis/certificates/v1beta1/defaults.go @@ -56,27 +56,27 @@ func DefaultSignerNameFromSpec(obj *certificatesv1beta1.CertificateSigningReques // Set the signerName to 'legacy-unknown' as the CSR could not be // recognised. return certificatesv1beta1.LegacyUnknownSignerName - case IsKubeletClientCSR(csr, obj.Usages): + case IsKubeletClientCSR(csr, obj.Usages, false): return certificatesv1beta1.KubeAPIServerClientKubeletSignerName - case IsKubeletServingCSR(csr, obj.Usages): + case IsKubeletServingCSR(csr, obj.Usages, false): return certificatesv1beta1.KubeletServingSignerName default: return certificatesv1beta1.LegacyUnknownSignerName } } -func IsKubeletServingCSR(req *x509.CertificateRequest, usages []certificatesv1beta1.KeyUsage) bool { - return certificates.IsKubeletServingCSR(req, usagesToSet(usages)) +func IsKubeletServingCSR(req *x509.CertificateRequest, usages []certificatesv1beta1.KeyUsage, allowOmittingUsageKeyEncipherment bool) bool { + return certificates.IsKubeletServingCSR(req, usagesToSet(usages), allowOmittingUsageKeyEncipherment) } -func ValidateKubeletServingCSR(req *x509.CertificateRequest, usages []certificatesv1beta1.KeyUsage) error { - return certificates.ValidateKubeletServingCSR(req, usagesToSet(usages)) +func ValidateKubeletServingCSR(req *x509.CertificateRequest, usages []certificatesv1beta1.KeyUsage, allowOmittingUsageKeyEncipherment bool) error { + return certificates.ValidateKubeletServingCSR(req, usagesToSet(usages), allowOmittingUsageKeyEncipherment) } -func IsKubeletClientCSR(req *x509.CertificateRequest, usages []certificatesv1beta1.KeyUsage) bool { - return certificates.IsKubeletClientCSR(req, usagesToSet(usages)) +func IsKubeletClientCSR(req *x509.CertificateRequest, usages []certificatesv1beta1.KeyUsage, allowOmittingUsageKeyEncipherment bool) bool { + return certificates.IsKubeletClientCSR(req, usagesToSet(usages), allowOmittingUsageKeyEncipherment) } -func ValidateKubeletClientCSR(req *x509.CertificateRequest, usages []certificatesv1beta1.KeyUsage) error { - return certificates.ValidateKubeletClientCSR(req, usagesToSet(usages)) +func ValidateKubeletClientCSR(req *x509.CertificateRequest, usages []certificatesv1beta1.KeyUsage, allowOmittingUsageKeyEncipherment bool) error { + return certificates.ValidateKubeletClientCSR(req, usagesToSet(usages), allowOmittingUsageKeyEncipherment) } func usagesToSet(usages []certificatesv1beta1.KeyUsage) sets.String { diff --git a/pkg/apis/certificates/v1beta1/defaults_test.go b/pkg/apis/certificates/v1beta1/defaults_test.go index f0bdee3a538..82e2599fbbb 100644 --- a/pkg/apis/certificates/v1beta1/defaults_test.go +++ b/pkg/apis/certificates/v1beta1/defaults_test.go @@ -40,15 +40,28 @@ func TestIsKubeletServingCSR(t *testing.T) { return csr } tests := map[string]struct { - req *x509.CertificateRequest - usages []capi.KeyUsage - exp bool + req *x509.CertificateRequest + usages []capi.KeyUsage + allowOmittingUsageKeyEncipherment bool + exp bool }{ "defaults for kubelet-serving": { req: newCSR(kubeletServerPEMOptions), usages: kubeletServerUsages, exp: true, }, + "defaults without key encipherment for kubelet-serving if allow omitting key encipherment": { + req: newCSR(kubeletServerPEMOptions), + usages: kubeletServerUsagesNoRSA, + allowOmittingUsageKeyEncipherment: true, + exp: true, + }, + "defaults for kubelet-serving if allow omitting key encipherment": { + req: newCSR(kubeletServerPEMOptions), + usages: kubeletServerUsages, + allowOmittingUsageKeyEncipherment: true, + exp: true, + }, "does not default to kube-apiserver-client-kubelet if org is not 'system:nodes'": { req: newCSR(kubeletServerPEMOptions, pemOptions{org: "not-system:nodes"}), usages: kubeletServerUsages, @@ -69,6 +82,17 @@ func TestIsKubeletServingCSR(t *testing.T) { usages: kubeletServerUsages[1:], exp: false, }, + "does not default to kubelet-serving if it is missing an expected usage if allow omitting key encipherment": { + req: newCSR(kubeletServerPEMOptions), + usages: kubeletServerUsagesNoRSA[1:], + allowOmittingUsageKeyEncipherment: true, + exp: false, + }, + "does not default to kubelet-serving if it is missing an expected usage withou key encipherment": { + req: newCSR(kubeletServerPEMOptions), + usages: kubeletServerUsagesNoRSA, + exp: false, + }, "does not default to kubelet-serving if it does not specify any dnsNames or ipAddresses": { req: newCSR(kubeletServerPEMOptions, pemOptions{ipAddresses: []net.IP{}, dnsNames: []string{}}), usages: kubeletServerUsages[1:], @@ -87,7 +111,7 @@ func TestIsKubeletServingCSR(t *testing.T) { } for name, test := range tests { t.Run(name, func(t *testing.T) { - got := IsKubeletServingCSR(test.req, test.usages) + got := IsKubeletServingCSR(test.req, test.usages, test.allowOmittingUsageKeyEncipherment) if test.exp != got { t.Errorf("unexpected IsKubeletClientCSR output: exp=%v, got=%v", test.exp, got) } @@ -105,9 +129,10 @@ func TestIsKubeletClientCSR(t *testing.T) { return csr } tests := map[string]struct { - req *x509.CertificateRequest - usages []capi.KeyUsage - exp bool + req *x509.CertificateRequest + usages []capi.KeyUsage + allowOmittingUsageKeyEncipherment bool + exp bool }{ "defaults for kube-apiserver-client-kubelet": { req: newCSR(kubeletClientPEMOptions), @@ -154,10 +179,28 @@ func TestIsKubeletClientCSR(t *testing.T) { usages: kubeletClientUsages[1:], exp: false, }, + "does not default to kube-apiserver-client-kubelet if it is missing an expected usage without key encipherment": { + req: newCSR(kubeletClientPEMOptions), + usages: kubeletClientUsagesNoRSA[1:], + allowOmittingUsageKeyEncipherment: true, + exp: false, + }, + "default to kube-apiserver-client-kubelet with key encipherment": { + req: newCSR(kubeletClientPEMOptions), + usages: kubeletClientUsages, + allowOmittingUsageKeyEncipherment: true, + exp: true, + }, + "default to kube-apiserver-client-kubelet without key encipherment": { + req: newCSR(kubeletClientPEMOptions), + usages: kubeletClientUsagesNoRSA, + allowOmittingUsageKeyEncipherment: true, + exp: true, + }, } for name, test := range tests { t.Run(name, func(t *testing.T) { - got := IsKubeletClientCSR(test.req, test.usages) + got := IsKubeletClientCSR(test.req, test.usages, test.allowOmittingUsageKeyEncipherment) if test.exp != got { t.Errorf("unexpected IsKubeletClientCSR output: exp=%v, got=%v", test.exp, got) } @@ -171,6 +214,10 @@ var ( capi.UsageKeyEncipherment, capi.UsageClientAuth, } + kubeletClientUsagesNoRSA = []capi.KeyUsage{ + capi.UsageDigitalSignature, + capi.UsageClientAuth, + } kubeletClientPEMOptions = pemOptions{ cn: "system:node:nodename", org: "system:nodes", @@ -181,6 +228,10 @@ var ( capi.UsageKeyEncipherment, capi.UsageServerAuth, } + kubeletServerUsagesNoRSA = []capi.KeyUsage{ + capi.UsageDigitalSignature, + capi.UsageServerAuth, + } kubeletServerPEMOptions = pemOptions{ cn: "system:node:requester-name", org: "system:nodes", diff --git a/pkg/controller/certificates/approver/sarapprove.go b/pkg/controller/certificates/approver/sarapprove.go index d739fc783b3..5b7750402b0 100644 --- a/pkg/controller/certificates/approver/sarapprove.go +++ b/pkg/controller/certificates/approver/sarapprove.go @@ -152,7 +152,7 @@ func isNodeClientCert(csr *capi.CertificateSigningRequest, x509cr *x509.Certific if csr.Spec.SignerName != capi.KubeAPIServerClientKubeletSignerName { return false } - return capihelper.IsKubeletClientCSR(x509cr, usagesToSet(csr.Spec.Usages)) + return capihelper.IsKubeletClientCSR(x509cr, usagesToSet(csr.Spec.Usages), true) } func isSelfNodeClientCert(csr *capi.CertificateSigningRequest, x509cr *x509.CertificateRequest) bool { diff --git a/pkg/controller/certificates/approver/sarapprove_test.go b/pkg/controller/certificates/approver/sarapprove_test.go index 9bbbcf114b1..614ca011882 100644 --- a/pkg/controller/certificates/approver/sarapprove_test.go +++ b/pkg/controller/certificates/approver/sarapprove_test.go @@ -211,6 +211,28 @@ func testRecognizer(t *testing.T, cases []func(b *csrBuilder), recognizeFunc fun t.Errorf("expected recognized to be %v", shouldRecognize) } }) + // reset the builder to run testcase without usage key encipherment + d := csrBuilder{ + signerName: capi.KubeAPIServerClientKubeletSignerName, + cn: "system:node:foo", + orgs: []string{"system:nodes"}, + requestor: "system:node:foo", + usages: []capi.KeyUsage{ + capi.UsageDigitalSignature, + capi.UsageClientAuth, + }, + } + c(&d) + t.Run(fmt.Sprintf("csr:%#v", d), func(t *testing.T) { + csr := makeFancyTestCsr(d) + x509cr, err := k8s_certificates_v1.ParseCSR(csr.Spec.Request) + if err != nil { + t.Errorf("unexpected err: %v", err) + } + if recognizeFunc(csr, x509cr) != shouldRecognize { + t.Errorf("expected recognized to be %v", shouldRecognize) + } + }) } } diff --git a/pkg/controller/certificates/signer/signer.go b/pkg/controller/certificates/signer/signer.go index 588912801c8..c81445f84b2 100644 --- a/pkg/controller/certificates/signer/signer.go +++ b/pkg/controller/certificates/signer/signer.go @@ -248,14 +248,14 @@ func isKubeletServing(req *x509.CertificateRequest, usages []capi.KeyUsage, sign if signerName != capi.KubeletServingSignerName { return false, nil } - return true, capihelper.ValidateKubeletServingCSR(req, usagesToSet(usages)) + return true, capihelper.ValidateKubeletServingCSR(req, usagesToSet(usages), true) } func isKubeletClient(req *x509.CertificateRequest, usages []capi.KeyUsage, signerName string) (bool, error) { if signerName != capi.KubeAPIServerClientKubeletSignerName { return false, nil } - return true, capihelper.ValidateKubeletClientCSR(req, usagesToSet(usages)) + return true, capihelper.ValidateKubeletClientCSR(req, usagesToSet(usages), true) } func isKubeAPIServerClient(req *x509.CertificateRequest, usages []capi.KeyUsage, signerName string) (bool, error) { diff --git a/pkg/controller/certificates/signer/signer_test.go b/pkg/controller/certificates/signer/signer_test.go index 02bea53ae7a..4366fe7ea70 100644 --- a/pkg/controller/certificates/signer/signer_test.go +++ b/pkg/controller/certificates/signer/signer_test.go @@ -143,6 +143,24 @@ func TestHandle(t *testing.T) { } }, }, + { + name: "should sign without key encipherment if signerName is kubernetes.io/kube-apiserver-client", + signerName: "kubernetes.io/kube-apiserver-client", + commonName: "hello-world", + org: []string{"some-org"}, + usages: []capi.KeyUsage{capi.UsageClientAuth, capi.UsageDigitalSignature}, + approved: true, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 1 { + t.Errorf("expected one Update action but got %d", len(as)) + return + } + csr := as[0].(testclient.UpdateAction).GetObject().(*capi.CertificateSigningRequest) + if len(csr.Status.Certificate) == 0 { + t.Errorf("expected certificate to be issued but it was not") + } + }, + }, { name: "should refuse to sign if signerName is kubernetes.io/kube-apiserver-client and contains an unexpected usage", signerName: "kubernetes.io/kube-apiserver-client", @@ -182,6 +200,24 @@ func TestHandle(t *testing.T) { } }, }, + { + name: "should sign without usage key encipherment if signerName is kubernetes.io/kube-apiserver-client-kubelet", + signerName: "kubernetes.io/kube-apiserver-client-kubelet", + commonName: "system:node:hello-world", + org: []string{"system:nodes"}, + usages: []capi.KeyUsage{capi.UsageClientAuth, capi.UsageDigitalSignature}, + approved: true, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 1 { + t.Errorf("expected one Update action but got %d", len(as)) + return + } + csr := as[0].(testclient.UpdateAction).GetObject().(*capi.CertificateSigningRequest) + if len(csr.Status.Certificate) == 0 { + t.Errorf("expected certificate to be issued but it was not") + } + }, + }, { name: "should sign if signerName is kubernetes.io/legacy-unknown", signerName: "kubernetes.io/legacy-unknown", @@ -216,6 +252,25 @@ func TestHandle(t *testing.T) { } }, }, + { + name: "should sign without usage key encipherment if signerName is kubernetes.io/kubelet-serving", + signerName: "kubernetes.io/kubelet-serving", + commonName: "system:node:testnode", + org: []string{"system:nodes"}, + usages: []capi.KeyUsage{capi.UsageServerAuth, capi.UsageDigitalSignature}, + dnsNames: []string{"example.com"}, + approved: true, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 1 { + t.Errorf("expected one Update action but got %d", len(as)) + return + } + csr := as[0].(testclient.UpdateAction).GetObject().(*capi.CertificateSigningRequest) + if len(csr.Status.Certificate) == 0 { + t.Errorf("expected certificate to be issued but it was not") + } + }, + }, { name: "should do nothing if failed", signerName: "kubernetes.io/kubelet-serving",