diff --git a/cmd/cli/dns_intercept_darwin.go b/cmd/cli/dns_intercept_darwin.go index d60bc32..8d1e21c 100644 --- a/cmd/cli/dns_intercept_darwin.go +++ b/cmd/cli/dns_intercept_darwin.go @@ -528,6 +528,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") @@ -1734,6 +1739,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 5fe0d43..ee08527 100644 --- a/cmd/cli/dns_proxy.go +++ b/cmd/cli/dns_proxy.go @@ -2074,8 +2074,9 @@ func (p *prog) debounceRecovery() { func (p *prog) handleRecovery(reason RecoveryReason) { p.Debug().Msg("Starting recovery process: removing DNS settings") - // Handle recovery cancellation based on reason - if !p.shouldStartRecovery(reason) { + recoveryCtx, gen, interceptRecovery, ok := p.beginRecovery(reason) + if !ok { + p.Debug().Msg("Upstream recovery already in progress; skipping duplicate trigger") return } @@ -2096,87 +2097,44 @@ func (p *prog) handleRecovery(reason RecoveryReason) { p.Info().Msg("Force-reset upstream transports for network change recovery") } - // Create recovery context and cleanup function - recoveryCtx, cleanup := p.createRecoveryContext() - defer cleanup() - - // Remove DNS settings and prepare for recovery - if err := p.prepareForRecovery(reason); err != nil { + if err := p.prepareForRecovery(reason, interceptRecovery); err != nil { p.Error().Err(err).Msg("Failed to prepare for recovery") + p.recoveryCanceledCleanup(gen) return } - // Build upstream map based on the recovery reason upstreams := p.buildRecoveryUpstreams(reason) - - // Wait for upstream recovery recovered, err := p.waitForUpstreamRecovery(recoveryCtx, upstreams) if err != nil { p.Error().Err(err).Msg("Recovery failed; DNS settings remain removed") + p.recoveryCanceledCleanup(gen) + return + } + if !p.recoveryOwnsState(gen) { + p.Debug().Msgf("Recovery generation %d was superseded after upstream success; skipping stale completion", gen) return } - // Complete recovery process - if err := p.completeRecovery(reason, recovered); err != nil { + if err := p.completeRecoveryWork(reason, recovered, interceptRecovery); err != nil { p.Error().Err(err).Msg("Failed to complete recovery") + p.recoveryCanceledCleanup(gen) + return + } + if !p.completeRecoveryState(gen) { + p.Debug().Msgf("Recovery generation %d was superseded during completion; preserving successor state", gen) return } p.Info().Msgf("Recovery completed successfully for upstream %q", recovered) } -// shouldStartRecovery determines if recovery should start based on the reason and current state. -// Returns true if recovery should proceed, false otherwise. -func (p *prog) shouldStartRecovery(reason RecoveryReason) bool { - p.recoveryCancelMu.Lock() - defer p.recoveryCancelMu.Unlock() - - if reason == RecoveryReasonNetworkChange { - // For network changes, cancel any existing recovery check because the network state has changed. - if p.recoveryCancel != nil { - p.Debug().Msg("Cancelling existing recovery check (network change)") - p.recoveryCancel() - p.recoveryCancel = nil - } - return true - } - - // For upstream failures, if a recovery is already in progress, do nothing new. - if p.recoveryCancel != nil { - p.Debug().Msg("Upstream recovery already in progress; skipping duplicate trigger") - return false - } - - return true -} - -// createRecoveryContext creates a new recovery context and returns it along with a cleanup function. -func (p *prog) createRecoveryContext() (context.Context, func()) { - p.recoveryCancelMu.Lock() - recoveryCtx, cancel := context.WithCancel(context.Background()) - p.recoveryCancel = cancel - p.recoveryCancelMu.Unlock() - - cleanup := func() { - p.recoveryCancelMu.Lock() - p.recoveryCancel = nil - p.recoveryCancelMu.Unlock() - } - - return recoveryCtx, cleanup -} - // prepareForRecovery removes DNS settings and initializes OS resolver if needed. -func (p *prog) prepareForRecovery(reason RecoveryReason) error { - // Set recoveryRunning to true to prevent watchdogs from putting the listener back on the interface - p.recoveryRunning.Store(true) - +func (p *prog) prepareForRecovery(reason RecoveryReason, interceptRecovery bool) error { // 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 { p.Info().Msg("DNS intercept recovery: enabling DHCP bypass (filters stay active)") // Reinitialize OS resolver to discover DHCP servers on the new network. @@ -2184,13 +2142,15 @@ func (p *prog) prepareForRecovery(reason RecoveryReason) error { // to resolve the auth page. p.Debug().Msg("DNS intercept recovery: discovering DHCP nameservers") loggerCtx := ctrld.LoggerCtx(context.Background(), p.logger.Load()) - dhcpServers := ctrld.InitializeOsResolver(loggerCtx, true) + dhcpServers, systemNameservers := initializeOsResolverWithSystemNameserversFn(loggerCtx, true) if len(dhcpServers) == 0 { p.Warn().Msg("DNS intercept recovery: no DHCP nameservers found") } else { p.Info().Msgf("DNS intercept recovery: found DHCP nameservers: %v", dhcpServers) } + ensureInterceptDNSTargetFn(p, systemNameservers) + // Exempt DHCP nameservers from intercept filters so the OS resolver // can actually reach them on port 53. Without this, the WFP block // or pf redirect would intercept ctrld's own recovery queries. @@ -2242,52 +2202,37 @@ func (p *prog) reinitializeOSResolver(message string) error { return nil } -// completeRecovery completes the recovery process by resetting upstream state and reapplying DNS settings. -func (p *prog) completeRecovery(reason RecoveryReason, recovered string) error { - // Reset the upstream failure count and down state +// completeRecoveryWork performs owner-specific recovery work. Shared recovery +// flags are released separately by completeRecoveryState under the ownership lock. +func (p *prog) completeRecoveryWork(reason RecoveryReason, recovered string, interceptRecovery bool) error { p.um.reset(recovered) - // In DNS intercept mode, just disable the bypass — filters are still active. - if dnsIntercept && p.dnsInterceptState != nil { - // Always reset recoveryRunning, even on error paths below. - defer p.recoveryRunning.Store(false) - - p.recoveryBypass.Store(false) - p.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. - // This also re-exempts VPN DNS servers (which may have changed) and - // removes any DHCP exemptions that were added during recovery. if p.vpnDNS != nil { p.vpnDNS.Refresh(ctrld.LoggerCtx(context.Background(), p.logger.Load()), true) } - - // Reinitialize OS resolver for the recovered state. if reason == RecoveryReasonNetworkChange { if err := p.reinitializeOSResolver("Network change detected during recovery"); err != nil { return fmt.Errorf("failed to reinitialize OS resolver during network change: %w", err) } } - return nil } - // Traditional flow: reapply DNS settings. - - // For network changes we also reinitialize the OS resolver. - if reason == RecoveryReasonNetworkChange { + var systemNameservers []string + if dnsIntercept { + // Intercept was requested but was not active when recovery began. A + // retry must use a completed raw discovery result, never nil. + systemNameservers = p.systemNameserversForInterceptRetry() + } else if reason == RecoveryReasonNetworkChange { if err := p.reinitializeOSResolver("Network change detected during recovery"); err != nil { return fmt.Errorf("failed to reinitialize OS resolver during network change: %w", err) } } - // 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 it's changed for any reason - p.recoveryRunning.Store(false) - return nil } @@ -2366,7 +2311,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_proxy_test.go b/cmd/cli/dns_proxy_test.go index 492b248..34b0d7a 100644 --- a/cmd/cli/dns_proxy_test.go +++ b/cmd/cli/dns_proxy_test.go @@ -558,187 +558,6 @@ func Test_isWanClient(t *testing.T) { } } -func Test_shouldStartRecovery(t *testing.T) { - tests := []struct { - name string - reason RecoveryReason - hasExistingRecovery bool - expectedResult bool - description string - }{ - { - name: "network change with existing recovery", - reason: RecoveryReasonNetworkChange, - hasExistingRecovery: true, - expectedResult: true, - description: "should cancel existing recovery and start new one for network change", - }, - { - name: "network change without existing recovery", - reason: RecoveryReasonNetworkChange, - hasExistingRecovery: false, - expectedResult: true, - description: "should start new recovery for network change", - }, - { - name: "regular failure with existing recovery", - reason: RecoveryReasonRegularFailure, - hasExistingRecovery: true, - expectedResult: false, - description: "should skip duplicate recovery for regular failure", - }, - { - name: "regular failure without existing recovery", - reason: RecoveryReasonRegularFailure, - hasExistingRecovery: false, - expectedResult: true, - description: "should start new recovery for regular failure", - }, - { - name: "OS failure with existing recovery", - reason: RecoveryReasonOSFailure, - hasExistingRecovery: true, - expectedResult: false, - description: "should skip duplicate recovery for OS failure", - }, - { - name: "OS failure without existing recovery", - reason: RecoveryReasonOSFailure, - hasExistingRecovery: false, - expectedResult: true, - description: "should start new recovery for OS failure", - }, - } - - for _, tc := range tests { - tc := tc - t.Run(tc.name, func(t *testing.T) { - p := newTestProg(t) - - // Setup existing recovery if needed - if tc.hasExistingRecovery { - p.recoveryCancelMu.Lock() - p.recoveryCancel = func() {} // Mock cancel function - p.recoveryCancelMu.Unlock() - } - - result := p.shouldStartRecovery(tc.reason) - assert.Equal(t, tc.expectedResult, result, tc.description) - }) - } -} - -func Test_createRecoveryContext(t *testing.T) { - p := newTestProg(t) - - ctx, cleanup := p.createRecoveryContext() - - // Verify context is created - assert.NotNil(t, ctx) - assert.NotNil(t, cleanup) - - // Verify recoveryCancel is set - p.recoveryCancelMu.Lock() - assert.NotNil(t, p.recoveryCancel) - p.recoveryCancelMu.Unlock() - - // Test cleanup function - cleanup() - - // Verify recoveryCancel is cleared - p.recoveryCancelMu.Lock() - assert.Nil(t, p.recoveryCancel) - p.recoveryCancelMu.Unlock() -} - -func Test_prepareForRecovery(t *testing.T) { - tests := []struct { - name string - reason RecoveryReason - wantErr bool - }{ - { - name: "regular failure", - reason: RecoveryReasonRegularFailure, - wantErr: false, - }, - { - name: "network change", - reason: RecoveryReasonNetworkChange, - wantErr: false, - }, - { - name: "OS failure", - reason: RecoveryReasonOSFailure, - wantErr: false, - }, - } - - for _, tc := range tests { - tc := tc - t.Run(tc.name, func(t *testing.T) { - p := newTestProg(t) - - err := p.prepareForRecovery(tc.reason) - - if tc.wantErr { - assert.Error(t, err) - } else { - assert.NoError(t, err) - } - - // Verify recoveryRunning is set to true - assert.True(t, p.recoveryRunning.Load()) - }) - } -} - -func Test_completeRecovery(t *testing.T) { - tests := []struct { - name string - reason RecoveryReason - recovered string - wantErr bool - }{ - { - name: "regular failure recovery", - reason: RecoveryReasonRegularFailure, - recovered: "upstream1", - wantErr: false, - }, - { - name: "network change recovery", - reason: RecoveryReasonNetworkChange, - recovered: "upstream2", - wantErr: false, - }, - { - name: "OS failure recovery", - reason: RecoveryReasonOSFailure, - recovered: "upstream3", - wantErr: false, - }, - } - - for _, tc := range tests { - tc := tc - t.Run(tc.name, func(t *testing.T) { - p := newTestProg(t) - - err := p.completeRecovery(tc.reason, tc.recovered) - - if tc.wantErr { - assert.Error(t, err) - } else { - assert.NoError(t, err) - } - - // Verify recoveryRunning is set to false - assert.False(t, p.recoveryRunning.Load()) - }) - } -} - func Test_reinitializeOSResolver(t *testing.T) { p := newTestProg(t) @@ -749,58 +568,6 @@ func Test_reinitializeOSResolver(t *testing.T) { assert.NoError(t, err) } -func Test_handleRecovery_Integration(t *testing.T) { - tests := []struct { - name string - reason RecoveryReason - wantErr bool - }{ - { - name: "network change recovery", - reason: RecoveryReasonNetworkChange, - wantErr: false, - }, - { - name: "regular failure recovery", - reason: RecoveryReasonRegularFailure, - wantErr: false, - }, - { - name: "OS failure recovery", - reason: RecoveryReasonOSFailure, - wantErr: false, - }, - } - - for _, tc := range tests { - tc := tc - t.Run(tc.name, func(t *testing.T) { - p := newTestProg(t) - - // This is an integration test that exercises the full recovery flow - // In a real test environment, you would mock the dependencies - // For now, we're just testing that the method doesn't panic - // and that the recovery logic flows correctly - assert.NotPanics(t, func() { - // Test only the preparation phase to avoid actual upstream checking - if !p.shouldStartRecovery(tc.reason) { - return - } - - _, cleanup := p.createRecoveryContext() - defer cleanup() - - if err := p.prepareForRecovery(tc.reason); err != nil { - return - } - - // Skip the actual upstream recovery check for this test - // as it requires properly configured upstreams - }) - }) - } -} - func Test_prog_queryFromSelf(t *testing.T) { p := newTestProg(t) require.NotPanics(t, func() { 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..3d411c6 --- /dev/null +++ b/cmd/cli/dns_target_darwin.go @@ -0,0 +1,243 @@ +//go:build darwin + +package cli + +import ( + "encoding/json" + "net" + "os" + "path/filepath" + + "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 = interceptDNSTargetStatePath + interceptDefaultRouteInterfaceFn = netmon.DefaultRouteInterface + interceptInterfaceByNameFn = net.InterfaceByName + interceptPatchNetIfaceNameFn = patchNetIfaceName + interceptCurrentStaticDNSFn = currentStaticDNS + interceptSaveCurrentStaticDNSFn = saveCurrentStaticDNS + interceptSetDNSFn = setDNS + interceptSavedStaticNameserversFn = ctrld.SavedStaticNameservers + interceptResetDNSIgnoreUnusableIfaceFn = resetDnsIgnoreUnusableInterface + interceptDHCPNameserversForInterfaceFn = ctrld.DHCPNameserversForInterface +) + +func interceptDNSTargetStatePath() string { + dir, err := userHomeDir() + if err != nil { + return interceptDNSTargetStateFile + } + return filepath.Join(dir, interceptDNSTargetStateFile) +} + +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 c9cb5c8..63a5a7f 100644 --- a/cmd/cli/prog.go +++ b/cmd/cli/prog.go @@ -145,6 +145,7 @@ type prog struct { recoveryCancelMu sync.Mutex recoveryCancel context.CancelFunc recoveryRunning atomic.Bool + recoveryGen atomic.Uint64 // recoveryDebounceTimer coalesces rapid NetworkChange recovery triggers // into a single handleRecovery call. Only handleRecovery is debounced — @@ -157,6 +158,14 @@ type prog struct { // authentication without tearing down WFP/pf filters. recoveryBypass atomic.Bool + // interceptDNSTargetService names the macOS network service on which + // ctrld set a temporary DNS target; interceptDNSTargetSetValue records the + // exact value. Both are guarded by interceptDNSTargetMu. + interceptDNSTargetMu sync.Mutex //lint:ignore U1000 used on darwin + interceptDNSTargetService string //lint:ignore U1000 used on darwin + interceptDNSTargetSetValue string //lint:ignore U1000 used on darwin + interceptDNSTargetLoaded bool //lint:ignore U1000 used on darwin + // DNS intercept mode state (platform-specific). // On Windows: *wfpState, on macOS: *pfState, nil on other platforms. dnsInterceptState any @@ -481,9 +490,10 @@ func (p *prog) postRun() { if !p.skipInitialDNSReset() { p.resetDNS(false, false) } - ns := ctrld.InitializeOsResolver(ctrld.LoggerCtx(context.Background(), p.logger.Load()), false) + loggerCtx := ctrld.LoggerCtx(context.Background(), p.logger.Load()) + ns, systemNameservers := initializeOsResolverWithSystemNameserversFn(loggerCtx, false) p.Debug().Msgf("Initialized os resolver with nameservers: %v", ns) - p.setDNS() + p.setDNS(systemNameservers) if p.allowList != nil { p.initPlatformFirewall() } @@ -958,10 +968,13 @@ 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 ( - startDNSInterceptFn = (*prog).startDNSIntercept - setDnsForRunningIfaceFn = (*prog).setDnsForRunningIface - resetDNSFn = (*prog).resetDNS - refuseFallbackFatal = func(format string, v ...any) { + 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...) } ) @@ -983,7 +996,7 @@ func interfaceDNSFallbackViable(lc *ctrld.ListenerConfig) bool { return lc == nil || lc.Port == 0 || lc.Port == 53 } -func (p *prog) setDNS() { +func (p *prog) setDNS(systemNameservers []string) { setDnsOK := false defer func() { p.csSetDnsOk = setDnsOK @@ -1017,6 +1030,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") // An external GP catch-all still owns the namespace even when its probe // fails. In either external-owner state, rewriting adapter DNS would violate // the policy that startup deliberately preserved. Only the verified case has @@ -1056,6 +1070,7 @@ func (p *prog) setDNS() { p.Error().Err(err).Msg("DNS intercept mode failed — falling back to interface DNS settings") // Fall through to traditional setDNS behavior. } else { + ensureInterceptDNSTargetFn(p, systemNameservers) if hardIntercept { p.Info().Msg("Hard intercept mode active — all DNS through ctrld, no VPN split routing") } else { @@ -1073,6 +1088,9 @@ func (p *prog) setDNS() { return } } + if !dnsIntercept { + removeInterceptDNSTargetFn(p, "intercept mode inactive") + } if cfg.Listener == nil { return @@ -1305,6 +1323,7 @@ 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) { + removeInterceptDNSTargetFn(p, "DNS reset") if dnsIntercept && p.dnsInterceptState != nil { if err := p.stopDNSIntercept(); err != nil { p.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 06592d8..96291e8 100644 --- a/cmd/cli/prog_intercept_fallback_test.go +++ b/cmd/cli/prog_intercept_fallback_test.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "net" + "slices" "strings" "testing" @@ -81,9 +82,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 } @@ -91,11 +95,11 @@ 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 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 cfg, interceptMode, dnsIntercept, hardIntercept = origCfg, origMode, origIntercept, origHard }) @@ -106,6 +110,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 @@ -129,7 +138,29 @@ func (h *interceptFallbackHarness) run(t *testing.T) { t.Helper() p := &prog{cfg: &cfg} p.logger.Store(mainLog.Load()) - 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.logger.Store(mainLog.Load()) + p.setDNS(want) + + if h.interceptCalls != 1 || h.ensureTargetCalls != 1 { + t.Fatalf("intercept calls=%d ensure calls=%d, want 1 each", h.interceptCalls, 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) { @@ -146,6 +177,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 @@ -168,6 +202,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..e062925 --- /dev/null +++ b/cmd/cli/recovery_state.go @@ -0,0 +1,80 @@ +package cli + +import ( + "context" + + "github.com/Control-D-Inc/ctrld" +) + +// 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 (p *prog) systemNameserversForInterceptRetry() []string { + loggerCtx := ctrld.LoggerCtx(context.Background(), p.logger.Load()) + _, system := initializeOsResolverWithSystemNameserversFn(loggerCtx, 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) completeRecoveryState(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.completeRecoveryState(gen) { + // Superseded: the newer recovery owns the shared state. + return + } + p.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..9407e89 --- /dev/null +++ b/cmd/cli/recovery_state_test.go @@ -0,0 +1,167 @@ +package cli + +import ( + "context" + "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.logger.Store(mainLog.Load()) + 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.logger.Store(mainLog.Load()) + 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.completeRecoveryState(successorGen) { + t.Fatal("successor could not complete its own recovery state") + } +} + +func TestRecoveryCleanupClearsBypassAfterInterceptStateDisappears(t *testing.T) { + p := &prog{} + p.logger.Store(mainLog.Load()) + 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(_ context.Context, 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 }) + + p := &prog{} + p.logger.Store(mainLog.Load()) + if got := p.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 eff05bb..92540c1 100644 --- a/nameservers_darwin.go +++ b/nameservers_darwin.go @@ -10,7 +10,6 @@ import ( "io" "net" "os/exec" - "regexp" "slices" "strings" "time" @@ -89,24 +88,29 @@ func getDNSFromScutil(ctx context.Context) []string { } func getDHCPNameservers(iface string) ([]string, error) { - // 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(ctx context.Context) []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 fed1dc7..06bb481 100644 --- a/resolver.go +++ b/resolver.go @@ -115,17 +115,37 @@ func availableNameservers(ctx context.Context) []string { // It's the caller's responsibility to ensure the system DNS is in a clean state before // calling this function. func InitializeOsResolver(ctx context.Context, guardAgainstNoNameservers bool) []string { + ns, _ := InitializeOsResolverWithSystemNameservers(ctx, 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(ctx context.Context, guardAgainstNoNameservers bool) (effective, system []string) { resolverMutex.Lock() defer resolverMutex.Unlock() - nameservers := availableNameservers(ctx) - // if no nameservers, return empty slice so we dont remove all nameservers - if len(nameservers) == 0 && guardAgainstNoNameservers { - return []string{} + system = availableNameservers(ctx) + 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 len(system) == 0 && guardAgainstNoNameservers { + return []string{}, system, true + } + return initializeOsResolver(system), system, false } // OsResolverNameservers returns the current OS resolver nameservers (host:port format). diff --git a/resolver_test.go b/resolver_test.go index 672c626..0cf7330 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" + } + '