certificates: update controllers to understand signerName field

Signed-off-by: James Munnelly <james.munnelly@jetstack.io>

pkg/controller/certificates/BUILD

@@ -13,6 +13,7 @@ go_library(
     importpath = "k8s.io/kubernetes/pkg/controller/certificates",
     visibility = ["//visibility:public"],
     deps = [
+        "//pkg/apis/certificates/v1beta1:go_default_library",
         "//pkg/controller:go_default_library",
         "//staging/src/k8s.io/api/certificates/v1beta1:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/api/errors:go_default_library",

pkg/controller/certificates/approver/sarapprove.go

@@ -21,14 +21,13 @@ import (
 	"context"
 	"crypto/x509"
 	"fmt"
-	"reflect"
-	"strings"
 
 	authorization "k8s.io/api/authorization/v1"
 	capi "k8s.io/api/certificates/v1beta1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	certificatesinformers "k8s.io/client-go/informers/certificates/v1beta1"
 	clientset "k8s.io/client-go/kubernetes"
+
 	capihelper "k8s.io/kubernetes/pkg/apis/certificates/v1beta1"
 	"k8s.io/kubernetes/pkg/controller/certificates"
 )
@@ -146,45 +145,12 @@ func appendApprovalCondition(csr *capi.CertificateSigningRequest, message string
 	})
 }
 
