From 5b7f84159940519e89df4d95465538c1797cee8b Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Wed, 24 Jun 2026 11:30:04 -0400 Subject: [PATCH] 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 LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com Reviewed-by: Carlos Amedee Auto-Submit: Daniel McCarney --- acme/autocert/autocert.go | 44 ++++++++++++++++------------------ acme/autocert/autocert_test.go | 27 +++++++++++++++++++++ 2 files changed, 48 insertions(+), 23 deletions(-) diff --git a/acme/autocert/autocert.go b/acme/autocert/autocert.go index 69461e3..bee6de9 100644 --- a/acme/autocert/autocert.go +++ b/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. diff --git a/acme/autocert/autocert_test.go b/acme/autocert/autocert_test.go index d9f19c2..51273de 100644 --- a/acme/autocert/autocert_test.go +++ b/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