From d2aa155ff3e2244b3146055f230f4ed788fcaf47 Mon Sep 17 00:00:00 2001 From: Dev Scribe Date: Fri, 21 Aug 2026 14:54:40 -0400 Subject: [PATCH 1/2] pkg: port managed DNS mode review fixes --- cmd/cli/cli.go | 2 +- cmd/cli/commands.go | 7 +-- cmd/cli/intercept_mode_config_test.go | 27 ++++++++ cmd/cli/main.go | 2 +- cmd/cli/prog.go | 6 +- cmd/cli/service.go | 6 +- cmd/cli/service_args_darwin_test.go | 5 ++ cmd/cli/service_args_windows.go | 2 +- .../darwin/test-pkg-intercept-mode.sh | 63 ++++++++++++++++--- 9 files changed, 96 insertions(+), 24 deletions(-) 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/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..02a9405 100644 --- a/cmd/cli/prog.go +++ b/cmd/cli/prog.go @@ -1091,11 +1091,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 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/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' From 8013bb72d2b9269f7108e3c90987ad63899e19f4 Mon Sep 17 00:00:00 2001 From: Dev Scribe Date: Tue, 1 Sep 2026 03:57:53 +0000 Subject: [PATCH 2/2] dns intercept: handle DNS-less networks and canceled-recovery state leak macOS intercept mode fails totally on networks that provide no usable IPv4 DNS (e.g. IPv6-only iPhone tethering with 464XLAT): the pf ruleset blocks all outbound IPv6 port 53, and with no IPv4 DNS configured mDNSResponder emits no DNS packets at all, so pf has nothing to intercept while the Control D upstream stays provably healthy. When network-change recovery discovers no usable IPv4 DNS on the default-route service, set 127.0.0.1 as that service DNS so macOS can emit queries that land directly on the listener. The entry is removed when the network regains IPv4 DNS and on intercept shutdown; networks that provide IPv4 DNS are never modified. Runs on the already-debounced recovery path so interface flaps do not churn networksetup. Also fix the canceled-recovery state leak: the cancellation early return never reset recoveryBypass/recoveryRunning, so a flap burst ending in a canceled recovery left the daemon in bypass forever with the DNS watchdog disabled. Cleanup is generation-gated so a superseded recovery never clears state owned by its successor. --- cmd/cli/dns_intercept_darwin.go | 10 + cmd/cli/dns_proxy.go | 96 +++++---- cmd/cli/dns_target.go | 116 +++++++++++ cmd/cli/dns_target_darwin.go | 234 ++++++++++++++++++++++ cmd/cli/dns_target_darwin_test.go | 248 ++++++++++++++++++++++++ cmd/cli/dns_target_filter_test.go | 57 ++++++ cmd/cli/dns_target_stub.go | 14 ++ cmd/cli/dns_target_test.go | 113 +++++++++++ cmd/cli/prog.go | 52 ++++- cmd/cli/prog_intercept_fallback_test.go | 44 ++++- cmd/cli/recovery_state.go | 75 +++++++ cmd/cli/recovery_state_test.go | 161 +++++++++++++++ nameservers_darwin.go | 34 ++-- nameservers_darwin_parser.go | 44 +++++ nameservers_darwin_parser_test.go | 64 ++++++ resolver.go | 35 +++- resolver_test.go | 25 +++ scripts/macos-533-net-check.sh | 53 +++++ 18 files changed, 1392 insertions(+), 83 deletions(-) create mode 100644 cmd/cli/dns_target.go create mode 100644 cmd/cli/dns_target_darwin.go create mode 100644 cmd/cli/dns_target_darwin_test.go create mode 100644 cmd/cli/dns_target_filter_test.go create mode 100644 cmd/cli/dns_target_stub.go create mode 100644 cmd/cli/dns_target_test.go create mode 100644 cmd/cli/recovery_state.go create mode 100644 cmd/cli/recovery_state_test.go create mode 100644 nameservers_darwin_parser.go create mode 100644 nameservers_darwin_parser_test.go create mode 100755 scripts/macos-533-net-check.sh 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/prog.go b/cmd/cli/prog.go index 02a9405..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 @@ -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/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" + } + '