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