X Tutup
Skip to content

Commit 20f1bf1

Browse files
aarongableaarongable
authored
Compute validity periods inclusive of notAfter second (letsencrypt#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 letsencrypt#5473
1 parent fd7427a commit 20f1bf1

File tree

8 files changed

+32
-16
lines changed

8 files changed

+32
-16
lines changed

‎ca/ca.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -386,10 +386,10 @@ func (ca *certificateAuthorityImpl) generateSerialNumberAndValidity() (*big.Int,
386386
serialBigInt := big.NewInt(0)
387387
serialBigInt = serialBigInt.SetBytes(serialBytes)
388388

389-
notBefore := ca.clk.Now().Add(-1 * ca.backdate)
389+
notBefore := ca.clk.Now().Add(-ca.backdate)
390390
validity := validity{
391391
NotBefore: notBefore,
392-
NotAfter: notBefore.Add(ca.validityPeriod),
392+
NotAfter: notBefore.Add(ca.validityPeriod - time.Second),
393393
}
394394

395395
return serialBigInt, validity, nil

‎ca/ca_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -434,7 +434,7 @@ func issueCertificateSubTestIssuePrecertificate(t *testing.T, i *TestCertificate
434434

435435
func issueCertificateSubTestValidityUsesCAClock(t *testing.T, i *TestCertificateIssuance) {
436436
test.AssertEquals(t, i.cert.NotBefore, i.ca.clk.Now().Add(-1*i.ca.backdate))
437-
test.AssertEquals(t, i.cert.NotAfter, i.cert.NotBefore.Add(i.ca.validityPeriod))
437+
test.AssertEquals(t, i.cert.NotAfter.Add(time.Second).Sub(i.cert.NotBefore), i.ca.validityPeriod)
438438
}
439439

440440
// Test issuing when multiple issuers are present.

‎issuance/issuance.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -314,7 +314,9 @@ func (p *Profile) requestValid(clk clock.Clock, req *IssuanceRequest) error {
314314
return errors.New("common name cannot be included")
315315
}
316316

317-
validity := req.NotAfter.Sub(req.NotBefore)
317+
// The validity period is calculated inclusive of the whole second represented
318+
// by the notAfter timestamp.
319+
validity := req.NotAfter.Add(time.Second).Sub(req.NotBefore)
318320
if validity <= 0 {
319321
return errors.New("NotAfter must be after NotBefore")
320322
}

‎issuance/issuance_test.go‎

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -267,10 +267,23 @@ func TestRequestValid(t *testing.T) {
267267
request: &IssuanceRequest{
268268
PublicKey: &ecdsa.PublicKey{},
269269
NotBefore: fc.Now(),
270-
NotAfter: fc.Now().Add(time.Hour),
270+
NotAfter: fc.Now().Add(time.Hour - time.Second),
271271
},
272272
expectedError: "validity period is more than the maximum allowed period (1h0m0s>1m0s)",
273273
},
274+
{
275+
name: "validity larger than max due to inclusivity",
276+
profile: &Profile{
277+
useForECDSALeaves: true,
278+
maxValidity: time.Hour,
279+
},
280+
request: &IssuanceRequest{
281+
PublicKey: &ecdsa.PublicKey{},
282+
NotBefore: fc.Now(),
283+
NotAfter: fc.Now().Add(time.Hour),
284+
},
285+
expectedError: "validity period is more than the maximum allowed period (1h0m1s>1h0m0s)",
286+
},
274287
{
275288
name: "validity backdated more than max",
276289
profile: &Profile{
@@ -536,7 +549,7 @@ func TestIssue(t *testing.T) {
536549
CommonName: "example.com",
537550
DNSNames: []string{"example.com"},
538551
NotBefore: fc.Now(),
539-
NotAfter: fc.Now().Add(time.Hour),
552+
NotAfter: fc.Now().Add(time.Hour - time.Second),
540553
})
541554
test.AssertNotError(t, err, "Issue failed")
542555
cert, err := x509.ParseCertificate(certBytes)
@@ -569,7 +582,7 @@ func TestIssueRSA(t *testing.T) {
569582
Serial: []byte{1, 2, 3, 4, 5, 6, 7, 8},
570583
DNSNames: []string{"example.com"},
571584
NotBefore: fc.Now(),
572-
NotAfter: fc.Now().Add(time.Hour),
585+
NotAfter: fc.Now().Add(time.Hour - time.Second),
573586
})
574587
test.AssertNotError(t, err, "Issue failed")
575588
cert, err := x509.ParseCertificate(certBytes)
@@ -599,7 +612,7 @@ func TestIssueCTPoison(t *testing.T) {
599612
DNSNames: []string{"example.com"},
600613
IncludeCTPoison: true,
601614
NotBefore: fc.Now(),
602-
NotAfter: fc.Now().Add(time.Hour),
615+
NotAfter: fc.Now().Add(time.Hour - time.Second),
603616
})
604617
test.AssertNotError(t, err, "Issue failed")
605618
cert, err := x509.ParseCertificate(certBytes)
@@ -631,7 +644,7 @@ func TestIssueSCTList(t *testing.T) {
631644
{},
632645
},
633646
NotBefore: fc.Now(),
634-
NotAfter: fc.Now().Add(time.Hour),
647+
NotAfter: fc.Now().Add(time.Hour - time.Second),
635648
})
636649
test.AssertNotError(t, err, "Issue failed")
637650
cert, err := x509.ParseCertificate(certBytes)
@@ -664,7 +677,7 @@ func TestIssueMustStaple(t *testing.T) {
664677
DNSNames: []string{"example.com"},
665678
IncludeMustStaple: true,
666679
NotBefore: fc.Now(),
667-
NotAfter: fc.Now().Add(time.Hour),
680+
NotAfter: fc.Now().Add(time.Hour - time.Second),
668681
})
669682
test.AssertNotError(t, err, "Issue failed")
670683
cert, err := x509.ParseCertificate(certBytes)
@@ -690,7 +703,7 @@ func TestIssueBadLint(t *testing.T) {
690703
Serial: []byte{1, 2, 3, 4, 5, 6, 7, 8},
691704
DNSNames: []string{"example.com"},
692705
NotBefore: fc.Now(),
693-
NotAfter: fc.Now().Add(time.Hour),
706+
NotAfter: fc.Now().Add(time.Hour - time.Second),
694707
})
695708
test.AssertError(t, err, "Issue didn't fail")
696709
test.AssertEquals(t, err.Error(), "tbsCertificate linting failed: failed lints: w_ct_sct_policy_count_unsatisfied")