-func hasExactUsages(csr *capi.CertificateSigningRequest, usages []capi.KeyUsage) bool {
-	if len(usages) != len(csr.Spec.Usages) {
-		return false
-	}
-
-	usageMap := map[capi.KeyUsage]struct{}{}
-	for _, u := range usages {
-		usageMap[u] = struct{}{}
-	}
-
-	for _, u := range csr.Spec.Usages {
-		if _, ok := usageMap[u]; !ok {
-			return false
-		}
-	}
-
-	return true
-}
-
-var kubeletClientUsages = []capi.KeyUsage{
-	capi.UsageKeyEncipherment,
-	capi.UsageDigitalSignature,
-	capi.UsageClientAuth,
-}
-
 func isNodeClientCert(csr *capi.CertificateSigningRequest, x509cr *x509.CertificateRequest) bool {
-	if !reflect.DeepEqual([]string{"system:nodes"}, x509cr.Subject.Organization) {
+	isClientCSR := capihelper.IsKubeletClientCSR(x509cr, csr.Spec.Usages)
+	if !isClientCSR {
 		return false
 	}
-	if len(x509cr.DNSNames) > 0 || len(x509cr.EmailAddresses) > 0 || len(x509cr.IPAddresses) > 0 || len(x509cr.URIs) > 0 {
-		return false
-	}
-	if !hasExactUsages(csr, kubeletClientUsages) {
-		return false
-	}
-	if !strings.HasPrefix(x509cr.Subject.CommonName, "system:node:") {
-		return false
-	}
-	return true
+	return *csr.Spec.SignerName == capi.KubeAPIServerClientKubeletSignerName
 }
 
 func isSelfNodeClientCert(csr *capi.CertificateSigningRequest, x509cr *x509.CertificateRequest) bool {

pkg/controller/certificates/approver/sarapprove_test.go

@@ -36,54 +36,6 @@ import (
 	k8s_certificates_v1beta1 "k8s.io/kubernetes/pkg/apis/certificates/v1beta1"
 )
 
-func TestHasKubeletUsages(t *testing.T) {
-	cases := []struct {
-		usages   []capi.KeyUsage
-		expected bool
-	}{
-		{
-			usages:   nil,
-			expected: false,
-		},
-		{
-			usages:   []capi.KeyUsage{},
-			expected: false,
-		},
-		{
-			usages: []capi.KeyUsage{
-				capi.UsageKeyEncipherment,
-				capi.UsageDigitalSignature,
-			},
-			expected: false,
-		},
-		{
-			usages: []capi.KeyUsage{
-				capi.UsageKeyEncipherment,
-				capi.UsageDigitalSignature,
-				capi.UsageServerAuth,
-			},
-			expected: false,
-		},
-		{
-			usages: []capi.KeyUsage{
-				capi.UsageKeyEncipherment,
-				capi.UsageDigitalSignature,
-				capi.UsageClientAuth,
-			},
-			expected: true,
-		},
-	}
-	for _, c := range cases {
-		if hasExactUsages(&capi.CertificateSigningRequest{
-			Spec: capi.CertificateSigningRequestSpec{
-				Usages: c.usages,
-			},
-		}, kubeletClientUsages) != c.expected {
-			t.Errorf("unexpected result of hasKubeletUsages(%v), expecting: %v", c.usages, c.expected)
-		}
-	}
-}
-
 func TestHandle(t *testing.T) {
 	cases := []struct {
 		allowed    bool
@@ -208,6 +160,12 @@ func TestRecognizers(t *testing.T) {
 		func(b *csrBuilder) {
 			b.usages = append(b.usages, capi.UsageServerAuth)
 		},
+		func(b *csrBuilder) {
+			b.signerName = "example.com/not-correct"
+		},
+		func(b *csrBuilder) {
+			b.signerName = capi.KubeletServingSignerName
+		},
 	}
 
 	testRecognizer(t, badCases, isNodeClientCert, false)
@@ -230,9 +188,10 @@ func TestRecognizers(t *testing.T) {
 func testRecognizer(t *testing.T, cases []func(b *csrBuilder), recognizeFunc func(csr *capi.CertificateSigningRequest, x509cr *x509.CertificateRequest) bool, shouldRecognize bool) {
 	for _, c := range cases {
 		b := csrBuilder{
-			cn:        "system:node:foo",
-			orgs:      []string{"system:nodes"},
-			requestor: "system:node:foo",
+			signerName: capi.KubeAPIServerClientKubeletSignerName,
+			cn:         "system:node:foo",
+			orgs:       []string{"system:nodes"},
+			requestor:  "system:node:foo",
 			usages: []capi.KeyUsage{
 				capi.UsageKeyEncipherment,
 				capi.UsageDigitalSignature,
@@ -262,13 +221,14 @@ func makeTestCsr() *capi.CertificateSigningRequest {
 }
 
 type csrBuilder struct {
-	cn        string
-	orgs      []string
-	requestor string
-	usages    []capi.KeyUsage
-	dns       []string
-	emails    []string
-	ips       []net.IP
+	cn         string
+	orgs       []string
+	requestor  string
+	usages     []capi.KeyUsage
+	dns        []string
+	emails     []string
+	ips        []net.IP
+	signerName string
 }
 
 func makeFancyTestCsr(b csrBuilder) *capi.CertificateSigningRequest {
@@ -290,9 +250,10 @@ func makeFancyTestCsr(b csrBuilder) *capi.CertificateSigningRequest {
 	}
 	return &capi.CertificateSigningRequest{
 		Spec: capi.CertificateSigningRequestSpec{
-			Username: b.requestor,
-			Usages:   b.usages,
-			Request:  pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE REQUEST", Bytes: csrb}),
+			Username:   b.requestor,
+			Usages:     b.usages,
+			SignerName: &b.signerName,
+			Request:    pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE REQUEST", Bytes: csrb}),
 		},
 	}
 }

pkg/controller/certificates/certificate_controller.go

@@ -36,6 +36,7 @@ import (
 	"k8s.io/client-go/tools/record"
 	"k8s.io/client-go/util/workqueue"
 	"k8s.io/klog"
+	capihelper "k8s.io/kubernetes/pkg/apis/certificates/v1beta1"
 	"k8s.io/kubernetes/pkg/controller"
 )
 
@@ -192,7 +193,15 @@ func (cc *CertificateController) syncFunc(key string) error {
 
 	// need to operate on a copy so we don't mutate the csr in the shared cache
 	csr = csr.DeepCopy()
-
+	// If the `signerName` field is not set, we are talking to a pre-1.18 apiserver.
+	// As per the KEP document for the certificates API, this will be defaulted here
+	// in the controller to maintain backwards compatibility.
+	// This should be removed after a deprecation window has passed.
+	// Default here to allow handlers to assume the field is set.
+	if csr.Spec.SignerName == nil {
+		signerName := capihelper.DefaultSignerNameFromSpec(&csr.Spec)
+		csr.Spec.SignerName = &signerName
+	}
 	return cc.handler(csr)
 }
 

pkg/controller/certificates/signer/BUILD

@@ -16,9 +16,12 @@ go_test(
     ],
     embed = [":go_default_library"],
     deps = [
+        "//pkg/apis/certificates/v1beta1:go_default_library",
         "//staging/src/k8s.io/api/certificates/v1beta1:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/util/clock:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/util/diff:go_default_library",
+        "//staging/src/k8s.io/client-go/kubernetes/fake:go_default_library",
+        "//staging/src/k8s.io/client-go/testing:go_default_library",
         "//staging/src/k8s.io/client-go/util/cert:go_default_library",
         "//vendor/github.com/google/go-cmp/cmp:go_default_library",
     ],

pkg/controller/certificates/signer/signer.go

@@ -19,8 +19,10 @@ package signer
 
 import (
 	"context"
+	"crypto/x509"
 	"encoding/pem"
 	"fmt"
+	"strings"
 	"time"
 
 	capi "k8s.io/api/certificates/v1beta1"
@@ -89,13 +91,31 @@ func newSigner(caFile, caKeyFile string, client clientset.Interface, certificate
 }
 
 func (s *signer) handle(csr *capi.CertificateSigningRequest) error {
+	// Ignore unapproved requests
 	if !certificates.IsCertificateRequestApproved(csr) {
 		return nil
 	}
-	csr, err := s.sign(csr)
+
+	// Fast-path to avoid any additional processing if the CSRs signerName does
+	// not have a 'kubernetes.io/' prefix.
+	if !strings.HasPrefix(*csr.Spec.SignerName, "kubernetes.io/") {
+		return nil
+	}
+
+	x509cr, err := capihelper.ParseCSR(csr.Spec.Request)
+	if err != nil {
+		return fmt.Errorf("unable to parse csr %q: %v", csr.Name, err)
+	}
+	if !requestValidForSignerName(x509cr, csr.Spec.Usages, *csr.Spec.SignerName) {
+		// TODO: mark the CertificateRequest as being in a terminal state and
+		//  communicate to the user why the request has been refused.
+		return nil
+	}
+	cert, err := s.sign(x509cr, csr.Spec.Usages)
 	if err != nil {
 		return fmt.Errorf("error auto signing csr: %v", err)
 	}
+	csr.Status.Certificate = cert
 	_, err = s.client.CertificatesV1beta1().CertificateSigningRequests().UpdateStatus(context.TODO(), csr, metav1.UpdateOptions{})
 	if err != nil {
 		return fmt.Errorf("error updating signature for csr: %v", err)
@@ -103,23 +123,50 @@ func (s *signer) handle(csr *capi.CertificateSigningRequest) error {
 	return nil
 }
 
-func (s *signer) sign(csr *capi.CertificateSigningRequest) (*capi.CertificateSigningRequest, error) {
-	x509cr, err := capihelper.ParseCSR(csr.Spec.Request)
-	if err != nil {
-		return nil, fmt.Errorf("unable to parse csr %q: %v", csr.Name, err)
-	}
-
+func (s *signer) sign(x509cr *x509.CertificateRequest, usages []capi.KeyUsage) ([]byte, error) {
 	currCA, err := s.caProvider.currentCA()
 	if err != nil {
 		return nil, err
 	}
 	der, err := currCA.Sign(x509cr.Raw, authority.PermissiveSigningPolicy{
 		TTL:    s.certTTL,
-		Usages: csr.Spec.Usages,
+		Usages: usages,
 	})
 	if err != nil {
 		return nil, err
 	}
-	csr.Status.Certificate = pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: der})
-	return csr, nil
+	return pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: der}), nil
+}
+
+func requestValidForSignerName(req *x509.CertificateRequest, usages []capi.KeyUsage, signerName string) bool {
+	// Only handle CSRs with the specific known signerNames.
+	switch signerName {
+	case capi.KubeletServingSignerName:
+		return capihelper.IsKubeletServingCSR(req, usages)
+	case capi.KubeAPIServerClientKubeletSignerName:
+		return capihelper.IsKubeletClientCSR(req, usages)
+	case capi.KubeAPIServerClientSignerName:
+		return validAPIServerClientUsages(usages)
+	case capi.LegacyUnknownSignerName:
+		// No restrictions are applied to the legacy-unknown signerName to
+		// maintain backward compatibility in v1beta1.
+		return true
+	default:
+		return false
+	}
+}
+
+func validAPIServerClientUsages(usages []capi.KeyUsage) bool {
+	hasClientAuth := false
+	for _, u := range usages {
+		switch u {
+		// these usages are optional
+		case capi.UsageDigitalSignature, capi.UsageKeyEncipherment:
+		case capi.UsageClientAuth:
+			hasClientAuth = true
+		default:
+			return false
+		}
+	}
+	return hasClientAuth
 }

pkg/controller/certificates/signer/signer_test.go

@@ -17,9 +17,13 @@ limitations under the License.
 package signer
 
 import (
+	"crypto/ecdsa"
+	"crypto/elliptic"
 	"crypto/x509"
 	"crypto/x509/pkix"
+	"encoding/pem"
 	"io/ioutil"
+	"math/rand"
 	"testing"
 	"time"
 
@@ -28,7 +32,11 @@ import (
 	capi "k8s.io/api/certificates/v1beta1"
 	"k8s.io/apimachinery/pkg/util/clock"
 	"k8s.io/apimachinery/pkg/util/diff"
+	"k8s.io/client-go/kubernetes/fake"
+	testclient "k8s.io/client-go/testing"
 	"k8s.io/client-go/util/cert"
+
+	capihelper "k8s.io/kubernetes/pkg/apis/certificates/v1beta1"
 )
 
 func TestSigner(t *testing.T) {
@@ -50,24 +58,20 @@ func TestSigner(t *testing.T) {
 	if err != nil {
 		t.Fatalf("failed to read CSR: %v", err)
 	}
-
-	csr := &capi.CertificateSigningRequest{
-		Spec: capi.CertificateSigningRequestSpec{
-			Request: []byte(csrb),
-			Usages: []capi.KeyUsage{
-				capi.UsageSigning,
-				capi.UsageKeyEncipherment,
-				capi.UsageServerAuth,
-				capi.UsageClientAuth,
-			},
-		},
+	x509cr, err := capihelper.ParseCSR(csrb)
+	if err != nil {
+		t.Fatalf("failed to parse CSR: %v", err)
 	}
 
-	csr, err = s.sign(csr)
+	certData, err := s.sign(x509cr, []capi.KeyUsage{
+		capi.UsageSigning,
+		capi.UsageKeyEncipherment,
+		capi.UsageServerAuth,
+		capi.UsageClientAuth,
+	})
 	if err != nil {
 		t.Fatalf("failed to sign CSR: %v", err)
 	}
-	certData := csr.Status.Certificate
 	if len(certData) == 0 {
 		t.Fatalf("expected a certificate after signing")
 	}
@@ -99,3 +103,207 @@ func TestSigner(t *testing.T) {
 		t.Errorf("unexpected diff: %v", cmp.Diff(certs[0], want, diff.IgnoreUnset()))
 	}
 }
+
+func TestHandle(t *testing.T) {
+	cases := []struct {
+		name string
+		// parameters to be set on the generated CSR
+		commonName string
+		dnsNames   []string
+		org        []string
+		usages     []capi.KeyUsage
+		// whether the generated CSR should be marked as approved
+		approved bool
+		// the signerName to be set on the generated CSR
+		signerName string
+		// if true, expect an error to be returned
+		err bool
+		// additional verification function
+		verify func(*testing.T, []testclient.Action)
+	}{
+		{
+			name:       "should sign 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, capi.UsageKeyEncipherment},
+			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",
+			commonName: "hello-world",
+			org:        []string{"some-org"},
+			usages:     []capi.KeyUsage{capi.UsageServerAuth, capi.UsageClientAuth, capi.UsageDigitalSignature, capi.UsageKeyEncipherment},
+			approved:   true,
+			verify: func(t *testing.T, as []testclient.Action) {
+				if len(as) != 0 {
+					t.Errorf("expected no Update action but got %d", len(as))
+					return
+				}
+			},
+		},
+		{
+			name:       "should sign 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, capi.UsageKeyEncipherment},
+			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",
+			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/kubelet-serving",
+			signerName: "kubernetes.io/kubelet-serving",
+			commonName: "system:node:testnode",
+			org:        []string{"system:nodes"},
+			usages:     []capi.KeyUsage{capi.UsageServerAuth, capi.UsageDigitalSignature, capi.UsageKeyEncipherment},
+			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 an unrecognised signerName is used",
+			signerName: "kubernetes.io/not-recognised",
+			approved:   true,
+			verify: func(t *testing.T, as []testclient.Action) {
+				if len(as) != 0 {
+					t.Errorf("expected no action to be taken")
+				}
+			},
+		},
+		{
+			name:       "should do nothing if not approved",
+			signerName: "kubernetes.io/kubelet-serving",
+			verify: func(t *testing.T, as []testclient.Action) {
+				if len(as) != 0 {
+					t.Errorf("expected no action to be taken")
+				}
+			},
+		},
+		{
+			name:       "should do nothing if signerName does not start with kubernetes.io",
+			signerName: "example.com/sample-name",
+			approved:   true,
+			verify: func(t *testing.T, as []testclient.Action) {
+				if len(as) != 0 {
+					t.Errorf("expected no action to be taken")
+				}
+			},
+		},
+		{
+			name:       "should do nothing if signerName starts with kubernetes.io but is unrecognised",
+			signerName: "kubernetes.io/not-a-real-signer",
+			approved:   true,
+			verify: func(t *testing.T, as []testclient.Action) {
+				if len(as) != 0 {
+					t.Errorf("expected no action to be taken")
+				}
+			},
+		},
+	}
+
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			client := &fake.Clientset{}
+			s, err := newSigner("./testdata/ca.crt", "./testdata/ca.key", client, 1*time.Hour)
+			if err != nil {
+				t.Fatalf("failed to create signer: %v", err)
+			}
+
+			csr := makeTestCSR(csrBuilder{cn: c.commonName, signerName: c.signerName, approved: c.approved, usages: c.usages, org: c.org, dnsNames: c.dnsNames})
+			if err := s.handle(csr); err != nil && !c.err {
+				t.Errorf("unexpected err: %v", err)
+			}
+			c.verify(t, client.Actions())
+		})
+	}
+}
+
+// noncryptographic for faster testing
+// DO NOT COPY THIS CODE
+var insecureRand = rand.New(rand.NewSource(0))
+
+type csrBuilder struct {
+	cn         string
+	dnsNames   []string
+	org        []string
+	signerName string
+	approved   bool
+	usages     []capi.KeyUsage
+}
+
+func makeTestCSR(b csrBuilder) *capi.CertificateSigningRequest {
+	pk, err := ecdsa.GenerateKey(elliptic.P256(), insecureRand)
+	if err != nil {
+		panic(err)
+	}
+	csrb, err := x509.CreateCertificateRequest(insecureRand, &x509.CertificateRequest{
+		Subject: pkix.Name{
+			CommonName:   b.cn,
+			Organization: b.org,
+		},
+		DNSNames: b.dnsNames,
+	}, pk)
+	if err != nil {
+		panic(err)
+	}
+	csr := &capi.CertificateSigningRequest{
+		Spec: capi.CertificateSigningRequestSpec{
+			Request: pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE REQUEST", Bytes: csrb}),
+			Usages:  b.usages,
+		},
+	}
+	if b.signerName != "" {
+		csr.Spec.SignerName = &b.signerName
+	}
+	if b.approved {
+		csr.Status.Conditions = append(csr.Status.Conditions, capi.CertificateSigningRequestCondition{
+			Type: capi.CertificateApproved,
+		})
+	}
+	return csr
+}