csr: correctly handle backdating of short lived certs

This change updates the backdating logic to only be applied to the
NotBefore date and not the NotAfter date when the certificate is
short lived. Thus when such a certificate is issued, it will not be
immediately expired.  Long lived certificates continue to have the
same lifetime as before.

Consolidated all certificate lifetime logic into the
PermissiveSigningPolicy.policy method.

Signed-off-by: Monis Khan <mok@vmware.com>

cmd/kubelet/app/server_bootstrap_test.go

@@ -298,14 +298,14 @@ func (s *csrSimulator) ServeHTTP(w http.ResponseWriter, req *http.Request) {
 		ca := &authority.CertificateAuthority{
 			Certificate: s.serverCA,
 			PrivateKey:  s.serverPrivateKey,
-			Backdate:    s.backdate,
 		}
 		cr, err := capihelper.ParseCSR(csr.Spec.Request)
 		if err != nil {
 			t.Fatal(err)
 		}
 		der, err := ca.Sign(cr.Raw, authority.PermissiveSigningPolicy{
-			TTL: time.Hour,
+			TTL:      time.Hour,
+			Backdate: s.backdate,
 		})
 		if err != nil {
 			t.Fatal(err)

pkg/controller/certificates/authority/authority.go

@@ -22,7 +22,6 @@ import (
 	"crypto/x509"
 	"fmt"
 	"math/big"
-	"time"
 )
 
 var serialNumberLimit = new(big.Int).Lsh(big.NewInt(1), 128)
@@ -37,23 +36,11 @@ type CertificateAuthority struct {
 
 	Certificate *x509.Certificate
 	PrivateKey  crypto.Signer
-	Backdate    time.Duration
-	Now         func() time.Time
 }
 
 // Sign signs a certificate request, applying a SigningPolicy and returns a DER
 // encoded x509 certificate.
 func (ca *CertificateAuthority) Sign(crDER []byte, policy SigningPolicy) ([]byte, error) {
-	now := time.Now()
-	if ca.Now != nil {
-		now = ca.Now()
-	}
-
-	nbf := now.Add(-ca.Backdate)
-	if !nbf.Before(ca.Certificate.NotAfter) {
-		return nil, fmt.Errorf("the signer has expired: NotAfter=%v", ca.Certificate.NotAfter)
-	}
-
 	cr, err := x509.ParseCertificateRequest(crDER)
 	if err != nil {
 		return nil, fmt.Errorf("unable to parse certificate request: %v", err)
@@ -78,19 +65,11 @@ func (ca *CertificateAuthority) Sign(crDER []byte, policy SigningPolicy) ([]byte
 		PublicKey:          cr.PublicKey,
 		Extensions:         cr.Extensions,
 		ExtraExtensions:    cr.ExtraExtensions,
-		NotBefore:          nbf,
 	}
-	if err := policy.apply(tmpl); err != nil {
+	if err := policy.apply(tmpl, ca.Certificate.NotAfter); err != nil {
 		return nil, err
 	}
 
-	if !tmpl.NotAfter.Before(ca.Certificate.NotAfter) {
-		tmpl.NotAfter = ca.Certificate.NotAfter
-	}
-	if !now.Before(ca.Certificate.NotAfter) {
-		return nil, fmt.Errorf("refusing to sign a certificate that expired in the past")
-	}
-
 	der, err := x509.CreateCertificate(rand.Reader, tmpl, ca.Certificate, cr.PublicKey, ca.PrivateKey)
 	if err != nil {
 		return nil, fmt.Errorf("failed to sign certificate: %v", err)

pkg/controller/certificates/authority/authority_test.go

@@ -40,6 +40,7 @@ func TestCertificateAuthority(t *testing.T) {
 		t.Fatal(err)
 	}
 	now := time.Now()
+	nowFunc := func() time.Time { return now }
 	tmpl := &x509.Certificate{
 		SerialNumber: big.NewInt(42),
 		Subject: pkix.Name{
@@ -68,15 +69,15 @@ func TestCertificateAuthority(t *testing.T) {
 	tests := []struct {
 		name     string
 		cr       x509.CertificateRequest
-		backdate time.Duration
 		policy   SigningPolicy
+		mutateCA func(ca *CertificateAuthority)
 
 		want    x509.Certificate
-		wantErr bool
+		wantErr string
 	}{
 		{
 			name:   "ca info",
-			policy: PermissiveSigningPolicy{TTL: time.Hour},
+			policy: PermissiveSigningPolicy{TTL: time.Hour, Now: nowFunc},
 			want: x509.Certificate{
 				Issuer:                caCert.Subject,
 				AuthorityKeyId:        caCert.SubjectKeyId,
@@ -87,7 +88,7 @@ func TestCertificateAuthority(t *testing.T) {
 		},
 		{
 			name:   "key usage",
-			policy: PermissiveSigningPolicy{TTL: time.Hour, Usages: []capi.KeyUsage{"signing"}},
+			policy: PermissiveSigningPolicy{TTL: time.Hour, Usages: []capi.KeyUsage{"signing"}, Now: nowFunc},
 			want: x509.Certificate{
 				NotBefore:             now,
 				NotAfter:              now.Add(1 * time.Hour),
@@ -97,7 +98,7 @@ func TestCertificateAuthority(t *testing.T) {
 		},
 		{
 			name:   "ext key usage",
-			policy: PermissiveSigningPolicy{TTL: time.Hour, Usages: []capi.KeyUsage{"client auth"}},
+			policy: PermissiveSigningPolicy{TTL: time.Hour, Usages: []capi.KeyUsage{"client auth"}, Now: nowFunc},
 			want: x509.Certificate{
 				NotBefore:             now,
 				NotAfter:              now.Add(1 * time.Hour),
@@ -106,18 +107,53 @@ func TestCertificateAuthority(t *testing.T) {
 			},
 		},
 		{
-			name:     "backdate",
-			policy:   PermissiveSigningPolicy{TTL: time.Hour},
-			backdate: 5 * time.Minute,
+			name:   "backdate without short",
+			policy: PermissiveSigningPolicy{TTL: time.Hour, Backdate: 5 * time.Minute, Now: nowFunc},
 			want: x509.Certificate{
 				NotBefore:             now.Add(-5 * time.Minute),
 				NotAfter:              now.Add(55 * time.Minute),
 				BasicConstraintsValid: true,
 			},
 		},
+		{
+			name:   "backdate without short and super small ttl",
+			policy: PermissiveSigningPolicy{TTL: time.Minute, Backdate: 5 * time.Minute, Now: nowFunc},
+			want: x509.Certificate{
+				NotBefore:             now.Add(-5 * time.Minute),
+				NotAfter:              now.Add(-4 * time.Minute),
+				BasicConstraintsValid: true,
+			},
+		},
+		{
+			name:   "backdate with short",
+			policy: PermissiveSigningPolicy{TTL: time.Hour, Backdate: 5 * time.Minute, Short: 8 * time.Hour, Now: nowFunc},
+			want: x509.Certificate{
+				NotBefore:             now.Add(-5 * time.Minute),
+				NotAfter:              now.Add(time.Hour),
+				BasicConstraintsValid: true,
+			},
+		},
+		{
+			name:   "backdate with short and super small ttl",
+			policy: PermissiveSigningPolicy{TTL: time.Minute, Backdate: 5 * time.Minute, Short: 8 * time.Hour, Now: nowFunc},
+			want: x509.Certificate{
+				NotBefore:             now.Add(-5 * time.Minute),
+				NotAfter:              now.Add(time.Minute),
+				BasicConstraintsValid: true,
+			},
+		},
+		{
+			name:   "backdate with short but longer ttl",
+			policy: PermissiveSigningPolicy{TTL: 24 * time.Hour, Backdate: 5 * time.Minute, Short: 8 * time.Hour, Now: nowFunc},
+			want: x509.Certificate{
+				NotBefore:             now.Add(-5 * time.Minute),
+				NotAfter:              now.Add(24*time.Hour - 5*time.Minute),
+				BasicConstraintsValid: true,
+			},
+		},
 		{
 			name:   "truncate expiration",
-			policy: PermissiveSigningPolicy{TTL: 48 * time.Hour},
+			policy: PermissiveSigningPolicy{TTL: 48 * time.Hour, Now: nowFunc},
 			want: x509.Certificate{
 				NotBefore:             now,
 				NotAfter:              now.Add(24 * time.Hour),
@@ -126,7 +162,7 @@ func TestCertificateAuthority(t *testing.T) {
 		},
 		{
 			name:   "uri sans",
-			policy: PermissiveSigningPolicy{TTL: time.Hour},
+			policy: PermissiveSigningPolicy{TTL: time.Hour, Now: nowFunc},
 			cr: x509.CertificateRequest{
 				URIs: []*url.URL{uri},
 			},
@@ -137,6 +173,22 @@ func TestCertificateAuthority(t *testing.T) {
 				BasicConstraintsValid: true,
 			},
 		},
+		{
+			name:   "expired ca",
+			policy: PermissiveSigningPolicy{TTL: time.Hour, Now: nowFunc},
+			mutateCA: func(ca *CertificateAuthority) {
+				ca.Certificate.NotAfter = now // pretend that the CA has expired
+			},
+			wantErr: "the signer has expired: NotAfter=" + now.String(),
+		},
+		{
+			name:   "expired ca with backdate",
+			policy: PermissiveSigningPolicy{TTL: time.Hour, Backdate: 5 * time.Minute, Now: nowFunc},
+			mutateCA: func(ca *CertificateAuthority) {
+				ca.Certificate.NotAfter = now // pretend that the CA has expired
+			},
+			wantErr: "refusing to sign a certificate that expired in the past: NotAfter=" + now.String(),
+		},
 	}
 
 	crKey, err := ecdsa.GenerateKey(elliptic.P224(), rand.Reader)
@@ -146,13 +198,15 @@ func TestCertificateAuthority(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
+			caCertShallowCopy := *caCert
+
 			ca := &CertificateAuthority{
-				Certificate: caCert,
+				Certificate: &caCertShallowCopy,
 				PrivateKey:  caKey,
-				Now: func() time.Time {
-					return now
-				},
-				Backdate: test.backdate,
+			}
+
+			if test.mutateCA != nil {
+				test.mutateCA(ca)
 			}
 
 			csr, err := x509.CreateCertificateRequest(rand.Reader, &test.cr, crKey)
@@ -161,15 +215,15 @@ func TestCertificateAuthority(t *testing.T) {
 			}
 
 			certDER, err := ca.Sign(csr, test.policy)
-			if err != nil {
-				t.Fatal(err)
-			}
-			if test.wantErr {
-				if err == nil {
-					t.Fatal("expected error")
+			if len(test.wantErr) > 0 {
+				if errStr := errString(err); test.wantErr != errStr {
+					t.Fatalf("expected error %s but got %s", test.wantErr, errStr)
 				}
 				return
 			}
+			if err != nil {
+				t.Fatal(err)
+			}
 
 			cert, err := x509.ParseCertificate(certDER)
 			if err != nil {
@@ -197,3 +251,11 @@ func TestCertificateAuthority(t *testing.T) {
 		})
 	}
 }
+
+func errString(err error) string {
+	if err == nil {
+		return ""
+	}
+
+	return err.Error()
+}

pkg/controller/certificates/authority/policies.go

@@ -31,7 +31,7 @@ import (
 type SigningPolicy interface {
 	// not-exporting apply forces signing policy implementations to be internal
 	// to this package.
-	apply(template *x509.Certificate) error
+	apply(template *x509.Certificate, signerNotAfter time.Time) error
 }
 
 // PermissiveSigningPolicy is the signing policy historically used by the local
@@ -39,32 +39,75 @@ type SigningPolicy interface {
 //
 //  * It forwards all SANs from the original signing request.
 //  * It sets allowed usages as configured in the policy.
-//  * It sets NotAfter based on the TTL configured in the policy.
 //  * It zeros all extensions.
 //  * It sets BasicConstraints to true.
 //  * It sets IsCA to false.
+//  * It validates that the signer has not expired.
+//  * It sets NotBefore and NotAfter:
+//    All certificates set NotBefore = Now() - Backdate.
+//    Long-lived certificates set NotAfter = Now() + TTL - Backdate.
+//    Short-lived certificates set NotAfter = Now() + TTL.
+//    All certificates truncate NotAfter to the expiration date of the signer.
 type PermissiveSigningPolicy struct {
-	// TTL is the certificate TTL. It's used to calculate the NotAfter value of
-	// the certificate.
+	// TTL is used in certificate NotAfter calculation as described above.
 	TTL time.Duration
+
 	// Usages are the allowed usages of a certificate.
 	Usages []capi.KeyUsage
+
+	// Backdate is used in certificate NotBefore calculation as described above.
+	Backdate time.Duration
+
+	// Short is the duration used to determine if the lifetime of a certificate should be considered short.
+	Short time.Duration
+
+	// Now defaults to time.Now but can be stubbed for testing
+	Now func() time.Time
 }
 
-func (p PermissiveSigningPolicy) apply(tmpl *x509.Certificate) error {
+func (p PermissiveSigningPolicy) apply(tmpl *x509.Certificate, signerNotAfter time.Time) error {
+	var now time.Time
+	if p.Now != nil {
+		now = p.Now()
+	} else {
+		now = time.Now()
+	}
+
+	ttl := p.TTL
+
 	usage, extUsages, err := keyUsagesFromStrings(p.Usages)
 	if err != nil {
 		return err
 	}
 	tmpl.KeyUsage = usage
 	tmpl.ExtKeyUsage = extUsages
-	tmpl.NotAfter = tmpl.NotBefore.Add(p.TTL)
 
 	tmpl.ExtraExtensions = nil
 	tmpl.Extensions = nil
 	tmpl.BasicConstraintsValid = true
 	tmpl.IsCA = false
 
+	tmpl.NotBefore = now.Add(-p.Backdate)
+
+	if ttl < p.Short {
+		// do not backdate the end time if we consider this to be a short lived certificate
+		tmpl.NotAfter = now.Add(ttl)
+	} else {
+		tmpl.NotAfter = now.Add(ttl - p.Backdate)
+	}
+
+	if !tmpl.NotAfter.Before(signerNotAfter) {
+		tmpl.NotAfter = signerNotAfter
+	}
+
+	if !tmpl.NotBefore.Before(signerNotAfter) {
+		return fmt.Errorf("the signer has expired: NotAfter=%v", signerNotAfter)
+	}
+
+	if !now.Before(signerNotAfter) {
+		return fmt.Errorf("refusing to sign a certificate that expired in the past: NotAfter=%v", signerNotAfter)
+	}
+
 	return nil
 }
 
@@ -124,7 +167,7 @@ func keyUsagesFromStrings(usages []capi.KeyUsage) (x509.KeyUsage, []x509.ExtKeyU
 		return 0, nil, fmt.Errorf("unrecognized usage values: %q", unrecognized)
 	}
 
-	return keyUsage, []x509.ExtKeyUsage(sorted), nil
+	return keyUsage, sorted, nil
 }
 
 type sortedExtKeyUsage []x509.ExtKeyUsage

pkg/controller/certificates/signer/ca_provider.go

@@ -21,7 +21,6 @@ import (
 	"crypto"
 	"fmt"
 	"sync/atomic"
-	"time"
 
 	"k8s.io/apiserver/pkg/server/dynamiccertificates"
 	"k8s.io/client-go/util/cert"
@@ -77,7 +76,6 @@ func (p *caProvider) setCA() error {
 
 		Certificate: certs[0],
 		PrivateKey:  priv,
-		Backdate:    5 * time.Minute,
 	}
 	p.caValue.Store(ca)
 

pkg/controller/certificates/signer/signer.go

@@ -173,7 +173,7 @@ func (s *signer) handle(csr *capi.CertificateSigningRequest) error {
 		// Ignore requests for kubernetes.io signerNames we don't recognize
 		return nil
 	}
-	cert, err := s.sign(x509cr, csr.Spec.Usages)
+	cert, err := s.sign(x509cr, csr.Spec.Usages, nil)
 	if err != nil {
 		return fmt.Errorf("error auto signing csr: %v", err)
 	}
@@ -185,14 +185,17 @@ func (s *signer) handle(csr *capi.CertificateSigningRequest) error {
 	return nil
 }
 
-func (s *signer) sign(x509cr *x509.CertificateRequest, usages []capi.KeyUsage) ([]byte, error) {
+func (s *signer) sign(x509cr *x509.CertificateRequest, usages []capi.KeyUsage, now func() time.Time) ([]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: usages,
+		TTL:      s.certTTL,
+		Usages:   usages,
+		Backdate: 5 * time.Minute, // this must always be less than the minimum TTL requested by a user
+		Short:    8 * time.Hour,   // 5 minutes of backdating is roughly 1% of 8 hours
+		Now:      now,
 	})
 	if err != nil {
 		return nil, err

pkg/controller/certificates/signer/signer_test.go

@@ -35,25 +35,17 @@ 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/v1"
+	"k8s.io/kubernetes/pkg/controller/certificates"
 )
 
 func TestSigner(t *testing.T) {
-	clock := clock.FakeClock{}
+	fakeClock := clock.FakeClock{}
 
 	s, err := newSigner("kubernetes.io/legacy-unknown", "./testdata/ca.crt", "./testdata/ca.key", nil, 1*time.Hour)
 	if err != nil {
 		t.Fatalf("failed to create signer: %v", err)
 	}
-	currCA, err := s.caProvider.currentCA()
-	if err != nil {
-		t.Fatal(err)
-	}
-	currCA.Now = clock.Now
-	currCA.Backdate = 0
-	s.caProvider.caValue.Store(currCA)
 
 	csrb, err := ioutil.ReadFile("./testdata/kubelet.csr")
 	if err != nil {
@@ -69,7 +61,7 @@ func TestSigner(t *testing.T) {
 		capi.UsageKeyEncipherment,
 		capi.UsageServerAuth,
 		capi.UsageClientAuth,
-	})
+	}, fakeClock.Now)
 	if err != nil {
 		t.Fatalf("failed to sign CSR: %v", err)
 	}
@@ -94,7 +86,8 @@ func TestSigner(t *testing.T) {
 		KeyUsage:              x509.KeyUsageDigitalSignature | x509.KeyUsageKeyEncipherment,
 		ExtKeyUsage:           []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth, x509.ExtKeyUsageClientAuth},
 		BasicConstraintsValid: true,
-		NotAfter:              clock.Now().Add(1 * time.Hour),
+		NotBefore:             fakeClock.Now().Add(-5 * time.Minute),
+		NotAfter:              fakeClock.Now().Add(1 * time.Hour),
 		PublicKeyAlgorithm:    x509.ECDSA,
 		SignatureAlgorithm:    x509.SHA256WithRSA,
 		MaxPathLen:            -1,