acme/autocert: fix data race in Manager.createCert

Previously Manager.createCert() read state.locked without holding any
lock, while the owner goroutine wrote state.locked = false under the
state's write lock. Concurrent goroutines for the same domain raced on
that field.

We fix the issue by removing the certState.locked field and instead
having Manager.certState() return an additional owner bool computed
under the m.stateMu lock. This allows the Manager.createCert() caller to
know whether it must block on the read lock and return the state handled
by another goroutine, or instead do the work itself and release the
write lock.

A regression unit test is included that fails under -race without the
fix, and passes with the fix in-place.

Fixes golang/go#80119

Change-Id: I0f4c5c98f7e6bcf1ab71a3c0707bb0c7ce73415b
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/793840
Reviewed-by: Roland Shoemaker <roland@golang.org>
LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Carlos Amedee <carlos@golang.org>
Auto-Submit: Daniel McCarney <daniel@binaryparadox.net>

acme/autocert/autocert.go

@@ -564,24 +564,21 @@ func encodeECDSAKey(w io.Writer, key *ecdsa.PrivateKey) error {
 // If the domain is already being verified, it waits for the existing verification to complete.
 // Either way, createCert blocks for the duration of the whole process.
 func (m *Manager) createCert(ctx context.Context, ck certKey) (*tls.Certificate, error) {
-	// TODO: maybe rewrite this whole piece using sync.Once
-	state, err := m.certState(ck)
+	state, owner, err := m.certState(ck)
 	if err != nil {
 		return nil, err
 	}
-	// state may exist if another goroutine is already working on it
-	// in which case just wait for it to finish
-	if !state.locked {
+	// If another goroutine is already working on this state, wait for it
+	// to finish by taking the read lock
+	if !owner {
 		state.RLock()
 		defer state.RUnlock()
 		return state.tlscert()
 	}
 
-	// We are the first; state is locked.
-	// Unblock the readers when domain ownership is verified
-	// and we got the cert or the process failed.
+	// We are the first to work on this certKey, so state is write-locked.
+	// Unblock the readers when our work is complete.
 	defer state.Unlock()
-	state.locked = false
 
 	der, leaf, err := m.authorizedCert(ctx, state.key, ck)
 	if err != nil {
@@ -611,10 +608,15 @@ func (m *Manager) createCert(ctx context.Context, ck certKey) (*tls.Certificate,
 	return state.tlscert()
 }
 
-// certState returns a new or existing certState.
-// If a new certState is returned, state.exist is false and the state is locked.
+// certState returns a new or existing certState along with a boolean
+// indicating whether the caller is the owner of the state.
+//
+// The owner of the state is responsible for performing the ACME work and
+// must unlock the state's write lock when done. Non-owner callers should
+// wait on the state's read lock for the owner to finish.
+//
 // The returned error is non-nil only in the case where a new state could not be created.
-func (m *Manager) certState(ck certKey) (*certState, error) {
+func (m *Manager) certState(ck certKey) (*certState, bool, error) {
 	m.stateMu.Lock()
 	defer m.stateMu.Unlock()
 	if m.state == nil {
@@ -622,7 +624,7 @@ func (m *Manager) certState(ck certKey) (*certState, error) {
 	}
 	// existing state
 	if state, ok := m.state[ck]; ok {
-		return state, nil
+		return state, false, nil
 	}
 
 	// new locked state
@@ -636,16 +638,13 @@ func (m *Manager) certState(ck certKey) (*certState, error) {
 		key, err = ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
 	}
 	if err != nil {
-		return nil, err
+		return nil, false, err
 	}
 
-	state := &certState{
-		key:    key,
-		locked: true,
-	}
+	state := &certState{key: key}
 	state.Lock() // will be unlocked by m.certState caller
 	m.state[ck] = state
-	return state, nil
+	return state, true, nil
 }
 
 // authorizedCert starts the domain ownership verification process and requests a new cert upon success.
@@ -1035,10 +1034,9 @@ func (m *Manager) now() time.Time {
 // certState is ready when its mutex is unlocked for reading.
 type certState struct {
 	sync.RWMutex
-	locked bool              // locked for read/write
-	key    crypto.Signer     // private key for cert
-	cert   [][]byte          // DER encoding
-	leaf   *x509.Certificate // parsed cert[0]; always non-nil if cert != nil
+	key  crypto.Signer     // private key for cert
+	cert [][]byte          // DER encoding
+	leaf *x509.Certificate // parsed cert[0]; always non-nil if cert != nil
 }
 
 // tlscert creates a tls.Certificate from s.key and s.cert.

acme/autocert/autocert_test.go

@@ -513,6 +513,33 @@ func TestGetCertificate_failedAttempt(t *testing.T) {
 	}
 }
 
+// TestGetCertificate_concurrent guards against a data race on certState
+// fields between the goroutine performing the ACME work for a domain and
+// the other goroutines waiting on the result. See golang/go#80119.
+func TestGetCertificate_concurrent(t *testing.T) {
+	// An unreachable directory URL makes the owner goroutine fail
+	// quickly inside createCert, so its writes overlap with the
+	// non-owner goroutines reaching the same code path.
+	m := &Manager{
+		Prompt: AcceptTOS,
+		Client: &acme.Client{DirectoryURL: "http://127.0.0.1:1/"},
+	}
+	defer m.stopRenew()
+
+	start := make(chan struct{})
+	var wg sync.WaitGroup
+	for i := 0; i < 64; i++ {
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			<-start
+			m.GetCertificate(clientHelloInfo(exampleDomain, algECDSA))
+		}()
+	}
+	close(start)
+	wg.Wait()
+}
+
 func TestRevokeFailedAuthz(t *testing.T) {
 	ca := acmetest.NewCAServer(t)
 	// Make the authz unfulfillable on the client side, so it will be left