feature/acme, ipn/ipnlocal: start moving ACME/cert state into an extension

The ACME serialization mutex (acmeMu) was a package-level global, and
several ACME-related fields lived on LocalBackend even though the
cert code is conditional and not linked into every binary. With
multiple tsnet.Servers in one process (each its own LocalBackend),
a process-wide acmeMu also serialized unrelated backends.

Introduce a new feature/acme extension that owns the per-LocalBackend
ACME/cert state in an ipnlocal.CertState value:

  - acmeMu, renewMu, renewCertAt (previously package globals)
  - pendingACMETLSALPNCerts, pendingCertDomains{,Mu},
    getCertForTest, certRefreshCancel (previously LocalBackend
    fields, only meaningful when ACME was compiled in)

ipnlocal/cert.go now reaches the state through b.certState(), which
is routed by a feature.Hook installed at init by feature/acme. The
CertState type lives in ipnlocal so cert.go can access its fields
directly without a method explosion; the extension in feature/acme
constructs and owns it.

This is a baby step. The end goal is for the entire cert/ACME code
to live in feature/acme, with ipnlocal only retaining whatever thin
hooks the rest of LocalBackend needs to call into it. The current
split (CertState and most of cert.go in ipnlocal, extension wrapper
in feature/acme) is a deliberately temporary middle ground that
keeps this PR small while making the next moves mechanical.

The package is named feature/acme to match the existing HasACME /
ts_omit_acme naming. condregister/maybe_acme.go wires it in for
non-js builds.

Updates #12614
Updates #20248
Updates #20249

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
Change-Id: I520909f24ad11a9622ef33c2290fe36ad44d6f71

cmd/tailscaled/depaware.txt

@@ -290,6 +290,7 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de
         tailscale.com/envknob/featureknob                            from tailscale.com/client/web+
         tailscale.com/feature                                        from tailscale.com/feature/wakeonlan+
         tailscale.com/feature/ace                                    from tailscale.com/feature/condregister
+        tailscale.com/feature/acme                                   from tailscale.com/feature/condregister
         tailscale.com/feature/appconnectors                          from tailscale.com/feature/condregister
         tailscale.com/feature/buildfeatures                          from tailscale.com/wgengine/magicsock+
         tailscale.com/feature/c2n                                    from tailscale.com/feature/condregister

feature/acme/acme.go

@@ -0,0 +1,62 @@
+// Copyright (c) Tailscale Inc & contributors
+// SPDX-License-Identifier: BSD-3-Clause
+
+// Package acme registers the ACME/TLS-cert feature and implements its
+// associated [ipnext.Extension]. The extension owns the per-LocalBackend
+// state previously held in package-level globals and on LocalBackend
+// fields (ACME serialization mutex, in-flight cert tracking, etc.).
+//
+// The cert code that runs against this state still lives in
+// [tailscale.com/ipn/ipnlocal]; this extension simply owns the state
+// and installs a hook so cert.go can find it from a *LocalBackend.
+package acme
+
+import (
+	"tailscale.com/feature"
+	"tailscale.com/ipn/ipnext"
+	"tailscale.com/ipn/ipnlocal"
+	"tailscale.com/types/logger"
+)
+
+// featureName is the name of the feature implemented by this package.
+const featureName = "acme"
+
+func init() {
+	feature.Register(featureName)
+	ipnext.RegisterExtension(featureName, newExtension)
+	ipnlocal.HookCertState.Set(certStateFor)
+}
+
+func newExtension(logf logger.Logf, _ ipnext.SafeBackend) (ipnext.Extension, error) {
+	return &extension{
+		state: new(ipnlocal.CertState),
+		logf:  logger.WithPrefix(logf, featureName+": "),
+	}, nil
+}
+
+// extension is an [ipnext.Extension] that owns the per-LocalBackend
+// ACME/cert state. Most of the cert logic still lives in ipnlocal;
+// this extension exists to give that state a non-global home.
+type extension struct {
+	state *ipnlocal.CertState
+	logf  logger.Logf
+}
+
+// Name implements [ipnext.Extension].
+func (e *extension) Name() string { return featureName }
+
+// Init implements [ipnext.Extension].
+func (e *extension) Init(ipnext.Host) error { return nil }
+
+// Shutdown implements [ipnext.Extension].
+func (e *extension) Shutdown() error { return nil }
+
+// certStateFor returns the [ipnlocal.CertState] owned by the acme
+// extension registered on b, or nil if none.
+func certStateFor(b *ipnlocal.LocalBackend) *ipnlocal.CertState {
+	e, ok := ipnlocal.GetExt[*extension](b)
+	if !ok {
+		return nil
+	}
+	return e.state
+}

