Merge pull request #99412 from enj/enj/i/ttl_backdate

csr: correctly handle backdating of short lived certs

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",
+			name:   "backdate without short",
-			policy:   PermissiveSigningPolicy{TTL: time.Hour},
+			policy: PermissiveSigningPolicy{TTL: time.Hour, Backdate: 5 * time.Minute, Now: nowFunc},
-			backdate: 5 * time.Minute,
 			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
+
-				},
+			if test.mutateCA != nil {
-				Backdate: test.backdate,
+				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 {
+			if len(test.wantErr) > 0 {
-				t.Fatal(err)
+				if errStr := errString(err); test.wantErr != errStr {
-			}
+					t.Fatalf("expected error %s but got %s", test.wantErr, errStr)
-			if test.wantErr {
-				if err == nil {
-					t.Fatal("expected error")
 				}
 				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
+	// TTL is used in certificate NotAfter calculation as described above.
-	// the certificate.
 	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,
+		TTL:      s.certTTL,
-		Usages: usages,
+		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,