‎test/config-next/ca-a.json‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@
4848
]
4949
}
5050
],
51-
"maxValidityPeriod": "7775999s",
51+
"maxValidityPeriod": "7776000s",
5252
"maxValidityBackdate": "1h5m"
5353
},
5454
"issuers": [
@@ -79,7 +79,7 @@
7979
],
8080
"ignoredLints": ["n_subject_common_name_included"]
8181
},
82-
"expiry": "7775999s",
82+
"expiry": "7776000s",
8383
"backdate": "1h",
8484
"serialPrefix": 255,
8585
"maxNames": 100,

‎test/config-next/ca-b.json‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@
4848
]
4949
}
5050
],
51-
"maxValidityPeriod": "7775999s",
51+
"maxValidityPeriod": "7776000s",
5252
"maxValidityBackdate": "1h5m"
5353
},
5454
"issuers": [
@@ -79,7 +79,7 @@
7979
],
8080
"ignoredLints": ["n_subject_common_name_included"]
8181
},
82-
"expiry": "7775999s",
82+
"expiry": "7776000s",
8383
"backdate": "1h",
8484
"serialPrefix": 255,
8585
"maxNames": 100,

‎test/config-next/cert-checker.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
"maxOpenConns": 10
66
},
77
"hostnamePolicyFile": "test/hostname-policy.yaml",
8-
"acceptableValidityPeriods": [7775999, 7776000],
8+
"acceptableValidityPeriods": [7776000],
99
"ignoredLints": [
1010
"n_subject_common_name_included"
1111
]

‎test/config/cert-checker.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
"maxOpenConns": 10
66
},
77
"hostnamePolicyFile": "test/hostname-policy.yaml",
8+
"acceptableValidityPeriods": [7775999, 7776000],
89
"ignoredLints": [
910
"n_subject_common_name_included"
1011
]

0 commit comments

Comments
 (0)
X Tutup