feature/condregister/maybe_acme.go

@@ -0,0 +1,8 @@
+// Copyright (c) Tailscale Inc & contributors
+// SPDX-License-Identifier: BSD-3-Clause
+
+//go:build !js && !ts_omit_acme
+
+package condregister
+
+import _ "tailscale.com/feature/acme"

ipn/ipnlocal/cert.go

@@ -41,12 +41,12 @@
 	"tailscale.com/ipn/store"
 	"tailscale.com/ipn/store/mem"
 	"tailscale.com/net/bakedroots"
-	"tailscale.com/syncs"
 	"tailscale.com/tailcfg"
 	"tailscale.com/tempfork/acme"
 	"tailscale.com/tsconst"
 	"tailscale.com/types/logger"
 	"tailscale.com/util/clientmetric"
+	"tailscale.com/util/mak"
 	"tailscale.com/util/set"
 	"tailscale.com/util/slicesx"
 	"tailscale.com/util/testenv"
@@ -59,18 +59,6 @@ func init() {
 	hookCertRefreshLoop.Set(certRefreshLoop)
 }
 
-// Process-wide cache. (A new *Handler is created per connection,
-// effectively per request)
-var (
-	// acmeMu guards all ACME operations, so concurrent requests
-	// for certs don't slam ACME. The first will go through and
-	// populate the on-disk cache and the rest should use that.
-	acmeMu syncs.Mutex
-
-	renewMu     syncs.Mutex // lock order: acmeMu before renewMu
-	renewCertAt = map[string]time.Time{}
-)
-
 var (
 	metricACMEDNS01Start       = clientmetric.NewCounter("cert_acme_dns01_start")
 	metricACMEDNS01Success     = clientmetric.NewCounter("cert_acme_dns01_success")
@@ -114,7 +102,11 @@ func (b *LocalBackend) getACMETLSALPNCert(hi *tls.ClientHelloInfo) (cert *tls.Ce
 	if hi == nil || hi.ServerName == "" || !slices.Contains(hi.SupportedProtos, acme.ALPNProto) {
 		return nil, false
 	}
-	cert, ok = b.pendingACMETLSALPNCerts.Load(hi.ServerName)
+	cs := b.certState()
+	if cs == nil {
+		return nil, false
+	}
+	cert, ok = cs.pendingACMETLSALPNCerts.Load(hi.ServerName)
 	return cert, ok
 }
 
@@ -133,10 +125,16 @@ func (b *LocalBackend) getACMETLSALPNProto(hi *tls.ClientHelloInfo) (proto strin
 
 // storeACMETLSALPNCert publishes cert to Serve TLS handshakes for domain until
 // the returned cleanup function is called.
+//
+// It returns a no-op cleanup if the cert extension is not registered.
 func (b *LocalBackend) storeACMETLSALPNCert(domain string, cert *tls.Certificate) (cleanup func()) {
-	b.pendingACMETLSALPNCerts.Store(domain, cert)
+	cs := b.certState()
+	if cs == nil {
+		return func() {}
+	}
+	cs.pendingACMETLSALPNCerts.Store(domain, cert)
 	return func() {
-		b.pendingACMETLSALPNCerts.Delete(domain)
+		cs.pendingACMETLSALPNCerts.Delete(domain)
 	}
 }
 
@@ -193,8 +191,13 @@ func (b *LocalBackend) GetCertPEM(ctx context.Context, domain string) (*TLSCertK
 // Funnel domains are issued via tls-alpn-01 over the same Funnel TLS path
 // that serves real traffic.
 func (b *LocalBackend) GetCertPEMWithValidity(ctx context.Context, domain string, minValidity time.Duration) (*TLSCertKeyPair, error) {
+	state := b.certState()
+	if state == nil {
+		return nil, errors.New("cert extension not registered")
+	}
+
 	b.mu.Lock()
-	getCertForTest := b.getCertForTest
+	getCertForTest := state.getCertForTest
 	b.mu.Unlock()
 
 	if getCertForTest != nil {
@@ -278,9 +281,13 @@ func (b *LocalBackend) shouldStartDomainRenewal(cs certStore, domain string, now
 		}
 		return cert.NotAfter.Sub(now) < minValidity, nil
 	}
-	renewMu.Lock()
-	defer renewMu.Unlock()
-	if renewAt, ok := renewCertAt[domain]; ok {
+	state := b.certState()
+	if state == nil {
+		return false, errors.New("cert extension not registered")
+	}
+	state.renewMu.Lock()
+	defer state.renewMu.Unlock()
+	if renewAt, ok := state.renewCertAt[domain]; ok {
 		return now.After(renewAt), nil
 	}
 
@@ -294,14 +301,18 @@ func (b *LocalBackend) shouldStartDomainRenewal(cs certStore, domain string, now
 		}
 	}
 
-	renewCertAt[domain] = renewTime
+	mak.Set(&state.renewCertAt, domain, renewTime)
 	return now.After(renewTime), nil
 }
 
 func (b *LocalBackend) domainRenewed(domain string) {
-	renewMu.Lock()
-	defer renewMu.Unlock()
-	delete(renewCertAt, domain)
+	state := b.certState()
+	if state == nil {
+		return
+	}
+	state.renewMu.Lock()
+	defer state.renewMu.Unlock()
+	delete(state.renewCertAt, domain)
 }
 
 func (b *LocalBackend) domainRenewalTimeByExpiry(pair *TLSCertKeyPair) (time.Time, error) {
@@ -461,8 +472,12 @@ func (b *LocalBackend) getCertStore() (certStore, error) {
 // only be used in tests.
 func (b *LocalBackend) ConfigureCertsForTest(getCert func(hostname string) (*TLSCertKeyPair, error)) {
 	testenv.AssertInTest()
+	cs := b.certState()
+	if cs == nil {
+		panic("ConfigureCertsForTest called without cert extension registered")
+	}
 	b.mu.Lock()
-	b.getCertForTest = getCert
+	cs.getCertForTest = getCert
 	b.mu.Unlock()
 }
 
@@ -657,8 +672,12 @@ func getCertPEMCached(cs certStore, domain string, now time.Time) (p *TLSCertKey
 // 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, b *LocalBackend, cs certStore, logf logger.Logf, traceACME func(any), domain string, now time.Time, minValidity time.Duration) (*TLSCertKeyPair, error) {
-	acmeMu.Lock()
-	defer acmeMu.Unlock()
+	state := b.certState()
+	if state == nil {
+		return nil, errors.New("cert extension not registered")
+	}
+	state.acmeMu.Lock()
+	defer state.acmeMu.Unlock()
 
 	// In case this method was triggered multiple times in parallel (when
 	// serving incoming requests), check whether one of the other goroutines
@@ -1239,20 +1258,24 @@ func handleC2NTLSCertStatus(b *LocalBackend, w http.ResponseWriter, r *http.Requ
 // domain and updates the [certPendingWarnable] to reflect the current set
 // of pending domains.
 func (b *LocalBackend) setCertPending(domain string, pending bool) {
-	b.pendingCertDomainsMu.Lock()
-	defer b.pendingCertDomainsMu.Unlock()
-	if pending {
-		b.pendingCertDomains.Make()
-		b.pendingCertDomains.Add(domain)
-	} else {
-		b.pendingCertDomains.Delete(domain)
+	state := b.certState()
+	if state == nil {
+		return
 	}
-	if b.pendingCertDomains.Len() == 0 {
+	state.pendingCertDomainsMu.Lock()
+	defer state.pendingCertDomainsMu.Unlock()
+	if pending {
+		state.pendingCertDomains.Make()
+		state.pendingCertDomains.Add(domain)
+	} else {
+		state.pendingCertDomains.Delete(domain)
+	}
+	if state.pendingCertDomains.Len() == 0 {
 		b.health.SetHealthy(certPendingWarnable)
 		return
 	}
 	b.health.SetUnhealthy(certPendingWarnable, health.Args{
-		health.ArgDomains: joinedPendingCertDomainsLocked(b.pendingCertDomains),
+		health.ArgDomains: joinedPendingCertDomainsLocked(state.pendingCertDomains),
 	})
 }
 

ipn/ipnlocal/cert_state.go

@@ -0,0 +1,84 @@
+// Copyright (c) Tailscale Inc & contributors
+// SPDX-License-Identifier: BSD-3-Clause
+
+package ipnlocal
+
+import (
+	"context"
+	"crypto/tls"
+	"sync"
+	"time"
+
+	"tailscale.com/feature"
+	"tailscale.com/syncs"
+	"tailscale.com/util/set"
+)
+
+// CertState holds the per-[LocalBackend] state owned by the
+// feature/acme extension. The struct lives in this package so that
+// cert.go can access its fields directly without method indirection,
+// while the extension in feature/acme remains the canonical owner.
+//
+// In builds without ACME support (js or ts_omit_acme), no extension
+// constructs a CertState, and [LocalBackend.certState] returns nil.
+//
+// CertState is safe for concurrent use; the individual fields document
+// their own synchronization.
+//
+// TODO(bradfitz): continue moving all this cert code into feature/acme's package.
+// This type being here was a compromise to keep the PR small during the move.
+type CertState struct {
+	// 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
+
+	// renewMu guards renewCertAt.
+	// Lock order: acmeMu before renewMu.
+	renewMu     syncs.Mutex
+	renewCertAt map[string]time.Time // lazily initialized under renewMu
+
+	// pendingACMETLSALPNCerts maps SNI names to short-lived ACME
+	// tls-alpn-01 challenge certificates while an ACME order is
+	// waiting for validation. Entries are deleted by the cleanup
+	// function returned from storeACMETLSALPNCert after the challenge
+	// validation path finishes, whether it succeeds or fails.
+	pendingACMETLSALPNCerts syncs.Map[string, *tls.Certificate] // "foo.bar.com" => challenge cert
+
+	// pendingCertDomains tracks the set of domains for which an ACME
+	// issuance is currently in flight with no usable cached cert. It
+	// backs the tls-cert-pending health Warnable.
+	// Guarded by pendingCertDomainsMu.
+	pendingCertDomainsMu sync.Mutex
+	pendingCertDomains   set.Set[string]
+
+	// getCertForTest is used to retrieve TLS certificates in tests.
+	// See [LocalBackend.ConfigureCertsForTest]. Guarded by the
+	// containing [LocalBackend]'s mutex (b.mu).
+	getCertForTest func(hostname string) (*TLSCertKeyPair, error)
+
+	// certRefreshCancel cancels the background TLS cert refresh loop
+	// that periodically pokes [LocalBackend.GetCertPEM] so renewals
+	// happen on idle nodes. Guarded by the containing [LocalBackend]'s
+	// mutex (b.mu). Non-nil while the loop is running.
+	certRefreshCancel context.CancelFunc
+}
+
+// hookCertState is set by the feature/acme extension at init time
+// to a function that returns the [CertState] for backend b, or nil
+// if the cert extension is not registered (e.g. in builds with
+// ts_omit_acme or js).
+var hookCertState feature.Hook[func(*LocalBackend) *CertState]
+
+// HookCertState exposes [hookCertState] to the feature/acme package
+// for installation. It must be set exactly once at init time.
+var HookCertState = &hookCertState
+
+// certState returns the cert state for b, or nil if the cert
+// extension is not registered.
+func (b *LocalBackend) certState() *CertState {
+	if f, ok := hookCertState.GetOk(); ok {
+		return f(b)
+	}
+	return nil
+}

ipn/ipnlocal/cert_state_test.go

@@ -0,0 +1,36 @@
+// Copyright (c) Tailscale Inc & contributors
+// SPDX-License-Identifier: BSD-3-Clause
+
+//go:build !js && !ts_omit_acme
+
+package ipnlocal
+
+import "sync"
+
+// In tests we can't import feature/acme (it would import this package
+// and form a cycle), so the real cert extension is never registered.
+// Install a default [hookCertState] provider here that lazily creates
+// a [CertState] per [LocalBackend].
+//
+// Tests that want different behavior can use
+// [feature.Hook.SetForTest] to override this hook for the duration
+// of the test.
+func init() {
+	if hookCertState.IsSet() {
+		return
+	}
+	var (
+		mu     sync.Mutex
+		states = map[*LocalBackend]*CertState{}
+	)
+	hookCertState.Set(func(b *LocalBackend) *CertState {
+		mu.Lock()
+		defer mu.Unlock()
+		if s, ok := states[b]; ok {
+			return s
+		}
+		s := new(CertState)
+		states[b] = s
+		return s
+	})
+}

ipn/ipnlocal/cert_test.go

@@ -586,12 +586,6 @@ func TestCertStoreRoundTrip(t *testing.T) {
 }
 
 func TestShouldStartDomainRenewal(t *testing.T) {
-	reset := func() {
-		renewMu.Lock()
-		defer renewMu.Unlock()
-		clear(renewCertAt)
-	}
-
 	mustMakePair := func(template *x509.Certificate) *TLSCertKeyPair {
 		priv, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
 		if err != nil {
@@ -653,8 +647,6 @@ func TestShouldStartDomainRenewal(t *testing.T) {
 	b := new(LocalBackend)
 	for _, tt := range testCases {
 		t.Run(tt.name, func(t *testing.T) {
-			reset()
-
 			ret, err := b.domainRenewalTimeByExpiry(mustMakePair(&x509.Certificate{
 				SerialNumber: big.NewInt(2019),
 				Subject:      subject,

ipn/ipnlocal/local.go

@@ -9,7 +9,6 @@
 	"bufio"
 	"cmp"
 	"context"
-	"crypto/tls"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -272,13 +271,6 @@ type LocalBackend struct {
 	// is never called.
 	getTCPHandlerForFunnelFlow func(srcAddr netip.AddrPort, dstPort uint16) (handler func(net.Conn))
 
-	// pendingACMETLSALPNCerts maps SNI names to short-lived ACME tls-alpn-01
-	// challenge certificates while an ACME order is waiting for validation.
-	// Entries are deleted by the cleanup function returned from
-	// storeACMETLSALPNCert after the challenge validation path finishes,
-	// whether it succeeds or fails.
-	pendingACMETLSALPNCerts syncs.Map[string, *tls.Certificate] // "foo.bar.com" => challenge cert
-
 	containsViaIPFuncAtomic                 syncs.AtomicValue[func(netip.Addr) bool]     // TODO(nickkhyl): move to nodeBackend
 	shouldInterceptTCPPortAtomic            syncs.AtomicValue[func(uint16) bool]         // TODO(nickkhyl): move to nodeBackend
 	shouldInterceptVIPServicesTCPPortAtomic syncs.AtomicValue[func(netip.AddrPort) bool] // TODO(nickkhyl): move to nodeBackend
@@ -454,18 +446,6 @@ type LocalBackend struct {
 	// (sending false).
 	needsCaptiveDetection chan bool
 
-	// certRefreshCancel cancels the background TLS cert refresh loop that
-	// periodically pokes [LocalBackend.GetCertPEM] so renewals happen on
-	// idle nodes. It is protected by mu and is non-nil while the loop is
-	// running.
-	certRefreshCancel context.CancelFunc
-
-	// pendingCertDomains tracks the set of domains for which an ACME
-	// issuance is currently in flight with no usable cached cert. It backs
-	// the tls-cert-pending health Warnable. Guarded by pendingCertDomainsMu.
-	pendingCertDomainsMu sync.Mutex
-	pendingCertDomains   set.Set[string]
-
 	// overrideAlwaysOn is whether [pkey.AlwaysOn] is overridden by the user
 	// and should have no impact on the WantRunning state until the policy changes,
 	// or the user re-connects manually, switches to a different profile, etc.
@@ -495,10 +475,6 @@ type LocalBackend struct {
 	// bind the node identity to this device.
 	hardwareAttested atomic.Bool
 
-	// getCertForTest is used to retrieve TLS certificates in tests.
-	// See [LocalBackend.ConfigureCertsForTest].
-	getCertForTest func(hostname string) (*TLSCertKeyPair, error)
-
 	// existsPendingAuthReconfig tracks if a goroutine is waiting to
 	// acquire [LocalBackend]'s mutex inside of [LocalBackend.AuthReconfig].
 	// It is used to prevent goroutines from piling up to do the same
@@ -1361,9 +1337,11 @@ func (b *LocalBackend) Shutdown() {
 		b.captiveCancel()
 	}
 
-	if buildfeatures.HasACME && b.certRefreshCancel != nil {
-		b.certRefreshCancel()
-		b.certRefreshCancel = nil
+	if buildfeatures.HasACME {
+		if state := b.certState(); state != nil && state.certRefreshCancel != nil {
+			state.certRefreshCancel()
+			state.certRefreshCancel = nil
+		}
 	}
 
 	b.stopReconnectTimerLocked()
@@ -7571,18 +7549,22 @@ func (b *LocalBackend) updateCertRefreshLoopLocked() {
 	if !buildfeatures.HasACME {
 		return
 	}
+	state := b.certState()
+	if state == nil {
+		return
+	}
 	shouldRun := hookCertRefreshLoop.IsSet() &&
 		b.state == ipn.Running &&
 		serveConfigUsesACMECerts(b.serveConfig)
 
 	switch {
-	case shouldRun && b.certRefreshCancel == nil:
+	case shouldRun && state.certRefreshCancel == nil:
 		ctx, cancel := context.WithCancel(b.ctx)
-		b.certRefreshCancel = cancel
+		state.certRefreshCancel = cancel
 		b.goTracker.Go(func() { hookCertRefreshLoop.Get()(b, ctx) })
-	case !shouldRun && b.certRefreshCancel != nil:
-		b.certRefreshCancel()
-		b.certRefreshCancel = nil
+	case !shouldRun && state.certRefreshCancel != nil:
+		state.certRefreshCancel()
+		state.certRefreshCancel = nil
 	}
 }