From 179dc4ca43d6dc7bf934d2cb504cebc60fe358f0 Mon Sep 17 00:00:00 2001 From: David Eads Date: Mon, 4 Feb 2019 13:19:42 -0500 Subject: [PATCH] csr signer has no need to sign certificates for a duration longer than the signer itself --- .../certificates/signer/cfssl_signer.go | 18 ++- .../certificates/signer/cfssl_signer_test.go | 111 ++++++++++++++++++ 2 files changed, 127 insertions(+), 2 deletions(-) diff --git a/pkg/controller/certificates/signer/cfssl_signer.go b/pkg/controller/certificates/signer/cfssl_signer.go index dd57d68a0dc..3c704f86d92 100644 --- a/pkg/controller/certificates/signer/cfssl_signer.go +++ b/pkg/controller/certificates/signer/cfssl_signer.go @@ -59,6 +59,9 @@ type cfsslSigner struct { sigAlgo x509.SignatureAlgorithm client clientset.Interface certificateDuration time.Duration + + // nowFn returns the current time. We have here for unit testing + nowFn func() time.Time } func newCFSSLSigner(caFile, caKeyFile string, client clientset.Interface, certificateDuration time.Duration) (*cfsslSigner, error) { @@ -92,6 +95,7 @@ func newCFSSLSigner(caFile, caKeyFile string, client clientset.Interface, certif sigAlgo: signer.DefaultSigAlgo(priv), client: client, certificateDuration: certificateDuration, + nowFn: time.Now, }, nil } @@ -115,11 +119,21 @@ func (s *cfsslSigner) sign(csr *capi.CertificateSigningRequest) (*capi.Certifica for _, usage := range csr.Spec.Usages { usages = append(usages, string(usage)) } + + certExpiryDuration := s.certificateDuration + durationUntilExpiry := s.ca.NotAfter.Sub(s.nowFn()) + if durationUntilExpiry <= 0 { + return nil, fmt.Errorf("the signer has expired: %v", s.ca.NotAfter) + } + if durationUntilExpiry < certExpiryDuration { + certExpiryDuration = durationUntilExpiry + } + policy := &config.Signing{ Default: &config.SigningProfile{ Usage: usages, - Expiry: s.certificateDuration, - ExpiryString: s.certificateDuration.String(), + Expiry: certExpiryDuration, + ExpiryString: certExpiryDuration.String(), }, } cfs, err := local.NewSigner(s.priv, s.ca, s.sigAlgo, policy) diff --git a/pkg/controller/certificates/signer/cfssl_signer_test.go b/pkg/controller/certificates/signer/cfssl_signer_test.go index a62827c6623..d7ea1a95de1 100644 --- a/pkg/controller/certificates/signer/cfssl_signer_test.go +++ b/pkg/controller/certificates/signer/cfssl_signer_test.go @@ -20,6 +20,7 @@ import ( "crypto/x509" "io/ioutil" "reflect" + "strings" "testing" "time" @@ -28,10 +29,16 @@ import ( ) func TestSigner(t *testing.T) { + testNow := time.Now() + testNowFn := func() time.Time { + return testNow + } + s, err := newCFSSLSigner("./testdata/ca.crt", "./testdata/ca.key", nil, 1*time.Hour) if err != nil { t.Fatalf("failed to create signer: %v", err) } + s.nowFn = testNowFn csrb, err := ioutil.ReadFile("./testdata/kubelet.csr") if err != nil { @@ -81,4 +88,108 @@ func TestSigner(t *testing.T) { if !reflect.DeepEqual(crt.ExtKeyUsage, []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth, x509.ExtKeyUsageClientAuth}) { t.Errorf("bad extended key usage") } + + expectedTime := testNow.Add(1 * time.Hour) + // there is some jitter that we need to tolerate + diff := expectedTime.Sub(crt.NotAfter) + if diff > 10*time.Minute || diff < -10*time.Minute { + t.Fatal(crt.NotAfter) + } +} + +func TestSignerExpired(t *testing.T) { + hundredYearsFromNowFn := func() time.Time { + return time.Now().Add(24 * time.Hour * 365 * 100) + } + s, err := newCFSSLSigner("./testdata/ca.crt", "./testdata/ca.key", nil, 1*time.Hour) + if err != nil { + t.Fatalf("failed to create signer: %v", err) + } + s.nowFn = hundredYearsFromNowFn + + csrb, err := ioutil.ReadFile("./testdata/kubelet.csr") + if err != nil { + t.Fatalf("failed to read CSR: %v", err) + } + + csr := &capi.CertificateSigningRequest{ + Spec: capi.CertificateSigningRequestSpec{ + Request: []byte(csrb), + Usages: []capi.KeyUsage{ + capi.UsageSigning, + capi.UsageKeyEncipherment, + capi.UsageServerAuth, + capi.UsageClientAuth, + }, + }, + } + + _, err = s.sign(csr) + if err == nil { + t.Fatal("missing error") + } + if !strings.HasPrefix(err.Error(), "the signer has expired") { + t.Fatal(err) + } +} + +func TestDurationLongerThanExpiry(t *testing.T) { + testNow := time.Now() + testNowFn := func() time.Time { + return testNow + } + + hundredYears := 24 * time.Hour * 365 * 100 + s, err := newCFSSLSigner("./testdata/ca.crt", "./testdata/ca.key", nil, hundredYears) + if err != nil { + t.Fatalf("failed to create signer: %v", err) + } + s.nowFn = testNowFn + + csrb, err := ioutil.ReadFile("./testdata/kubelet.csr") + if err != nil { + t.Fatalf("failed to read CSR: %v", err) + } + + csr := &capi.CertificateSigningRequest{ + Spec: capi.CertificateSigningRequestSpec{ + Request: []byte(csrb), + Usages: []capi.KeyUsage{ + capi.UsageSigning, + capi.UsageKeyEncipherment, + capi.UsageServerAuth, + capi.UsageClientAuth, + }, + }, + } + + _, err = s.sign(csr) + if err != nil { + t.Fatalf("failed to sign CSR: %v", err) + } + + // now we just need to verify that the expiry is based on the signing cert + certData := csr.Status.Certificate + if len(certData) == 0 { + t.Fatalf("expected a certificate after signing") + } + + certs, err := cert.ParseCertsPEM(certData) + if err != nil { + t.Fatalf("failed to parse certificate: %v", err) + } + if len(certs) != 1 { + t.Fatalf("expected one certificate") + } + + crt := certs[0] + expected, err := time.Parse("2006-01-02 15:04:05.999999999 -0700 MST", "2044-05-09 00:20:11 +0000 UTC") + if err != nil { + t.Fatal(err) + } + // there is some jitter that we need to tolerate + diff := expected.Sub(crt.NotAfter) + if diff > 10*time.Minute || diff < -10*time.Minute { + t.Fatal(crt.NotAfter) + } }