From a6ab7efa4fd95c17c278eda6a5ff0b34a8be89c6 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Thu, 28 May 2026 20:36:06 +0000 Subject: [PATCH] ipn/ipnlocal, cmd/tailscale/cli: auto-renew TLS certs and warn while pending The Tailscale daemon only refreshed TLS certs as a side effect of inbound TLS handshakes or "tailscale cert" CLI calls. A node that doesn't see inbound traffic during the renewal window silently rolls past expiry. Add a once-per-hour background loop on LocalBackend that enumerates Serve and Funnel HTTPS hostnames (filtered against the netmap's CertDomains so we don't poke ACME for other nodes' service hostnames) and calls the existing GetCertPEM path. The renewal decision (ARI window, then 2/3 expiry fallback) is unchanged; the loop just guarantees it runs. For visibility during initial issuance or restart with a long-expired cached cert, add a "tls-cert-pending" health Warnable that's set while ACME is in flight and no usable cached cert exists. Async renewal of a still-valid cert intentionally doesn't fire it. And then make the CLI "cert" subcommand print out a warning if it's blocking due to a cert fetch in flight, using that health info. Fixes #19911 Fixes #19912 Signed-off-by: Brad Fitzpatrick Change-Id: I144e46c40e957b2e879587decace32a523a6eade --- cmd/tailscale/cli/cert.go | 46 ++++++++++ health/args.go | 4 + ipn/ipnlocal/cert.go | 143 +++++++++++++++++++++++++++++++ ipn/ipnlocal/cert_test.go | 172 ++++++++++++++++++++++++++++++++++++++ ipn/ipnlocal/local.go | 88 +++++++++++++++++++ ipn/ipnlocal/serve.go | 3 + tsconst/health.go | 1 + 7 files changed, 457 insertions(+) diff --git a/cmd/tailscale/cli/cert.go b/cmd/tailscale/cli/cert.go index 6d78a8d8a..bab83901f 100644 --- a/cmd/tailscale/cli/cert.go +++ b/cmd/tailscale/cli/cert.go @@ -23,7 +23,10 @@ import ( "github.com/peterbourgon/ff/v3/ffcli" "software.sslmate.com/src/go-pkcs12" "tailscale.com/atomicfile" + "tailscale.com/feature/buildfeatures" + "tailscale.com/health" "tailscale.com/ipn" + "tailscale.com/tsconst" "tailscale.com/version" ) @@ -112,6 +115,11 @@ func runCert(ctx context.Context, args []string) error { certArgs.certFile = fileBase + ".crt" certArgs.keyFile = fileBase + ".key" } + if buildfeatures.HasHealth { + watchCtx, cancel := context.WithCancel(ctx) + defer cancel() + go watchCertPendingHealth(watchCtx, domain) + } certPEM, keyPEM, err := localClient.CertPairWithValidity(ctx, domain, certArgs.minValidity) if err != nil { return err @@ -167,6 +175,44 @@ func runCert(ctx context.Context, args []string) error { return nil } +// watchCertPendingHealth subscribes to the IPN bus and prints the +// [tsconst.HealthWarnableTLSCertPending] warning to stderr if it appears +// for domain while a cert fetch is in flight. It returns once it has +// printed the warning or ctx is done. +// +// Subscription is delayed 1 second so we don't print anything when the +// daemon returns a cached cert quickly. +func watchCertPendingHealth(ctx context.Context, domain string) { + select { + case <-time.After(1 * time.Second): + case <-ctx.Done(): + return + } + watcher, err := localClient.WatchIPNBus(ctx, ipn.NotifyInitialHealthState|ipn.NotifyNoNetMap) + if err != nil { + return + } + defer watcher.Close() + for { + n, err := watcher.Next() + if err != nil { + return + } + if n.Health == nil { + continue + } + ws, ok := n.Health.Warnings[tsconst.HealthWarnableTLSCertPending] + if !ok { + continue + } + if !strings.Contains(ws.Args[health.ArgDomains], domain) { + continue + } + fmt.Fprintf(os.Stderr, "%s: %s\n", ws.Title, ws.Text) + return + } +} + func writeIfChanged(filename string, contents []byte, mode os.FileMode) (changed bool, err error) { if filename == "-" { Stdout.Write(contents) diff --git a/health/args.go b/health/args.go index e89f7676f..5f606bdf4 100644 --- a/health/args.go +++ b/health/args.go @@ -36,4 +36,8 @@ const ( // ArgServerName provides a Warnable with comma delimited list of the hostname of the servers involved in the unhealthy state. // If no nameservers were available to query, this will be an empty string. ArgDNSServers Arg = "dns-servers" + + // ArgDomains provides a Warnable with a comma-delimited list of domain + // names involved in the unhealthy state. + ArgDomains Arg = "domains" ) diff --git a/ipn/ipnlocal/cert.go b/ipn/ipnlocal/cert.go index ef4cdf728..5e6034654 100644 --- a/ipn/ipnlocal/cert.go +++ b/ipn/ipnlocal/cert.go @@ -35,6 +35,7 @@ import ( "tailscale.com/atomicfile" "tailscale.com/envknob" "tailscale.com/feature/buildfeatures" + "tailscale.com/health" "tailscale.com/hostinfo" "tailscale.com/ipn" "tailscale.com/ipn/store" @@ -43,8 +44,11 @@ import ( "tailscale.com/syncs" "tailscale.com/tailcfg" "tailscale.com/tempfork/acme" + "tailscale.com/tsconst" "tailscale.com/types/logger" "tailscale.com/util/clientmetric" + "tailscale.com/util/set" + "tailscale.com/util/slicesx" "tailscale.com/util/testenv" "tailscale.com/version" "tailscale.com/version/distro" @@ -52,6 +56,7 @@ import ( func init() { RegisterC2N("GET /tls-cert-status", handleC2NTLSCertStatus) + hookCertRefreshLoop.Set(certRefreshLoop) } // Process-wide cache. (A new *Handler is created per connection, @@ -75,6 +80,18 @@ var ( metricACMETLSALPN01Failure = clientmetric.NewCounter("cert_acme_tls_alpn01_failure") ) +// certPendingWarnable fires while ACME is fetching a TLS certificate for +// which no usable cached copy exists (initial issuance or after the cached +// cert has expired). Async renewal of a still-valid cert does not fire it. +var certPendingWarnable = health.Register(&health.Warnable{ + Code: tsconst.HealthWarnableTLSCertPending, + Title: "Fetching TLS certificate", + Severity: health.SeverityLow, + Text: func(args health.Args) string { + return fmt.Sprintf("Fetching TLS certificate via ACME for: %s", args[health.ArgDomains]) + }, +}) + type acmeChallengeType string const ( @@ -660,6 +677,16 @@ var getCertPEM = func(ctx context.Context, b *LocalBackend, cs certStore, logf l return nil, err } + // If we have no usable cached cert (either nothing on disk, or what is + // on disk has expired or otherwise failed verification), surface a + // health warning to the user for the duration of the ACME flow. We + // don't fire the warning when previous is non-nil because then we have + // a working cert and the renewal is happening behind the scenes. + if previous == nil { + b.setCertPending(domain, true) + defer b.setCertPending(domain, false) + } + ac, err := acmeClient(cs) if err != nil { return nil, err @@ -1207,3 +1234,119 @@ func handleC2NTLSCertStatus(b *LocalBackend, w http.ResponseWriter, r *http.Requ writeJSON(w, ret) } + +// setCertPending sets or clears the in-flight ACME issuance state for +// 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) + } + if b.pendingCertDomains.Len() == 0 { + b.health.SetHealthy(certPendingWarnable) + return + } + b.health.SetUnhealthy(certPendingWarnable, health.Args{ + health.ArgDomains: joinedPendingCertDomainsLocked(b.pendingCertDomains), + }) +} + +func joinedPendingCertDomainsLocked(s set.Set[string]) string { + ds := slicesx.MapKeys(s) + slices.Sort(ds) + return strings.Join(ds, ", ") +} + +// certRefreshInterval is how often the background loop iterates the set of +// applicable cert domains and pokes the renewal machinery. The loop is +// only started while there's at least one HTTPS Web entry in the +// ServeConfig, so this cadence doesn't tick on idle/mobile nodes. +const certRefreshInterval = time.Hour + +// certRefreshLoop periodically iterates the domains configured for Serve or +// Funnel HTTPS and calls GetCertPEM on each. The existing renewal machinery +// in getCertPEM decides whether anything needs to happen (ARI check or +// expiry-based fallback); the loop just ensures it runs even on nodes that +// see no inbound TLS traffic. +// +// The first iteration runs immediately so that a node coming back online +// with stale or absent certs starts ACME within seconds rather than +// waiting a full interval. +// +// Set as [hookCertRefreshLoop] in cert.go's init. +func certRefreshLoop(b *LocalBackend, ctx context.Context) { + if envknob.IsCertShareReadOnlyMode() { + b.logf("cert refresh loop: cert-share read-only mode; loop is a no-op") + return + } + + b.refreshApplicableCerts(ctx) + + ticker, tickerCh := b.clock.NewTicker(certRefreshInterval) + defer ticker.Stop() + for { + select { + case <-tickerCh: + b.refreshApplicableCerts(ctx) + case <-ctx.Done(): + return + } + } +} + +// refreshApplicableCerts is one iteration of the cert refresh loop. +// +// It enumerates the Serve/Funnel-configured HTTPS hostnames, keeps those +// that [LocalBackend.resolveCertDomain] accepts (CertDomain, wildcard, or +// BYO Funnel domain), and calls [LocalBackend.GetCertPEM] for each. The +// renewal decision is delegated to the existing logic in [getCertPEM]. +func (b *LocalBackend) refreshApplicableCerts(ctx context.Context) { + sc := b.ServeConfig() + if !sc.Valid() { + return + } + + want := set.Set[string]{} + consider := func(host string) { + if host == "" { + return + } + if _, err := b.resolveCertDomain(host); err != nil { + return + } + want.Add(host) + } + for hp := range sc.Webs() { + host, _, err := net.SplitHostPort(string(hp)) + if err != nil { + continue + } + consider(host) + } + for _, tcp := range sc.TCPs() { + consider(tcp.TerminateTLS()) + } + for _, svc := range sc.Services().All() { + for _, tcp := range svc.TCP().All() { + consider(tcp.TerminateTLS()) + } + } + if want.Len() == 0 { + return + } + + for d := range want { + b.goTracker.Go(func() { + ctx, cancel := context.WithTimeout(ctx, 2*time.Minute) + defer cancel() + if _, err := b.GetCertPEM(ctx, d); err != nil { + b.logf("cert refresh: %s: %v", d, err) + } + }) + } +} diff --git a/ipn/ipnlocal/cert_test.go b/ipn/ipnlocal/cert_test.go index af39ea0bc..224ba834d 100644 --- a/ipn/ipnlocal/cert_test.go +++ b/ipn/ipnlocal/cert_test.go @@ -15,6 +15,7 @@ import ( "crypto/x509/pkix" "embed" "encoding/pem" + "maps" "math/big" "os" "path/filepath" @@ -24,10 +25,12 @@ import ( "github.com/google/go-cmp/cmp" "tailscale.com/envknob" + "tailscale.com/health" "tailscale.com/ipn" "tailscale.com/ipn/store/mem" "tailscale.com/tailcfg" "tailscale.com/tempfork/acme" + "tailscale.com/tsconst" "tailscale.com/tstest" "tailscale.com/types/logger" "tailscale.com/types/netmap" @@ -848,3 +851,172 @@ func TestGetCertPEMWithValidity(t *testing.T) { }) } } + +func TestCertPendingWarnable(t *testing.T) { + b := newTestLocalBackend(t) + + // currentWarning returns the pending warning's rendered text and + // domain-list arg, or "", "" if the warnable is currently healthy. + currentWarning := func() (text, domains string) { + ws, ok := b.health.CurrentState().Warnings[tsconst.HealthWarnableTLSCertPending] + if !ok { + return "", "" + } + return ws.Text, ws.Args[health.ArgDomains] + } + + if b.health.IsUnhealthy(certPendingWarnable) { + t.Fatal("warnable unexpectedly unhealthy before any setCertPending") + } + + b.setCertPending("a.example.com", true) + if !b.health.IsUnhealthy(certPendingWarnable) { + t.Fatal("warnable not unhealthy after first setCertPending") + } + if text, domains := currentWarning(); domains != "a.example.com" || + text != "Fetching TLS certificate via ACME for: a.example.com" { + t.Errorf("after first setCertPending: text=%q domains=%q", text, domains) + } + + b.setCertPending("b.example.com", true) + if !b.health.IsUnhealthy(certPendingWarnable) { + t.Fatal("warnable not unhealthy after second setCertPending") + } + if text, domains := currentWarning(); domains != "a.example.com, b.example.com" || + text != "Fetching TLS certificate via ACME for: a.example.com, b.example.com" { + t.Errorf("after second setCertPending: text=%q domains=%q", text, domains) + } + + b.setCertPending("a.example.com", false) + if !b.health.IsUnhealthy(certPendingWarnable) { + t.Fatal("warnable cleared too early; one domain still pending") + } + if text, domains := currentWarning(); domains != "b.example.com" || + text != "Fetching TLS certificate via ACME for: b.example.com" { + t.Errorf("after clearing a.example.com: text=%q domains=%q", text, domains) + } + + b.setCertPending("b.example.com", false) + if b.health.IsUnhealthy(certPendingWarnable) { + t.Fatal("warnable still unhealthy after clearing all domains") + } + if text, domains := currentWarning(); text != "" || domains != "" { + t.Errorf("after clearing all domains: text=%q domains=%q", text, domains) + } +} + +func TestServeConfigUsesACMECerts(t *testing.T) { + tests := []struct { + name string + sc *ipn.ServeConfig + want bool + }{ + {"nil", nil, false}, + {"empty", &ipn.ServeConfig{}, false}, + { + name: "background_web", + sc: &ipn.ServeConfig{ + Web: map[ipn.HostPort]*ipn.WebServerConfig{ + "node.ts.net:443": {}, + }, + }, + want: true, + }, + { + name: "tcp_forward_no_tls", + sc: &ipn.ServeConfig{ + TCP: map[uint16]*ipn.TCPPortHandler{443: {TCPForward: "127.0.0.1:443"}}, + }, + want: false, + }, + { + name: "tls_terminated_tcp", + sc: &ipn.ServeConfig{ + TCP: map[uint16]*ipn.TCPPortHandler{ + 443: {TCPForward: "127.0.0.1:443", TerminateTLS: "node.ts.net"}, + }, + }, + want: true, + }, + { + name: "service_tls_terminated_tcp", + sc: &ipn.ServeConfig{ + Services: map[tailcfg.ServiceName]*ipn.ServiceConfig{ + "svc:web": { + TCP: map[uint16]*ipn.TCPPortHandler{ + 443: {TCPForward: "127.0.0.1:443", TerminateTLS: "web.svc.ts.net"}, + }, + }, + }, + }, + want: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var v ipn.ServeConfigView + if tt.sc != nil { + v = tt.sc.View() + } + if got := serveConfigUsesACMECerts(v); got != tt.want { + t.Errorf("serveConfigUsesACMECerts = %v, want %v", got, tt.want) + } + }) + } +} + +func TestRefreshApplicableCerts(t *testing.T) { + const ( + certDomain = "node1.example.com" + byoDomain = "byo.example.org" + ) + b := newTestLocalBackend(t) + b.varRoot = t.TempDir() + + b.mu.Lock() + b.currentNode().SetNetMap(&netmap.NetworkMap{ + SelfNode: (&tailcfg.Node{}).View(), + DNS: tailcfg.DNSConfig{ + CertDomains: []string{certDomain}, + }, + }) + b.serveConfig = (&ipn.ServeConfig{ + Web: map[ipn.HostPort]*ipn.WebServerConfig{ + ipn.HostPort(certDomain + ":443"): {}, + ipn.HostPort(byoDomain + ":443"): {}, + // Not in CertDomains and no Funnel entry; must be filtered out. + ipn.HostPort("not-ours.other.tld:443"): {}, + }, + AllowFunnel: map[ipn.HostPort]bool{ + ipn.HostPort(byoDomain + ":443"): true, + }, + }).View() + b.mu.Unlock() + + gotCh := make(chan string, 4) + b.ConfigureCertsForTest(func(host string) (*TLSCertKeyPair, error) { + gotCh <- host + return &TLSCertKeyPair{}, nil + }) + + b.refreshApplicableCerts(context.Background()) + + want := set.Of(certDomain, byoDomain) + got := set.Set[string]{} + for got.Len() < want.Len() { + select { + case h := <-gotCh: + got.Add(h) + case <-time.After(5 * time.Second): + t.Fatalf("timed out waiting for refresh workers; got %v, want %v", got, want) + } + } + if !maps.Equal(got, want) { + t.Errorf("got fetches %v, want %v", got, want) + } + select { + case h := <-gotCh: + t.Errorf("unexpected extra fetch for %q", h) + default: + } +} diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index ac423bde2..31ca7f941 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -422,6 +422,18 @@ 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. @@ -1176,6 +1188,11 @@ var ( hookCheckCaptivePortalLoop feature.Hook[func(*LocalBackend, context.Context)] ) +// hookCertRefreshLoop is set by the ACME-enabled cert code to a function +// that periodically refreshes TLS certs for Serve/Funnel-configured +// domains so renewals proceed even on otherwise-idle nodes. +var hookCertRefreshLoop feature.Hook[func(*LocalBackend, context.Context)] + func (b *LocalBackend) onHealthChange(change health.Change) { if !buildfeatures.HasHealth { return @@ -1274,6 +1291,11 @@ func (b *LocalBackend) Shutdown() { b.captiveCancel() } + if buildfeatures.HasACME && b.certRefreshCancel != nil { + b.certRefreshCancel() + b.certRefreshCancel = nil + } + b.stopReconnectTimerLocked() if b.loginFlags&controlclient.LoginEphemeral != 0 { @@ -6444,6 +6466,11 @@ func (b *LocalBackend) enterStateLocked(newState ipn.State) { b.goTracker.Go(func() { hookCheckCaptivePortalLoop.Get()(b, captiveCtx) }) } } + + // (Re)evaluate the background TLS cert refresh loop. It runs + // only while we're Running and ServeConfig has at least one + // HTTPS Web entry, so idle nodes don't hold a timer open. + b.updateCertRefreshLoopLocked() } else if oldState == ipn.Running { // Transitioning away from running. b.closePeerAPIListenersLocked() @@ -6457,6 +6484,8 @@ func (b *LocalBackend) enterStateLocked(newState ipn.State) { // that we always have a (canceled) context to wait on // in onHealthChange. } + + b.updateCertRefreshLoopLocked() } b.pauseOrResumeControlClientLocked() @@ -7188,6 +7217,65 @@ func (b *LocalBackend) setTCPPortsInterceptedFromNetmapAndPrefsLocked(prefs ipn. // The LocalBackend's mutex is held while calling. var hookMaybeMutateHostinfoLocked feature.Hooks[func(*LocalBackend, *tailcfg.Hostinfo, ipn.PrefsView) bool] +// updateCertRefreshLoopLocked starts or stops the background TLS cert +// refresh loop based on whether we currently have any HTTPS-serving +// hostname whose cert we should keep fresh. The loop runs only while: +// +// - ACME support is compiled in, +// - the node is in [ipn.Running], and +// - the current [ipn.ServeConfig] has at least one HTTPS Web entry. +// +// We deliberately don't keep an idle timer around on hosts that have no +// certs to maintain (e.g. mobile devices that never run Serve), so this +// must be called whenever any of those inputs change: state transitions +// and ServeConfig reloads. +// +// b.mu must be held. +func (b *LocalBackend) updateCertRefreshLoopLocked() { + if !buildfeatures.HasACME { + return + } + shouldRun := hookCertRefreshLoop.IsSet() && + b.state == ipn.Running && + serveConfigUsesACMECerts(b.serveConfig) + + switch { + case shouldRun && b.certRefreshCancel == nil: + ctx, cancel := context.WithCancel(b.ctx) + b.certRefreshCancel = cancel + b.goTracker.Go(func() { hookCertRefreshLoop.Get()(b, ctx) }) + case !shouldRun && b.certRefreshCancel != nil: + b.certRefreshCancel() + b.certRefreshCancel = nil + } +} + +// serveConfigUsesACMECerts reports whether sc has any entry that +// causes tailscaled to obtain ACME-managed TLS certs: an HTTPS Web +// entry (background, foreground, or service) or a TCP handler with +// TerminateTLS set (`tailscale serve --tls-terminated-tcp`). +func serveConfigUsesACMECerts(sc ipn.ServeConfigView) bool { + if !sc.Valid() { + return false + } + for range sc.Webs() { + return true + } + for _, tcp := range sc.TCPs() { + if tcp.TerminateTLS() != "" { + return true + } + } + for _, svc := range sc.Services().All() { + for _, tcp := range svc.TCP().All() { + if tcp.TerminateTLS() != "" { + return true + } + } + } + return false +} + // maybeSentHostinfoIfChangedLocked updates the hostinfo.ServicesHash, hostinfo.WireIngress and // hostinfo.IngressEnabled fields and kicks off a Hostinfo update if the values have changed. // diff --git a/ipn/ipnlocal/serve.go b/ipn/ipnlocal/serve.go index 58fa17eef..3a7db7939 100644 --- a/ipn/ipnlocal/serve.go +++ b/ipn/ipnlocal/serve.go @@ -1544,6 +1544,7 @@ func (b *LocalBackend) reloadServeConfigLocked(prefs ipn.PrefsView) { if err != nil { b.lastServeConfJSON = mem.B(nil) b.serveConfig = ipn.ServeConfigView{} + b.updateCertRefreshLoopLocked() return } if b.lastServeConfJSON.Equal(mem.B(confj)) { @@ -1554,6 +1555,7 @@ func (b *LocalBackend) reloadServeConfigLocked(prefs ipn.PrefsView) { if err := json.Unmarshal(confj, &conf); err != nil { b.logf("invalid ServeConfig %q in StateStore: %v", confKey, err) b.serveConfig = ipn.ServeConfigView{} + b.updateCertRefreshLoopLocked() return } @@ -1564,6 +1566,7 @@ func (b *LocalBackend) reloadServeConfigLocked(prefs ipn.PrefsView) { }) b.serveConfig = conf.View() + b.updateCertRefreshLoopLocked() } func (b *LocalBackend) setVIPServicesTCPPortsInterceptedLocked(svcPorts map[tailcfg.ServiceName][]uint16) { diff --git a/tsconst/health.go b/tsconst/health.go index 93c6550ef..dbcb8d29a 100644 --- a/tsconst/health.go +++ b/tsconst/health.go @@ -23,4 +23,5 @@ const ( HealthWarnableTestWarnable = "test-warnable" HealthWarnableApplyDiskConfig = "apply-disk-config" HealthWarnableWarmingUp = "warming-up" + HealthWarnableTLSCertPending = "tls-cert-pending" )