From 7e891e5d6cf271adea2359faea81687d347eb74f Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Wed, 5 May 2021 17:48:32 -0400 Subject: [PATCH] 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 --- cmd/kubelet/app/server_bootstrap_test.go | 4 +- .../certificates/authority/authority.go | 23 +--- .../certificates/authority/authority_test.go | 104 ++++++++++++++---- .../certificates/authority/policies.go | 57 ++++++++-- .../certificates/signer/ca_provider.go | 2 - pkg/controller/certificates/signer/signer.go | 11 +- .../certificates/signer/signer_test.go | 17 +-- 7 files changed, 148 insertions(+), 70 deletions(-) diff --git a/cmd/kubelet/app/server_bootstrap_test.go b/cmd/kubelet/app/server_bootstrap_test.go index c83f604340a..8cf2ee852da 100644 --- a/cmd/kubelet/app/server_bootstrap_test.go +++ b/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) diff --git a/pkg/controller/certificates/authority/authority.go b/pkg/controller/certificates/authority/authority.go index 5d064bbb66d..804b633a802 100644 --- a/pkg/controller/certificates/authority/authority.go +++ b/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) diff --git a/pkg/controller/certificates/authority/authority_test.go b/pkg/controller/certificates/authority/authority_test.go index a875e4f737f..7ae13ec3823 100644 --- a/pkg/controller/certificates/authority/authority_test.go +++ b/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() +} diff --git a/pkg/controller/certificates/authority/policies.go b/pkg/controller/certificates/authority/policies.go index 0d8d9b374d1..411fafeec4b 100644 --- a/pkg/controller/certificates/authority/policies.go +++ b/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 diff --git a/pkg/controller/certificates/signer/ca_provider.go b/pkg/controller/certificates/signer/ca_provider.go index 1d0d560aa3a..405b7a0506a 100644 --- a/pkg/controller/certificates/signer/ca_provider.go +++ b/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) diff --git a/pkg/controller/certificates/signer/signer.go b/pkg/controller/certificates/signer/signer.go index a6c084ef45c..604765515e0 100644 --- a/pkg/controller/certificates/signer/signer.go +++ b/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 diff --git a/pkg/controller/certificates/signer/signer_test.go b/pkg/controller/certificates/signer/signer_test.go index 045349a4b77..215838d7dba 100644 --- a/pkg/controller/certificates/signer/signer_test.go +++ b/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,