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