From 83035890ad604bf1ee141b632178faeda0c59077 Mon Sep 17 00:00:00 2001 From: David Eads Date: Mon, 4 May 2020 11:29:56 -0400 Subject: [PATCH] refactor the CSR controller into distinct controllers to allow easy configuration of multiple signing keys --- cmd/kube-controller-manager/app/BUILD | 1 + .../app/certificates.go | 45 +++++-- pkg/controller/certificates/signer/signer.go | 119 +++++++++++++++--- .../certificates/signer/signer_test.go | 37 ++++-- 4 files changed, 159 insertions(+), 43 deletions(-) diff --git a/cmd/kube-controller-manager/app/BUILD b/cmd/kube-controller-manager/app/BUILD index 03cd1306bcf..3c6e9c2ce16 100644 --- a/cmd/kube-controller-manager/app/BUILD +++ b/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", diff --git a/cmd/kube-controller-manager/app/certificates.go b/cmd/kube-controller-manager/app/certificates.go index 16708d726c2..c06e6255a50 100644 --- a/cmd/kube-controller-manager/app/certificates.go +++ b/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( - c, - ctx.InformerFactory.Certificates().V1beta1().CertificateSigningRequests(), - ctx.ComponentConfig.CSRSigningController.ClusterSigningCertFile, - ctx.ComponentConfig.CSRSigningController.ClusterSigningKeyFile, - ctx.ComponentConfig.CSRSigningController.ClusterSigningDuration.Duration, - ) + // TODO get different signer cert and key files for each signer when we add flags. + + kubeletServingSigner, err := signer.NewKubeletServingCSRSigningController(c, csrInformer, caFile, caKeyFile, certTTL) 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] { diff --git a/pkg/controller/certificates/signer/signer.go b/pkg/controller/certificates/signer/signer.go index 836119bd8ea..452e727b08a 100644 --- a/pkg/controller/certificates/signer/signer.go +++ b/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, - client: client, - certTTL: certificateDuration, + caProvider: caProvider, + client: client, + 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 - // not have a 'kubernetes.io/' prefix. - if !strings.HasPrefix(*csr.Spec.SignerName, "kubernetes.io/") { + // Fast-path to avoid any additional processing if the CSRs signerName does not match + if *csr.Spec.SignerName != s.signerName { 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 { - // Only handle CSRs with the specific known signerNames. +// getCSRVerificationFuncForSignerName is a function that provides reliable mapping of signer names to verification so that +// 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 - // maintain backward compatibility in v1beta1. - return true + return isLegacyUnknown, nil 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 { diff --git a/pkg/controller/certificates/signer/signer_test.go b/pkg/controller/certificates/signer/signer_test.go index 802e7ade4c6..baeeb1420a3 100644 --- a/pkg/controller/certificates/signer/signer_test.go +++ b/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", - signerName: "kubernetes.io/not-recognised", - approved: true, + name: "should do nothing if an unrecognised signerName is used", + signerName: "kubernetes.io/not-recognised", + 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", - signerName: "example.com/sample-name", - approved: true, + name: "should do nothing if signerName does not start with kubernetes.io", + signerName: "example.com/sample-name", + 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", - signerName: "kubernetes.io/not-a-real-signer", - approved: true, + name: "should do nothing if signerName starts with kubernetes.io but is unrecognised", + signerName: "kubernetes.io/not-a-real-signer", + 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) - if err != nil { + s, err := newSigner(c.signerName, "./testdata/ca.crt", "./testdata/ca.key", client, 1*time.Hour) + 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})