Merge pull request #88760 from munnerz/signername-follow-up

signerName: extend client-go ensureCompatibility and additional unit tests

pkg/apis/certificates/v1beta1/defaults_test.go

@@ -30,6 +30,141 @@ import (
 	capi "k8s.io/api/certificates/v1beta1"
 )
 
+func TestIsKubeletServingCSR(t *testing.T) {
+	newCSR := func(base pemOptions, overlays ...pemOptions) *x509.CertificateRequest {
+		b := csrWithOpts(base, overlays...)
+		csr, err := ParseCSR(b)
+		if err != nil {
+			t.Fatal(err)
+		}
+		return csr
+	}
+	tests := map[string]struct {
+		req    *x509.CertificateRequest
+		usages []capi.KeyUsage
+		exp    bool
+	}{
+		"defaults for kubelet-serving": {
+			req:    newCSR(kubeletServerPEMOptions),
+			usages: kubeletServerUsages,
+			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,
+			exp:    false,
+		},
+		"does not default to kubelet-serving if CN does not have system:node: prefix": {
+			req:    newCSR(kubeletServerPEMOptions, pemOptions{cn: "notprefixed"}),
+			usages: kubeletServerUsages,
+			exp:    false,
+		},
+		"does not default to kubelet-serving if it has an unexpected usage": {
+			req:    newCSR(kubeletServerPEMOptions),
+			usages: append(kubeletServerUsages, capi.UsageClientAuth),
+			exp:    false,
+		},
+		"does not default to kubelet-serving if it is missing an expected usage": {
+			req:    newCSR(kubeletServerPEMOptions),
+			usages: kubeletServerUsages[1:],
+			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:],
+			exp:    false,
+		},
+		"does not default to kubelet-serving if it specifies a URI SAN": {
+			req:    newCSR(kubeletServerPEMOptions, pemOptions{uris: []string{"http://something"}}),
+			usages: kubeletServerUsages,
+			exp:    false,
+		},
+		"does not default to kubelet-serving if it specifies an emailAddress SAN": {
+			req:    newCSR(kubeletServerPEMOptions, pemOptions{emailAddresses: []string{"something"}}),
+			usages: kubeletServerUsages,
+			exp:    false,
+		},
+	}
+	for name, test := range tests {
+		t.Run(name, func(t *testing.T) {
+			got := IsKubeletServingCSR(test.req, test.usages)
+			if test.exp != got {
+				t.Errorf("unexpected IsKubeletClientCSR output: exp=%v, got=%v", test.exp, got)
+			}
+		})
+	}
+}
+
+func TestIsKubeletClientCSR(t *testing.T) {
+	newCSR := func(base pemOptions, overlays ...pemOptions) *x509.CertificateRequest {
+		b := csrWithOpts(base, overlays...)
+		csr, err := ParseCSR(b)
+		if err != nil {
+			t.Fatal(err)
+		}
+		return csr
+	}
+	tests := map[string]struct {
+		req    *x509.CertificateRequest
+		usages []capi.KeyUsage
+		exp    bool
+	}{
+		"defaults for kube-apiserver-client-kubelet": {
+			req:    newCSR(kubeletClientPEMOptions),
+			usages: kubeletClientUsages,
+			exp:    true,
+		},
+		"does not default to kube-apiserver-client-kubelet if org is not 'system:nodes'": {
+			req:    newCSR(kubeletClientPEMOptions, pemOptions{org: "not-system:nodes"}),
+			usages: kubeletClientUsages,
+			exp:    false,
+		},
+		"does not default to kube-apiserver-client-kubelet if a dnsName is set": {
+			req:    newCSR(kubeletClientPEMOptions, pemOptions{dnsNames: []string{"something"}}),
+			usages: kubeletClientUsages,
+			exp:    false,
+		},
+		"does not default to kube-apiserver-client-kubelet if an emailAddress is set": {
+			req:    newCSR(kubeletClientPEMOptions, pemOptions{emailAddresses: []string{"something"}}),
+			usages: kubeletClientUsages,
+			exp:    false,
+		},
+		"does not default to kube-apiserver-client-kubelet if a uri SAN is set": {
+			req:    newCSR(kubeletClientPEMOptions, pemOptions{uris: []string{"http://something"}}),
+			usages: kubeletClientUsages,
+			exp:    false,
+		},
+		"does not default to kube-apiserver-client-kubelet if an ipAddress is set": {
+			req:    newCSR(kubeletClientPEMOptions, pemOptions{ipAddresses: []net.IP{{0, 0, 0, 0}}}),
+			usages: kubeletClientUsages,
+			exp:    false,
+		},
+		"does not default to kube-apiserver-client-kubelet if CN does not have 'system:node:' prefix": {
+			req:    newCSR(kubeletClientPEMOptions, pemOptions{cn: "not-prefixed"}),
+			usages: kubeletClientUsages,
+			exp:    false,
+		},
+		"does not default to kube-apiserver-client-kubelet if it has an unexpected usage": {
+			req:    newCSR(kubeletClientPEMOptions),
+			usages: append(kubeletClientUsages, capi.UsageServerAuth),
+			exp:    false,
+		},
+		"does not default to kube-apiserver-client-kubelet if it is missing an expected usage": {
+			req:    newCSR(kubeletClientPEMOptions),
+			usages: kubeletClientUsages[1:],
+			exp:    false,
+		},
+	}
+	for name, test := range tests {
+		t.Run(name, func(t *testing.T) {
+			got := IsKubeletClientCSR(test.req, test.usages)
+			if test.exp != got {
+				t.Errorf("unexpected IsKubeletClientCSR output: exp=%v, got=%v", test.exp, got)
+			}
+		})
+	}
+}
+
 var (
 	kubeletClientUsages = []capi.KeyUsage{
 		capi.UsageDigitalSignature,

staging/src/k8s.io/client-go/util/certificate/csr/BUILD

@@ -1,6 +1,6 @@
 package(default_visibility = ["//visibility:public"])
 
-load("@io_bazel_rules_go//go:def.bzl", "go_library")
+load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
 
 go_library(
     name = "go_default_library",
@@ -35,3 +35,13 @@ filegroup(
     srcs = [":package-srcs"],
     tags = ["automanaged"],
 )
+
+go_test(
+    name = "go_default_test",
+    srcs = ["csr_test.go"],
+    embed = [":go_default_library"],
+    deps = [
+        "//staging/src/k8s.io/api/certificates/v1beta1:go_default_library",
+        "//vendor/k8s.io/utils/pointer:go_default_library",
+    ],
+)

staging/src/k8s.io/client-go/util/certificate/csr/csr.go

@@ -150,6 +150,9 @@ func ensureCompatible(new, orig *certificates.CertificateSigningRequest, private
 	if !reflect.DeepEqual(newCSR.Subject, origCSR.Subject) {
 		return fmt.Errorf("csr subjects differ: new: %#v, orig: %#v", newCSR.Subject, origCSR.Subject)
 	}
+	if new.Spec.SignerName != nil && orig.Spec.SignerName != nil && *new.Spec.SignerName != *orig.Spec.SignerName {
+		return fmt.Errorf("csr signerNames differ: new %q, orig: %q", *new.Spec.SignerName, *orig.Spec.SignerName)
+	}
 	signer, ok := privateKey.(crypto.Signer)
 	if !ok {
 		return fmt.Errorf("privateKey is not a signer")

staging/src/k8s.io/client-go/util/certificate/csr/csr_test.go

@@ -0,0 +1,146 @@
+/*
+Copyright 2020 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package csr
+
+import (
+	"crypto"
+	"crypto/rand"
+	"crypto/rsa"
+	"crypto/x509"
+	"crypto/x509/pkix"
+	"encoding/pem"
+	"testing"
+
+	certificates "k8s.io/api/certificates/v1beta1"
+	"k8s.io/utils/pointer"
+)
+
+func TestEnsureCompatible(t *testing.T) {
+	privateKey, err := rsa.GenerateKey(rand.Reader, 2048)
+	if err != nil {
+		t.Fatal(err)
+	}
+	req := pemWithPrivateKey(privateKey)
+
+	tests := map[string]struct {
+		new, orig  *certificates.CertificateSigningRequest
+		privateKey interface{}
+		err        string
+	}{
+		"nil signerName on 'new' matches any signerName on 'orig'": {
+			new: &certificates.CertificateSigningRequest{
+				Spec: certificates.CertificateSigningRequestSpec{
+					Request: req,
+				},
+			},
+			orig: &certificates.CertificateSigningRequest{
+				Spec: certificates.CertificateSigningRequestSpec{
+					Request:    req,
+					SignerName: pointer.StringPtr("example.com/test"),
+				},
+			},
+			privateKey: privateKey,
+		},
+		"nil signerName on 'orig' matches any signerName on 'new'": {
+			new: &certificates.CertificateSigningRequest{
+				Spec: certificates.CertificateSigningRequestSpec{
+					Request:    req,
+					SignerName: pointer.StringPtr("example.com/test"),
+				},
+			},
+			orig: &certificates.CertificateSigningRequest{
+				Spec: certificates.CertificateSigningRequestSpec{
+					Request: req,
+				},
+			},
+			privateKey: privateKey,
+		},
+		"signerName on 'orig' matches signerName on 'new'": {
+			new: &certificates.CertificateSigningRequest{
+				Spec: certificates.CertificateSigningRequestSpec{
+					Request:    req,
+					SignerName: pointer.StringPtr("example.com/test"),
+				},
+			},
+			orig: &certificates.CertificateSigningRequest{
+				Spec: certificates.CertificateSigningRequestSpec{
+					Request:    req,
+					SignerName: pointer.StringPtr("example.com/test"),
+				},
+			},
+			privateKey: privateKey,
+		},
+		"signerName on 'orig' does not match signerName on 'new'": {
+			new: &certificates.CertificateSigningRequest{
+				Spec: certificates.CertificateSigningRequestSpec{
+					Request:    req,
+					SignerName: pointer.StringPtr("example.com/test"),
+				},
+			},
+			orig: &certificates.CertificateSigningRequest{
+				Spec: certificates.CertificateSigningRequestSpec{
+					Request:    req,
+					SignerName: pointer.StringPtr("example.com/not-test"),
+				},
+			},
+			privateKey: privateKey,
+			err:        `csr signerNames differ: new "example.com/test", orig: "example.com/not-test"`,
+		},
+	}
+	for name, test := range tests {
+		t.Run(name, func(t *testing.T) {
+			err := ensureCompatible(test.new, test.orig, test.privateKey)
+			if err != nil && test.err == "" {
+				t.Errorf("expected no error, but got: %v", err)
+			} else if err != nil && test.err != err.Error() {
+				t.Errorf("error did not match as expected, got=%v, exp=%s", err, test.err)
+			}
+			if err == nil && test.err != "" {
+				t.Errorf("expected to get an error but got none")
+			}
+		})
+	}
+}
+
+func pemWithPrivateKey(pk crypto.PrivateKey) []byte {
+	template := &x509.CertificateRequest{
+		Subject: pkix.Name{
+			CommonName:   "something",
+			Organization: []string{"test"},
+		},
+	}
+	return pemWithTemplate(template, pk)
+}
+
+func pemWithTemplate(template *x509.CertificateRequest, key crypto.PrivateKey) []byte {
+	csrDER, err := x509.CreateCertificateRequest(rand.Reader, template, key)
+	if err != nil {
+		panic(err)
+	}
+
+	csrPemBlock := &pem.Block{
+		Type:  "CERTIFICATE REQUEST",
+		Bytes: csrDER,
+	}
+
+	p := pem.EncodeToMemory(csrPemBlock)
+	if p == nil {
+		panic("invalid pem block")
+	}
+
+	return p
+}