refactor the CSR controller into distinct controllers to allow easy configuration of multiple signing keys

cmd/kube-controller-manager/app/BUILD

@@ -48,6 +48,7 @@ go_library(
         "//pkg/controller/certificates/cleaner:go_default_library",
         "//pkg/controller/certificates/rootcacertpublisher:go_default_library",
         "//pkg/controller/certificates/signer:go_default_library",
+        "//pkg/controller/certificates/signer/config:go_default_library",
         "//pkg/controller/cloud:go_default_library",
         "//pkg/controller/clusterroleaggregation:go_default_library",
         "//pkg/controller/cronjob:go_default_library",

cmd/kube-controller-manager/app/certificates.go

@@ -24,17 +24,17 @@ import (
 	"fmt"
 	"os"
 
-	"k8s.io/klog"
-
 	"net/http"
 
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	utilfeature "k8s.io/apiserver/pkg/util/feature"
+	"k8s.io/klog"
 	kubeoptions "k8s.io/kubernetes/cmd/kube-controller-manager/app/options"
 	"k8s.io/kubernetes/pkg/controller/certificates/approver"
 	"k8s.io/kubernetes/pkg/controller/certificates/cleaner"
 	"k8s.io/kubernetes/pkg/controller/certificates/rootcacertpublisher"
 	"k8s.io/kubernetes/pkg/controller/certificates/signer"
+	csrsigningconfig "k8s.io/kubernetes/pkg/controller/certificates/signer/config"
 	"k8s.io/kubernetes/pkg/features"
 )
 
@@ -89,22 +89,45 @@ func startCSRSigningController(ctx ControllerContext) (http.Handler, bool, error
 	}
 
 	c := ctx.ClientBuilder.ClientOrDie("certificate-controller")
+	csrInformer := ctx.InformerFactory.Certificates().V1beta1().CertificateSigningRequests()
+	certTTL := ctx.ComponentConfig.CSRSigningController.ClusterSigningDuration.Duration
+	caFile, caKeyFile := getKubeletServingSignerFiles(ctx.ComponentConfig.CSRSigningController)
 
-	signer, err := signer.NewCSRSigningController(
+	// TODO get different signer cert and key files for each signer when we add flags.
-		c,
+
-		ctx.InformerFactory.Certificates().V1beta1().CertificateSigningRequests(),
+	kubeletServingSigner, err := signer.NewKubeletServingCSRSigningController(c, csrInformer, caFile, caKeyFile, certTTL)
-		ctx.ComponentConfig.CSRSigningController.ClusterSigningCertFile,
-		ctx.ComponentConfig.CSRSigningController.ClusterSigningKeyFile,
-		ctx.ComponentConfig.CSRSigningController.ClusterSigningDuration.Duration,
-	)
 	if err != nil {
-		return nil, false, fmt.Errorf("failed to start certificate controller: %v", err)
+		return nil, false, fmt.Errorf("failed to start kubernetes.io/kubelet-serving certificate controller: %v", err)
 	}
-	go signer.Run(1, ctx.Stop)
+	go kubeletServingSigner.Run(1, ctx.Stop)
+
+	kubeletClientSigner, err := signer.NewKubeletClientCSRSigningController(c, csrInformer, caFile, caKeyFile, certTTL)
+	if err != nil {
+		return nil, false, fmt.Errorf("failed to start kubernetes.io/kube-apiserver-client-kubelet certificate controller: %v", err)
+	}
+	go kubeletClientSigner.Run(1, ctx.Stop)
+
+	kubeAPIServerClientSigner, err := signer.NewKubeAPIServerClientCSRSigningController(c, csrInformer, caFile, caKeyFile, certTTL)
+	if err != nil {
+		return nil, false, fmt.Errorf("failed to start kubernetes.io/kube-apiserver-client certificate controller: %v", err)
+	}
+	go kubeAPIServerClientSigner.Run(1, ctx.Stop)
+
+	legacyUnknownSigner, err := signer.NewLegacyUnknownCSRSigningController(c, csrInformer, caFile, caKeyFile, certTTL)
+	if err != nil {
+		return nil, false, fmt.Errorf("failed to start kubernetes.io/legacy-unknown certificate controller: %v", err)
+	}
+	go legacyUnknownSigner.Run(1, ctx.Stop)
 
 	return nil, true, nil
 }
 
