Record Failed condition in signer controller

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 {
+	if len(req.EmailAddresses) > 0 {
-		return false
+		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 {
+	if len(req.DNSNames) > 0 {
-		return false
+		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

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
 	}

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 {

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
+	// Ignore unapproved or failed requests
-	if !certificates.IsCertificateRequestApproved(csr) {
+	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) {
+	if recognized, err := s.isRequestForSignerFn(x509cr, csr.Spec.Usages, *csr.Spec.SignerName); err != nil {
-		// TODO: mark the CertificateRequest as being in a terminal state and
+		csr.Status.Conditions = append(csr.Status.Conditions, capi.CertificateSigningRequestCondition{
-		//  communicate to the user why the request has been refused.
+			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
 }

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 {
+				if len(as) != 1 {
-					t.Errorf("expected no Update action but got %d", len(as))
+					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
 }