From 20f1bf1d0da43622e8229eba00a5dec0cb38f686 Mon Sep 17 00:00:00 2001 From: Aaron Gable Date: Thu, 24 Jun 2021 13:17:29 -0700 Subject: [PATCH] Compute validity periods inclusive of notAfter second (#5494) In the CA, compute the notAfter timestamp such that the cert is actually valid for the intended duration, not for one second longer. In the Issuance library, compute the validity period by including the full length of the final second indicated by the notAfter date when determining if the certificate request matches our profile. Update tests and config files to match. Fixes #5473 --- ca/ca.go | 4 ++-- ca/ca_test.go | 2 +- issuance/issuance.go | 4 +++- issuance/issuance_test.go | 27 ++++++++++++++++++++------- test/config-next/ca-a.json | 4 ++-- test/config-next/ca-b.json | 4 ++-- test/config-next/cert-checker.json | 2 +- test/config/cert-checker.json | 1 + 8 files changed, 32 insertions(+), 16 deletions(-) diff --git a/ca/ca.go b/ca/ca.go index 8110a8620..c5bb318e6 100644 --- a/ca/ca.go +++ b/ca/ca.go @@ -386,10 +386,10 @@ func (ca *certificateAuthorityImpl) generateSerialNumberAndValidity() (*big.Int, serialBigInt := big.NewInt(0) serialBigInt = serialBigInt.SetBytes(serialBytes) - notBefore := ca.clk.Now().Add(-1 * ca.backdate) + notBefore := ca.clk.Now().Add(-ca.backdate) validity := validity{ NotBefore: notBefore, - NotAfter: notBefore.Add(ca.validityPeriod), + NotAfter: notBefore.Add(ca.validityPeriod - time.Second), } return serialBigInt, validity, nil diff --git a/ca/ca_test.go b/ca/ca_test.go index 5ae99ad9b..34c6d7e87 100644 --- a/ca/ca_test.go +++ b/ca/ca_test.go @@ -434,7 +434,7 @@ func issueCertificateSubTestIssuePrecertificate(t *testing.T, i *TestCertificate func issueCertificateSubTestValidityUsesCAClock(t *testing.T, i *TestCertificateIssuance) { test.AssertEquals(t, i.cert.NotBefore, i.ca.clk.Now().Add(-1*i.ca.backdate)) - test.AssertEquals(t, i.cert.NotAfter, i.cert.NotBefore.Add(i.ca.validityPeriod)) + test.AssertEquals(t, i.cert.NotAfter.Add(time.Second).Sub(i.cert.NotBefore), i.ca.validityPeriod) } // Test issuing when multiple issuers are present. diff --git a/issuance/issuance.go b/issuance/issuance.go index 8be0f366b..2c78fd479 100644 --- a/issuance/issuance.go +++ b/issuance/issuance.go @@ -314,7 +314,9 @@ func (p *Profile) requestValid(clk clock.Clock, req *IssuanceRequest) error { return errors.New("common name cannot be included") } - validity := req.NotAfter.Sub(req.NotBefore) + // The validity period is calculated inclusive of the whole second represented + // by the notAfter timestamp. + validity := req.NotAfter.Add(time.Second).Sub(req.NotBefore) if validity <= 0 { return errors.New("NotAfter must be after NotBefore") } diff --git a/issuance/issuance_test.go b/issuance/issuance_test.go index 752059514..961ba1cd9 100644 --- a/issuance/issuance_test.go +++ b/issuance/issuance_test.go @@ -267,10 +267,23 @@ func TestRequestValid(t *testing.T) { request: &IssuanceRequest{ PublicKey: &ecdsa.PublicKey{}, NotBefore: fc.Now(), - NotAfter: fc.Now().Add(time.Hour), + NotAfter: fc.Now().Add(time.Hour - time.Second), }, expectedError: "validity period is more than the maximum allowed period (1h0m0s>1m0s)", }, + { + name: "validity larger than max due to inclusivity", + profile: &Profile{ + useForECDSALeaves: true, + maxValidity: time.Hour, + }, + request: &IssuanceRequest{ + PublicKey: &ecdsa.PublicKey{}, + NotBefore: fc.Now(), + NotAfter: fc.Now().Add(time.Hour), + }, + expectedError: "validity period is more than the maximum allowed period (1h0m1s>1h0m0s)", + }, { name: "validity backdated more than max", profile: &Profile{ @@ -536,7 +549,7 @@ func TestIssue(t *testing.T) { CommonName: "example.com", DNSNames: []string{"example.com"}, NotBefore: fc.Now(), - NotAfter: fc.Now().Add(time.Hour), + NotAfter: fc.Now().Add(time.Hour - time.Second), }) test.AssertNotError(t, err, "Issue failed") cert, err := x509.ParseCertificate(certBytes) @@ -569,7 +582,7 @@ func TestIssueRSA(t *testing.T) { Serial: []byte{1, 2, 3, 4, 5, 6, 7, 8}, DNSNames: []string{"example.com"}, NotBefore: fc.Now(), - NotAfter: fc.Now().Add(time.Hour), + NotAfter: fc.Now().Add(time.Hour - time.Second), }) test.AssertNotError(t, err, "Issue failed") cert, err := x509.ParseCertificate(certBytes) @@ -599,7 +612,7 @@ func TestIssueCTPoison(t *testing.T) { DNSNames: []string{"example.com"}, IncludeCTPoison: true, NotBefore: fc.Now(), - NotAfter: fc.Now().Add(time.Hour), + NotAfter: fc.Now().Add(time.Hour - time.Second), }) test.AssertNotError(t, err, "Issue failed") cert, err := x509.ParseCertificate(certBytes) @@ -631,7 +644,7 @@ func TestIssueSCTList(t *testing.T) { {}, }, NotBefore: fc.Now(), - NotAfter: fc.Now().Add(time.Hour), + NotAfter: fc.Now().Add(time.Hour - time.Second), }) test.AssertNotError(t, err, "Issue failed") cert, err := x509.ParseCertificate(certBytes) @@ -664,7 +677,7 @@ func TestIssueMustStaple(t *testing.T) { DNSNames: []string{"example.com"}, IncludeMustStaple: true, NotBefore: fc.Now(), - NotAfter: fc.Now().Add(time.Hour), + NotAfter: fc.Now().Add(time.Hour - time.Second), }) test.AssertNotError(t, err, "Issue failed") cert, err := x509.ParseCertificate(certBytes) @@ -690,7 +703,7 @@ func TestIssueBadLint(t *testing.T) { Serial: []byte{1, 2, 3, 4, 5, 6, 7, 8}, DNSNames: []string{"example.com"}, NotBefore: fc.Now(), - NotAfter: fc.Now().Add(time.Hour), + NotAfter: fc.Now().Add(time.Hour - time.Second), }) test.AssertError(t, err, "Issue didn't fail") test.AssertEquals(t, err.Error(), "tbsCertificate linting failed: failed lints: w_ct_sct_policy_count_unsatisfied") diff --git a/test/config-next/ca-a.json b/test/config-next/ca-a.json index 35855d4de..04b4f6565 100644 --- a/test/config-next/ca-a.json +++ b/test/config-next/ca-a.json @@ -48,7 +48,7 @@ ] } ], - "maxValidityPeriod": "7775999s", + "maxValidityPeriod": "7776000s", "maxValidityBackdate": "1h5m" }, "issuers": [ @@ -79,7 +79,7 @@ ], "ignoredLints": ["n_subject_common_name_included"] }, - "expiry": "7775999s", + "expiry": "7776000s", "backdate": "1h", "serialPrefix": 255, "maxNames": 100, diff --git a/test/config-next/ca-b.json b/test/config-next/ca-b.json index b9bc93fca..ae81a75f9 100644 --- a/test/config-next/ca-b.json +++ b/test/config-next/ca-b.json @@ -48,7 +48,7 @@ ] } ], - "maxValidityPeriod": "7775999s", + "maxValidityPeriod": "7776000s", "maxValidityBackdate": "1h5m" }, "issuers": [ @@ -79,7 +79,7 @@ ], "ignoredLints": ["n_subject_common_name_included"] }, - "expiry": "7775999s", + "expiry": "7776000s", "backdate": "1h", "serialPrefix": 255, "maxNames": 100, diff --git a/test/config-next/cert-checker.json b/test/config-next/cert-checker.json index 338cad328..f5335861e 100644 --- a/test/config-next/cert-checker.json +++ b/test/config-next/cert-checker.json @@ -5,7 +5,7 @@ "maxOpenConns": 10 }, "hostnamePolicyFile": "test/hostname-policy.yaml", - "acceptableValidityPeriods": [7775999, 7776000], + "acceptableValidityPeriods": [7776000], "ignoredLints": [ "n_subject_common_name_included" ] diff --git a/test/config/cert-checker.json b/test/config/cert-checker.json index 4c9f6d9b7..338cad328 100644 --- a/test/config/cert-checker.json +++ b/test/config/cert-checker.json @@ -5,6 +5,7 @@ "maxOpenConns": 10 }, "hostnamePolicyFile": "test/hostname-policy.yaml", + "acceptableValidityPeriods": [7775999, 7776000], "ignoredLints": [ "n_subject_common_name_included" ]