+// getKubeletServingSignerFiles returns the cert and key for signing.
+// TODO we will extended this for each signer so that it prefers the specific flag (to be added) and falls back to the single flag
+func getKubeletServingSignerFiles(config csrsigningconfig.CSRSigningControllerConfiguration) (string, string) {
+	return config.ClusterSigningCertFile, config.ClusterSigningKeyFile
+}
+
 func startCSRApprovingController(ctx ControllerContext) (http.Handler, bool, error) {
 	gvr := schema.GroupVersionResource{Group: "certificates.k8s.io", Version: "v1beta1", Resource: "certificatesigningrequests"}
 	if !ctx.AvailableResources[gvr] {

pkg/controller/certificates/signer/signer.go

@@ -22,7 +22,6 @@ import (
 	"crypto/x509"
 	"encoding/pem"
 	"fmt"
-	"strings"
 	"time"
 
 	capi "k8s.io/api/certificates/v1beta1"
@@ -40,20 +39,58 @@ type CSRSigningController struct {
 	dynamicCertReloader   dynamiccertificates.ControllerRunner
 }
 
-func NewCSRSigningController(
+func NewKubeletServingCSRSigningController(
 	client clientset.Interface,
 	csrInformer certificatesinformers.CertificateSigningRequestInformer,
 	caFile, caKeyFile string,
 	certTTL time.Duration,
 ) (*CSRSigningController, error) {
-	signer, err := newSigner(caFile, caKeyFile, client, certTTL)
+	return NewCSRSigningController("csrsigning-kubelet-serving", capi.KubeletServingSignerName, client, csrInformer, caFile, caKeyFile, certTTL)
+}
+
+func NewKubeletClientCSRSigningController(
+	client clientset.Interface,
+	csrInformer certificatesinformers.CertificateSigningRequestInformer,
+	caFile, caKeyFile string,
+	certTTL time.Duration,
+) (*CSRSigningController, error) {
+	return NewCSRSigningController("csrsigning-kubelet-client", capi.KubeAPIServerClientKubeletSignerName, client, csrInformer, caFile, caKeyFile, certTTL)
+}
+
+func NewKubeAPIServerClientCSRSigningController(
+	client clientset.Interface,
+	csrInformer certificatesinformers.CertificateSigningRequestInformer,
+	caFile, caKeyFile string,
+	certTTL time.Duration,
+) (*CSRSigningController, error) {
+	return NewCSRSigningController("csrsigning-kube-apiserver-client", capi.KubeAPIServerClientSignerName, client, csrInformer, caFile, caKeyFile, certTTL)
+}
+
+func NewLegacyUnknownCSRSigningController(
+	client clientset.Interface,
+	csrInformer certificatesinformers.CertificateSigningRequestInformer,
+	caFile, caKeyFile string,
+	certTTL time.Duration,
+) (*CSRSigningController, error) {
+	return NewCSRSigningController("csrsigning-legacy-unknown", capi.LegacyUnknownSignerName, client, csrInformer, caFile, caKeyFile, certTTL)
+}
+
+func NewCSRSigningController(
+	controllerName string,
+	signerName string,
+	client clientset.Interface,
+	csrInformer certificatesinformers.CertificateSigningRequestInformer,
+	caFile, caKeyFile string,
+	certTTL time.Duration,
+) (*CSRSigningController, error) {
+	signer, err := newSigner(signerName, caFile, caKeyFile, client, certTTL)
 	if err != nil {
 		return nil, err
 	}
 
 	return &CSRSigningController{
 		certificateController: certificates.NewCertificateController(
-			"csrsigning",
+			controllerName,
 			client,
 			csrInformer,
 			signer.handle,
@@ -69,23 +106,34 @@ func (c *CSRSigningController) Run(workers int, stopCh <-chan struct{}) {
 	c.certificateController.Run(workers, stopCh)
 }
 
+type isRequestForSignerFunc func(req *x509.CertificateRequest, usages []capi.KeyUsage, signerName string) bool
+
 type signer struct {
 	caProvider *caProvider
 
 	client  clientset.Interface
 	certTTL time.Duration
+
+	signerName           string
+	isRequestForSignerFn isRequestForSignerFunc
 }
 
-func newSigner(caFile, caKeyFile string, client clientset.Interface, certificateDuration time.Duration) (*signer, error) {
+func newSigner(signerName, caFile, caKeyFile string, client clientset.Interface, certificateDuration time.Duration) (*signer, error) {
+	isRequestForSignerFn, err := getCSRVerificationFuncForSignerName(signerName)
+	if err != nil {
+		return nil, err
+	}
 	caProvider, err := newCAProvider(caFile, caKeyFile)
 	if err != nil {
 		return nil, err
 	}
 
 	ret := &signer{
-		caProvider: caProvider,
+		caProvider:           caProvider,
-		client:     client,
+		client:               client,
-		certTTL:    certificateDuration,
+		certTTL:              certificateDuration,
+		signerName:           signerName,
+		isRequestForSignerFn: isRequestForSignerFn,
 	}
 	return ret, nil
 }
@@ -96,9 +144,8 @@ func (s *signer) handle(csr *capi.CertificateSigningRequest) error {
 		return nil
 	}
 
-	// Fast-path to avoid any additional processing if the CSRs signerName does
+	// Fast-path to avoid any additional processing if the CSRs signerName does not match
-	// not have a 'kubernetes.io/' prefix.
+	if *csr.Spec.SignerName != s.signerName {
-	if !strings.HasPrefix(*csr.Spec.SignerName, "kubernetes.io/") {
 		return nil
 	}
 
@@ -106,7 +153,7 @@ func (s *signer) handle(csr *capi.CertificateSigningRequest) error {
 	if err != nil {
 		return fmt.Errorf("unable to parse csr %q: %v", csr.Name, err)
 	}
-	if !requestValidForSignerName(x509cr, csr.Spec.Usages, *csr.Spec.SignerName) {
+	if !s.isRequestForSignerFn(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
@@ -138,22 +185,54 @@ func (s *signer) sign(x509cr *x509.CertificateRequest, usages []capi.KeyUsage) (
 	return pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: der}), nil
 }
 
-func requestValidForSignerName(req *x509.CertificateRequest, usages []capi.KeyUsage, signerName string) bool {
+// getCSRVerificationFuncForSignerName is a function that provides reliable mapping of signer names to verification so that
-	// Only handle CSRs with the specific known signerNames.
+// we don't have accidents with wiring at some later date.
+func getCSRVerificationFuncForSignerName(signerName string) (isRequestForSignerFunc, error) {
 	switch signerName {
 	case capi.KubeletServingSignerName:
-		return capihelper.IsKubeletServingCSR(req, usages)
+		return isKubeletServing, nil
 	case capi.KubeAPIServerClientKubeletSignerName:
-		return capihelper.IsKubeletClientCSR(req, usages)
+		return isKubeletClient, nil
 	case capi.KubeAPIServerClientSignerName:
-		return validAPIServerClientUsages(usages)
+		return isKubeAPIServerClient, nil
 	case capi.LegacyUnknownSignerName:
-		// No restrictions are applied to the legacy-unknown signerName to
+		return isLegacyUnknown, nil
-		// maintain backward compatibility in v1beta1.
-		return true
 	default:
+		// TODO type this error so that a different reporting loop (one without a signing cert), can mark
+		//  CSRs with unknown kube signers as terminal if we wish.  This largely depends on how tightly we want to control
+		//  our signerNames.
+		return nil, fmt.Errorf("unrecongized signerName: %q", signerName)
+	}
+}
+
+func isKubeletServing(req *x509.CertificateRequest, usages []capi.KeyUsage, signerName string) bool {
+	if signerName != capi.KubeletServingSignerName {
 		return false
 	}
+	return capihelper.IsKubeletServingCSR(req, usages)
+}
+
+func isKubeletClient(req *x509.CertificateRequest, usages []capi.KeyUsage, signerName string) bool {
+	if signerName != capi.KubeAPIServerClientKubeletSignerName {
+		return false
+	}
+	return capihelper.IsKubeletClientCSR(req, usages)
+}
+
+func isKubeAPIServerClient(req *x509.CertificateRequest, usages []capi.KeyUsage, signerName string) bool {
+	if signerName != capi.KubeAPIServerClientSignerName {
+		return false
+	}
+	return validAPIServerClientUsages(usages)
+}
+
+func isLegacyUnknown(req *x509.CertificateRequest, usages []capi.KeyUsage, signerName string) bool {
+	if signerName != capi.LegacyUnknownSignerName {
+		return false
+	}
+	// No restrictions are applied to the legacy-unknown signerName to
+	// maintain backward compatibility in v1beta1.
+	return true
 }
 
 func validAPIServerClientUsages(usages []capi.KeyUsage) bool {

pkg/controller/certificates/signer/signer_test.go

@@ -42,7 +42,7 @@ import (
 func TestSigner(t *testing.T) {
 	clock := clock.FakeClock{}
 
-	s, err := newSigner("./testdata/ca.crt", "./testdata/ca.key", nil, 1*time.Hour)
+	s, err := newSigner(capi.LegacyUnknownSignerName, "./testdata/ca.crt", "./testdata/ca.key", nil, 1*time.Hour)
 	if err != nil {
 		t.Fatalf("failed to create signer: %v", err)
 	}
@@ -118,6 +118,8 @@ func TestHandle(t *testing.T) {
 		signerName string
 		// if true, expect an error to be returned
 		err bool
+		// if true, expect an error to be returned during construction
+		constructionErr bool
 		// additional verification function
 		verify func(*testing.T, []testclient.Action)
 	}{
@@ -206,9 +208,10 @@ func TestHandle(t *testing.T) {
 			},
 		},
 		{
-			name:       "should do nothing if an unrecognised signerName is used",
+			name:            "should do nothing if an unrecognised signerName is used",
-			signerName: "kubernetes.io/not-recognised",
+			signerName:      "kubernetes.io/not-recognised",
-			approved:   true,
+			constructionErr: true,
+			approved:        true,
 			verify: func(t *testing.T, as []testclient.Action) {
 				if len(as) != 0 {
 					t.Errorf("expected no action to be taken")
@@ -225,9 +228,10 @@ func TestHandle(t *testing.T) {
 			},
 		},
 		{
-			name:       "should do nothing if signerName does not start with kubernetes.io",
+			name:            "should do nothing if signerName does not start with kubernetes.io",
-			signerName: "example.com/sample-name",
+			signerName:      "example.com/sample-name",
-			approved:   true,
+			constructionErr: true,
+			approved:        true,
 			verify: func(t *testing.T, as []testclient.Action) {
 				if len(as) != 0 {
 					t.Errorf("expected no action to be taken")
@@ -235,9 +239,10 @@ func TestHandle(t *testing.T) {
 			},
 		},
 		{
-			name:       "should do nothing if signerName starts with kubernetes.io but is unrecognised",
+			name:            "should do nothing if signerName starts with kubernetes.io but is unrecognised",
-			signerName: "kubernetes.io/not-a-real-signer",
+			signerName:      "kubernetes.io/not-a-real-signer",
-			approved:   true,
+			constructionErr: true,
+			approved:        true,
 			verify: func(t *testing.T, as []testclient.Action) {
 				if len(as) != 0 {
 					t.Errorf("expected no action to be taken")
@@ -249,9 +254,17 @@ func TestHandle(t *testing.T) {
 	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)
+			s, err := newSigner(c.signerName, "./testdata/ca.crt", "./testdata/ca.key", client, 1*time.Hour)
-			if err != nil {
+			switch {
+			case c.constructionErr && err != nil:
+				return
+			case c.constructionErr && err == nil:
+				t.Fatalf("expected failure during construction of controller")
+			case !c.constructionErr && err != nil:
 				t.Fatalf("failed to create signer: %v", err)
+
+			case !c.constructionErr && err == nil:
+				// continue with rest of test
 			}
 
 			csr := makeTestCSR(csrBuilder{cn: c.commonName, signerName: c.signerName, approved: c.approved, usages: c.usages, org: c.org, dnsNames: c.dnsNames})