From 57eddd5e044efa0987d7e412c9db2a89b4f8e7c2 Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Thu, 16 Apr 2020 00:51:33 -0400 Subject: [PATCH] Record Failed condition in signer controller --- pkg/apis/certificates/v1beta1/defaults.go | 55 +++++++++++++---- .../certificates/certificate_controller.go | 2 +- .../certificate_controller_utils.go | 15 ++++- pkg/controller/certificates/signer/signer.go | 60 ++++++++++++------- .../certificates/signer/signer_test.go | 37 +++++++++++- 5 files changed, 129 insertions(+), 40 deletions(-) diff --git a/pkg/apis/certificates/v1beta1/defaults.go b/pkg/apis/certificates/v1beta1/defaults.go index ca0913735f6..83a43876b43 100644 --- a/pkg/apis/certificates/v1beta1/defaults.go +++ b/pkg/apis/certificates/v1beta1/defaults.go @@ -18,6 +18,7 @@ package v1beta1 import ( "crypto/x509" + "fmt" "reflect" "strings" @@ -66,18 +67,34 @@ func DefaultSignerNameFromSpec(obj *certificatesv1beta1.CertificateSigningReques } } +var ( + organizationNotSystemNodesErr = fmt.Errorf("subject organization is not system:nodes") + commonNameNotSystemNode = fmt.Errorf("subject common name does not begin with system:node:") + dnsOrIPSANRequiredErr = fmt.Errorf("DNS or IP subjectAltName is required") + dnsSANNotAllowedErr = fmt.Errorf("DNS subjectAltNames are not allowed") + emailSANNotAllowedErr = fmt.Errorf("Email subjectAltNames are not allowed") + ipSANNotAllowedErr = fmt.Errorf("IP subjectAltNames are not allowed") + uriSANNotAllowedErr = fmt.Errorf("URI subjectAltNames are not allowed") +) + func IsKubeletServingCSR(req *x509.CertificateRequest, usages []certificatesv1beta1.KeyUsage) bool { + return ValidateKubeletServingCSR(req, usages) == nil +} +func ValidateKubeletServingCSR(req *x509.CertificateRequest, usages []certificatesv1beta1.KeyUsage) error { if !reflect.DeepEqual([]string{"system:nodes"}, req.Subject.Organization) { - return false + return organizationNotSystemNodesErr } // at least one of dnsNames or ipAddresses must be specified if len(req.DNSNames) == 0 && len(req.IPAddresses) == 0 { - return false + return dnsOrIPSANRequiredErr } - if len(req.EmailAddresses) > 0 || len(req.URIs) > 0 { - return false + if len(req.EmailAddresses) > 0 { + return emailSANNotAllowedErr + } + if len(req.URIs) > 0 { + return uriSANNotAllowedErr } requiredUsages := []certificatesv1beta1.KeyUsage{ @@ -86,27 +103,39 @@ func IsKubeletServingCSR(req *x509.CertificateRequest, usages []certificatesv1be certificatesv1beta1.UsageServerAuth, } if !equalUnsorted(requiredUsages, usages) { - return false + return fmt.Errorf("usages did not match %v", requiredUsages) } if !strings.HasPrefix(req.Subject.CommonName, "system:node:") { - return false + return commonNameNotSystemNode } - return true + return nil } func IsKubeletClientCSR(req *x509.CertificateRequest, usages []certificatesv1beta1.KeyUsage) bool { + return ValidateKubeletClientCSR(req, usages) == nil +} +func ValidateKubeletClientCSR(req *x509.CertificateRequest, usages []certificatesv1beta1.KeyUsage) error { if !reflect.DeepEqual([]string{"system:nodes"}, req.Subject.Organization) { - return false + return organizationNotSystemNodesErr } - if len(req.DNSNames) > 0 || len(req.EmailAddresses) > 0 || len(req.IPAddresses) > 0 || len(req.URIs) > 0 { - return false + if len(req.DNSNames) > 0 { + return dnsSANNotAllowedErr + } + if len(req.EmailAddresses) > 0 { + return emailSANNotAllowedErr + } + if len(req.IPAddresses) > 0 { + return ipSANNotAllowedErr + } + if len(req.URIs) > 0 { + return uriSANNotAllowedErr } if !strings.HasPrefix(req.Subject.CommonName, "system:node:") { - return false + return commonNameNotSystemNode } requiredUsages := []certificatesv1beta1.KeyUsage{ @@ -115,10 +144,10 @@ func IsKubeletClientCSR(req *x509.CertificateRequest, usages []certificatesv1bet certificatesv1beta1.UsageClientAuth, } if !equalUnsorted(requiredUsages, usages) { - return false + return fmt.Errorf("usages did not match %v", requiredUsages) } - return true + return nil } // equalUnsorted compares two []string for equality of contents regardless of diff --git a/pkg/controller/certificates/certificate_controller.go b/pkg/controller/certificates/certificate_controller.go index e2dc80e8ac2..e030a945407 100644 --- a/pkg/controller/certificates/certificate_controller.go +++ b/pkg/controller/certificates/certificate_controller.go @@ -186,7 +186,7 @@ func (cc *CertificateController) syncFunc(key string) error { return err } - if csr.Status.Certificate != nil { + if len(csr.Status.Certificate) > 0 { // no need to do anything because it already has a cert return nil } diff --git a/pkg/controller/certificates/certificate_controller_utils.go b/pkg/controller/certificates/certificate_controller_utils.go index 4a5d0d4f2bb..941a72dca23 100644 --- a/pkg/controller/certificates/certificate_controller_utils.go +++ b/pkg/controller/certificates/certificate_controller_utils.go @@ -16,7 +16,10 @@ limitations under the License. package certificates -import certificates "k8s.io/api/certificates/v1beta1" +import ( + certificates "k8s.io/api/certificates/v1beta1" + v1 "k8s.io/api/core/v1" +) // IsCertificateRequestApproved returns true if a certificate request has the // "Approved" condition and no "Denied" conditions; false otherwise. @@ -25,6 +28,16 @@ func IsCertificateRequestApproved(csr *certificates.CertificateSigningRequest) b return approved && !denied } +// HasCondition returns true if the csr contains a condition of the specified type with a status that is set to True or is empty +func HasTrueCondition(csr *certificates.CertificateSigningRequest, conditionType certificates.RequestConditionType) bool { + for _, c := range csr.Status.Conditions { + if c.Type == conditionType && (len(c.Status) == 0 || c.Status == v1.ConditionTrue) { + return true + } + } + return false +} + func GetCertApprovalCondition(status *certificates.CertificateSigningRequestStatus) (approved bool, denied bool) { for _, c := range status.Conditions { if c.Type == certificates.CertificateApproved { diff --git a/pkg/controller/certificates/signer/signer.go b/pkg/controller/certificates/signer/signer.go index 452e727b08a..986a95f94c8 100644 --- a/pkg/controller/certificates/signer/signer.go +++ b/pkg/controller/certificates/signer/signer.go @@ -25,6 +25,7 @@ import ( "time" capi "k8s.io/api/certificates/v1beta1" + v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apiserver/pkg/server/dynamiccertificates" certificatesinformers "k8s.io/client-go/informers/certificates/v1beta1" @@ -106,7 +107,7 @@ 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 isRequestForSignerFunc func(req *x509.CertificateRequest, usages []capi.KeyUsage, signerName string) (bool, error) type signer struct { caProvider *caProvider @@ -139,8 +140,8 @@ func newSigner(signerName, caFile, caKeyFile string, client clientset.Interface, } func (s *signer) handle(csr *capi.CertificateSigningRequest) error { - // Ignore unapproved requests - if !certificates.IsCertificateRequestApproved(csr) { + // Ignore unapproved or failed requests + if !certificates.IsCertificateRequestApproved(csr) || certificates.HasTrueCondition(csr, capi.CertificateFailed) { return nil } @@ -153,9 +154,21 @@ func (s *signer) handle(csr *capi.CertificateSigningRequest) error { if err != nil { return fmt.Errorf("unable to parse csr %q: %v", csr.Name, err) } - 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. + if recognized, err := s.isRequestForSignerFn(x509cr, csr.Spec.Usages, *csr.Spec.SignerName); err != nil { + csr.Status.Conditions = append(csr.Status.Conditions, capi.CertificateSigningRequestCondition{ + Type: capi.CertificateFailed, + Status: v1.ConditionTrue, + Reason: "SignerValidationFailure", + Message: err.Error(), + LastUpdateTime: metav1.Now(), + }) + _, err = s.client.CertificatesV1beta1().CertificateSigningRequests().UpdateStatus(context.TODO(), csr, metav1.UpdateOptions{}) + if err != nil { + return fmt.Errorf("error adding failure condition for csr: %v", err) + } + return nil + } else if !recognized { + // Ignore requests for kubernetes.io signerNames we don't recognize return nil } cert, err := s.sign(x509cr, csr.Spec.Usages) @@ -201,41 +214,41 @@ func getCSRVerificationFuncForSignerName(signerName string) (isRequestForSignerF // 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) + return nil, fmt.Errorf("unrecognized signerName: %q", signerName) } } -func isKubeletServing(req *x509.CertificateRequest, usages []capi.KeyUsage, signerName string) bool { +func isKubeletServing(req *x509.CertificateRequest, usages []capi.KeyUsage, signerName string) (bool, error) { if signerName != capi.KubeletServingSignerName { - return false + return false, nil } - return capihelper.IsKubeletServingCSR(req, usages) + return true, capihelper.ValidateKubeletServingCSR(req, usages) } -func isKubeletClient(req *x509.CertificateRequest, usages []capi.KeyUsage, signerName string) bool { +func isKubeletClient(req *x509.CertificateRequest, usages []capi.KeyUsage, signerName string) (bool, error) { if signerName != capi.KubeAPIServerClientKubeletSignerName { - return false + return false, nil } - return capihelper.IsKubeletClientCSR(req, usages) + return true, capihelper.ValidateKubeletClientCSR(req, usages) } -func isKubeAPIServerClient(req *x509.CertificateRequest, usages []capi.KeyUsage, signerName string) bool { +func isKubeAPIServerClient(req *x509.CertificateRequest, usages []capi.KeyUsage, signerName string) (bool, error) { if signerName != capi.KubeAPIServerClientSignerName { - return false + return false, nil } - return validAPIServerClientUsages(usages) + return true, validAPIServerClientUsages(usages) } -func isLegacyUnknown(req *x509.CertificateRequest, usages []capi.KeyUsage, signerName string) bool { +func isLegacyUnknown(req *x509.CertificateRequest, usages []capi.KeyUsage, signerName string) (bool, error) { if signerName != capi.LegacyUnknownSignerName { - return false + return false, nil } // No restrictions are applied to the legacy-unknown signerName to // maintain backward compatibility in v1beta1. - return true + return true, nil } -func validAPIServerClientUsages(usages []capi.KeyUsage) bool { +func validAPIServerClientUsages(usages []capi.KeyUsage) error { hasClientAuth := false for _, u := range usages { switch u { @@ -244,8 +257,11 @@ func validAPIServerClientUsages(usages []capi.KeyUsage) bool { case capi.UsageClientAuth: hasClientAuth = true default: - return false + return fmt.Errorf("invalid usage for client certificate: %s", u) } } - return hasClientAuth + if !hasClientAuth { + return fmt.Errorf("missing required usage for client certificate: %s", capi.UsageClientAuth) + } + return nil } diff --git a/pkg/controller/certificates/signer/signer_test.go b/pkg/controller/certificates/signer/signer_test.go index baeeb1420a3..b44c3cb4772 100644 --- a/pkg/controller/certificates/signer/signer_test.go +++ b/pkg/controller/certificates/signer/signer_test.go @@ -35,6 +35,7 @@ import ( "k8s.io/client-go/kubernetes/fake" testclient "k8s.io/client-go/testing" "k8s.io/client-go/util/cert" + "k8s.io/kubernetes/pkg/controller/certificates" capihelper "k8s.io/kubernetes/pkg/apis/certificates/v1beta1" ) @@ -114,6 +115,8 @@ func TestHandle(t *testing.T) { usages []capi.KeyUsage // whether the generated CSR should be marked as approved approved bool + // whether the generated CSR should be marked as failed + failed bool // the signerName to be set on the generated CSR signerName string // if true, expect an error to be returned @@ -149,10 +152,17 @@ func TestHandle(t *testing.T) { 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)) + 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 no certificate to be issued") + } + if !certificates.HasTrueCondition(csr, capi.CertificateFailed) { + t.Errorf("expected Failed condition") + } }, }, { @@ -207,6 +217,21 @@ func TestHandle(t *testing.T) { } }, }, + { + name: "should do nothing if failed", + 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, + failed: 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 an unrecognised signerName is used", signerName: "kubernetes.io/not-recognised", @@ -267,7 +292,7 @@ func TestHandle(t *testing.T) { // 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}) + csr := makeTestCSR(csrBuilder{cn: c.commonName, signerName: c.signerName, approved: c.approved, failed: c.failed, usages: c.usages, org: c.org, dnsNames: c.dnsNames}) if err := s.handle(csr); err != nil && !c.err { t.Errorf("unexpected err: %v", err) } @@ -286,6 +311,7 @@ type csrBuilder struct { org []string signerName string approved bool + failed bool usages []capi.KeyUsage } @@ -318,5 +344,10 @@ func makeTestCSR(b csrBuilder) *capi.CertificateSigningRequest { Type: capi.CertificateApproved, }) } + if b.failed { + csr.Status.Conditions = append(csr.Status.Conditions, capi.CertificateSigningRequestCondition{ + Type: capi.CertificateFailed, + }) + } return csr }