cmd/k8s-operator/depaware.txt

@@ -784,7 +784,6 @@ tailscale.com/cmd/k8s-operator dependencies: (generated by github.com/tailscale/
         tailscale.com/metrics                                        from tailscale.com/tsweb+
         tailscale.com/net/bakedroots                                 from tailscale.com/net/tlsdial+
      💣 tailscale.com/net/batching                                   from tailscale.com/wgengine/magicsock
-        tailscale.com/net/captivedetection                           from tailscale.com/ipn/ipnlocal+
         tailscale.com/net/dns                                        from tailscale.com/ipn/ipnlocal+
         tailscale.com/net/dns/publicdns                              from tailscale.com/net/dns+
         tailscale.com/net/dns/resolvconffile                         from tailscale.com/cmd/k8s-operator+

cmd/tailscale/cli/maybe_captiveportal.go

@@ -0,0 +1,10 @@
+// Copyright (c) Tailscale Inc & contributors
+// SPDX-License-Identifier: BSD-3-Clause
+
+//go:build !ts_omit_captiveportal
+
+package cli
+
+// Import the netcheck captive portal hook package so that the netcheck
+// command also probes for captive portals during its report.
+import _ "tailscale.com/feature/captiveportal/netcheckhook"

cmd/tailscale/depaware.txt

@@ -198,6 +198,7 @@ tailscale.com/cmd/tailscale dependencies: (generated by github.com/tailscale/dep
         tailscale.com/feature                                        from tailscale.com/tsweb+
    L    tailscale.com/feature/awsparamstore                          from tailscale.com/feature/condregister/awsparamstore
         tailscale.com/feature/buildfeatures                          from tailscale.com/cmd/tailscale/cli+
+        tailscale.com/feature/captiveportal/netcheckhook             from tailscale.com/cmd/tailscale/cli
         tailscale.com/feature/capture/dissector                      from tailscale.com/cmd/tailscale/cli
         tailscale.com/feature/condregister/awsparamstore             from tailscale.com/cmd/tailscale/cli
         tailscale.com/feature/condregister/identityfederation        from tailscale.com/cmd/tailscale/cli
@@ -222,11 +223,11 @@ tailscale.com/cmd/tailscale dependencies: (generated by github.com/tailscale/dep
         tailscale.com/metrics                                        from tailscale.com/tsweb+
         tailscale.com/net/ace                                        from tailscale.com/cmd/tailscale/cli
         tailscale.com/net/bakedroots                                 from tailscale.com/net/tlsdial
-        tailscale.com/net/captivedetection                           from tailscale.com/net/netcheck
+        tailscale.com/net/captivedetection                           from tailscale.com/feature/captiveportal/netcheckhook
         tailscale.com/net/dnscache                                   from tailscale.com/control/controlhttp+
         tailscale.com/net/dnsfallback                                from tailscale.com/control/controlhttp+
         tailscale.com/net/netaddr                                    from tailscale.com/ipn+
-        tailscale.com/net/netcheck                                   from tailscale.com/cmd/tailscale/cli
+        tailscale.com/net/netcheck                                   from tailscale.com/cmd/tailscale/cli+
         tailscale.com/net/neterror                                   from tailscale.com/net/netcheck+
         tailscale.com/net/netknob                                    from tailscale.com/net/netns+
      💣 tailscale.com/net/netmon                                     from tailscale.com/cmd/tailscale/cli+

cmd/tailscaled/depaware.txt

@@ -295,6 +295,8 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de
   LD    tailscale.com/feature/bird                                   from tailscale.com/feature/condregister
         tailscale.com/feature/buildfeatures                          from tailscale.com/wgengine/magicsock+
         tailscale.com/feature/c2n                                    from tailscale.com/feature/condregister
+        tailscale.com/feature/captiveportal                          from tailscale.com/feature/condregister
+        tailscale.com/feature/captiveportal/netcheckhook             from tailscale.com/feature/captiveportal
         tailscale.com/feature/capture                                from tailscale.com/feature/condregister
         tailscale.com/feature/clientupdate                           from tailscale.com/feature/condregister
         tailscale.com/feature/condlite/expvar                        from tailscale.com/wgengine/magicsock
@@ -358,7 +360,7 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de
         tailscale.com/net/ace                                        from tailscale.com/feature/ace
         tailscale.com/net/bakedroots                                 from tailscale.com/net/tlsdial+
      💣 tailscale.com/net/batching                                   from tailscale.com/wgengine/magicsock+
-        tailscale.com/net/captivedetection                           from tailscale.com/ipn/ipnlocal+
+        tailscale.com/net/captivedetection                           from tailscale.com/feature/captiveportal+
         tailscale.com/net/dns                                        from tailscale.com/cmd/tailscaled+
         tailscale.com/net/dns/publicdns                              from tailscale.com/net/dns+
         tailscale.com/net/dns/resolvconffile                         from tailscale.com/net/dns+

cmd/tsidp/depaware.txt

@@ -183,7 +183,6 @@ tailscale.com/cmd/tsidp dependencies: (generated by github.com/tailscale/depawar
         tailscale.com/metrics                                        from tailscale.com/tsweb+
         tailscale.com/net/bakedroots                                 from tailscale.com/net/tlsdial+
      💣 tailscale.com/net/batching                                   from tailscale.com/wgengine/magicsock
-        tailscale.com/net/captivedetection                           from tailscale.com/ipn/ipnlocal+
         tailscale.com/net/dns                                        from tailscale.com/ipn/ipnlocal+
         tailscale.com/net/dns/publicdns                              from tailscale.com/net/dns+
         tailscale.com/net/dns/resolvconffile                         from tailscale.com/net/dns+

feature/captiveportal/captiveportal.go

@@ -0,0 +1,285 @@
+// Copyright (c) Tailscale Inc & contributors
+// SPDX-License-Identifier: BSD-3-Clause
+
+// Package captiveportal provides optional captive portal detection,
+// warning the user via the health tracker when their network requires
+// a browser login before traffic can flow.
+package captiveportal
+
+import (
+	"context"
+	"time"
+
+	"tailscale.com/feature"
+	_ "tailscale.com/feature/captiveportal/netcheckhook" // install the netcheck probe hook too
+	"tailscale.com/health"
+	"tailscale.com/ipn"
+	"tailscale.com/ipn/ipnext"
+	"tailscale.com/net/captivedetection"
+	"tailscale.com/syncs"
+	"tailscale.com/types/logger"
+	"tailscale.com/util/clientmetric"
+	"tailscale.com/util/eventbus"
+)
+
+const featureName = "captiveportal"
+
+func init() {
+	feature.Register(featureName)
+	ipnext.RegisterExtension(featureName, newExtension)
+}
+
+var metricCaptivePortalDetected = clientmetric.NewCounter("captiveportal_detected")
+
+// captivePortalDetectionInterval is the duration to wait in an unhealthy state with connectivity broken
+// before running captive portal detection.
+const captivePortalDetectionInterval = 2 * time.Second
+
+// captivePortalWarnable is a Warnable which is set to an unhealthy state when a captive portal is detected.
+var captivePortalWarnable = health.Register(&health.Warnable{
+	Code:  "captive-portal-detected",
+	Title: "Captive portal detected",
+	// High severity, because captive portals block all traffic and require user intervention.
+	Severity:            health.SeverityHigh,
+	Text:                health.StaticMessage("This network requires you to log in using your web browser."),
+	ImpactsConnectivity: true,
+})
+
+// Extension is the captive portal detection extension.
+// There is one per [ipnext.Host] (and hence per LocalBackend).
+type Extension struct {
+	logf   logger.Logf
+	sb     ipnext.SafeBackend
+	host   ipnext.Host // from Init
+	health *health.Tracker
+	ec     *eventbus.Client
+
+	mu syncs.Mutex
+	// captiveCtx and captiveCancel are used to control captive portal
+	// detection. They are protected by 'mu' and can be changed during the
+	// lifetime of the extension.
+	//
+	// captiveCtx will always be non-nil, though it might be a canceled
+	// context. captiveCancel is non-nil if checkCaptivePortalLoop is
+	// running, and is set to nil after being canceled.
+	captiveCtx    context.Context
+	captiveCancel context.CancelFunc
+
+	// needsCaptiveDetection is a channel that is used to signal either
+	// that captive portal detection is required (sending true) or that the
+	// backend is healthy and captive portal detection is not required
+	// (sending false).
+	needsCaptiveDetection chan bool
+}
+
+func newExtension(logf logger.Logf, sb ipnext.SafeBackend) (ipnext.Extension, error) {
+	// Initialize the context used to control the captive portal detection
+	// goroutine in a canceled state, so that it is always non-nil and safe
+	// to wait on even before the loop has ever started.
+	ctx, cancel := context.WithCancel(context.Background())
+	cancel()
+	return &Extension{
+		logf:                  logf,
+		sb:                    sb,
+		health:                sb.Sys().HealthTracker.Get(),
+		captiveCtx:            ctx,
+		needsCaptiveDetection: make(chan bool),
+	}, nil
+}
+
+func (e *Extension) Name() string { return featureName }
+
+func (e *Extension) Init(h ipnext.Host) error {
+	e.host = h
+	h.Hooks().BackendStateChange.Add(e.onBackendStateChange)
+	e.ec = e.sb.Sys().Bus.Get().Client("captiveportal")
+	eventbus.SubscribeFunc(e.ec, e.onHealthChange)
+	return nil
+}
+
+func (e *Extension) Shutdown() error {
+	e.mu.Lock()
+	if e.captiveCancel != nil {
+		e.captiveCancel()
+		e.captiveCancel = nil
+	}
+	e.mu.Unlock()
+	if e.ec != nil {
+		e.ec.Close()
+	}
+	return nil
+}
+
+// onBackendStateChange starts or stops the captive portal detection loop as
+// the backend enters or leaves the Running state. It is called with
+// LocalBackend's mutex held, so it must not call back into LocalBackend.
+func (e *Extension) onBackendStateChange(st ipn.State) {
+	e.mu.Lock()
+	defer e.mu.Unlock()
+	if st == ipn.Running {
+		// Start a captive portal detection loop if none has been
+		// started.
+		if e.captiveCancel == nil {
+			e.captiveCtx, e.captiveCancel = context.WithCancel(context.Background())
+			go e.checkCaptivePortalLoop(e.captiveCtx)
+		}
+	} else if e.captiveCancel != nil {
+		// Transitioning away from Running; stop any existing captive
+		// portal detection loop.
+		e.captiveCancel()
+		e.captiveCancel = nil
+
+		// NOTE: don't set captiveCtx to nil here, to ensure that we
+		// always have a (canceled) context to wait on in
+		// onHealthChange.
+	}
+}
+
+// onHealthChange is called (via the eventbus) whenever the node's health
+// state changes. If connectivity appears to be impacted, it signals the
+// detection loop to check for a captive portal.
+func (e *Extension) onHealthChange(health.Change) {
+	state := e.health.CurrentState()
+	isConnectivityImpacted := false
+	for _, w := range state.Warnings {
+		// Ignore the captive portal warnable itself.
+		if w.ImpactsConnectivity && w.WarnableCode != captivePortalWarnable.Code {
+			isConnectivityImpacted = true
+			break
+		}
+	}
+
+	// captiveCtx can be changed, and is protected with 'mu'; grab that
+	// before we start our select, below.
+	//
+	// It is guaranteed to be non-nil.
+	e.mu.Lock()
+	ctx := e.captiveCtx
+	e.mu.Unlock()
+
+	// If the context is canceled, we don't need to do anything.
+	if ctx.Err() != nil {
+		return
+	}
+
+	if isConnectivityImpacted {
+		e.logf("health: connectivity impacted; triggering captive portal detection")
+
+		// Ensure that we select on captiveCtx so that we can time out
+		// triggering captive portal detection if the loop is shut down.
+		select {
+		case e.needsCaptiveDetection <- true:
+		case <-ctx.Done():
+		}
+	} else {
+		// If connectivity is not impacted, we know for sure we're not behind a captive portal,
+		// so drop any warning, and signal that we don't need captive portal detection.
+		e.health.SetHealthy(captivePortalWarnable)
+		select {
+		case e.needsCaptiveDetection <- false:
+		case <-ctx.Done():
+		}
+	}
+}
+
+func (e *Extension) checkCaptivePortalLoop(ctx context.Context) {
+	var tmr *time.Timer
+
+	maybeStartTimer := func() {
+		// If there's an existing timer, nothing to do; just continue
+		// waiting for it to expire. Otherwise, create a new timer.
+		if tmr == nil {
+			tmr = time.NewTimer(captivePortalDetectionInterval)
+		}
+	}
+	maybeStopTimer := func() {
+		if tmr == nil {
+			return
+		}
+		if !tmr.Stop() {
+			<-tmr.C
+		}
+		tmr = nil
+	}
+
+	for {
+		if ctx.Err() != nil {
+			maybeStopTimer()
+			return
+		}
+
+		// First, see if we have a signal on our "healthy" channel, which
+		// takes priority over an existing timer. Because a select is
+		// nondeterministic, we explicitly check this channel before
+		// entering the main select below, so that we're guaranteed to
+		// stop the timer before starting captive portal detection.
+		select {
+		case needsCaptiveDetection := <-e.needsCaptiveDetection:
+			if needsCaptiveDetection {
+				maybeStartTimer()
+			} else {
+				maybeStopTimer()
+			}
+		default:
+		}
+
+		var timerChan <-chan time.Time
+		if tmr != nil {
+			timerChan = tmr.C
+		}
+		select {
+		case <-ctx.Done():
+			// All done; stop the timer and then exit.
+			maybeStopTimer()
+			return
+		case <-timerChan:
+			// Kick off captive portal check
+			e.performCaptiveDetection(ctx)
+			// nil out the timer and its channel to force recreation
+			tmr, timerChan = nil, nil
+		case needsCaptiveDetection := <-e.needsCaptiveDetection:
+			if needsCaptiveDetection {
+				maybeStartTimer()
+			} else {
+				// Healthy; cancel any existing timer
+				maybeStopTimer()
+			}
+		}
+	}
+}
+
+// shouldRunCaptivePortalDetection reports whether captive portal detection
+// should be run. It is enabled by default, but can be disabled via a control
+// knob. It is also only run when the user explicitly wants the backend to be
+// running.
+func (e *Extension) shouldRunCaptivePortalDetection() bool {
+	return !e.sb.Sys().ControlKnobs().DisableCaptivePortalDetection.Load() &&
+		e.host.Profiles().CurrentPrefs().WantRunning()
+}
+
+// performCaptiveDetection checks if captive portal detection is enabled via controlknob. If so, it runs
+// the detection and updates the Warnable accordingly.
+func (e *Extension) performCaptiveDetection(ctx context.Context) {
+	if !e.shouldRunCaptivePortalDetection() {
+		return
+	}
+
+	d := captivedetection.NewDetector(e.logf)
+	dm := e.host.NodeBackend().DERPMap()
+	preferredDERP := 0
+	if mc, ok := e.sb.Sys().MagicSock.GetOK(); ok {
+		if report := mc.GetLastNetcheckReport(ctx); report != nil {
+			preferredDERP = report.PreferredDERP
+		}
+	}
+	netMon := e.sb.Sys().NetMon.Get()
+	found := d.Detect(ctx, netMon, dm, preferredDERP)
+	if found {
+		if !e.health.IsUnhealthy(captivePortalWarnable) {
+			metricCaptivePortalDetected.Add(1)
+		}
+		e.health.SetUnhealthy(captivePortalWarnable, health.Args{})
+	} else {
+		e.health.SetHealthy(captivePortalWarnable)
+	}
+}

feature/captiveportal/netcheckhook/netcheckhook.go

@@ -0,0 +1,67 @@
+// Copyright (c) Tailscale Inc & contributors
+// SPDX-License-Identifier: BSD-3-Clause
+
+// Package netcheckhook makes netcheck probe for captive portals during
+// full reports. It does so as a side effect of being imported, by
+// installing a netcheck hook from init.
+package netcheckhook
+
+import (
+	"context"
+	"log"
+	"time"
+
+	"tailscale.com/net/captivedetection"
+	"tailscale.com/net/netcheck"
+	"tailscale.com/tailcfg"
+)
+
+func init() {
+	netcheck.HookStartCaptivePortalDetection.Set(startCaptivePortalDetection)
+}
+
+// captivePortalDelay is the duration to wait after starting a netcheck before
+// also probing for a captive portal, to let UDP STUN finish first and avoid
+// the probe if it's unnecessary. Chosen semi-arbitrarily.
+const captivePortalDelay = 200 * time.Millisecond
+
+func startCaptivePortalDetection(ctx context.Context, c *netcheck.Client, dm *tailcfg.DERPMap, preferredDERP int, setCaptivePortal func(bool)) (done <-chan struct{}, stop func()) {
+	logf := c.Logf
+	if logf == nil {
+		logf = log.Printf
+	}
+
+	// This goroutine can't be tracked by the wait group that
+	// netcheck.GetReport uses for its probes, since GetReport doesn't
+	// wait for that group to finish before returning and we'd get a
+	// data race. Instead, completion is signaled by closing ch, which
+	// GetReport receives as the done channel.
+	ch := make(chan struct{})
+
+	tmr := time.AfterFunc(captivePortalDelay, func() {
+		defer close(ch)
+		d := captivedetection.NewDetector(logf)
+		found := d.Detect(ctx, c.NetMon, dm, preferredDERP)
+		setCaptivePortal(found)
+	})
+
+	if c.Verbose {
+		// Don't cancel our captive portal check if we're
+		// explicitly doing a verbose netcheck.
+		return ch, func() {}
+	}
+
+	stop = func() {
+		if tmr.Stop() {
+			// Stopped successfully; need to close the
+			// signal channel ourselves.
+			close(ch)
+			return
+		}
+
+		// Did not stop; do nothing and it'll finish by itself
+		// and close the signal channel.
+	}
+
+	return ch, stop
+}

feature/condregister/maybe_captiveportal.go

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

ipn/ipnext/ipnext.go

@@ -531,4 +531,8 @@ type NodeBackend interface {
 	// node that the portlist service collection is desirable, should it
 	// choose to report them.
 	CollectServices() bool
+
+	// DERPMap returns the current DERP map from the current netmap,
+	// or nil if there is no netmap.
+	DERPMap() *tailcfg.DERPMap
 }

ipn/ipnlocal/captiveportal.go

@@ -1,186 +0,0 @@
-// Copyright (c) Tailscale Inc & contributors
-// SPDX-License-Identifier: BSD-3-Clause
-
-//go:build !ts_omit_captiveportal
-
-package ipnlocal
-
-import (
-	"context"
-	"time"
-
-	"tailscale.com/health"
-	"tailscale.com/net/captivedetection"
-	"tailscale.com/util/clientmetric"
-)
-
-func init() {
-	hookCaptivePortalHealthChange.Set(captivePortalHealthChange)
-	hookCheckCaptivePortalLoop.Set(checkCaptivePortalLoop)
-}
-
-var metricCaptivePortalDetected = clientmetric.NewCounter("captiveportal_detected")
-
-// captivePortalDetectionInterval is the duration to wait in an unhealthy state with connectivity broken
-// before running captive portal detection.
-const captivePortalDetectionInterval = 2 * time.Second
-
-func captivePortalHealthChange(b *LocalBackend, state *health.State) {
-	isConnectivityImpacted := false
-	for _, w := range state.Warnings {
-		// Ignore the captive portal warnable itself.
-		if w.ImpactsConnectivity && w.WarnableCode != captivePortalWarnable.Code {
-			isConnectivityImpacted = true
-			break
-		}
-	}
-
-	// captiveCtx can be changed, and is protected with 'mu'; grab that
-	// before we start our select, below.
-	//
-	// It is guaranteed to be non-nil.
-	b.mu.Lock()
-	ctx := b.captiveCtx
-	b.mu.Unlock()
-
-	// If the context is canceled, we don't need to do anything.
-	if ctx.Err() != nil {
-		return
-	}
-
-	if isConnectivityImpacted {
-		b.logf("health: connectivity impacted; triggering captive portal detection")
-
-		// Ensure that we select on captiveCtx so that we can time out
-		// triggering captive portal detection if the backend is shutdown.
-		select {
-		case b.needsCaptiveDetection <- true:
-		case <-ctx.Done():
-		}
-	} else {
-		// If connectivity is not impacted, we know for sure we're not behind a captive portal,
-		// so drop any warning, and signal that we don't need captive portal detection.
-		b.health.SetHealthy(captivePortalWarnable)
-		select {
-		case b.needsCaptiveDetection <- false:
-		case <-ctx.Done():
-		}
-	}
-}
-
-// captivePortalWarnable is a Warnable which is set to an unhealthy state when a captive portal is detected.
-var captivePortalWarnable = health.Register(&health.Warnable{
-	Code:  "captive-portal-detected",
-	Title: "Captive portal detected",
-	// High severity, because captive portals block all traffic and require user intervention.
-	Severity:            health.SeverityHigh,
-	Text:                health.StaticMessage("This network requires you to log in using your web browser."),
-	ImpactsConnectivity: true,
-})
-
-func checkCaptivePortalLoop(b *LocalBackend, ctx context.Context) {
-	var tmr *time.Timer
-
-	maybeStartTimer := func() {
-		// If there's an existing timer, nothing to do; just continue
-		// waiting for it to expire. Otherwise, create a new timer.
-		if tmr == nil {
-			tmr = time.NewTimer(captivePortalDetectionInterval)
-		}
-	}
-	maybeStopTimer := func() {
-		if tmr == nil {
-			return
-		}
-		if !tmr.Stop() {
-			<-tmr.C
-		}
-		tmr = nil
-	}
-
-	for {
-		if ctx.Err() != nil {
-			maybeStopTimer()
-			return
-		}
-
-		// First, see if we have a signal on our "healthy" channel, which
-		// takes priority over an existing timer. Because a select is
-		// nondeterministic, we explicitly check this channel before
-		// entering the main select below, so that we're guaranteed to
-		// stop the timer before starting captive portal detection.
-		select {
-		case needsCaptiveDetection := <-b.needsCaptiveDetection:
-			if needsCaptiveDetection {
-				maybeStartTimer()
-			} else {
-				maybeStopTimer()
-			}
-		default:
-		}
-
-		var timerChan <-chan time.Time
-		if tmr != nil {
-			timerChan = tmr.C
-		}
-		select {
-		case <-ctx.Done():
-			// All done; stop the timer and then exit.
-			maybeStopTimer()
-			return
-		case <-timerChan:
-			// Kick off captive portal check
-			b.performCaptiveDetection()
-			// nil out timer to force recreation
-			tmr = nil
-		case needsCaptiveDetection := <-b.needsCaptiveDetection:
-			if needsCaptiveDetection {
-				maybeStartTimer()
-			} else {
-				// Healthy; cancel any existing timer
-				maybeStopTimer()
-			}
-		}
-	}
-}
-
-// shouldRunCaptivePortalDetection reports whether captive portal detection
-// should be run. It is enabled by default, but can be disabled via a control
-// knob. It is also only run when the user explicitly wants the backend to be
-// running.
-func (b *LocalBackend) shouldRunCaptivePortalDetection() bool {
-	b.mu.Lock()
-	defer b.mu.Unlock()
-	return !b.ControlKnobs().DisableCaptivePortalDetection.Load() && b.pm.prefs.WantRunning()
-}
-
-// performCaptiveDetection checks if captive portal detection is enabled via controlknob. If so, it runs
-// the detection and updates the Warnable accordingly.
-func (b *LocalBackend) performCaptiveDetection() {
-	if !b.shouldRunCaptivePortalDetection() {
-		return
-	}
-
-	d := captivedetection.NewDetector(b.logf)
-	b.mu.Lock() // for b.hostinfo
-	cn := b.currentNode()
-	dm := cn.DERPMap()
-	preferredDERP := 0
-	if b.hostinfo != nil {
-		if b.hostinfo.NetInfo != nil {
-			preferredDERP = b.hostinfo.NetInfo.PreferredDERP
-		}
-	}
-	ctx := b.ctx
-	netMon := b.NetMon()
-	b.mu.Unlock()
-	found := d.Detect(ctx, netMon, dm, preferredDERP)
-	if found {
-		if !b.health.IsUnhealthy(captivePortalWarnable) {
-			metricCaptivePortalDetected.Add(1)
-		}
-		b.health.SetUnhealthy(captivePortalWarnable, health.Args{})
-	} else {
-		b.health.SetHealthy(captivePortalWarnable)
-	}
-}

ipn/ipnlocal/local.go

@@ -433,21 +433,6 @@ type LocalBackend struct {
 	// refreshAutoExitNode indicates if the exit node should be recomputed when the next netcheck report is available.
 	refreshAutoExitNode bool // guarded by mu
 
-	// captiveCtx and captiveCancel are used to control captive portal
-	// detection. They are protected by 'mu' and can be changed during the
-	// lifetime of a LocalBackend.
-	//
-	// captiveCtx will always be non-nil, though it might be a canceled
-	// context. captiveCancel is non-nil if checkCaptivePortalLoop is
-	// running, and is set to nil after being canceled.
-	captiveCtx    context.Context
-	captiveCancel context.CancelFunc
-	// needsCaptiveDetection is a channel that is used to signal either
-	// that captive portal detection is required (sending true) or that the
-	// backend is healthy and captive portal detection is not required
-	// (sending false).
-	needsCaptiveDetection chan bool
-
 	// 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.
@@ -583,11 +568,6 @@ func NewLocalBackend(logf logger.Logf, logID logid.PublicID, sys *tsd.System, lo
 	ctx, cancel := context.WithCancelCause(context.Background())
 	clock := tstime.StdClock{}
 
-	// Until we transition to a Running state, use a canceled context for
-	// our captive portal detection.
-	captiveCtx, captiveCancel := context.WithCancel(ctx)
-	captiveCancel()
-
 	m := metrics{
 		advertisedRoutes: sys.UserMetricsRegistry().NewGauge(
 			"tailscaled_advertised_routes", "Number of advertised network routes (e.g. by a subnet router)"),
@@ -606,27 +586,24 @@ func NewLocalBackend(logf logger.Logf, logID logid.PublicID, sys *tsd.System, lo
 	}
 
 	b := &LocalBackend{
-		ctx:                   ctx,
-		ctxCancel:             cancel,
-		logf:                  logf,
-		keyLogf:               logger.LogOnChange(logf, 5*time.Minute, clock.Now),
-		statsLogf:             logger.LogOnChange(logf, 5*time.Minute, clock.Now),
-		sys:                   sys,
-		polc:                  sys.PolicyClientOrDefault(),
-		health:                sys.HealthTracker.Get(),
-		metrics:               m,
-		e:                     e,
-		dialer:                dialer,
-		store:                 store,
-		pm:                    pm,
-		backendLogID:          logID,
-		state:                 ipn.NoState,
-		em:                    newExpiryManager(logf, sys.Bus.Get()),
-		loginFlags:            loginFlags,
-		clock:                 clock,
-		captiveCtx:            captiveCtx,
-		captiveCancel:         nil, // so that we start checkCaptivePortalLoop when Running
-		needsCaptiveDetection: make(chan bool),
+		ctx:          ctx,
+		ctxCancel:    cancel,
+		logf:         logf,
+		keyLogf:      logger.LogOnChange(logf, 5*time.Minute, clock.Now),
+		statsLogf:    logger.LogOnChange(logf, 5*time.Minute, clock.Now),
+		sys:          sys,
+		polc:         sys.PolicyClientOrDefault(),
+		health:       sys.HealthTracker.Get(),
+		metrics:      m,
+		e:            e,
+		dialer:       dialer,
+		store:        store,
+		pm:           pm,
+		backendLogID: logID,
+		state:        ipn.NoState,
+		em:           newExpiryManager(logf, sys.Bus.Get()),
+		loginFlags:   loginFlags,
+		clock:        clock,
 	}
 
 	sys.NoiseRoundTripper.Set(noiseRoundTripper{b})
@@ -1241,12 +1218,6 @@ func (b *LocalBackend) linkChange(delta *netmon.ChangeDelta) {
 	}
 }
 
-// Captive portal detection hooks.
-var (
-	hookCaptivePortalHealthChange feature.Hook[func(*LocalBackend, *health.State)]
-	hookCheckCaptivePortalLoop    feature.Hook[func(*LocalBackend, context.Context)]
-)
-
 func (b *LocalBackend) onHealthChange(change health.Change) {
 	if !buildfeatures.HasHealth {
 		return
@@ -1274,10 +1245,6 @@ func (b *LocalBackend) onHealthChange(change health.Change) {
 		b.cc.SetIPForwardingBroken(broken)
 	}
 	b.mu.Unlock()
-
-	if f, ok := hookCaptivePortalHealthChange.GetOk(); ok {
-		f(b, state)
-	}
 }
 
 // GetOrSetCaptureSink returns the current packet capture sink, creating it
@@ -1343,11 +1310,6 @@ func (b *LocalBackend) Shutdown() {
 	}
 	b.shutdownCalled = true
 
-	if buildfeatures.HasCaptivePortal && b.captiveCancel != nil {
-		b.logf("canceling captive portal context")
-		b.captiveCancel()
-	}
-
 	b.shutdownCertRefreshLoopLocked()
 
 	b.stopReconnectTimerLocked()
@@ -6779,17 +6741,6 @@ func (b *LocalBackend) enterStateLocked(newState ipn.State) {
 			b.resetAuthURLLocked()
 		}
 
-		// Start a captive portal detection loop if none has been
-		// started. Create a new context if none is present, since it
-		// can be shut down if we transition away from Running.
-		if buildfeatures.HasCaptivePortal {
-			if b.captiveCancel == nil {
-				captiveCtx, captiveCancel := context.WithCancel(b.ctx)
-				b.captiveCtx, b.captiveCancel = captiveCtx, captiveCancel
-				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.
@@ -6798,16 +6749,6 @@ func (b *LocalBackend) enterStateLocked(newState ipn.State) {
 		// Transitioning away from running.
 		b.closePeerAPIListenersLocked()
 
-		// Stop any existing captive portal detection loop.
-		if buildfeatures.HasCaptivePortal && b.captiveCancel != nil {
-			b.captiveCancel()
-			b.captiveCancel = nil
-
-			// NOTE: don't set captiveCtx to nil here, to ensure
-			// that we always have a (canceled) context to wait on
-			// in onHealthChange.
-		}
-
 		b.updateCertRefreshLoopLocked()
 	}
 	b.pauseOrResumeControlClientLocked()

net/netcheck/captiveportal.go

@@ -1,55 +0,0 @@
-// Copyright (c) Tailscale Inc & contributors
-// SPDX-License-Identifier: BSD-3-Clause
-
-//go:build !ts_omit_captiveportal
-
-package netcheck
-
-import (
-	"context"
-	"time"
-
-	"tailscale.com/net/captivedetection"
-	"tailscale.com/tailcfg"
-)
-
-func init() {
-	hookStartCaptivePortalDetection.Set(startCaptivePortalDetection)
-}
-
-func startCaptivePortalDetection(ctx context.Context, rs *reportState, dm *tailcfg.DERPMap, preferredDERP int) (done <-chan struct{}, stop func()) {
-	c := rs.c
-
-	// NOTE(andrew): we can't simply add this goroutine to the
-	// `NewWaitGroupChan` below, since we don't wait for that
-	// waitgroup to finish when exiting this function and thus get
-	// a data race.
-	ch := make(chan struct{})
-
-	tmr := time.AfterFunc(c.captivePortalDelay(), func() {
-		defer close(ch)
-		d := captivedetection.NewDetector(c.logf)
-		found := d.Detect(ctx, c.NetMon, dm, preferredDERP)
-		rs.report.CaptivePortal.Set(found)
-	})
-
-	stop = func() {
-		// Don't cancel our captive portal check if we're
-		// explicitly doing a verbose netcheck.
-		if c.Verbose {
-			return
-		}
-
-		if tmr.Stop() {
-			// Stopped successfully; need to close the
-			// signal channel ourselves.
-			close(ch)
-			return
-		}
-
-		// Did not stop; do nothing and it'll finish by itself
-		// and close the signal channel.
-	}
-
-	return ch, stop
-}

net/netcheck/netcheck.go

@@ -233,8 +233,7 @@ type Client struct {
 	ForcePreferredDERP int
 
 	// For tests
-	testEnoughRegions      int
-	testCaptivePortalDelay time.Duration
+	testEnoughRegions int
 
 	mu       syncs.Mutex           // guards following
 	nextFull bool                  // do a full region scan, even if last != nil
@@ -257,14 +256,6 @@ func (c *Client) enoughRegions() int {
 	return 3
 }
 
-func (c *Client) captivePortalDelay() time.Duration {
-	if c.testCaptivePortalDelay > 0 {
-		return c.testCaptivePortalDelay
-	}
-	// Chosen semi-arbitrarily
-	return 200 * time.Millisecond
-}
-
 func (c *Client) logf(format string, a ...any) {
 	if c.Logf != nil {
 		c.Logf(format, a...)
@@ -788,7 +779,15 @@ func (c *Client) SetForcePreferredDERP(region int) {
 	c.ForcePreferredDERP = region
 }
 
-var hookStartCaptivePortalDetection feature.Hook[func(ctx context.Context, rs *reportState, dm *tailcfg.DERPMap, preferredDERP int) (<-chan struct{}, func())]
+// HookStartCaptivePortalDetection, if set, is called by GetReport to
+// asynchronously start captive portal detection during a full (non-incremental)
+// netcheck. It is set at init time by the optional
+// tailscale.com/feature/captiveportal/netcheckhook package.
+//
+// The returned done channel is closed when detection has finished (and
+// setCaptivePortal has been called with the result, if it ran); the returned
+// stop function cancels a detection that has not yet started.
+var HookStartCaptivePortalDetection feature.Hook[func(ctx context.Context, c *Client, dm *tailcfg.DERPMap, preferredDERP int, setCaptivePortal func(bool)) (done <-chan struct{}, stop func())]
 
 // GetReport gets a report. The 'opts' argument is optional and can be nil.
 // Callers are discouraged from passing a ctx with an arbitrary deadline as this
@@ -915,8 +914,11 @@ func (c *Client) GetReport(ctx context.Context, dm *tailcfg.DERPMap, opts *GetRe
 	captivePortalDone := syncs.ClosedChan()
 	captivePortalStop := func() {}
 	if buildfeatures.HasCaptivePortal && !rs.incremental && !onlySTUN {
-		start := hookStartCaptivePortalDetection.Get()
-		captivePortalDone, captivePortalStop = start(ctx, rs, dm, preferredDERP)
+		if start, ok := HookStartCaptivePortalDetection.GetOk(); ok {
+			captivePortalDone, captivePortalStop = start(ctx, c, dm, preferredDERP, func(found bool) {
+				rs.report.CaptivePortal.Set(found)
+			})
+		}
 	}
 
 	wg := syncs.NewWaitGroupChan()

tsnet/depaware.txt

@@ -179,7 +179,6 @@ tailscale.com/tsnet dependencies: (generated by github.com/tailscale/depaware)
         tailscale.com/metrics                                        from tailscale.com/tsweb+
         tailscale.com/net/bakedroots                                 from tailscale.com/net/tlsdial+
      💣 tailscale.com/net/batching                                   from tailscale.com/wgengine/magicsock
-        tailscale.com/net/captivedetection                           from tailscale.com/ipn/ipnlocal+
         tailscale.com/net/dns                                        from tailscale.com/ipn/ipnlocal+
         tailscale.com/net/dns/publicdns                              from tailscale.com/net/dns+
         tailscale.com/net/dns/resolvconffile                         from tailscale.com/net/dns+

tsnet/tsnet_test.go

@@ -3411,6 +3411,7 @@ func TestDeps(t *testing.T) {
 			"golang.org/x/crypto/ssh/internal/bcrypt_pbkdf": "tsnet should not depend on SSH",
 			"tailscale.com/chirp":                           "tsnet should not depend on BIRD integration",
 			"tailscale.com/feature/bird":                    "tsnet should not depend on BIRD integration",
+			"tailscale.com/feature/captiveportal":           "tsnet apps don't need captive portal detection; import it explicitly if desired",
 			"tailscale.com/feature/clientupdate":            "tsnet should not depend on feature/clientupdate",
 			"tailscale.com/feature/remoteconfig":            "tsnet should not depend on feature/remoteconfig",
 			"tailscale.com/feature/syspolicy":               "tsnet should not depend on syspolicy",