diff --git a/cmd/tailscaled/depaware.txt b/cmd/tailscaled/depaware.txt index 20ade2f18..b54571abb 100644 --- a/cmd/tailscaled/depaware.txt +++ b/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 diff --git a/feature/acme/acme.go b/feature/acme/acme.go new file mode 100644 index 000000000..4f9cfd8fd --- /dev/null +++ b/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 +} diff --git a/feature/condregister/maybe_acme.go b/feature/condregister/maybe_acme.go new file mode 100644 index 000000000..5701701fc --- /dev/null +++ b/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" diff --git a/ipn/ipnlocal/cert.go b/ipn/ipnlocal/cert.go index 5e6034654..87aa016a9 100644 --- a/ipn/ipnlocal/cert.go +++ b/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), }) } diff --git a/ipn/ipnlocal/cert_state.go b/ipn/ipnlocal/cert_state.go new file mode 100644 index 000000000..fe5e2fbb4 --- /dev/null +++ b/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 +} diff --git a/ipn/ipnlocal/cert_state_test.go b/ipn/ipnlocal/cert_state_test.go new file mode 100644 index 000000000..32e22cdeb --- /dev/null +++ b/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 + }) +} diff --git a/ipn/ipnlocal/cert_test.go b/ipn/ipnlocal/cert_test.go index 224ba834d..6e35939c5 100644 --- a/ipn/ipnlocal/cert_test.go +++ b/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, diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 2c1215194..24e5e0500 100644 --- a/ipn/ipnlocal/local.go +++ b/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 } }