diff --git a/cmd/cli/cli.go b/cmd/cli/cli.go index c497678..98dc763 100644 --- a/cmd/cli/cli.go +++ b/cmd/cli/cli.go @@ -385,7 +385,7 @@ func run(appCallback *AppCallback, stopCh chan struct{}) { // would silently re-enable interception from config. if updateConfigInterceptMode(&cfg, interceptMode) { updated = true - mainLog.Load().Info().Msgf("writing intercept_mode = %q to config", cfg.Service.InterceptMode) + mainLog.Load().Info().Msgf("writing intercept_mode = %q to config (requested %q)", cfg.Service.InterceptMode, interceptMode) } if updated { diff --git a/cmd/cli/commands.go b/cmd/cli/commands.go index b58ae78..6ebf5c7 100644 --- a/cmd/cli/commands.go +++ b/cmd/cli/commands.go @@ -275,7 +275,7 @@ func initRunCmd() *cobra.Command { _ = runCmd.Flags().MarkHidden("iface") runCmd.Flags().StringVarP(&cdUpstreamProto, "proto", "", ctrld.ResolverTypeDOH, `Control D upstream type, either "doh" or "doh3"`) runCmd.Flags().BoolVarP(&rfc1918, "rfc1918", "", false, "Listen on RFC1918 addresses when 127.0.0.1 is the only listener") - runCmd.Flags().StringVarP(&interceptMode, "intercept-mode", "", "", "OS-level DNS interception mode: 'dns' (with VPN split routing) or 'hard' (all DNS through ctrld, no VPN split routing)") + runCmd.Flags().StringVarP(&interceptMode, "intercept-mode", "", "", "OS-level DNS interception mode: 'off' (disable interception and clear a persisted intercept_mode), 'dns' (with VPN split routing), or 'hard' (all DNS through ctrld, no VPN split routing)") runCmd.FParseErrWhitelist = cobra.FParseErrWhitelist{UnknownFlags: true} rootCmd.AddCommand(runCmd) @@ -394,8 +394,7 @@ NOTE: running "ctrld start" without any arguments will start already installed c svcExists := serviceConfigFileExists() mainLog.Load().Debug().Msgf("intercept upgrade check: args=%v interceptOnly=%v svcConfigExists=%v interceptMode=%q", osArgsEarly, interceptOnly, svcExists, interceptMode) if interceptOnly && svcExists { - // Replace any existing split or --intercept-mode= form. Keep an - // explicit "off" argument so it overrides a previously persisted config + // An explicit "off" argument must override a previously persisted config // value while the service clears that value on startup. if err := removeServiceFlag("--intercept-mode"); err != nil { mainLog.Load().Fatal().Err(err).Msg("failed to remove existing intercept mode from service arguments") @@ -778,7 +777,7 @@ NOTE: running "ctrld start" without any arguments will start already installed c startCmd.Flags().BoolVarP(&startOnly, "start_only", "", false, "Do not install new service") _ = startCmd.Flags().MarkHidden("start_only") startCmd.Flags().BoolVarP(&rfc1918, "rfc1918", "", false, "Listen on RFC1918 addresses when 127.0.0.1 is the only listener") - startCmd.Flags().StringVarP(&interceptMode, "intercept-mode", "", "", "OS-level DNS interception mode: 'dns' (with VPN split routing) or 'hard' (all DNS through ctrld, no VPN split routing)") + startCmd.Flags().StringVarP(&interceptMode, "intercept-mode", "", "", "OS-level DNS interception mode: 'off' (disable interception and clear a persisted intercept_mode), 'dns' (with VPN split routing), or 'hard' (all DNS through ctrld, no VPN split routing)") routerCmd := &cobra.Command{ Use: "setup", diff --git a/cmd/cli/dns_intercept_darwin.go b/cmd/cli/dns_intercept_darwin.go index a894886..9a6e5b8 100644 --- a/cmd/cli/dns_intercept_darwin.go +++ b/cmd/cli/dns_intercept_darwin.go @@ -484,6 +484,11 @@ func (p *prog) checkAnchorOrdering(filterLines []string, ourAnchorRef string) { // stopDNSIntercept removes all pf rules and cleans up the DNS interception. func (p *prog) stopDNSIntercept() error { + // Remove a loopback DNS target set for a DNS-less network (issue #533) + // before tearing down pf, so the service is returned to its saved or + // empty DNS state. + p.removeInterceptDNSTarget("intercept shutdown") + state, ok := p.dnsInterceptState.(*pfState) if !ok || state == nil { mainLog.Load().Debug().Msg("DNS intercept: no pf state to clean up") @@ -1801,6 +1806,11 @@ func (p *prog) pfWatchdog() { return } + // Reconcile the temporary service DNS target even when macOS emits no + // major network delta. This converges both DHCP-return cleanup and a + // later return to a DNS-less network. + ensureInterceptDNSTargetFn(p, []string{}) + result := p.ensurePFAnchorActive() if result == pfAnchorCheckIntact { // Only an authoritative intact result may trigger the functional probe. diff --git a/cmd/cli/dns_proxy.go b/cmd/cli/dns_proxy.go index b7d2038..a3f6b57 100644 --- a/cmd/cli/dns_proxy.go +++ b/cmd/cli/dns_proxy.go @@ -1852,24 +1852,13 @@ func (p *prog) debounceRecovery() { func (p *prog) handleRecovery(reason RecoveryReason) { mainLog.Load().Debug().Msg("Starting recovery process: removing DNS settings") - // For network changes, cancel any existing recovery check because the network state has changed. + recoveryCtx, gen, interceptRecovery, ok := p.beginRecovery(reason) + if !ok { + mainLog.Load().Debug().Msg("Upstream recovery already in progress; skipping duplicate trigger") + return + } if reason == RecoveryReasonNetworkChange { - p.recoveryCancelMu.Lock() - if p.recoveryCancel != nil { - mainLog.Load().Debug().Msg("Cancelling existing recovery check (network change)") - p.recoveryCancel() - p.recoveryCancel = nil - } - p.recoveryCancelMu.Unlock() - } else { - // For upstream failures, if a recovery is already in progress, do nothing new. - p.recoveryCancelMu.Lock() - if p.recoveryCancel != nil { - mainLog.Load().Debug().Msg("Upstream recovery already in progress; skipping duplicate trigger") - p.recoveryCancelMu.Unlock() - return - } - p.recoveryCancelMu.Unlock() + mainLog.Load().Debug().Msg("Network change recovery now owns shared recovery state") } // For network changes, force-reset all upstream transports synchronously. @@ -1887,32 +1876,28 @@ func (p *prog) handleRecovery(reason RecoveryReason) { mainLog.Load().Info().Msg("Force-reset upstream transports for network change recovery") } - // Create a new recovery context without a fixed timeout. - p.recoveryCancelMu.Lock() - recoveryCtx, cancel := context.WithCancel(context.Background()) - p.recoveryCancel = cancel - p.recoveryCancelMu.Unlock() - - // set recoveryRunning to true to prevent watchdogs from putting the listener back on the interface - p.recoveryRunning.Store(true) - // In DNS intercept mode, don't tear down WFP/pf filters. // Instead, enable recovery bypass so proxy() forwards queries to // the OS/DHCP resolver. This handles captive portal authentication // without the overhead of filter teardown/rebuild. - if dnsIntercept && p.dnsInterceptState != nil { - p.recoveryBypass.Store(true) + if interceptRecovery { mainLog.Load().Info().Msg("DNS intercept recovery: enabling DHCP bypass (filters stay active)") // Reinitialize OS resolver to discover DHCP servers on the new network. mainLog.Load().Debug().Msg("DNS intercept recovery: discovering DHCP nameservers") - dhcpServers := ctrld.InitializeOsResolver(true) + dhcpServers, systemNameservers := ctrld.InitializeOsResolverWithSystemNameservers(true) if len(dhcpServers) == 0 { mainLog.Load().Warn().Msg("DNS intercept recovery: no DHCP nameservers found") } else { mainLog.Load().Info().Msgf("DNS intercept recovery: found DHCP nameservers: %v", dhcpServers) } + // If the new network provides no usable IPv4 DNS (e.g. IPv6-only + // tethering with 464XLAT), macOS cannot emit DNS queries at all and + // pf has nothing to intercept. Ensure a loopback DNS target exists + // so the OS keeps sending queries to ctrld's listener (issue #533). + ensureInterceptDNSTargetFn(p, systemNameservers) + // Exempt DHCP nameservers from intercept filters so the OS resolver // can actually reach them on port 53. if len(dhcpServers) > 0 { @@ -1954,21 +1939,20 @@ func (p *prog) handleRecovery(reason RecoveryReason) { recovered, err := p.waitForUpstreamRecovery(recoveryCtx, upstreams) if err != nil { mainLog.Load().Error().Err(err).Msg("Recovery canceled; DNS settings remain removed") - p.recoveryCancelMu.Lock() - p.recoveryCancel = nil - p.recoveryCancelMu.Unlock() + p.recoveryCanceledCleanup(gen) + return + } + if !p.recoveryOwnsState(gen) { + mainLog.Load().Debug().Msgf("Recovery generation %d was superseded after upstream success; skipping stale completion", gen) return } mainLog.Load().Info().Msgf("Upstream %q recovered; re-applying DNS settings", recovered) - // reset the upstream failure count and down state + // Reset the upstream failure count and down state while this generation + // still owns recovery completion. p.um.reset(recovered) - // In DNS intercept mode, just disable the bypass — filters are still active. - if dnsIntercept && p.dnsInterceptState != nil { - p.recoveryBypass.Store(false) - mainLog.Load().Info().Msg("DNS intercept recovery complete: disabling DHCP bypass, resuming normal flow") - + if interceptRecovery { // Refresh VPN DNS routes in case VPN state changed during recovery. if p.vpnDNS != nil { p.vpnDNS.Refresh(true) @@ -1983,11 +1967,15 @@ func (p *prog) handleRecovery(reason RecoveryReason) { mainLog.Load().Info().Msgf("Reinitialized OS resolver with nameservers: %v", ns) } } - - p.recoveryRunning.Store(false) } else { - // For network changes we also reinitialize the OS resolver. - if reason == RecoveryReasonNetworkChange { + var systemNameservers []string + if dnsIntercept { + // Intercept was requested but no interceptor was active when recovery + // began. Rediscover on every recovery reason before retrying setDNS; + // passing nil could make a successful retry install a loopback target + // on a healthy DHCP network. + systemNameservers = systemNameserversForInterceptRetry() + } else if reason == RecoveryReasonNetworkChange { ns := ctrld.InitializeOsResolver(true) if len(ns) == 0 { mainLog.Load().Warn().Msg("No nameservers found for OS resolver during network-change recovery; using existing values") @@ -1997,17 +1985,17 @@ func (p *prog) handleRecovery(reason RecoveryReason) { } // Apply our DNS settings back and log the interface state. - p.setDNS() + p.setDNS(systemNameservers) p.logInterfacesState() - - // allow watchdogs to put the listener back on the interface if its changed for any reason - p.recoveryRunning.Store(false) } - // Clear the recovery cancellation for a clean slate. - p.recoveryCancelMu.Lock() - p.recoveryCancel = nil - p.recoveryCancelMu.Unlock() + if !p.completeRecovery(gen) { + mainLog.Load().Debug().Msgf("Recovery generation %d was superseded during completion; preserving successor state", gen) + return + } + if interceptRecovery { + mainLog.Load().Info().Msg("DNS intercept recovery complete: disabling DHCP bypass, resuming normal flow") + } } // waitForUpstreamRecovery checks the provided upstreams concurrently until one recovers. @@ -2085,7 +2073,15 @@ func (p *prog) waitForUpstreamRecovery(ctx context.Context, upstreams map[string var recovered string select { + case <-ctx.Done(): + return "", ctx.Err() + default: + } + select { case recovered = <-recoveredCh: + if err := ctx.Err(); err != nil { + return "", err + } case <-ctx.Done(): return "", ctx.Err() } diff --git a/cmd/cli/dns_target.go b/cmd/cli/dns_target.go new file mode 100644 index 0000000..e1451a0 --- /dev/null +++ b/cmd/cli/dns_target.go @@ -0,0 +1,116 @@ +package cli + +import "net" + +// interceptDNSRdrTarget is the loopback address used as the macOS service +// DNS value when ctrld's listener is NOT reachable at :53 +// directly (non-53 port, e.g. 127.0.0.1:5354 when mDNSResponder holds *:53). +// +// macOS resolvers always send DNS to port 53, so a direct-hit value is +// impossible in that case; delivery must go through the pf rdr rule +// ("rdr on lo0 ... to ! port 53 -> port "). +// The value therefore must be a loopback address DIFFERENT from the listener +// IP so the rdr's "! " matches. Any 127/8 address routes via lo0 +// on macOS. +const interceptDNSRdrTarget = "127.0.0.53" + +// interceptDNSTargetValue returns the nameserver value to set on a DNS-less +// macOS service so the OS emits DNS queries that reach ctrld, respecting the +// configured listener. The listener IP/port derivation mirrors +// buildPFAnchorRulesForTunnels so the value and the pf rules always agree. +// +// - listener on port 53: return the effective listener IP — queries hit the +// listener directly, no pf dependency for this leg. +// - listener on another port: return interceptDNSRdrTarget so the lo0 rdr +// rule fires and rewrites to the real listener address. +func (p *prog) interceptDNSTargetValue() string { + listenerIP := "127.0.0.1" + listenerPort := 53 + // FirstListener panics when no listener is configured; guard like the + // startup paths do. + if p.cfg != nil && len(p.cfg.Listener) > 0 { + if lc := p.cfg.FirstListener(); lc != nil { + if lc.IP != "" && lc.IP != "0.0.0.0" && lc.IP != "::" { + listenerIP = lc.IP + } + if lc.Port != 0 { + listenerPort = lc.Port + } + } + } + if listenerPort == 53 { + return listenerIP + } + if listenerIP == interceptDNSRdrTarget { + // Pathological config: the listener itself sits on the rdr target + // address (with a non-53 port). Pick a different loopback so the + // rdr's "! " still matches. + return "127.0.0.54" + } + return interceptDNSRdrTarget +} + +// hasIPv4DNS reports whether any of the given nameserver strings (bare IPs or +// host:port) is an IPv4 address. Loopback counts: an existing local resolver +// is treated conservatively as an intentional emittable DNS target; ctrld does +// not probe or replace another resolver's ownership. +func hasIPv4DNS(nameservers []string) bool { + for _, s := range nameservers { + host := s + if h, _, err := net.SplitHostPort(s); err == nil { + host = h + } + ip := net.ParseIP(host) + if ip == nil { + continue + } + if ip.To4() != nil { + return true + } + } + return false +} + +// needsInterceptDNSTarget reports whether the OS is left without any usable +// IPv4 DNS target: neither the default-route service's static DNS nor the +// discovered (DHCP/scutil) nameservers contain an IPv4 address. +// +// IPv6-only DNS is not usable under DNS intercept mode on macOS: the pf +// ruleset blocks all outbound IPv6 port-53 traffic (IPv6 interception is not +// supported, see issues #507/#533), and with no IPv4 DNS configured +// mDNSResponder emits no DNS packets at all — leaving pf nothing to +// intercept despite a healthy upstream. Observed in production on IPv6-only +// iPhone tethering with 464XLAT (issue #533). +func needsInterceptDNSTarget(staticDNS, discovered []string) bool { + return !hasIPv4DNS(staticDNS) && !hasIPv4DNS(discovered) +} + +// isInterceptDNSTargetOnly reports whether the given static DNS list is +// exactly the entry ctrld set via ensureInterceptDNSTarget (recorded in +// target), meaning it is safe for ctrld to remove. +func isInterceptDNSTargetOnly(nameservers []string, target string) bool { + return target != "" && len(nameservers) == 1 && nameservers[0] == target +} + +// filterOwnTarget returns nameservers with ctrld's own recorded target +// removed. A previously-set target must never be mistaken for user/network +// IPv4 DNS when judging whether the network still needs one — otherwise the +// second recovery on the same DNS-less network would see "IPv4 DNS present" +// and remove the entry, and the third would re-add it, oscillating on every +// recovery. +func filterOwnTarget(nameservers []string, target string) []string { + if target == "" { + return nameservers + } + out := nameservers[:0:0] + for _, s := range nameservers { + host := s + if h, _, err := net.SplitHostPort(s); err == nil { + host = h + } + if host != target { + out = append(out, s) + } + } + return out +} diff --git a/cmd/cli/dns_target_darwin.go b/cmd/cli/dns_target_darwin.go new file mode 100644 index 0000000..e39292c --- /dev/null +++ b/cmd/cli/dns_target_darwin.go @@ -0,0 +1,234 @@ +//go:build darwin + +package cli + +import ( + "encoding/json" + "net" + "os" + + "tailscale.com/net/netmon" + + "github.com/Control-D-Inc/ctrld" +) + +// interceptDNSTargetStateFile persists which service/value ctrld set, so a +// daemon restart (crash, upgrade, plain restart) does not orphan the entry: +// without it a restarted daemon would not know the entry is ctrld's own and +// could neither remove it on shutdown nor keep its bookkeeping consistent. +const interceptDNSTargetStateFile = ".intercept_dns_target" + +var ( + interceptDNSTargetStatePathFn = func() string { return absHomeDir(interceptDNSTargetStateFile) } + interceptDefaultRouteInterfaceFn = netmon.DefaultRouteInterface + interceptInterfaceByNameFn = net.InterfaceByName + interceptPatchNetIfaceNameFn = patchNetIfaceName + interceptCurrentStaticDNSFn = currentStaticDNS + interceptSaveCurrentStaticDNSFn = saveCurrentStaticDNS + interceptSetDNSFn = setDNS + interceptSavedStaticNameserversFn = savedStaticNameservers + interceptResetDNSIgnoreUnusableIfaceFn = resetDnsIgnoreUnusableInterface + interceptDHCPNameserversForInterfaceFn = ctrld.DHCPNameserversForInterface +) + +type interceptDNSTargetState struct { + Service string `json:"service"` + Value string `json:"value"` +} + +// loadInterceptDNSTargetStateLocked hydrates in-memory tracking from the +// state file once (only when memory is empty). Callers must hold +// interceptDNSTargetMu. +func (p *prog) loadInterceptDNSTargetStateLocked() { + if p.interceptDNSTargetService != "" || p.interceptDNSTargetLoaded { + return + } + p.interceptDNSTargetLoaded = true + data, err := os.ReadFile(interceptDNSTargetStatePathFn()) + if err != nil { + return + } + var st interceptDNSTargetState + if err := json.Unmarshal(data, &st); err != nil || st.Service == "" || st.Value == "" { + return + } + p.interceptDNSTargetService = st.Service + p.interceptDNSTargetSetValue = st.Value + mainLog.Load().Debug().Msgf("intercept DNS target: restored tracking of %s on %q from previous run", st.Value, st.Service) +} + +// persistInterceptDNSTargetStateLocked writes (or clears) the state file to +// match in-memory tracking. Callers must hold interceptDNSTargetMu. +func (p *prog) persistInterceptDNSTargetStateLocked() { + file := interceptDNSTargetStatePathFn() + if p.interceptDNSTargetService == "" { + _ = os.Remove(file) + return + } + data, err := json.Marshal(interceptDNSTargetState{Service: p.interceptDNSTargetService, Value: p.interceptDNSTargetSetValue}) + if err != nil { + return + } + if err := os.WriteFile(file, data, 0600); err != nil { + mainLog.Load().Debug().Err(err).Msg("intercept DNS target: could not persist state file") + } +} + +// ensureInterceptDNSTarget guarantees macOS always has an emittable DNS +// target while DNS intercept mode is active. +// +// Intercept mode deliberately never manages interface DNS: pf redirects DNS +// packets in flight. But pf can only redirect packets macOS actually sends, +// and mDNSResponder emits none when the active network service has no DNS +// configured. IPv6-only networks (e.g. iPhone tethering with 464XLAT) supply +// no IPv4 DNS, and the pf ruleset blocks all outbound IPv6 port 53, so such +// networks otherwise end in a total DNS outage with a healthy upstream +// (issue #533). +// +// Only when the default-route service has no usable IPv4 DNS at all does +// ctrld set a loopback DNS value on it — chosen by interceptDNSTargetValue to +// respect the configured listener: the listener IP directly when it serves +// port 53, else a distinct loopback address so the pf lo0 rdr rule rewrites +// to the listener's real port. The entry is removed when the network regains +// IPv4 DNS and on intercept shutdown. Networks that provide IPv4 DNS are +// never modified. +// +// Callers pass a non-nil raw system discovery result to prove discovery ran; +// an empty slice is a valid DNS-less result. The decision itself uses static +// DNS plus DHCP option 6 from the default-route interface, so resolvers on a +// second physical interface cannot suppress the target. Invoked during +// startup, debounced network recovery, and periodic pf watchdog reconciliation. +func (p *prog) ensureInterceptDNSTarget(systemDiscovery []string) { + if !dnsIntercept || p.dnsInterceptState == nil { + return + } + if systemDiscovery == nil { + mainLog.Load().Debug().Msg("intercept DNS target: system DNS discovery was not performed; not changing DNS") + return + } + p.interceptDNSTargetMu.Lock() + defer p.interceptDNSTargetMu.Unlock() + p.loadInterceptDNSTargetStateLocked() + + drIfaceName, err := interceptDefaultRouteInterfaceFn() + if err != nil || drIfaceName == "" { + // Mid-transition with no default route; the next recovery decides. + return + } + iface, err := interceptInterfaceByNameFn(drIfaceName) + if err != nil || iface == nil { + return + } + // Resolve the network service name (e.g. en5 -> "iPhone USB") so + // networksetup operates on the right service. + if _, err := interceptPatchNetIfaceNameFn(iface); err != nil { + mainLog.Load().Debug().Err(err).Msgf("intercept DNS target: could not resolve network service for %s", drIfaceName) + return + } + + staticDNS, err := interceptCurrentStaticDNSFn(iface) + if err != nil { + // Interfaces without a network service (utun/VPN tunnels) land here: + // networksetup cannot address them, ctrld never writes to them, and + // any target set on the underlying physical service stays in place — + // still correct while ctrld runs. + mainLog.Load().Debug().Err(err).Msgf("intercept DNS target: could not read static DNS for %q", iface.Name) + return + } + // Never count ctrld's own previously-set entry as network-provided DNS, + // or the next recovery on the same DNS-less network would remove it and + // the one after re-add it. + if p.interceptDNSTargetService == iface.Name { + staticDNS = filterOwnTarget(staticDNS, p.interceptDNSTargetSetValue) + } + if hasIPv4DNS(staticDNS) { + p.removeInterceptDNSTargetLocked("network has usable static IPv4 DNS") + return + } + + routeDHCPDNS, err := interceptDHCPNameserversForInterfaceFn(drIfaceName) + if err != nil { + mainLog.Load().Debug().Err(err).Msgf("intercept DNS target: could not read DHCP DNS for default-route service %q", iface.Name) + return + } + if hasIPv4DNS(routeDHCPDNS) { + // The default-route service regained DHCP option 6. Remove a target + // previously set on this or another service. + p.removeInterceptDNSTargetLocked("network has usable DHCP IPv4 DNS") + return + } + + target := p.interceptDNSTargetValue() + if p.interceptDNSTargetService == iface.Name && p.interceptDNSTargetSetValue == target { + return // already set on this service + } + // Default route moved to a different DNS-less service (or the listener + // config changed): clear the stale entry first. + p.removeInterceptDNSTargetLocked("default route service changed") + + // Preserve any existing (IPv6-only) static entries for later restore. + // saveCurrentStaticDNS filters loopback on write, and + // savedStaticNameservers filters loopback on read, so ctrld's own + // loopback target can never be recorded or restored as user DNS. + if err := interceptSaveCurrentStaticDNSFn(iface); err != nil { + mainLog.Load().Debug().Err(err).Msgf("intercept DNS target: could not save static DNS for %q", iface.Name) + } + if err := interceptSetDNSFn(iface, []string{target}); err != nil { + mainLog.Load().Warn().Err(err).Msgf("intercept DNS target: could not set %s on %q", target, iface.Name) + return + } + p.interceptDNSTargetService = iface.Name + p.interceptDNSTargetSetValue = target + p.persistInterceptDNSTargetStateLocked() + mainLog.Load().Warn().Msgf("intercept DNS target: service %q provides no usable IPv4 DNS; set %s so macOS can emit DNS queries (removed automatically when the network provides IPv4 DNS)", iface.Name, target) +} + +// removeInterceptDNSTarget removes a previously set intercept DNS target, +// restoring the service's saved static DNS (or empty). Safe no-op when no +// target was set. +func (p *prog) removeInterceptDNSTarget(reason string) { + p.interceptDNSTargetMu.Lock() + defer p.interceptDNSTargetMu.Unlock() + p.loadInterceptDNSTargetStateLocked() + p.removeInterceptDNSTargetLocked(reason) +} + +// removeInterceptDNSTargetLocked is removeInterceptDNSTarget without locking; +// callers must hold interceptDNSTargetMu. +func (p *prog) removeInterceptDNSTargetLocked(reason string) { + svc := p.interceptDNSTargetService + val := p.interceptDNSTargetSetValue + if svc == "" { + return + } + iface := &net.Interface{Name: svc} + // Only remove what ctrld set. If the service's DNS changed externally, + // leave that value alone and discard our stale ownership record. + cur, err := interceptCurrentStaticDNSFn(iface) + if err != nil { + mainLog.Load().Debug().Err(err).Msgf("intercept DNS target: could not read %q DNS; retaining cleanup state (%s)", svc, reason) + return + } + if !isInterceptDNSTargetOnly(cur, val) { + mainLog.Load().Debug().Msgf("intercept DNS target: %q DNS changed externally; not removing (%s)", svc, reason) + p.clearInterceptDNSTargetStateLocked() + return + } + if saved := interceptSavedStaticNameserversFn(iface); len(saved) > 0 { + if err := interceptSetDNSFn(iface, saved); err != nil { + mainLog.Load().Warn().Err(err).Msgf("intercept DNS target: could not restore saved DNS on %q; retaining cleanup state", svc) + return + } + } else if err := interceptResetDNSIgnoreUnusableIfaceFn(iface); err != nil { + mainLog.Load().Warn().Err(err).Msgf("intercept DNS target: could not reset DNS on %q; retaining cleanup state", svc) + return + } + p.clearInterceptDNSTargetStateLocked() + mainLog.Load().Info().Msgf("intercept DNS target: removed %s from %q (%s)", val, svc, reason) +} + +func (p *prog) clearInterceptDNSTargetStateLocked() { + p.interceptDNSTargetService = "" + p.interceptDNSTargetSetValue = "" + p.persistInterceptDNSTargetStateLocked() +} diff --git a/cmd/cli/dns_target_darwin_test.go b/cmd/cli/dns_target_darwin_test.go new file mode 100644 index 0000000..6f37a80 --- /dev/null +++ b/cmd/cli/dns_target_darwin_test.go @@ -0,0 +1,248 @@ +//go:build darwin + +package cli + +import ( + "errors" + "net" + "os" + "path/filepath" + "slices" + "testing" + + "github.com/Control-D-Inc/ctrld" +) + +type interceptTargetHarness struct { + dns map[string][]string + saved map[string][]string + serviceByDev map[string]string + dhcp []string + dhcpErr error + readErr error + setErr error + resetErr error + setCalls []string + resetCalls []string + statePath string +} + +func newInterceptTargetHarness(t *testing.T) *interceptTargetHarness { + t.Helper() + h := &interceptTargetHarness{ + dns: make(map[string][]string), + saved: make(map[string][]string), + serviceByDev: map[string]string{"en1": "Wi-Fi"}, + statePath: filepath.Join(t.TempDir(), interceptDNSTargetStateFile), + } + + origPath := interceptDNSTargetStatePathFn + origRoute := interceptDefaultRouteInterfaceFn + origIface := interceptInterfaceByNameFn + origPatch := interceptPatchNetIfaceNameFn + origCurrent := interceptCurrentStaticDNSFn + origSave := interceptSaveCurrentStaticDNSFn + origSet := interceptSetDNSFn + origSaved := interceptSavedStaticNameserversFn + origReset := interceptResetDNSIgnoreUnusableIfaceFn + origDHCP := interceptDHCPNameserversForInterfaceFn + origIntercept := dnsIntercept + t.Cleanup(func() { + interceptDNSTargetStatePathFn = origPath + interceptDefaultRouteInterfaceFn = origRoute + interceptInterfaceByNameFn = origIface + interceptPatchNetIfaceNameFn = origPatch + interceptCurrentStaticDNSFn = origCurrent + interceptSaveCurrentStaticDNSFn = origSave + interceptSetDNSFn = origSet + interceptSavedStaticNameserversFn = origSaved + interceptResetDNSIgnoreUnusableIfaceFn = origReset + interceptDHCPNameserversForInterfaceFn = origDHCP + dnsIntercept = origIntercept + }) + + dnsIntercept = true + interceptDNSTargetStatePathFn = func() string { return h.statePath } + interceptDefaultRouteInterfaceFn = func() (string, error) { return "en1", nil } + interceptInterfaceByNameFn = func(name string) (*net.Interface, error) { return &net.Interface{Name: name}, nil } + interceptPatchNetIfaceNameFn = func(iface *net.Interface) (bool, error) { + service, ok := h.serviceByDev[iface.Name] + if !ok { + return false, errors.New("unknown network service") + } + iface.Name = service + return true, nil + } + interceptCurrentStaticDNSFn = func(iface *net.Interface) ([]string, error) { + if h.readErr != nil { + return nil, h.readErr + } + return slices.Clone(h.dns[iface.Name]), nil + } + interceptSaveCurrentStaticDNSFn = func(iface *net.Interface) error { + h.saved[iface.Name] = slices.Clone(h.dns[iface.Name]) + return nil + } + interceptSetDNSFn = func(iface *net.Interface, nameservers []string) error { + h.setCalls = append(h.setCalls, iface.Name) + if h.setErr != nil { + return h.setErr + } + h.dns[iface.Name] = slices.Clone(nameservers) + return nil + } + interceptSavedStaticNameserversFn = func(iface *net.Interface) []string { + return slices.Clone(h.saved[iface.Name]) + } + interceptResetDNSIgnoreUnusableIfaceFn = func(iface *net.Interface) error { + h.resetCalls = append(h.resetCalls, iface.Name) + if h.resetErr != nil { + return h.resetErr + } + h.dns[iface.Name] = nil + return nil + } + interceptDHCPNameserversForInterfaceFn = func(iface string) ([]string, error) { + if iface != "en1" { + return nil, errors.New("DHCP lookup used a non-default interface") + } + return slices.Clone(h.dhcp), h.dhcpErr + } + return h +} + +func newInterceptTargetProg() *prog { + return &prog{ + cfg: &ctrld.Config{Listener: map[string]*ctrld.ListenerConfig{ + "0": {IP: "127.0.0.1", Port: 5354}, + }}, + dnsInterceptState: &interceptStateStub{}, + } +} + +func persistInterceptTargetForTest(t *testing.T, p *prog, service, value string) { + t.Helper() + p.interceptDNSTargetMu.Lock() + defer p.interceptDNSTargetMu.Unlock() + p.interceptDNSTargetLoaded = true + p.interceptDNSTargetService = service + p.interceptDNSTargetSetValue = value + p.persistInterceptDNSTargetStateLocked() +} + +func TestEnsureInterceptDNSTargetRequiresCompletedDiscovery(t *testing.T) { + h := newInterceptTargetHarness(t) + p := newInterceptTargetProg() + p.ensureInterceptDNSTarget(nil) + if len(h.setCalls) != 0 || len(h.resetCalls) != 0 { + t.Fatal("nil system discovery changed service DNS") + } +} + +func TestEnsureInterceptDNSTargetMigratesService(t *testing.T) { + h := newInterceptTargetHarness(t) + p := newInterceptTargetProg() + persistInterceptTargetForTest(t, p, "iPhone USB", "127.0.0.53") + h.dns["iPhone USB"] = []string{"127.0.0.53"} + h.dns["Wi-Fi"] = nil + + p.ensureInterceptDNSTarget([]string{}) + + if len(h.dns["iPhone USB"]) != 0 { + t.Fatalf("old service DNS = %v, want empty", h.dns["iPhone USB"]) + } + if got := h.dns["Wi-Fi"]; !slices.Equal(got, []string{"127.0.0.53"}) { + t.Fatalf("new service DNS = %v, want [127.0.0.53]", got) + } + if p.interceptDNSTargetService != "Wi-Fi" || p.interceptDNSTargetSetValue != "127.0.0.53" { + t.Fatalf("tracking = %q/%q, want Wi-Fi/127.0.0.53", p.interceptDNSTargetService, p.interceptDNSTargetSetValue) + } +} + +func TestEnsureInterceptDNSTargetUsesDefaultRouteDHCPOnly(t *testing.T) { + t.Run("other interface IPv4 does not suppress target", func(t *testing.T) { + h := newInterceptTargetHarness(t) + p := newInterceptTargetProg() + p.ensureInterceptDNSTarget([]string{"10.10.10.1"}) + if got := h.dns["Wi-Fi"]; !slices.Equal(got, []string{"127.0.0.53"}) { + t.Fatalf("other interface DNS suppressed target: %v", got) + } + }) + + t.Run("returned default route DHCP removes target", func(t *testing.T) { + h := newInterceptTargetHarness(t) + p := newInterceptTargetProg() + persistInterceptTargetForTest(t, p, "Wi-Fi", "127.0.0.53") + h.dns["Wi-Fi"] = []string{"127.0.0.53"} + h.dhcp = []string{"192.168.10.1"} + + p.ensureInterceptDNSTarget([]string{"10.10.10.1"}) + + if len(h.dns["Wi-Fi"]) != 0 || p.interceptDNSTargetService != "" { + t.Fatalf("returned default-route DHCP DNS did not remove target: dns=%v service=%q", h.dns["Wi-Fi"], p.interceptDNSTargetService) + } + }) +} + +func TestRemoveInterceptDNSTargetRestoresStateFileAfterRestart(t *testing.T) { + h := newInterceptTargetHarness(t) + h.dns["iPhone USB"] = []string{"127.0.0.53"} + if err := os.WriteFile(h.statePath, []byte(`{"service":"iPhone USB","value":"127.0.0.53"}`), 0600); err != nil { + t.Fatal(err) + } + p := newInterceptTargetProg() + + p.removeInterceptDNSTarget("intercept mode inactive") + + if len(h.dns["iPhone USB"]) != 0 || p.interceptDNSTargetService != "" { + t.Fatalf("restart cleanup failed: dns=%v service=%q", h.dns["iPhone USB"], p.interceptDNSTargetService) + } + if _, err := os.Stat(h.statePath); !os.IsNotExist(err) { + t.Fatalf("state file still exists after cleanup: %v", err) + } +} + +func TestRemoveInterceptDNSTargetKeepsExternalDNS(t *testing.T) { + h := newInterceptTargetHarness(t) + p := newInterceptTargetProg() + persistInterceptTargetForTest(t, p, "Wi-Fi", "127.0.0.53") + h.dns["Wi-Fi"] = []string{"8.8.8.8"} + + p.removeInterceptDNSTarget("test") + + if !slices.Equal(h.dns["Wi-Fi"], []string{"8.8.8.8"}) || len(h.setCalls) != 0 || len(h.resetCalls) != 0 { + t.Fatalf("external DNS was changed: dns=%v set=%v reset=%v", h.dns["Wi-Fi"], h.setCalls, h.resetCalls) + } + if p.interceptDNSTargetService != "" { + t.Fatal("external change left stale ownership tracking") + } +} + +func TestRemoveInterceptDNSTargetRetainsStateOnFailure(t *testing.T) { + for _, tc := range []struct { + name string + readErr error + resetErr error + }{ + {"read failure", errors.New("networksetup read failed"), nil}, + {"restore failure", nil, errors.New("networksetup reset failed")}, + } { + t.Run(tc.name, func(t *testing.T) { + h := newInterceptTargetHarness(t) + p := newInterceptTargetProg() + persistInterceptTargetForTest(t, p, "Wi-Fi", "127.0.0.53") + h.dns["Wi-Fi"] = []string{"127.0.0.53"} + h.readErr = tc.readErr + h.resetErr = tc.resetErr + + p.removeInterceptDNSTarget("test") + + if p.interceptDNSTargetService != "Wi-Fi" || p.interceptDNSTargetSetValue != "127.0.0.53" { + t.Fatal("failed cleanup discarded retry state") + } + if _, err := os.Stat(h.statePath); err != nil { + t.Fatalf("failed cleanup removed persisted retry state: %v", err) + } + }) + } +} diff --git a/cmd/cli/dns_target_filter_test.go b/cmd/cli/dns_target_filter_test.go new file mode 100644 index 0000000..88b0a9d --- /dev/null +++ b/cmd/cli/dns_target_filter_test.go @@ -0,0 +1,57 @@ +package cli + +import "testing" + +func TestFilterOwnTarget(t *testing.T) { + tests := []struct { + name string + in []string + target string + wantLen int + }{ + // The oscillation guard (MR !997 review): the second recovery on the + // same DNS-less network must not count ctrld's own entry as + // network-provided IPv4 DNS. + {"removes own entry", []string{"127.0.0.1"}, "127.0.0.1", 0}, + {"removes own entry with resolver port", []string{"127.0.0.53:53"}, "127.0.0.53", 0}, + {"keeps user entries", []string{"127.0.0.1", "1.1.1.1"}, "127.0.0.1", 1}, + {"empty target keeps all", []string{"127.0.0.1"}, "", 1}, + {"no match keeps all", []string{"1.1.1.1"}, "127.0.0.53", 1}, + {"nil input", nil, "127.0.0.1", 0}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := filterOwnTarget(tc.in, tc.target) + if len(got) != tc.wantLen { + t.Errorf("filterOwnTarget(%v, %q) = %v, want len %d", tc.in, tc.target, got, tc.wantLen) + } + for _, s := range got { + if tc.target != "" && s == tc.target { + t.Errorf("filterOwnTarget(%v, %q) retained the target entry", tc.in, tc.target) + } + } + }) + } +} + +// TestFilterOwnTargetStability pins the recovery-cycle contract: on a +// DNS-less network where ctrld already set its target, needsInterceptDNSTarget +// over the filtered list must still report true (entry kept, no oscillation), +// while a genuine user-added IPv4 server must report false (entry removed). +func TestFilterOwnTargetStability(t *testing.T) { + target := "127.0.0.1" + + // Second recovery, same tether: only our own entry present. The OS resolver + // reports it with :53, while networksetup reports the bare address. + static := filterOwnTarget([]string{target}, target) + discovered := filterOwnTarget([]string{target + ":53"}, target) + if !needsInterceptDNSTarget(static, discovered) { + t.Error("second recovery on the same DNS-less network would remove the target (oscillation)") + } + + // User manually added a public server meanwhile: target no longer needed. + static = filterOwnTarget([]string{target, "1.1.1.1"}, target) + if needsInterceptDNSTarget(static, nil) { + t.Error("user-added IPv4 DNS not recognized; target would be kept unnecessarily") + } +} diff --git a/cmd/cli/dns_target_stub.go b/cmd/cli/dns_target_stub.go new file mode 100644 index 0000000..680ede3 --- /dev/null +++ b/cmd/cli/dns_target_stub.go @@ -0,0 +1,14 @@ +//go:build !darwin + +package cli + +// ensureInterceptDNSTarget is a no-op on non-Darwin platforms: the DNS-less +// network problem it solves is specific to macOS pf interception blocking +// IPv6 port 53 with no IPv4 fallback (issue #533). Windows intercept mode +// uses NRPT, which routes queries regardless of adapter DNS configuration. +func (p *prog) ensureInterceptDNSTarget(_ []string) {} + +// removeInterceptDNSTarget is a no-op on non-Darwin platforms. +// +//lint:ignore U1000 called from Darwin-only intercept shutdown; kept for API symmetry. +func (p *prog) removeInterceptDNSTarget(_ string) {} diff --git a/cmd/cli/dns_target_test.go b/cmd/cli/dns_target_test.go new file mode 100644 index 0000000..6a5ab63 --- /dev/null +++ b/cmd/cli/dns_target_test.go @@ -0,0 +1,113 @@ +package cli + +import ( + "testing" + + "github.com/Control-D-Inc/ctrld" +) + +func TestHasIPv4DNS(t *testing.T) { + tests := []struct { + name string + in []string + want bool + }{ + {"empty", nil, false}, + {"ipv4", []string{"8.8.8.8"}, true}, + {"ipv4 with port", []string{"192.168.1.1:53"}, true}, + {"loopback counts", []string{"127.0.0.1"}, true}, + {"ipv6 only", []string{"2001:4860:4860::8888"}, false}, + {"ipv6 with port", []string{"[2001:4860:4860::8888]:53"}, false}, + {"mixed", []string{"2001:4860:4860::8888", "9.9.9.9"}, true}, + {"garbage ignored", []string{"not-an-ip", ""}, false}, + {"garbage plus v4", []string{"not-an-ip", "1.1.1.1"}, true}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := hasIPv4DNS(tc.in); got != tc.want { + t.Errorf("hasIPv4DNS(%v) = %v, want %v", tc.in, got, tc.want) + } + }) + } +} + +func TestNeedsInterceptDNSTarget(t *testing.T) { + tests := []struct { + name string + static, discovered []string + want bool + }{ + {"no dns at all", nil, nil, true}, + {"ipv6-only tether (464XLAT, issue #533)", nil, []string{"2605:8d80::1"}, true}, + {"static v4 present", []string{"1.1.1.1"}, nil, false}, + {"discovered v4 present", nil, []string{"192.168.1.1:53"}, false}, + {"existing ctrld target satisfies", []string{"127.0.0.1"}, nil, false}, + {"ipv6 static, v4 discovered", []string{"2001:db8::1"}, []string{"10.0.0.1"}, false}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := needsInterceptDNSTarget(tc.static, tc.discovered); got != tc.want { + t.Errorf("needsInterceptDNSTarget(%v, %v) = %v, want %v", tc.static, tc.discovered, got, tc.want) + } + }) + } +} + +func TestIsInterceptDNSTargetOnly(t *testing.T) { + tests := []struct { + name string + in []string + target string + want bool + }{ + {"exactly ours (direct listener)", []string{"127.0.0.1"}, "127.0.0.1", true}, + {"exactly ours (rdr target)", []string{"127.0.0.53"}, "127.0.0.53", true}, + {"empty list", nil, "127.0.0.1", false}, + {"empty target never matches", []string{"127.0.0.1"}, "", false}, + {"ours plus user entry", []string{"127.0.0.1", "1.1.1.1"}, "127.0.0.1", false}, + {"user entry only", []string{"1.1.1.1"}, "127.0.0.1", false}, + {"different loopback than ours", []string{"127.0.0.53"}, "127.0.0.1", false}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := isInterceptDNSTargetOnly(tc.in, tc.target); got != tc.want { + t.Errorf("isInterceptDNSTargetOnly(%v, %q) = %v, want %v", tc.in, tc.target, got, tc.want) + } + }) + } +} + +func TestInterceptDNSTargetValue(t *testing.T) { + tests := []struct { + name string + ip string + port int + want string + }{ + {"default direct listener :53", "127.0.0.1", 53, "127.0.0.1"}, + {"custom loopback listener :53", "127.0.0.2", 53, "127.0.0.2"}, + {"non-53 port uses rdr target", "127.0.0.1", 5354, "127.0.0.53"}, + {"listener on rdr target with non-53 port", "127.0.0.53", 5354, "127.0.0.54"}, + {"wildcard ip :53 falls back to loopback", "0.0.0.0", 53, "127.0.0.1"}, + {"wildcard ip non-53 uses rdr target", "0.0.0.0", 5354, "127.0.0.53"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + p := &prog{cfg: &ctrld.Config{ + Listener: map[string]*ctrld.ListenerConfig{ + "0": {IP: tc.ip, Port: tc.port}, + }, + }} + if got := p.interceptDNSTargetValue(); got != tc.want { + t.Errorf("interceptDNSTargetValue() with listener %s:%d = %q, want %q", tc.ip, tc.port, got, tc.want) + } + }) + } +} + +func TestInterceptDNSTargetValue_NoListener(t *testing.T) { + p := &prog{cfg: &ctrld.Config{}} + if got := p.interceptDNSTargetValue(); got != "127.0.0.1" { + t.Errorf("interceptDNSTargetValue() with no listener = %q, want 127.0.0.1", got) + } +} diff --git a/cmd/cli/intercept_mode_config_test.go b/cmd/cli/intercept_mode_config_test.go index a3167eb..9d0fe4d 100644 --- a/cmd/cli/intercept_mode_config_test.go +++ b/cmd/cli/intercept_mode_config_test.go @@ -36,3 +36,30 @@ func TestUpdateConfigInterceptMode(t *testing.T) { }) } } + +func TestConfiguredInterceptMode(t *testing.T) { + oldInterceptMode := interceptMode + t.Cleanup(func() { interceptMode = oldInterceptMode }) + + p := &prog{cfg: &ctrld.Config{}} + p.cfg.Service.InterceptMode = "dns" + + tests := []struct { + name string + flag string + want string + }{ + {name: "empty flag falls back to config", flag: "", want: "dns"}, + {name: "explicit off is final", flag: "off", want: "off"}, + {name: "explicit hard wins over config", flag: "hard", want: "hard"}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + interceptMode = tc.flag + if got := p.configuredInterceptMode(); got != tc.want { + t.Fatalf("configuredInterceptMode() = %q, want %q", got, tc.want) + } + }) + } +} diff --git a/cmd/cli/main.go b/cmd/cli/main.go index 762777d..7e6dd8d 100644 --- a/cmd/cli/main.go +++ b/cmd/cli/main.go @@ -42,7 +42,7 @@ var ( cleanup bool startOnly bool rfc1918 bool - interceptMode string // "", "dns", or "hard" — set via --intercept-mode flag or config + interceptMode string // "", "off", "dns", or "hard" — set via --intercept-mode flag or config dnsIntercept bool // derived: interceptMode == "dns" || interceptMode == "hard" hardIntercept bool // derived: interceptMode == "hard" diff --git a/cmd/cli/prog.go b/cmd/cli/prog.go index a51417c..1c0fb29 100644 --- a/cmd/cli/prog.go +++ b/cmd/cli/prog.go @@ -156,6 +156,10 @@ type prog struct { recoveryCancelMu sync.Mutex recoveryCancel context.CancelFunc recoveryRunning atomic.Bool + // recoveryGen counts handleRecovery invocations that reached the + // recovery-context stage; each recovery captures its own generation and + // only touches shared recovery state if it is still the newest (#597). + recoveryGen atomic.Uint64 // recoveryDebounceTimer coalesces rapid NetworkChange recovery triggers // into a single handleRecovery call. Only handleRecovery is debounced — @@ -168,6 +172,21 @@ type prog struct { // instead of using the normal upstream flow. recoveryBypass atomic.Bool + // interceptDNSTargetService names the macOS network service on which + // ctrld set a loopback DNS value because the service provided no usable + // IPv4 DNS while DNS intercept mode was active (issue #533); + // interceptDNSTargetSetValue records the exact value set. Both empty when + // no target is set. Guarded by interceptDNSTargetMu. + // + //lint:ignore U1000 used in Darwin code. + interceptDNSTargetMu sync.Mutex + //lint:ignore U1000 used in Darwin code. + interceptDNSTargetService string + //lint:ignore U1000 used in Darwin code. + interceptDNSTargetSetValue string + //lint:ignore U1000 used in Darwin code. + interceptDNSTargetLoaded bool + // DNS intercept mode state (platform-specific). // On Windows: *wfpState, on macOS: *pfState, nil on other platforms. dnsInterceptState any @@ -453,9 +472,9 @@ func (p *prog) postRun() { if !p.skipInitialDNSReset() { p.resetDNS(false, false) } - ns := ctrld.InitializeOsResolver(false) + ns, systemNameservers := initializeOsResolverWithSystemNameserversFn(false) mainLog.Load().Debug().Msgf("initialized OS resolver with nameservers: %v", ns) - p.setDNS() + p.setDNS(systemNameservers) p.csSetDnsDone <- struct{}{} close(p.csSetDnsDone) p.logInterfacesState() @@ -891,11 +910,14 @@ func (p *prog) deAllocateIP() error { // NRPT rule, so a test of what happens *after* it fails must not be the thing that // runs it. var ( - localResolverIPFn = router.LocalResolverIP - startDNSInterceptFn = (*prog).startDNSIntercept - setDnsForRunningIfaceFn = (*prog).setDnsForRunningIface - resetDNSFn = (*prog).resetDNS - refuseFallbackFatal = func(format string, v ...any) { + localResolverIPFn = router.LocalResolverIP + startDNSInterceptFn = (*prog).startDNSIntercept + ensureInterceptDNSTargetFn = (*prog).ensureInterceptDNSTarget + removeInterceptDNSTargetFn = (*prog).removeInterceptDNSTarget + initializeOsResolverWithSystemNameserversFn = ctrld.InitializeOsResolverWithSystemNameservers + setDnsForRunningIfaceFn = (*prog).setDnsForRunningIface + resetDNSFn = (*prog).resetDNS + refuseFallbackFatal = func(format string, v ...any) { mainLog.Load().Fatal().Msgf(format, v...) } ) @@ -922,7 +944,7 @@ func interfaceDNSFallbackViable(lc *ctrld.ListenerConfig, localResolverIP string return lc == nil || lc.Port == 0 || lc.Port == 53 || localResolverIP != "" } -func (p *prog) setDNS() { +func (p *prog) setDNS(systemNameservers []string) { setDnsOK := false defer func() { p.csSetDnsOk = setDnsOK @@ -956,6 +978,7 @@ func (p *prog) setDNS() { // software that also manages DNS. See issue #489. if dnsIntercept { if err := startDNSInterceptFn(p); err != nil { + removeInterceptDNSTargetFn(p, "DNS intercept unavailable") // This check comes first: it is the one failure where DNS already works // without ctrld touching anything else, so neither the refusal below nor the // fallback applies. @@ -1006,6 +1029,12 @@ func (p *prog) setDNS() { mainLog.Load().Error().Err(err).Msg("DNS intercept mode failed — falling back to interface DNS settings") // Fall through to traditional setDNS behavior. } else { + // Intercept installation alone is insufficient on a DNS-less network: + // without an IPv4 DNS target macOS emits no packet for pf to redirect. + // Do this on startup as well as network-change recovery so starting or + // restarting while already tethered cannot leave DNS offline. + ensureInterceptDNSTargetFn(p, systemNameservers) + if hardIntercept { mainLog.Load().Info().Msg("Hard intercept mode active — all DNS through ctrld, no VPN split routing") } else { @@ -1023,6 +1052,9 @@ func (p *prog) setDNS() { return } } + if !dnsIntercept { + removeInterceptDNSTargetFn(p, "intercept mode inactive") + } if cfg.Listener == nil { return @@ -1091,11 +1123,11 @@ func (p *prog) setDNS() { } // configuredInterceptMode resolves the service's effective intercept mode without -// mutating package state. Platform startup preflights use the same precedence as -// setDNS so they do not make adapter-DNS decisions from a different mode value. +// mutating package state. An explicit flag value, including "off", takes priority +// over the persisted config value. func (p *prog) configuredInterceptMode() string { im := interceptMode - if im == "" || im == "off" { + if im == "" { im = p.cfg.Service.InterceptMode } return im @@ -1258,6 +1290,10 @@ func (p *prog) dnsWatchdog(iface *net.Interface, nameservers []string) { // resetDNS performs a DNS reset for all interfaces. // In DNS intercept mode, this tears down the WFP/pf filters instead. func (p *prog) resetDNS(isStart bool, restoreStatic bool) { + // A previous crash can leave a persisted macOS intercept target even when + // no live interceptor state exists. Cleanup must run for stop/uninstall and + // traditional-mode startup as well as the normal intercept shutdown path. + removeInterceptDNSTargetFn(p, "DNS reset") if dnsIntercept && p.dnsInterceptState != nil { if err := p.stopDNSIntercept(); err != nil { mainLog.Load().Error().Err(err).Msg("Failed to stop DNS intercept mode during reset") diff --git a/cmd/cli/prog_intercept_fallback_test.go b/cmd/cli/prog_intercept_fallback_test.go index 568e788..0cf98ab 100644 --- a/cmd/cli/prog_intercept_fallback_test.go +++ b/cmd/cli/prog_intercept_fallback_test.go @@ -91,9 +91,12 @@ func TestInterfaceDNSFallbackViable(t *testing.T) { // than depending on the runner denying a privileged operation. type interceptFallbackHarness struct { interceptCalls int + ensureTargetCalls int + ensuredNameservers []string installedNameservers []string installCalls int resetCalls int + removeTargetCalls int refusals []string } @@ -101,12 +104,12 @@ func newInterceptFallbackHarness(t *testing.T, lc *ctrld.ListenerConfig) *interc t.Helper() h := &interceptFallbackHarness{} - origStart, origInstall := startDNSInterceptFn, setDnsForRunningIfaceFn + origStart, origEnsure, origRemove, origInstall := startDNSInterceptFn, ensureInterceptDNSTargetFn, removeInterceptDNSTargetFn, setDnsForRunningIfaceFn origReset, origFatal := resetDNSFn, refuseFallbackFatal origResolver := localResolverIPFn origCfg, origMode, origIntercept, origHard := cfg, interceptMode, dnsIntercept, hardIntercept t.Cleanup(func() { - startDNSInterceptFn, setDnsForRunningIfaceFn = origStart, origInstall + startDNSInterceptFn, ensureInterceptDNSTargetFn, removeInterceptDNSTargetFn, setDnsForRunningIfaceFn = origStart, origEnsure, origRemove, origInstall resetDNSFn, refuseFallbackFatal = origReset, origFatal localResolverIPFn = origResolver cfg, interceptMode, dnsIntercept, hardIntercept = origCfg, origMode, origIntercept, origHard @@ -121,6 +124,11 @@ func newInterceptFallbackHarness(t *testing.T, lc *ctrld.ListenerConfig) *interc h.interceptCalls++ return errors.New("dns intercept: injected start failure") } + ensureInterceptDNSTargetFn = func(_ *prog, nameservers []string) { + h.ensureTargetCalls++ + h.ensuredNameservers = slices.Clone(nameservers) + } + removeInterceptDNSTargetFn = func(_ *prog, _ string) { h.removeTargetCalls++ } setDnsForRunningIfaceFn = func(_ *prog, nameservers []string) *net.Interface { h.installCalls++ h.installedNameservers = nameservers @@ -143,7 +151,31 @@ func newInterceptFallbackHarness(t *testing.T, lc *ctrld.ListenerConfig) *interc func (h *interceptFallbackHarness) run(t *testing.T) { t.Helper() p := &prog{cfg: &cfg} - p.setDNS() + p.setDNS(nil) +} + +func TestSetDNSEnsuresInterceptTargetAfterSuccessfulStart(t *testing.T) { + h := newInterceptFallbackHarness(t, &ctrld.ListenerConfig{IP: "127.0.0.1", Port: 5354}) + startDNSInterceptFn = func(_ *prog) error { + h.interceptCalls++ + return nil + } + want := []string{"fe80::1"} + p := &prog{cfg: &cfg} + p.setDNS(want) + + if h.interceptCalls != 1 { + t.Fatalf("intercept start called %d time(s), want 1", h.interceptCalls) + } + if h.ensureTargetCalls != 1 { + t.Fatalf("intercept DNS target ensured %d time(s), want 1 after successful start", h.ensureTargetCalls) + } + if !slices.Equal(h.ensuredNameservers, want) { + t.Fatalf("system nameservers = %v, want %v", h.ensuredNameservers, want) + } + if h.installCalls != 0 { + t.Fatalf("interface-DNS fallback installed %d time(s) after successful intercept start", h.installCalls) + } } func TestSetDNSExplicitOffOverridesConfig(t *testing.T) { @@ -160,6 +192,9 @@ func TestSetDNSExplicitOffOverridesConfig(t *testing.T) { if h.installCalls != 1 { t.Fatalf("interface DNS installed %d time(s), want 1", h.installCalls) } + if h.removeTargetCalls != 1 { + t.Fatalf("stale intercept DNS target cleanup called %d time(s), want 1", h.removeTargetCalls) + } } // TestSetDNSRefusesUnreachableFallback is the behaviour test for the reported outage: it @@ -182,6 +217,9 @@ func TestSetDNSRefusesUnreachableFallback(t *testing.T) { if h.resetCalls == 0 { t.Error("host DNS was not restored before refusing, leaving the interface pointed at a ctrld that is not serving") } + if h.removeTargetCalls != 1 { + t.Errorf("stale intercept DNS target cleanup called %d time(s), want 1 after intercept failure", h.removeTargetCalls) + } if len(h.refusals) == 0 { t.Fatal("refusal was not surfaced: startup must fail loudly rather than silently skip the fallback") } diff --git a/cmd/cli/recovery_state.go b/cmd/cli/recovery_state.go new file mode 100644 index 0000000..fa2e9de --- /dev/null +++ b/cmd/cli/recovery_state.go @@ -0,0 +1,75 @@ +package cli + +import "context" + +// beginRecovery atomically transfers ownership of shared recovery state. A +// network change cancels and replaces the current owner without exposing a nil +// recoveryCancel gap; other triggers are coalesced while an owner exists. +func (p *prog) beginRecovery(reason RecoveryReason) (ctx context.Context, gen uint64, intercept bool, ok bool) { + p.recoveryCancelMu.Lock() + defer p.recoveryCancelMu.Unlock() + + if reason != RecoveryReasonNetworkChange && p.recoveryCancel != nil { + return nil, 0, false, false + } + if p.recoveryCancel != nil { + p.recoveryCancel() + } + + ctx, cancel := context.WithCancel(context.Background()) + gen = p.recoveryGen.Add(1) + intercept = dnsIntercept && p.dnsInterceptState != nil + p.recoveryCancel = cancel + p.recoveryRunning.Store(true) + p.recoveryBypass.Store(intercept) + return ctx, gen, intercept, true +} + +func (p *prog) recoveryOwnsState(gen uint64) bool { + p.recoveryCancelMu.Lock() + defer p.recoveryCancelMu.Unlock() + return p.recoveryGen.Load() == gen && p.recoveryCancel != nil +} + +func systemNameserversForInterceptRetry() []string { + _, system := initializeOsResolverWithSystemNameserversFn(true) + if system == nil { + return []string{} + } + return system +} + +// completeRecovery releases shared state only if gen still owns it. The bypass +// reset is unconditional because live intercept state can disappear while a +// recovery is running, but a stale true flag still affects proxy routing. +func (p *prog) completeRecovery(gen uint64) bool { + p.recoveryCancelMu.Lock() + defer p.recoveryCancelMu.Unlock() + if p.recoveryGen.Load() != gen || p.recoveryCancel == nil { + return false + } + p.recoveryBypass.Store(false) + p.recoveryRunning.Store(false) + p.recoveryCancel = nil + return true +} + +// recoveryCanceledCleanup resets shared recovery state after a canceled or +// failed recovery, but only when the recovery identified by gen was NOT +// superseded by a newer one (issue #597). +// +// A network-change cancellation is normally followed immediately by a new +// handleRecovery that owns recoveryBypass/recoveryRunning/recoveryCancel; +// clearing them here would disable the successor's bypass mid-flight and +// make it uncancellable. But when the canceled recovery is the LAST one +// (e.g. the tail of a network flap burst), nothing else will ever clear the +// flags: the daemon would stay in recovery bypass forever — every query +// detouring to the OS resolver — and the DNS-settings watchdog would stay +// permanently disabled. +func (p *prog) recoveryCanceledCleanup(gen uint64) { + if !p.completeRecovery(gen) { + // Superseded: the newer recovery owns the shared state. + return + } + mainLog.Load().Info().Msg("Recovery canceled with no successor; cleared recovery state and DHCP bypass") +} diff --git a/cmd/cli/recovery_state_test.go b/cmd/cli/recovery_state_test.go new file mode 100644 index 0000000..4e615ad --- /dev/null +++ b/cmd/cli/recovery_state_test.go @@ -0,0 +1,161 @@ +package cli + +import ( + "testing" + "time" +) + +// interceptStateStub stands in for the platform pfState/wfpState; the +// recovery cleanup path only checks dnsInterceptState != nil. +type interceptStateStub struct{} + +// setupInterceptRecovery puts p into "intercept-mode recovery in flight" +// state and restores the package-level dnsIntercept flag on cleanup. +func setupInterceptRecovery(t *testing.T, p *prog) { + t.Helper() + oldIntercept := dnsIntercept + dnsIntercept = true + t.Cleanup(func() { dnsIntercept = oldIntercept }) + p.dnsInterceptState = &interceptStateStub{} + p.recoveryBypass.Store(true) + p.recoveryRunning.Store(true) + p.recoveryCancel = func() {} +} + +// TestRecoveryCanceledCleanup_LastRecoveryResetsState pins issue #597: a +// canceled recovery with no successor must clear recoveryBypass and +// recoveryRunning, or the daemon stays in bypass forever (every query +// detours to the OS resolver) and the DNS watchdog stays disabled. +func TestRecoveryCanceledCleanup_LastRecoveryResetsState(t *testing.T) { + p := &prog{} + setupInterceptRecovery(t, p) + gen := p.recoveryGen.Add(1) + + p.recoveryCanceledCleanup(gen) + + if p.recoveryBypass.Load() { + t.Error("recoveryBypass still set after canceled recovery with no successor") + } + if p.recoveryRunning.Load() { + t.Error("recoveryRunning still set after canceled recovery with no successor") + } + p.recoveryCancelMu.Lock() + cancelCleared := p.recoveryCancel == nil + p.recoveryCancelMu.Unlock() + if !cancelCleared { + t.Error("recoveryCancel not cleared after canceled recovery with no successor") + } +} + +// TestRecoveryCanceledCleanup_SupersededKeepsSuccessorState pins the +// captive-portal/network-flap contract: when a newer recovery superseded the +// canceled one, the canceled recovery must NOT clear shared state — the +// successor owns bypass for its own duration. +func TestRecoveryCanceledCleanup_SupersededKeepsSuccessorState(t *testing.T) { + p := &prog{} + setupInterceptRecovery(t, p) + gen := p.recoveryGen.Add(1) + // A successor recovery started. + p.recoveryGen.Add(1) + + p.recoveryCanceledCleanup(gen) + + if !p.recoveryBypass.Load() { + t.Error("superseded canceled recovery cleared recoveryBypass owned by its successor") + } + if !p.recoveryRunning.Load() { + t.Error("superseded canceled recovery cleared recoveryRunning owned by its successor") + } + p.recoveryCancelMu.Lock() + cancelKept := p.recoveryCancel != nil + p.recoveryCancelMu.Unlock() + if !cancelKept { + t.Error("superseded canceled recovery cleared the successor's recoveryCancel") + } +} + +// TestRecoveryCanceledCleanup_NonInterceptResetsRunning covers traditional +// (non-intercept) mode: recoveryRunning must still be reset so watchdogs +// resume, while bypass is untouched (it is never set in that mode). +func TestRecoveryCanceledCleanup_NonInterceptResetsRunning(t *testing.T) { + oldIntercept := dnsIntercept + dnsIntercept = false + t.Cleanup(func() { dnsIntercept = oldIntercept }) + + p := &prog{} + p.recoveryRunning.Store(true) + p.recoveryCancel = func() {} + gen := p.recoveryGen.Add(1) + + p.recoveryCanceledCleanup(gen) + + if p.recoveryRunning.Load() { + t.Error("recoveryRunning still set after canceled non-intercept recovery") + } +} + +func TestBeginRecoveryTransfersOwnershipAtomically(t *testing.T) { + oldIntercept := dnsIntercept + dnsIntercept = true + t.Cleanup(func() { dnsIntercept = oldIntercept }) + + p := &prog{dnsInterceptState: &interceptStateStub{}} + firstCtx, firstGen, _, ok := p.beginRecovery(RecoveryReasonRegularFailure) + if !ok { + t.Fatal("first recovery did not acquire ownership") + } + if _, _, _, ok := p.beginRecovery(RecoveryReasonRegularFailure); ok { + t.Fatal("duplicate upstream recovery acquired ownership") + } + + _, successorGen, intercept, ok := p.beginRecovery(RecoveryReasonNetworkChange) + if !ok || !intercept || successorGen <= firstGen { + t.Fatalf("network recovery did not replace owner: first=%d successor=%d intercept=%v ok=%v", firstGen, successorGen, intercept, ok) + } + select { + case <-firstCtx.Done(): + case <-time.After(time.Second): + t.Fatal("successor did not cancel the previous recovery") + } + + p.recoveryCanceledCleanup(firstGen) + if !p.recoveryRunning.Load() || !p.recoveryBypass.Load() || !p.recoveryOwnsState(successorGen) { + t.Fatal("stale cleanup changed successor-owned recovery state") + } + if !p.completeRecovery(successorGen) { + t.Fatal("successor could not complete its own recovery state") + } +} + +func TestRecoveryCleanupClearsBypassAfterInterceptStateDisappears(t *testing.T) { + p := &prog{} + p.recoveryBypass.Store(true) + p.recoveryRunning.Store(true) + p.recoveryCancel = func() {} + gen := p.recoveryGen.Add(1) + + p.recoveryCanceledCleanup(gen) + if p.recoveryBypass.Load() || p.recoveryRunning.Load() { + t.Fatal("cleanup retained recovery flags after intercept state disappeared") + } +} + +func TestSystemNameserversForInterceptRetryNormalizesEmptyDiscovery(t *testing.T) { + original := initializeOsResolverWithSystemNameserversFn + called := false + initializeOsResolverWithSystemNameserversFn = func(guard bool) ([]string, []string) { + called = true + if !guard { + t.Error("intercept retry discovery did not guard the existing resolver") + } + return nil, nil + } + t.Cleanup(func() { initializeOsResolverWithSystemNameserversFn = original }) + + if got := systemNameserversForInterceptRetry(); got == nil || len(got) != 0 { + t.Fatalf("system discovery = %#v, want non-nil empty slice", got) + } + if !called { + t.Fatal("system discovery was not called") + } +} diff --git a/cmd/cli/service.go b/cmd/cli/service.go index 0cad94f..0925b41 100644 --- a/cmd/cli/service.go +++ b/cmd/cli/service.go @@ -162,10 +162,8 @@ func (s *systemd) Start() error { // This is necessary for running self-upgrade flow. func ensureSystemdKillMode(r io.Reader) (opts []*unit.UnitOption, change bool) { opts, err := unit.DeserializeOptions(r) - // staticcheck sees only the explicit non-nil sends on the lexer's error - // channel, so it reports this comparison as always true. On success the - // lexer sends nothing and closes the channel, so the receive yields a nil - // error and this branch is not taken. + // On success the lexer sends nothing and closes the channel, so the receive + // yields a nil error and this branch is not taken. if err != nil { mainLog.Load().Error().Err(err).Msg("failed to deserialize options") return diff --git a/cmd/cli/service_args_darwin_test.go b/cmd/cli/service_args_darwin_test.go index dc23b3b..1e535f0 100644 --- a/cmd/cli/service_args_darwin_test.go +++ b/cmd/cli/service_args_darwin_test.go @@ -15,6 +15,11 @@ func TestServiceArgumentPresent(t *testing.T) { if serviceArgumentPresent(out, "off") { t.Fatal("substring in an unrelated path was mistaken for the off argument") } + + splitOut := []byte("Array {\n /usr/local/bin/ctrld\n run\n --intercept-mode\n dns\n}\n") + if !serviceArgumentPresent(splitOut, "--intercept-mode") { + t.Fatal("standalone flag argument was not found") + } } func TestServiceFlagPosition(t *testing.T) { diff --git a/cmd/cli/service_args_windows.go b/cmd/cli/service_args_windows.go index 1eed7da..625da28 100644 --- a/cmd/cli/service_args_windows.go +++ b/cmd/cli/service_args_windows.go @@ -89,7 +89,7 @@ func verifyServiceRegistration() error { mainLog.Load().Debug().Msgf("Service registry: BinaryPathName = %q", config.BinaryPathName) // If intercept mode is set, verify the flag is present in BinPath. - if interceptMode == "dns" || interceptMode == "hard" { + if interceptMode == "off" || interceptMode == "dns" || interceptMode == "hard" { if !strings.Contains(config.BinaryPathName, "--intercept-mode") { return fmt.Errorf("service registry: --intercept-mode flag missing from BinaryPathName (expected mode %q)", interceptMode) } diff --git a/nameservers_darwin.go b/nameservers_darwin.go index ed97cb4..d8ab704 100644 --- a/nameservers_darwin.go +++ b/nameservers_darwin.go @@ -10,7 +10,6 @@ import ( "io" "net" "os/exec" - "regexp" "runtime" "slices" "strings" @@ -101,24 +100,29 @@ func getDHCPNameservers(iface string) ([]string, error) { return nil, fmt.Errorf("ipconfig not available on mobile") } - // Run the ipconfig command for the given interface. - cmd := exec.Command("ipconfig", "getpacket", iface) - output, err := cmd.Output() - if err != nil { - return nil, fmt.Errorf("error running ipconfig: %v", err) + // getoption returns the selected interface's DHCP option directly and does + // not expose unrelated packet addresses to the parser. + output, err := exec.Command("ipconfig", "getoption", iface, "domain_name_server").Output() + if err == nil { + return parseDHCPOptionNameservers(output), nil } - // Look for a line like: - // domain_name_servers = 192.168.1.1 8.8.8.8; - re := regexp.MustCompile(`domain_name_servers\s*=\s*(.*);`) - matches := re.FindStringSubmatch(string(output)) - if len(matches) < 2 { - return nil, fmt.Errorf("no DHCP nameservers found") + // Older macOS releases can fail getoption while still exposing the packet. + // Parse the real macOS field shape, for example: + // domain_name_server (ip_mult): {192.168.1.1, 8.8.8.8} + output, packetErr := exec.Command("ipconfig", "getpacket", iface).Output() + if packetErr != nil { + if err != nil { + return nil, fmt.Errorf("error reading DHCP DNS option: getoption: %v; getpacket: %v", err, packetErr) + } + return nil, fmt.Errorf("error reading DHCP packet: %v", packetErr) } + return parseDHCPPacketNameservers(output), nil +} - // Split the nameservers by whitespace. - nameservers := strings.Fields(matches[1]) - return nameservers, nil +// DHCPNameserversForInterface returns DHCP option 6 for exactly iface. +func DHCPNameserversForInterface(iface string) ([]string, error) { + return getDHCPNameservers(iface) } func getAllDHCPNameservers() []string { diff --git a/nameservers_darwin_parser.go b/nameservers_darwin_parser.go new file mode 100644 index 0000000..f7d942d --- /dev/null +++ b/nameservers_darwin_parser.go @@ -0,0 +1,44 @@ +package ctrld + +import ( + "net" + "strings" + "unicode" +) + +func parseDHCPOptionNameservers(output []byte) []string { + return parseIPv4Nameservers(string(output)) +} + +func parseDHCPPacketNameservers(output []byte) []string { + for _, line := range strings.Split(string(output), "\n") { + field := strings.TrimSpace(line) + if strings.HasPrefix(field, "domain_name_server ") || + strings.HasPrefix(field, "domain_name_server:") || + strings.HasPrefix(field, "domain_name_servers ") || + strings.HasPrefix(field, "domain_name_servers:") { + return parseIPv4Nameservers(field) + } + } + return nil +} + +func parseIPv4Nameservers(value string) []string { + seen := make(map[string]struct{}) + var nameservers []string + for _, token := range strings.FieldsFunc(value, func(r rune) bool { + return r != '.' && !unicode.IsDigit(r) + }) { + ip := net.ParseIP(token) + if ip == nil || ip.To4() == nil { + continue + } + ns := ip.String() + if _, ok := seen[ns]; ok { + continue + } + seen[ns] = struct{}{} + nameservers = append(nameservers, ns) + } + return nameservers +} diff --git a/nameservers_darwin_parser_test.go b/nameservers_darwin_parser_test.go new file mode 100644 index 0000000..6ec8b72 --- /dev/null +++ b/nameservers_darwin_parser_test.go @@ -0,0 +1,64 @@ +package ctrld + +import ( + "slices" + "testing" +) + +func TestParseDHCPOptionNameservers(t *testing.T) { + tests := []struct { + name string + output string + want []string + }{ + {"single", "192.168.10.1\n", []string{"192.168.10.1"}}, + {"multiple", "192.168.10.1\n1.1.1.1\n", []string{"192.168.10.1", "1.1.1.1"}}, + {"deduplicate and reject invalid", "192.168.10.1 999.1.1.1 192.168.10.1", []string{"192.168.10.1"}}, + {"empty", "", nil}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := parseDHCPOptionNameservers([]byte(tc.output)); !slices.Equal(got, tc.want) { + t.Fatalf("parseDHCPOptionNameservers() = %v, want %v", got, tc.want) + } + }) + } +} + +func TestParseDHCPPacketNameservers(t *testing.T) { + tests := []struct { + name string + output string + want []string + }{ + { + name: "macos singular ip_mult", + output: `op = BOOTREPLY +` + + `yiaddr = 192.168.10.155 +` + + `domain_name_server (ip_mult): {192.168.10.1, 1.1.1.1} +` + + `server_identifier (ip): 192.168.10.1 +`, + want: []string{"192.168.10.1", "1.1.1.1"}, + }, + { + name: "legacy plural equals", + output: "domain_name_servers = 192.168.1.1 8.8.8.8;\n", + want: []string{"192.168.1.1", "8.8.8.8"}, + }, + { + name: "packet addresses without option are ignored", + output: "yiaddr = 192.168.10.155\nserver_identifier (ip): 192.168.10.1\n", + want: nil, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := parseDHCPPacketNameservers([]byte(tc.output)); !slices.Equal(got, tc.want) { + t.Fatalf("parseDHCPPacketNameservers() = %v, want %v", got, tc.want) + } + }) + } +} diff --git a/resolver.go b/resolver.go index d84acf9..08353a3 100644 --- a/resolver.go +++ b/resolver.go @@ -143,17 +143,38 @@ func availableNameservers() []string { // It's the caller's responsibility to ensure the system DNS is in a clean state before // calling this function. func InitializeOsResolver(guardAgainstNoNameservers bool) []string { + ns, _ := InitializeOsResolverWithSystemNameservers(guardAgainstNoNameservers) + return ns +} + +// InitializeOsResolverWithSystemNameservers initializes the OS resolver and +// returns both the effective resolver list and the unmodified nameservers +// discovered from the system. The latter deliberately excludes synthetic +// fallbacks added by initializeOsResolver. +func InitializeOsResolverWithSystemNameservers(guardAgainstNoNameservers bool) (effective, system []string) { resolverMutex.Lock() defer resolverMutex.Unlock() - nameservers := availableNameservers() - // if no nameservers, return empty slice so we dont remove all nameservers - if len(nameservers) == 0 && guardAgainstNoNameservers { - return []string{} + system = availableNameservers() + if system == nil { + // A non-nil empty slice means discovery completed and found no DNS. + // Callers use nil to mean that discovery was not attempted. + system = []string{} } - ns := initializeOsResolver(nameservers) - or = newResolverWithNameserver(ns) - return ns + effective, system, skip := osResolverNameserverSets(system, guardAgainstNoNameservers) + if skip { + return effective, system + } + or = newResolverWithNameserver(effective) + return effective, system +} + +func osResolverNameserverSets(system []string, guardAgainstNoNameservers bool) (effective, discovered []string, skip bool) { + // if no nameservers, return empty slice so we dont remove all nameservers + if len(system) == 0 && guardAgainstNoNameservers { + return []string{}, system, true + } + return initializeOsResolver(system), system, false } // initializeOsResolver performs logic for choosing OS resolver nameserver. diff --git a/resolver_test.go b/resolver_test.go index 930871a..1840cb3 100644 --- a/resolver_test.go +++ b/resolver_test.go @@ -392,6 +392,31 @@ func Test_osResolver_ResolveWithNonSuccessAnswer(t *testing.T) { } } +func TestOSResolverNameserverSetsKeepsSyntheticFallbackOutOfSystemDiscovery(t *testing.T) { + system := []string{"fe80::1"} + effective, discovered, skip := osResolverNameserverSets(system, false) + if skip { + t.Fatal("non-empty discovery unexpectedly skipped resolver replacement") + } + + if len(discovered) != 1 || discovered[0] != system[0] { + t.Fatalf("discovered nameservers = %v, want raw system list %v", discovered, system) + } + if len(effective) != 2 || effective[0] != "[fe80::1]:53" || effective[1] != controldPublicDnsWithPort { + t.Fatalf("effective nameservers = %v, want IPv6 system resolver plus synthetic fallback", effective) + } +} + +func TestOSResolverNameserverSetsHonorsEmptyGuard(t *testing.T) { + effective, discovered, skip := osResolverNameserverSets(nil, true) + if len(effective) != 0 || len(discovered) != 0 { + t.Fatalf("guarded empty discovery returned effective=%v discovered=%v", effective, discovered) + } + if !skip { + t.Fatal("guarded empty discovery did not return the skip decision") + } +} + func Test_osResolver_InitializationRace(t *testing.T) { var wg sync.WaitGroup n := 10 diff --git a/scripts/macos-533-net-check.sh b/scripts/macos-533-net-check.sh new file mode 100755 index 0000000..630be78 --- /dev/null +++ b/scripts/macos-533-net-check.sh @@ -0,0 +1,53 @@ +#!/bin/sh +set -eu + +iface=$(/sbin/route -n get default 2>/dev/null | /usr/bin/awk '/interface:/{print $2; exit}') + +printf 'default_interface=%s\n' "$iface" + +printf 'ipv4_address=' +/usr/sbin/ipconfig getifaddr "$iface" 2>/dev/null || printf '\n' + +printf 'dhcp_ipv4_dns=' +/usr/sbin/ipconfig getoption "$iface" domain_name_server 2>/dev/null | + /usr/bin/awk ' + { + for (i = 1; i <= NF; i++) { + value = $i + gsub(/[{},;]/, "", value) + if (value ~ /^([0-9]{1,3}\.){3}[0-9]{1,3}$/ && !seen[value]++) { + if (found) { + printf "," + } + printf "%s", value + found = 1 + } + } + } + END { + if (!found) { + printf "" + } + printf "\n" + } + ' + +printf 'effective_ipv4_dns=' +/usr/sbin/scutil --dns | + /usr/bin/awk ' + /nameserver\[[0-9]+\] : [0-9]+\./ { + if (!seen[$3]++) { + if (found) { + printf "," + } + printf "%s", $3 + found = 1 + } + } + END { + if (!found) { + printf "" + } + printf "\n" + } + ' diff --git a/test-scripts/darwin/test-pkg-intercept-mode.sh b/test-scripts/darwin/test-pkg-intercept-mode.sh index eb06e45..d1029cd 100755 --- a/test-scripts/darwin/test-pkg-intercept-mode.sh +++ b/test-scripts/darwin/test-pkg-intercept-mode.sh @@ -2,7 +2,7 @@ set -eu repo_root=$(CDPATH= cd -- "$(dirname -- "$0")/../.." && pwd) -postinstall="$repo_root/scripts/pkg/postinstall" +postinstall_source="$repo_root/scripts/pkg/postinstall" fixture=$(mktemp -d "${TMPDIR:-/tmp}/ctrld-pkg-intercept.XXXXXX") trap 'rm -rf "$fixture"' EXIT HUP INT TERM @@ -36,16 +36,26 @@ printf 'launchctl %s\n' "$*" >>"$CALLS" exit 0 EOF +cat >"$bin/mktemp" <<'EOF' +#!/bin/sh +[ "${1:-}" = "-t" ] && [ -n "${2:-}" ] || exit 1 +path="$FAKE_TMPDIR/$2.capture" +: >"$path" +printf '%s\n' "$path" +EOF + cat >"$bin/ctrld" <<'EOF' #!/bin/sh printf 'ctrld %s\n' "$*" >>"$CALLS" -case " $* " in - *" --cd-org="*) : >"$CTRLD_POSTINSTALL_PLIST" ;; -esac -exit 0 +if [ "${FAKE_CTRLD_EXIT:-0}" = "0" ]; then + case " $* " in + *" --cd-org="*) : >"$FAKE_PLIST" ;; + esac +fi +exit "${FAKE_CTRLD_EXIT:-0}" EOF -chmod +x "$bin/defaults" "$bin/launchctl" "$bin/ctrld" +chmod +x "$bin/defaults" "$bin/launchctl" "$bin/mktemp" "$bin/ctrld" assert_contains() { expected=$1 @@ -72,31 +82,50 @@ run_case() { existing=$2 mode_present=$3 mode=$4 + ctrld_exit=${5:-0} + expected_status=${6:-0} case_dir="$fixture/$name" mkdir -p "$case_dir" plist="$case_dir/ctrld.plist" prefs="$case_dir/preferences" calls="$case_dir/calls" output="$case_dir/output" + postinstall="$case_dir/postinstall" : >"$calls" if [ "$existing" = "1" ]; then : >"$plist" fi + sed \ + -e "s|^PLIST=\"/Library/LaunchDaemons/ctrld.plist\"$|PLIST=\"$plist\"|" \ + -e "s|^CTRLD=\"/usr/local/bin/ctrld\"$|CTRLD=\"$bin/ctrld\"|" \ + -e "s|^PREFS=\"/Library/Managed Preferences/com.controld.ctrld\"$|PREFS=\"$prefs\"|" \ + "$postinstall_source" >"$postinstall" + chmod +x "$postinstall" + + status=0 PATH="$bin:$PATH" \ CALLS="$calls" \ FAKE_TOKEN_PRESENT=1 \ FAKE_TOKEN=test-token \ FAKE_MODE_PRESENT="$mode_present" \ FAKE_MODE="$mode" \ - CTRLD_POSTINSTALL_PLIST="$plist" \ - CTRLD_POSTINSTALL_CTRLD="$bin/ctrld" \ - CTRLD_POSTINSTALL_PREFS="$prefs" \ - "$postinstall" >"$output" 2>&1 + FAKE_CTRLD_EXIT="$ctrld_exit" \ + FAKE_PLIST="$plist" \ + FAKE_TMPDIR="$case_dir" \ + "$postinstall" >"$output" 2>&1 || status=$? + + if [ "$status" -ne "$expected_status" ]; then + printf 'FAIL: %s exited %s, want %s\n' "$name" "$status" "$expected_status" >&2 + sed -n '1,120p' "$output" >&2 + exit 1 + fi printf '%s\n' "$case_dir" } +assert_not_contains 'CTRLD_POSTINSTALL_' "$postinstall_source" + case_dir=$(run_case fresh-legacy 0 0 '') assert_contains 'ctrld start --cd-org=test-token' "$case_dir/calls" assert_not_contains '--intercept-mode' "$case_dir/calls" @@ -120,4 +149,18 @@ case_dir=$(run_case upgrade-intercept 1 1 intercept-dns) assert_contains 'ctrld start --intercept-mode dns' "$case_dir/calls" assert_not_contains 'launchctl load' "$case_dir/calls" +case_dir=$(run_case fresh-invalid 0 1 invalid) +assert_contains 'WARNING: unsupported InterceptMode in managed preferences; using standard mode' "$case_dir/output" +assert_not_contains '--intercept-mode' "$case_dir/calls" + +case_dir=$(run_case upgrade-invalid 1 1 invalid) +assert_contains 'WARNING: unsupported InterceptMode in managed preferences; preserving existing service mode' "$case_dir/output" +assert_contains 'launchctl load' "$case_dir/calls" +assert_not_contains 'ctrld start' "$case_dir/calls" + +case_dir=$(run_case upgrade-standard-failure 1 1 standard 1 1) +assert_contains 'ctrld start --intercept-mode off' "$case_dir/calls" +assert_contains 'ERROR: upgrade installed but managed InterceptMode could not be applied' "$case_dir/output" +assert_not_contains 'launchctl load' "$case_dir/calls" + printf 'PASS: pkg postinstall preserves legacy mode and applies standard/intercept-dns policy\n'