feature/acme/acme.go

@@ -32,6 +32,7 @@ import (
 	"tailscale.com/tsconst"
 	"tailscale.com/types/logger"
 	"tailscale.com/util/clientmetric"
+	"tailscale.com/util/mak"
 	"tailscale.com/util/set"
 )
 
@@ -68,13 +69,22 @@ var errNoExt = errors.New("acme extension not registered on this LocalBackend")
 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()) {

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 @@ var getCertPEM = func(ctx context.Context, e *extension, b *ipnlocal.LocalBacken
 		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 @@ var getCertPEM = func(ctx context.Context, e *extension, b *ipnlocal.LocalBacken
 		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 @@ var getCertPEM = func(ctx context.Context, e *extension, b *ipnlocal.LocalBacken
 	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

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 @@ import (
 	"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)
+		}
+	}
+}

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