Merge pull request #111061 from pacoxu/key-encipherment-optional

modify the signing/approving controller to tolerate either set of usages for kubelet client and serving certificates

pkg/apis/certificates/helpers.go

@@ -50,16 +50,22 @@ var (
 	uriSANNotAllowedErr           = fmt.Errorf("URI subjectAltNames are not allowed")
 )
 
-var kubeletServingRequiredUsages = sets.NewString(
+var (
-	string(UsageDigitalSignature),
+	kubeletServingRequiredUsages = sets.NewString(
-	string(UsageKeyEncipherment),
+		string(UsageDigitalSignature),
-	string(UsageServerAuth),
+		string(UsageKeyEncipherment),
+		string(UsageServerAuth),
+	)
+	kubeletServingRequiredUsagesNoRSA = sets.NewString(
+		string(UsageDigitalSignature),
+		string(UsageServerAuth),
+	)
 )
 
-func IsKubeletServingCSR(req *x509.CertificateRequest, usages sets.String) bool {
+func IsKubeletServingCSR(req *x509.CertificateRequest, usages sets.String, allowOmittingUsageKeyEncipherment bool) bool {
-	return ValidateKubeletServingCSR(req, usages) == nil
+	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) {
+	if allowOmittingUsageKeyEncipherment {
-		return fmt.Errorf("usages did not match %v", kubeletServingRequiredUsages.List())
+		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(
+var (
-	string(UsageDigitalSignature),
+	kubeletClientRequiredUsagesNoRSA = sets.NewString(
-	string(UsageKeyEncipherment),
+		string(UsageDigitalSignature),
-	string(UsageClientAuth),
+		string(UsageClientAuth),
+	)
+	kubeletClientRequiredUsages = sets.NewString(
+		string(UsageDigitalSignature),
+		string(UsageKeyEncipherment),
+		string(UsageClientAuth),
+	)
 )
 
-func IsKubeletClientCSR(req *x509.CertificateRequest, usages sets.String) bool {
+func IsKubeletClientCSR(req *x509.CertificateRequest, usages sets.String, allowOmittingUsageKeyEncipherment bool) bool {
-	return ValidateKubeletClientCSR(req, usages) == nil
+	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) {
+	if allowOmittingUsageKeyEncipherment {
-		return fmt.Errorf("usages did not match %v", kubeletClientRequiredUsages.List())
+		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

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 {
+func IsKubeletServingCSR(req *x509.CertificateRequest, usages []certificatesv1beta1.KeyUsage, allowOmittingUsageKeyEncipherment bool) bool {
-	return certificates.IsKubeletServingCSR(req, usagesToSet(usages))
+	return certificates.IsKubeletServingCSR(req, usagesToSet(usages), allowOmittingUsageKeyEncipherment)
 }
-func ValidateKubeletServingCSR(req *x509.CertificateRequest, usages []certificatesv1beta1.KeyUsage) error {
+func ValidateKubeletServingCSR(req *x509.CertificateRequest, usages []certificatesv1beta1.KeyUsage, allowOmittingUsageKeyEncipherment bool) error {
-	return certificates.ValidateKubeletServingCSR(req, usagesToSet(usages))
+	return certificates.ValidateKubeletServingCSR(req, usagesToSet(usages), allowOmittingUsageKeyEncipherment)
 }
 
-func IsKubeletClientCSR(req *x509.CertificateRequest, usages []certificatesv1beta1.KeyUsage) bool {
+func IsKubeletClientCSR(req *x509.CertificateRequest, usages []certificatesv1beta1.KeyUsage, allowOmittingUsageKeyEncipherment bool) bool {
-	return certificates.IsKubeletClientCSR(req, usagesToSet(usages))
+	return certificates.IsKubeletClientCSR(req, usagesToSet(usages), allowOmittingUsageKeyEncipherment)
 }
-func ValidateKubeletClientCSR(req *x509.CertificateRequest, usages []certificatesv1beta1.KeyUsage) error {
+func ValidateKubeletClientCSR(req *x509.CertificateRequest, usages []certificatesv1beta1.KeyUsage, allowOmittingUsageKeyEncipherment bool) error {
-	return certificates.ValidateKubeletClientCSR(req, usagesToSet(usages))
+	return certificates.ValidateKubeletClientCSR(req, usagesToSet(usages), allowOmittingUsageKeyEncipherment)
 }
 
 func usagesToSet(usages []certificatesv1beta1.KeyUsage) sets.String {

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
+		req                               *x509.CertificateRequest
-		usages []capi.KeyUsage
+		usages                            []capi.KeyUsage
-		exp    bool
+		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
+		req                               *x509.CertificateRequest
-		usages []capi.KeyUsage
+		usages                            []capi.KeyUsage
-		exp    bool
+		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",

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 {

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)
+			}
+		})
 	}
 }
 

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) {

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",