From ff745d0763f824127273eee405c465dfc3254caf Mon Sep 17 00:00:00 2001 From: Adam Langley Date: Wed, 8 Aug 2018 12:05:25 -0700 Subject: [PATCH] acme/autocert: fix race in test. The timeNow package variable doesn't work well here: since the renewal functionality spawns goroutines that invoke timeNow, once a test has caused such goroutines to exist, another test can't fiddle with it without the race detector triggering. Instead, have a private member of Manager that the tests can set if they need. Change-Id: Iaf1a68d8efb84c9c5e2804aeb9cc6b2d3f3fef43 Reviewed-on: https://go-review.googlesource.com/128655 Run-TryBot: Adam Langley TryBot-Result: Gobot Gobot Reviewed-by: Brad Fitzpatrick --- acme/autocert/autocert.go | 23 +++++++++++++++-------- acme/autocert/autocert_test.go | 2 +- acme/autocert/renewal.go | 2 +- acme/autocert/renewal_test.go | 12 ++++++------ 4 files changed, 23 insertions(+), 16 deletions(-) diff --git a/acme/autocert/autocert.go b/acme/autocert/autocert.go index 23efae5..4c2fc07 100644 --- a/acme/autocert/autocert.go +++ b/acme/autocert/autocert.go @@ -44,7 +44,7 @@ var createCertRetryAfter = time.Minute var pseudoRand *lockedMathRand func init() { - src := mathrand.NewSource(timeNow().UnixNano()) + src := mathrand.NewSource(time.Now().UnixNano()) pseudoRand = &lockedMathRand{rnd: mathrand.New(src)} } @@ -183,6 +183,9 @@ type Manager struct { // for tls-alpn. // The entries are stored for the duration of the authorization flow. certTokens map[string]*tls.Certificate + // nowFunc, if not nil, returns the current time. This may be set for + // testing purposes. + nowFunc func() time.Time } // certKey is the key by which certificates are tracked in state, renewal and cache. @@ -480,7 +483,7 @@ func (m *Manager) cacheGet(ctx context.Context, ck certKey) (*tls.Certificate, e } // verify and create TLS cert - leaf, err := validCert(ck, pubDER, privKey) + leaf, err := validCert(ck, pubDER, privKey, m.now()) if err != nil { return nil, ErrCacheMiss } @@ -575,7 +578,7 @@ func (m *Manager) createCert(ctx context.Context, ck certKey) (*tls.Certificate, if !ok { return } - if _, err := validCert(ck, s.cert, s.key); err == nil { + if _, err := validCert(ck, s.cert, s.key, m.now()); err == nil { return } delete(m.state, ck) @@ -644,7 +647,7 @@ func (m *Manager) authorizedCert(ctx context.Context, key crypto.Signer, ck cert if err != nil { return nil, nil, err } - leaf, err = validCert(ck, der, key) + leaf, err = validCert(ck, der, key, m.now()) if err != nil { return nil, nil, err } @@ -988,6 +991,13 @@ func (m *Manager) renewBefore() time.Duration { return 720 * time.Hour // 30 days } +func (m *Manager) now() time.Time { + if m.nowFunc != nil { + return m.nowFunc() + } + return time.Now() +} + // certState is ready when its mutex is unlocked for reading. type certState struct { sync.RWMutex @@ -1054,7 +1064,7 @@ func parsePrivateKey(der []byte) (crypto.Signer, error) { // are valid. It doesn't do any revocation checking. // // The returned value is the verified leaf cert. -func validCert(ck certKey, der [][]byte, key crypto.Signer) (leaf *x509.Certificate, err error) { +func validCert(ck certKey, der [][]byte, key crypto.Signer, now time.Time) (leaf *x509.Certificate, err error) { // parse public part(s) var n int for _, b := range der { @@ -1071,7 +1081,6 @@ func validCert(ck certKey, der [][]byte, key crypto.Signer) (leaf *x509.Certific } // verify the leaf is not expired and matches the domain name leaf = x509Cert[0] - now := timeNow() if now.Before(leaf.NotBefore) { return nil, errors.New("acme/autocert: certificate is not valid yet") } @@ -1125,8 +1134,6 @@ func (r *lockedMathRand) int63n(max int64) int64 { // For easier testing. var ( - timeNow = time.Now - // Called when a state is removed. testDidRemoveState = func(certKey) {} ) diff --git a/acme/autocert/autocert_test.go b/acme/autocert/autocert_test.go index ced1759..95e12e1 100644 --- a/acme/autocert/autocert_test.go +++ b/acme/autocert/autocert_test.go @@ -986,7 +986,7 @@ func TestValidCert(t *testing.T) { {certKey{domain: "example.org"}, key3, [][]byte{cert3}, false}, } for i, test := range tt { - leaf, err := validCert(test.ck, test.cert, test.key) + leaf, err := validCert(test.ck, test.cert, test.key, now) if err != nil && test.ok { t.Errorf("%d: err = %v", i, err) } diff --git a/acme/autocert/renewal.go b/acme/autocert/renewal.go index ef3e44e..665f870 100644 --- a/acme/autocert/renewal.go +++ b/acme/autocert/renewal.go @@ -128,7 +128,7 @@ func (dr *domainRenewal) do(ctx context.Context) (time.Duration, error) { } func (dr *domainRenewal) next(expiry time.Time) time.Duration { - d := expiry.Sub(timeNow()) - dr.m.renewBefore() + d := expiry.Sub(dr.m.now()) - dr.m.renewBefore() // add a bit of randomness to renew deadline n := pseudoRand.int63n(int64(renewJitter)) d -= time.Duration(n) diff --git a/acme/autocert/renewal_test.go b/acme/autocert/renewal_test.go index 634305a..5d1c63f 100644 --- a/acme/autocert/renewal_test.go +++ b/acme/autocert/renewal_test.go @@ -23,10 +23,10 @@ import ( func TestRenewalNext(t *testing.T) { now := time.Now() - timeNow = func() time.Time { return now } - defer func() { timeNow = time.Now }() - - man := &Manager{RenewBefore: 7 * 24 * time.Hour} + man := &Manager{ + RenewBefore: 7 * 24 * time.Hour, + nowFunc: func() time.Time { return now }, + } defer man.stopRenew() tt := []struct { expiry time.Time @@ -207,7 +207,7 @@ func TestRenewFromCacheAlreadyRenewed(t *testing.T) { if err != nil { t.Fatal(err) } - newLeaf, err := validCert(exampleCertKey, [][]byte{newCert}, newKey) + newLeaf, err := validCert(exampleCertKey, [][]byte{newCert}, newKey, now) if err != nil { t.Fatal(err) } @@ -225,7 +225,7 @@ func TestRenewFromCacheAlreadyRenewed(t *testing.T) { if err != nil { t.Fatal(err) } - oldLeaf, err := validCert(exampleCertKey, [][]byte{oldCert}, key) + oldLeaf, err := validCert(exampleCertKey, [][]byte{oldCert}, key, now) if err != nil { t.Fatal(err) }