diff --git a/cmd/k8s-operator/depaware.txt b/cmd/k8s-operator/depaware.txt index 5df162e13..603af4fb2 100644 --- a/cmd/k8s-operator/depaware.txt +++ b/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+ diff --git a/cmd/tailscale/cli/maybe_captiveportal.go b/cmd/tailscale/cli/maybe_captiveportal.go new file mode 100644 index 000000000..1994b5628 --- /dev/null +++ b/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" diff --git a/cmd/tailscale/depaware.txt b/cmd/tailscale/depaware.txt index 58760a804..63ceb91b1 100644 --- a/cmd/tailscale/depaware.txt +++ b/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+ diff --git a/cmd/tailscaled/depaware.txt b/cmd/tailscaled/depaware.txt index 488fe57c5..effa4c042 100644 --- a/cmd/tailscaled/depaware.txt +++ b/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+ diff --git a/cmd/tsidp/depaware.txt b/cmd/tsidp/depaware.txt index 4b307b6f9..6c1a9e2c2 100644 --- a/cmd/tsidp/depaware.txt +++ b/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+ diff --git a/feature/captiveportal/captiveportal.go b/feature/captiveportal/captiveportal.go new file mode 100644 index 000000000..7295c02f6 --- /dev/null +++ b/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) + } +} diff --git a/feature/captiveportal/netcheckhook/netcheckhook.go b/feature/captiveportal/netcheckhook/netcheckhook.go new file mode 100644 index 000000000..15c2de249 --- /dev/null +++ b/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 +} diff --git a/feature/condregister/maybe_captiveportal.go b/feature/condregister/maybe_captiveportal.go new file mode 100644 index 000000000..8086e89fe --- /dev/null +++ b/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" diff --git a/ipn/ipnext/ipnext.go b/ipn/ipnext/ipnext.go index fd11fc204..30436e1ff 100644 --- a/ipn/ipnext/ipnext.go +++ b/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 } diff --git a/ipn/ipnlocal/captiveportal.go b/ipn/ipnlocal/captiveportal.go deleted file mode 100644 index ae310c7cc..000000000 --- a/ipn/ipnlocal/captiveportal.go +++ /dev/null @@ -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) - } -} diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 3e84e9268..e95f3409a 100644 --- a/ipn/ipnlocal/local.go +++ b/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() diff --git a/net/netcheck/captiveportal.go b/net/netcheck/captiveportal.go deleted file mode 100644 index 310e98ce7..000000000 --- a/net/netcheck/captiveportal.go +++ /dev/null @@ -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 -} diff --git a/net/netcheck/netcheck.go b/net/netcheck/netcheck.go index 8161ae48d..a3f7fe31a 100644 --- a/net/netcheck/netcheck.go +++ b/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() diff --git a/tsnet/depaware.txt b/tsnet/depaware.txt index 53304f583..19bb12ba8 100644 --- a/tsnet/depaware.txt +++ b/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+ diff --git a/tsnet/tsnet_test.go b/tsnet/tsnet_test.go index ca3ed035e..fc7b11691 100644 --- a/tsnet/tsnet_test.go +++ b/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",