From 69ee776dfb9afb07affcf0477889e9cdbb697a2a Mon Sep 17 00:00:00 2001 From: Tom Meadows Date: Thu, 9 Jul 2026 15:06:17 +0200 Subject: [PATCH] feature/acme: lock ACME per-domain instead of globally (#20303) The extension's acmeMu was a single lock around getCertPEM. Any in-flight ACME flow blocked every other domain. With many domains (ProxyGroup ingress) the queue would back up and per-call timeouts started firing while we were just waiting on the lock -- the cert loop treated that as a failure. Replace with one mutex per domain. Different domains run at the same time. Same domain still queues so the first run fills the cache and the rest read from it. The old global lock also kept ACME account setup safe by accident. Two goroutines could both find no account key, both generate one, both write -- last one wins on disk but each carries on with its own. Add acmeAccountMu around acmeKey and ensureACMEAccount to keep that path single-file. Otherwise two first-time issuances for different domains end up with separate accounts at LE. Updates #20288 Updates tailscale/corp#42164 Signed-off-by: chaosinthecrd --- feature/acme/acme.go | 32 +++++++++-- feature/acme/cert.go | 57 ++++++++++++-------- feature/acme/cert_test.go | 111 +++++++++++++++++++++++++++++++++++++- feature/acme/certstore.go | 11 ++-- 4 files changed, 179 insertions(+), 32 deletions(-) diff --git a/feature/acme/acme.go b/feature/acme/acme.go index a1f1537a7..2034494bd 100644 --- a/feature/acme/acme.go +++ b/feature/acme/acme.go @@ -32,6 +32,7 @@ "tailscale.com/tsconst" "tailscale.com/types/logger" "tailscale.com/util/clientmetric" + "tailscale.com/util/mak" "tailscale.com/util/set" ) @@ -68,13 +69,22 @@ func init() { type extension struct { logf logger.Logf - // acmeMu serializes ACME operations so concurrent requests for - // certs don't slam ACME. The first goroutine through populates the - // on-disk cache and the rest reuse it. - acmeMu syncs.Mutex + // domainsMu guards domainLocks. + domainsMu syncs.Mutex + // domainLocks holds a per-domain mutex serialising ACME operations + // for that domain. Different domains run concurrently; the same + // domain still queues so the first goroutine fills the on-disk + // cache and the rest reuse it. Entries are never removed; a node + // only ever has a handful of domains so the map stays small. + domainLocks map[string]*syncs.Mutex + + // accountMu serialises ACME account setup (key generation, LE + // registration) so two first-time issuances don't create separate + // accounts. + accountMu syncs.Mutex // renewMu guards renewCertAt. - // Lock order: acmeMu before renewMu. + // Lock order: per-domain lock before renewMu. renewMu syncs.Mutex renewCertAt map[string]time.Time // lazily initialized under renewMu @@ -114,6 +124,18 @@ type extension struct { certRefreshCancel context.CancelFunc } +// lockDomain returns the mutex for domain, creating it on first use. +func (e *extension) lockDomain(domain string) *syncs.Mutex { + e.domainsMu.Lock() + defer e.domainsMu.Unlock() + if m, ok := e.domainLocks[domain]; ok { + return m + } + m := new(syncs.Mutex) + mak.Set(&e.domainLocks, domain, m) + return m +} + // Go runs f in a new goroutine tracked by e.wg. [extension.Shutdown] // waits for all such goroutines to finish. func (e *extension) Go(f func()) { diff --git a/feature/acme/cert.go b/feature/acme/cert.go index 42b1472ff..e271a27d0 100644 --- a/feature/acme/cert.go +++ b/feature/acme/cert.go @@ -279,7 +279,7 @@ func (e *extension) domainRenewalTimeByARI(b *ipnlocal.LocalBackend, cs certStor if len(blocks) < 1 { return time.Time{}, fmt.Errorf("could not parse certificate chain from certStore, got %d PEM block(s)", len(blocks)) } - ac, err := acmeClient(cs) + ac, err := e.acmeClient(cs) if err != nil { return time.Time{}, err } @@ -305,8 +305,9 @@ func (e *extension) domainRenewalTimeByARI(b *ipnlocal.LocalBackend, cs certStor // domain is the resolved cert domain (e.g., "*.node.ts.net" for // wildcards). It can be overridden in tests. var getCertPEM = func(ctx context.Context, e *extension, b *ipnlocal.LocalBackend, cs certStore, logf logger.Logf, traceACME func(any), domain string, now time.Time, minValidity time.Duration) (*ipnlocal.TLSCertKeyPair, error) { - e.acmeMu.Lock() - defer e.acmeMu.Unlock() + dm := e.lockDomain(domain) + dm.Lock() + defer dm.Unlock() // In case this method was triggered multiple times in parallel (when // serving incoming requests), check whether one of the other goroutines @@ -335,7 +336,7 @@ func (e *extension) domainRenewalTimeByARI(b *ipnlocal.LocalBackend, cs certStor defer e.setCertPending(b, domain, false) } - ac, err := acmeClient(cs) + ac, err := e.acmeClient(cs) if err != nil { return nil, err } @@ -344,25 +345,9 @@ func (e *extension) domainRenewalTimeByARI(b *ipnlocal.LocalBackend, cs certStor logf("acme: using Directory URL %q", ac.DirectoryURL) } - a, err := ac.GetReg(ctx, "" /* pre-RFC param */) - switch { - case err == nil: - // Great, already registered. - logf("already had ACME account.") - case err == xacme.ErrNoAccount: - a, err = ac.Register(ctx, new(xacme.Account), xacme.AcceptTOS) - if err == xacme.ErrAccountAlreadyExists { - // Potential race. Double check. - a, err = ac.GetReg(ctx, "" /* pre-RFC param */) - } - if err != nil { - return nil, fmt.Errorf("acme.Register: %w", err) - } - logf("registered ACME account.") - traceACME(a) - default: - return nil, fmt.Errorf("acme.GetReg: %w", err) - + a, err := e.ensureAccount(ctx, ac, logf, traceACME) + if err != nil { + return nil, err } if a.Status != xacme.StatusValid { return nil, fmt.Errorf("unexpected ACME account status %q", a.Status) @@ -410,6 +395,32 @@ func (e *extension) domainRenewalTimeByARI(b *ipnlocal.LocalBackend, cs certStor return e.issueACMECert(ctx, b, ac, issueArgs) } +// ensureAccount returns a valid ACME account, registering one if +// needed. Locked so two first-time issuances don't both create one. +func (e *extension) ensureAccount(ctx context.Context, ac *xacme.Client, logf logger.Logf, traceACME func(any)) (*xacme.Account, error) { + e.accountMu.Lock() + defer e.accountMu.Unlock() + a, err := ac.GetReg(ctx, "" /* pre-RFC param */) + switch { + case err == nil: + logf("already had ACME account.") + return a, nil + case err == xacme.ErrNoAccount: + a, err = ac.Register(ctx, new(xacme.Account), xacme.AcceptTOS) + if err == xacme.ErrAccountAlreadyExists { + a, err = ac.GetReg(ctx, "" /* pre-RFC param */) + } + if err != nil { + return nil, fmt.Errorf("acme.Register: %w", err) + } + logf("registered ACME account.") + traceACME(a) + return a, nil + default: + return nil, fmt.Errorf("acme.GetReg: %w", err) + } +} + type acmeCertIssueArgs struct { cs certStore // certificate and ACME account storage logf logger.Logf // logs ACME progress and failures diff --git a/feature/acme/cert_test.go b/feature/acme/cert_test.go index c28a5e83c..730315b65 100644 --- a/feature/acme/cert_test.go +++ b/feature/acme/cert_test.go @@ -6,7 +6,9 @@ package acme import ( + "bytes" "context" + "crypto" "crypto/ecdsa" "crypto/elliptic" "crypto/rand" @@ -20,6 +22,7 @@ "os" "path/filepath" "slices" + "sync" "testing" "time" @@ -642,7 +645,8 @@ func TestDebugACMEDirectoryURL(t *testing.T) { const setting = "TS_DEBUG_ACME_DIRECTORY_URL" t.Run(tc, func(t *testing.T) { t.Setenv(setting, tc) - ac, err := acmeClient(certStateStore{StateStore: new(mem.Store)}) + e := &extension{} + ac, err := e.acmeClient(certStateStore{StateStore: new(mem.Store)}) if err != nil { t.Fatalf("acmeClient creation err: %v", err) } @@ -970,3 +974,108 @@ func TestRefreshApplicableCerts(t *testing.T) { default: } } + +// TestLockDomain_SameDomainReturnsSameMutex verifies the repeat +// calls for the same domain return the same mutex, and different +// domains get distinct mutexes. +func TestLockDomain_SameDomainReturnsSameMutex(t *testing.T) { + const domA, domB = "a.example.com", "b.example.com" + e := &extension{} + a1 := e.lockDomain(domA) + a2 := e.lockDomain(domA) + b := e.lockDomain(domB) + if a1 != a2 { + t.Errorf("lockDomain(%q) second call = %p, want %p", domA, a2, a1) + } + if a1 == b { + t.Errorf("lockDomain(%q) = lockDomain(%q) = %p, want different", domA, domB, a1) + } +} + +// TestLockDomain_PerDomainConcurrency verifies a lock on one +// domain doesn't block another, and the same domain blocks while held +// and is acquireable again after release. +func TestLockDomain_PerDomainConcurrency(t *testing.T) { + const domA, domB = "a.example.com", "b.example.com" + e := &extension{} + + aLock := e.lockDomain(domA) + aLock.Lock() + + bLock := e.lockDomain(domB) + if !bLock.TryLock() { + t.Errorf("lockDomain(%q).TryLock() = false while %q held, want true", domB, domA) + } else { + bLock.Unlock() + } + + if aLock.TryLock() { + t.Errorf("lockDomain(%q).TryLock() = true while held, want false", domA) + } + + aLock.Unlock() + + if !aLock.TryLock() { + t.Errorf("lockDomain(%q).TryLock() = false after release, want true", domA) + } + aLock.Unlock() +} + +// TestAcmeKey_ConcurrentFirstCall verifies that N goroutines racing on +// an empty store all return the key that ends up persisted. +func TestAcmeKey_ConcurrentFirstCall(t *testing.T) { + e := &extension{} + cs := certStateStore{StateStore: new(mem.Store)} + + const n = 16 + type result struct { + key crypto.Signer + err error + } + results := make(chan result, n) + var start sync.WaitGroup + start.Add(1) + for range n { + go func() { + start.Wait() + k, err := e.acmeKey(cs) + results <- result{k, err} + }() + } + start.Done() + + var keys []crypto.Signer + for range n { + r := <-results + if r.err != nil { + t.Fatalf("acmeKey: %v", r.err) + } + keys = append(keys, r.key) + } + + persisted, err := cs.ACMEKey() + if err != nil { + t.Fatalf("cs.ACMEKey after race: %v", err) + } + block, _ := pem.Decode(persisted) + if block == nil { + t.Fatal("no PEM block in persisted ACME key") + } + want, err := parsePrivateKey(block.Bytes) + if err != nil { + t.Fatalf("parsing persisted key: %v", err) + } + wantPub, err := x509.MarshalPKIXPublicKey(want.Public()) + if err != nil { + t.Fatalf("marshaling persisted pubkey: %v", err) + } + for i, k := range keys { + gotPub, err := x509.MarshalPKIXPublicKey(k.Public()) + if err != nil { + t.Fatalf("marshaling returned key %d: %v", i, err) + } + if !bytes.Equal(gotPub, wantPub) { + t.Errorf("goroutine %d returned a different account key than the persisted one", i) + } + } +} diff --git a/feature/acme/certstore.go b/feature/acme/certstore.go index 834bbc381..43ff87be6 100644 --- a/feature/acme/certstore.go +++ b/feature/acme/certstore.go @@ -329,7 +329,12 @@ func parsePrivateKey(der []byte) (crypto.Signer, error) { return nil, errors.New("acme/autocert: failed to parse private key") } -func acmeKey(cs certStore) (crypto.Signer, error) { +func (e *extension) acmeKey(cs certStore) (crypto.Signer, error) { + // Lock so two callers don't both generate a key and race on the + // write. + e.accountMu.Lock() + defer e.accountMu.Unlock() + if v, err := cs.ACMEKey(); err == nil { priv, _ := pem.Decode(v) if priv == nil || !strings.Contains(priv.Type, "PRIVATE") { @@ -354,8 +359,8 @@ func acmeKey(cs certStore) (crypto.Signer, error) { return privKey, nil } -func acmeClient(cs certStore) (*xacme.Client, error) { - key, err := acmeKey(cs) +func (e *extension) acmeClient(cs certStore) (*xacme.Client, error) { + key, err := e.acmeKey(cs) if err != nil { return nil, fmt.Errorf("acmeKey: %w", err) }