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 <agl@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

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) {}
 )

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)
 		}

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)

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)
 	}