diff --git a/cmd/cli/dns_intercept_settle.go b/cmd/cli/dns_intercept_settle.go index 2af934d..5cae95e 100644 --- a/cmd/cli/dns_intercept_settle.go +++ b/cmd/cli/dns_intercept_settle.go @@ -2,8 +2,14 @@ package cli import "github.com/Control-D-Inc/ctrld" +// initializeOsResolver re-reads the system nameservers into the OS resolver. +// Tests replace it, so they never read the host's resolver configuration. var initializeOsResolver = ctrld.InitializeOsResolverWithReason +// initializeOsResolverQuick is initializeOsResolver without waiting for the +// system to publish its nameservers. +var initializeOsResolverQuick = ctrld.InitializeOsResolverQuickWithReason + // Reasons that the journal reports when the resolver set changes. Each one // names the code path that read the resolvers of the host. const ( @@ -11,6 +17,7 @@ const ( osResolverReasonWakeProbe = "wake_probe" osResolverReasonVPNSettle = "vpn_settle" osResolverReasonDelayedRecheck = "delayed_recheck" + osResolverReasonNetworkChange = "network_change" ) func (p *prog) refreshDNSAfterVPNSettle(reason string) (routes, domainlessServers, exemptions int) { diff --git a/cmd/cli/dns_proxy.go b/cmd/cli/dns_proxy.go index 2f899b5..a5657ac 100644 --- a/cmd/cli/dns_proxy.go +++ b/cmd/cli/dns_proxy.go @@ -96,6 +96,9 @@ type proxyResponse struct { cached bool clientInfo bool upstream string + // drop asks the server to send no reply, so the client retries; answer + // is nil then. + drop bool } // upstreamForResult represents the result of processing rules for a request. @@ -248,6 +251,10 @@ func (p *prog) serveDNS(listenerNum string) error { failoverRcodes: failoverRcode, ufr: ur, }) + if pr.drop { + p.health.countQuery() + return + } go p.doSelfUninstall(pr.answer) answer = pr.answer @@ -1056,6 +1063,9 @@ func (p *prog) proxy(ctx context.Context, req *proxyRequest) *proxyResponse { } } ctrld.Log(ctx, p.querySampler.event(sampleClassAllEndpointsFailed, ""), "all %v endpoints failed", journalUpstreamNames(upstreams)) + // The OS resolver failed this query, as the only upstream or as the + // catch-all below. + osResolverFailed := len(upstreams) > 0 && upstreams[0] == upstreamOS // An Internal Domain with explicit resolvers never takes the OS-resolver // catch-all below: it can reach public nameservers, ctrld's own fallback @@ -1104,9 +1114,20 @@ func (p *prog) proxy(ctx context.Context, req *proxyRequest) *proxyResponse { return res } ctrld.Log(ctx, mainLog.Load().Debug(), "OS resolver retry query failed") + osResolverFailed = true } } + if osResolverFailed && isNAT64DiscoveryQuery(req.msg) && p.osResolverStale() { + // See os_resolver_stale.go: a SERVFAIL here would end the host's + // NAT64 prefix discovery, and it comes from the previous network's + // nameservers. Without an answer the host retries. + ctrld.Log(ctx, mainLog.Load().Info(), "OS resolver nameservers predate the network change; not answering %s so the host retries", dns64WellKnownName) + p.countFailedClientQuery(upstreams) + res.drop = true + return res + } + p.countFailedClientQuery(upstreams) answer := new(dns.Msg) answer.SetRcode(req.msg, dns.RcodeServerFailure) @@ -2018,6 +2039,12 @@ func (p *prog) handleNetworkChange(delta *netmon.ChangeDelta, isMajorChange bool p.networkAcceptedGen.Store(transitionID) p.networkSourceMu.Unlock() + // Before the re-bootstrap, so no query meets the previous network's + // nameservers unmarked. Routers run no recovery, as below. + if router.Name() == "" { + networkChangeReadOSResolverFn(p) + } + mainLog.Load().Debug().Msg("Link state changed, re-bootstrapping") for _, uc := range p.cfg.Upstream { uc.ReBootstrap() @@ -2497,6 +2524,9 @@ func (p *prog) handleRecoveryForTransition(reason RecoveryReason, transitionID u mainLog.Load().Info().Msg("Force-reset upstream transports for network change recovery") } + // Stops the OS resolver refresh of a network-change recovery; a no-op otherwise. + stopRefresh := func() {} + // 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 @@ -2510,6 +2540,7 @@ func (p *prog) handleRecoveryForTransition(reason RecoveryReason, transitionID u // gave none, so the journal reports the discovered list instead. resolverNameservers, systemNameservers := initializeOsResolverWithSystemNameserversFn(true, recoveryResolverReason) diagnostic.dhcpServers = systemNameservers + p.noteOSResolverRead(systemNameservers) if len(systemNameservers) == 0 { journal(mainLog.Load().Warn()).Msg("DNS intercept recovery: no DHCP nameservers found") } else { @@ -2546,20 +2577,32 @@ func (p *prog) handleRecoveryForTransition(reason RecoveryReason, transitionID u // Traditional flow: remove DNS settings to expose DHCP nameservers recoveryResetDNSFn(p, false, false) - // For an OS failure, reinitialize OS resolver nameservers immediately. - if reason == RecoveryReasonOSFailure { - mainLog.Load().Debug().Msg("OS resolver failure detected; reinitializing OS resolver nameservers") - ns := ctrld.InitializeOsResolverWithReason(true, recoveryResolverReason) + // Read the exposed nameservers right away, for an OS failure and for a + // network change. After a network change the OS resolver still holds + // the previous network's servers, and the wait below can need the new + // ones: on an IPv6-only network, an IPv4-only upstream comes back only + // once macOS starts its CLAT, and macOS discovers the NAT64 prefix by + // resolving ipv4only.arpa through ctrld's OS-resolver fallback. + if reason == RecoveryReasonOSFailure || reason == RecoveryReasonNetworkChange { + mainLog.Load().Debug().Msgf("%s; reinitializing OS resolver nameservers", recoveryReasonName(reason)) + ns := initializeOsResolver(true, recoveryResolverReason) + p.noteOSResolverRead(ns) if len(ns) == 0 { mainLog.Load().Warn().Msg("No nameservers found for OS resolver; using existing values") } else { journal(mainLog.Load().Info()).Msgf("Reinitialized OS resolver with nameservers: %v", ns) } } + // A new network can publish its resolvers after the reset, as with + // RDNSS, so keep reading them until the upstream recovers. + if reason == RecoveryReasonNetworkChange { + stopRefresh = p.refreshOSResolverWhileRecovering(recoveryCtx) + } } // Wait for the same upstream pool used to admit this recovery. recovered, err := waitForUpstreamRecoveryFn(p, recoveryCtx, upstreams, &diagnostic) + stopRefresh() if err != nil { if p.recoveryGen.Load() == gen { outcome = recoveryOutcomeCanceled @@ -2590,7 +2633,8 @@ func (p *prog) handleRecoveryForTransition(reason RecoveryReason, transitionID u // Reinitialize OS resolver for the recovered state. if reason == RecoveryReasonNetworkChange { - ns := ctrld.InitializeOsResolverWithReason(true, recoveryResolverReason) + ns := initializeOsResolver(true, recoveryResolverReason) + p.noteOSResolverRead(ns) if len(ns) == 0 { mainLog.Load().Warn().Msg("No nameservers found for OS resolver during network-change recovery; using existing values") } else { @@ -2606,7 +2650,8 @@ func (p *prog) handleRecoveryForTransition(reason RecoveryReason, transitionID u // on a healthy DHCP network. systemNameservers = systemNameserversForInterceptRetry() } else if reason == RecoveryReasonNetworkChange { - ns := ctrld.InitializeOsResolverWithReason(true, recoveryResolverReason) + ns := initializeOsResolver(true, recoveryResolverReason) + p.noteOSResolverRead(ns) if len(ns) == 0 { mainLog.Load().Warn().Msg("No nameservers found for OS resolver during network-change recovery; using existing values") } else { @@ -2616,7 +2661,7 @@ func (p *prog) handleRecoveryForTransition(reason RecoveryReason, transitionID u // Apply our DNS settings back. The snapshot that follows the end event // reports the interfaces. - p.setDNS(systemNameservers) + recoverySetDNSFn(p, systemNameservers) } if !p.completeRecovery(gen) { @@ -2655,6 +2700,46 @@ func (p *prog) refreshOSResolverAfterRecoverySkip(healthy string) { initializeOsResolverWithSystemNameserversFn(true, recoveryResolverReason) } +// osResolverRecoveryRefreshIntervals are the gaps between the re-reads of the +// system nameservers while a network-change recovery waits for an upstream. +// They start short, because a new network's resolvers usually appear within a +// few seconds of the reset, and the last one repeats: the wait has no timeout, +// and on macOS each read runs scutil, ipconfig and networksetup. +var osResolverRecoveryRefreshIntervals = []time.Duration{time.Second, 2 * time.Second, 5 * time.Second, 30 * time.Second} + +// refreshOSResolverWhileRecovering re-reads the system nameservers, at +// osResolverRecoveryRefreshIntervals, until ctx ends or the returned stop +// function is called, which also waits for the refresher to exit. +// +// A network-change recovery removes ctrld's DNS settings to expose the new +// network's resolvers, but a network can publish them later than that, as with +// IPv6 RDNSS. Reading once would leave the OS-resolver fallback on the previous +// network's servers until the upstream recovers, and on an IPv6-only network +// the upstream can only recover through that fallback (#628). Reads that find +// no nameservers keep the current ones. +func (p *prog) refreshOSResolverWhileRecovering(ctx context.Context) (stop func()) { + ctx, cancel := context.WithCancel(ctx) + done := make(chan struct{}) + go func() { + defer close(done) + for i := 0; ; i++ { + interval := osResolverRecoveryRefreshIntervals[min(i, len(osResolverRecoveryRefreshIntervals)-1)] + if !sleepWithContext(ctx, interval) { + return + } + ns := initializeOsResolver(true, recoveryResolverReason) + p.noteOSResolverRead(ns) + if len(ns) > 0 { + mainLog.Load().Debug().Msgf("Network-change recovery: OS resolver nameservers: %v", ns) + } + } + }() + return func() { + cancel() + <-done + } +} + // waitForUpstreamRecoveryFn is the seam of the upstream probe. A test drives // the whole recovery flow without a query to a real upstream. var waitForUpstreamRecoveryFn = (*prog).waitForUpstreamRecovery diff --git a/cmd/cli/network_transition.go b/cmd/cli/network_transition.go index 082516f..b1c91b6 100644 --- a/cmd/cli/network_transition.go +++ b/cmd/cli/network_transition.go @@ -41,6 +41,8 @@ var ( networkChangeIgnoredInterceptFn = (*prog).handleDNSInterceptIgnoredNetworkChange handleRecoveryForTransitionFn = (*prog).handleRecoveryForTransition recoveryResetDNSFn = (*prog).resetDNS + recoverySetDNSFn = (*prog).setDNS + networkChangeReadOSResolverFn = (*prog).readOSResolverOnNetworkChange ) // Minor callbacks have no observation sequence in netmon v1.74.0. Read only diff --git a/cmd/cli/network_transition_test.go b/cmd/cli/network_transition_test.go index 716c619..97294ce 100644 --- a/cmd/cli/network_transition_test.go +++ b/cmd/cli/network_transition_test.go @@ -51,15 +51,19 @@ func sourceTestGlobals(t *testing.T) { v4, v6 := ctrld.GetDefaultLocalIPv4(), ctrld.GetDefaultLocalIPv6() valid, route := networkChangeValidInterfacesFn, networkChangeDefaultRouteIPFn reconcile, ignored := networkChangeReconcileFn, networkChangeIgnoredInterceptFn + readOSResolver := networkChangeReadOSResolverFn intercept := dnsIntercept dnsIntercept = true networkChangeValidInterfacesFn = func() map[string]struct{} { return map[string]struct{}{"en0": {}} } networkChangeDefaultRouteIPFn = func() string { return "" } + // Nor may it read the host's nameservers. + networkChangeReadOSResolverFn = func(*prog) {} t.Cleanup(func() { ctrld.SetDefaultLocalIPv4(v4) ctrld.SetDefaultLocalIPv6(v6) networkChangeValidInterfacesFn, networkChangeDefaultRouteIPFn = valid, route networkChangeReconcileFn, networkChangeIgnoredInterceptFn = reconcile, ignored + networkChangeReadOSResolverFn = readOSResolver dnsIntercept = intercept }) } diff --git a/cmd/cli/os_resolver_stale.go b/cmd/cli/os_resolver_stale.go new file mode 100644 index 0000000..7562ff7 --- /dev/null +++ b/cmd/cli/os_resolver_stale.go @@ -0,0 +1,74 @@ +package cli + +import ( + "strings" + "time" + + "github.com/miekg/dns" +) + +// osResolverStaleHold bounds how long the OS resolver's nameservers count as +// read before the last network change, when no read finds new ones. +const osResolverStaleHold = 30 * time.Second + +// After a network change, the OS resolver keeps the previous network's +// nameservers until a read finds the new ones. In Standard mode that read +// usually has to wait for the recovery to remove ctrld's DNS settings: while +// ctrld's own address is the configured server, the system reports nothing +// else, and on macOS an RDNSS-only IPv6 network has no other source. +// +// macOS discovers the NAT64 prefix of an IPv6-only network by resolving +// ipv4only.arpa (RFC 7050), through ctrld while ctrld is still the system +// resolver. A SERVFAIL from the previous network's nameservers ends that +// discovery, and macOS then never starts its CLAT, which an IPv4-only upstream +// needs to come back (#628). So while the nameservers are stale, ctrld sends +// no answer to that lookup when it fails, and macOS retries it, by then +// against the new network's resolver. + +// markOSResolverStale records a network change the OS resolver may predate. +func (p *prog) markOSResolverStale() { + p.osResolverStaleUntil.Store(networkEventsNowFn().Add(osResolverStaleHold).UnixNano()) +} + +// noteOSResolverRead records the result of a nameserver read: a read that +// found nameservers holds the current network's, so they are no longer stale. +// A read that found none kept the stale ones. +func (p *prog) noteOSResolverRead(ns []string) { + if len(ns) > 0 { + p.osResolverStaleUntil.Store(0) + } +} + +// osResolverStale reports whether the OS resolver still holds nameservers +// read before the last network change. +func (p *prog) osResolverStale() bool { + return networkEventsNowFn().UnixNano() < p.osResolverStaleUntil.Load() +} + +// isNAT64DiscoveryQuery reports whether msg is the RFC 7050 NAT64 prefix +// discovery lookup, AAAA for ipv4only.arpa. +func isNAT64DiscoveryQuery(msg *dns.Msg) bool { + if msg == nil || len(msg.Question) != 1 { + return false + } + q := msg.Question[0] + return q.Qtype == dns.TypeAAAA && strings.EqualFold(dns.Fqdn(q.Name), dns64WellKnownName) +} + +// readOSResolverOnNetworkChange marks the OS resolver stale and reads the new +// network's nameservers once, without waiting for the system to publish them: +// in Standard mode ctrld's own address is still the configured server here, +// and on macOS a full read retries scutil for about a second while holding the +// lock that every OS resolver query takes. Where the system does report the +// new nameservers, as in intercept mode or with DHCPv4, the read replaces the +// stale ones before the re-bootstrap. +func (p *prog) readOSResolverOnNetworkChange() { + p.markOSResolverStale() + ns := initializeOsResolverQuick(osResolverReasonNetworkChange) + p.noteOSResolverRead(ns) + if len(ns) > 0 { + mainLog.Load().Debug().Msgf("Network change: OS resolver nameservers: %v", ns) + } else { + mainLog.Load().Debug().Msg("Network change: no new OS resolver nameservers yet; holding failed ipv4only.arpa lookups until they are read") + } +} diff --git a/cmd/cli/os_resolver_stale_test.go b/cmd/cli/os_resolver_stale_test.go new file mode 100644 index 0000000..608cf9b --- /dev/null +++ b/cmd/cli/os_resolver_stale_test.go @@ -0,0 +1,174 @@ +package cli + +import ( + "context" + "errors" + "slices" + "testing" + "time" + + "github.com/miekg/dns" + + "github.com/Control-D-Inc/ctrld" +) + +// A network change marks the OS resolver stale and reads the new network's +// nameservers before the re-bootstrap and the route lookup that follows it, so +// no query after the change meets the previous network's nameservers +// unmarked (#628). +func TestNetworkChangeReadsOSResolverBeforeReBootstrap(t *testing.T) { + h := newNetworkEventsHarness(t, "en0", "en7") + var events []string + networkChangeReadOSResolverFn = func(*prog) { events = append(events, "read") } + networkChangeDefaultRouteIPFn = func() string { + events = append(events, "route") + return "" + } + before := orderTestState("", "", map[string][]string{"en0": nil}, map[string]bool{"en0": true}) + before.HaveV4 = false + after := orderTestState("en7", "", map[string][]string{"en0": nil, "en7": {"192.0.2.30/24"}}, map[string]bool{"en0": true}) + + h.handle(before, after, false) + + if want := []string{"read", "route"}; !slices.Equal(events, want) { + t.Fatalf("events = %v, want %v", events, want) + } +} + +// The read at a network change does not wait for the system to publish the +// nameservers, and keeps the OS resolver stale when it finds none. +func TestReadOSResolverOnNetworkChange(t *testing.T) { + for _, tc := range []struct { + name string + found []string + wantStale bool + }{ + {"none yet", nil, true}, + {"found", []string{"[fd66:6:6::1]:53"}, false}, + } { + t.Run(tc.name, func(t *testing.T) { + captureDebugMainLog(t) + oldQuick, oldFull := initializeOsResolverQuick, initializeOsResolver + t.Cleanup(func() { initializeOsResolverQuick, initializeOsResolver = oldQuick, oldFull }) + var reasons []string + initializeOsResolverQuick = func(reason string) []string { + reasons = append(reasons, reason) + return tc.found + } + initializeOsResolver = func(bool, string) []string { + t.Fatal("the network-change read used the full read") + return nil + } + p := &prog{} + + p.readOSResolverOnNetworkChange() + + if want := []string{osResolverReasonNetworkChange}; !slices.Equal(reasons, want) { + t.Fatalf("reads = %v, want %v", reasons, want) + } + if got := p.osResolverStale(); got != tc.wantStale { + t.Fatalf("stale = %v, want %v", got, tc.wantStale) + } + }) + } +} + +// The stale mark ends by itself, so a network whose nameservers are never read +// does not hold the lookup for good. +func TestOSResolverStaleExpires(t *testing.T) { + now := time.Date(2026, 10, 14, 9, 0, 0, 0, time.UTC) + oldNow := networkEventsNowFn + t.Cleanup(func() { networkEventsNowFn = oldNow }) + networkEventsNowFn = func() time.Time { return now } + p := &prog{} + p.markOSResolverStale() + now = now.Add(osResolverStaleHold - time.Second) + if !p.osResolverStale() { + t.Fatal("not stale within the hold") + } + now = now.Add(time.Second) + if p.osResolverStale() { + t.Fatal("still stale after the hold") + } +} + +// staleTestResolver fails every upstream, and answers through the OS resolver +// only when osAnswers is set. +type staleTestResolver struct { + os bool + osAnswers bool +} + +func (r staleTestResolver) Resolve(_ context.Context, msg *dns.Msg) (*dns.Msg, error) { + if !r.os || !r.osAnswers { + return nil, errors.New("network is unreachable") + } + answer := new(dns.Msg) + answer.SetReply(msg) + return answer, nil +} + +// While the OS resolver holds nameservers read before the network change, a +// failed ipv4only.arpa AAAA lookup gets no answer, so the host retries it +// instead of ending its NAT64 prefix discovery on a SERVFAIL from the previous +// network's nameservers (#628). Every other failure still gets SERVFAIL. +func TestStaleOSResolverHoldsNAT64Discovery(t *testing.T) { + for _, tc := range []struct { + name string + qname string + qtype uint16 + stale bool + osAnswers bool + wantDrop bool + wantRcode int + }{ + {"stale discovery", "ipv4only.arpa.", dns.TypeAAAA, true, false, true, 0}, + {"stale discovery, any case", "IPv4Only.Arpa.", dns.TypeAAAA, true, false, true, 0}, + {"fresh discovery", "ipv4only.arpa.", dns.TypeAAAA, false, false, false, dns.RcodeServerFailure}, + {"stale A lookup", "ipv4only.arpa.", dns.TypeA, true, false, false, dns.RcodeServerFailure}, + {"stale other name", "example.com.", dns.TypeAAAA, true, false, false, dns.RcodeServerFailure}, + {"stale discovery answered", "ipv4only.arpa.", dns.TypeAAAA, true, true, false, dns.RcodeSuccess}, + } { + t.Run(tc.name, func(t *testing.T) { + captureDebugMainLog(t) + leak := true + cfg := &ctrld.Config{Upstream: map[string]*ctrld.UpstreamConfig{ + "0": {Name: "control d", Type: ctrld.ResolverTypeDOH, Endpoint: "https://192.0.2.1/dns-query", Timeout: 100}, + }} + cfg.Service.LeakOnUpstreamFailure = &leak + p := &prog{cfg: cfg, um: newUpstreamMonitor(cfg)} + p.um.after = func(time.Duration, func()) {} + p.lanLoopGuard = newLoopGuard() + p.ptrLoopGuard = newLoopGuard() + t.Cleanup(func() { p.querySampler.closeExpired(time.Now().Add(querySampleWindow)) }) + oldResolver, oldRecovery := newResolverFn, queryRecoveryFn + t.Cleanup(func() { newResolverFn, queryRecoveryFn = oldResolver, oldRecovery }) + newResolverFn = func(uc *ctrld.UpstreamConfig) (ctrld.Resolver, error) { + return staleTestResolver{os: uc.Name == osUpstreamConfig.Name, osAnswers: tc.osAnswers}, nil + } + queryRecoveryFn = func(*prog, RecoveryReason) {} + if tc.stale { + p.markOSResolverStale() + } + msg := new(dns.Msg) + msg.SetQuestion(tc.qname, tc.qtype) + + res := p.proxy(context.Background(), &proxyRequest{msg: msg, ufr: &upstreamForResult{ + upstreams: []string{upstreamPrefix + "0"}, srcAddr: "127.0.0.1:5353", + }}) + + if res.drop != tc.wantDrop { + t.Fatalf("drop = %v, want %v", res.drop, tc.wantDrop) + } + if tc.wantDrop { + if res.answer != nil { + t.Fatalf("dropped query has an answer: %v", res.answer) + } + return + } + if res.answer == nil || res.answer.Rcode != tc.wantRcode { + t.Fatalf("answer = %v, want rcode %s", res.answer, dns.RcodeToString[tc.wantRcode]) + } + }) + } +} diff --git a/cmd/cli/prog.go b/cmd/cli/prog.go index f51b31d..de56eca 100644 --- a/cmd/cli/prog.go +++ b/cmd/cli/prog.go @@ -210,6 +210,11 @@ type prog struct { // instead of using the normal upstream flow. recoveryBypass atomic.Bool + // osResolverStaleUntil is when the OS resolver's nameservers stop being + // treated as read before the last network change, in Unix nanoseconds. + // See os_resolver_stale.go. + osResolverStaleUntil atomic.Int64 + // dns64 tracks DNS64/NAT64 synthesis state for IPv6-only networks // without client-side 464XLAT. See cmd/cli/dns64.go. dns64 dns64State diff --git a/cmd/cli/recovery_os_refresh_test.go b/cmd/cli/recovery_os_refresh_test.go new file mode 100644 index 0000000..b40be17 --- /dev/null +++ b/cmd/cli/recovery_os_refresh_test.go @@ -0,0 +1,194 @@ +package cli + +import ( + "context" + "errors" + "slices" + "sync" + "testing" + "time" + + "github.com/Control-D-Inc/ctrld" +) + +// standardRecoveryHarness runs handleRecovery in Standard mode with the DNS +// reset and the system nameserver reads stubbed, and records their order. +type standardRecoveryHarness struct { + *recoveryEventHarness + mu sync.Mutex + events []string +} + +func (h *standardRecoveryHarness) record(event string) { + h.mu.Lock() + defer h.mu.Unlock() + h.events = append(h.events, event) +} + +func (h *standardRecoveryHarness) snapshot() []string { + h.mu.Lock() + defer h.mu.Unlock() + return append([]string{}, h.events...) +} + +func (h *standardRecoveryHarness) reads() int { + n := 0 + for _, e := range h.snapshot() { + if e == "read" { + n++ + } + } + return n +} + +func newStandardRecoveryHarness(t *testing.T) *standardRecoveryHarness { + t.Helper() + h := &standardRecoveryHarness{recoveryEventHarness: newRecoveryEventHarness(t)} + dnsIntercept = false // restored by the event harness + oldReset, oldSet, oldRead, oldIntervals := recoveryResetDNSFn, recoverySetDNSFn, initializeOsResolver, osResolverRecoveryRefreshIntervals + t.Cleanup(func() { + recoveryResetDNSFn, recoverySetDNSFn, initializeOsResolver, osResolverRecoveryRefreshIntervals = oldReset, oldSet, oldRead, oldIntervals + }) + recoveryResetDNSFn = func(*prog, bool, bool) { h.record("reset") } + recoverySetDNSFn = func(*prog, []string) { h.record("setDNS") } + initializeOsResolver = func(bool, string) []string { + h.record("read") + return []string{"[fd66:6:6::1]:53", "76.76.2.0:53"} + } + osResolverRecoveryRefreshIntervals = []time.Duration{refreshTestInterval} + return h +} + +const refreshTestInterval = 5 * time.Millisecond + +// errUpstreamStillDown ends a test recovery before it re-applies DNS settings, +// so the test never touches the host's DNS configuration. +var errUpstreamStillDown = errors.New("upstream still down") + +// After a network change, Standard mode removes its DNS settings to expose the +// new network's resolvers. It must read them right away and keep reading them +// while it waits: an IPv6-only network publishes its resolver by RDNSS, and an +// IPv4-only upstream there only comes back once macOS starts its CLAT, which +// needs ctrld's OS-resolver fallback to reach that resolver (#628). +func TestNetworkChangeRecoveryRefreshesOSResolver(t *testing.T) { + h := newStandardRecoveryHarness(t) + var readsAtWaitStart int + waitForUpstreamRecoveryFn = func(_ *prog, ctx context.Context, _ map[string]*ctrld.UpstreamConfig, _ *recoveryDiagnostic) (string, error) { + readsAtWaitStart = h.reads() + deadline := time.Now().Add(2 * time.Second) + for h.reads() < readsAtWaitStart+3 { + if time.Now().After(deadline) { + return "", errors.New("the resolvers were not re-read during the wait") + } + time.Sleep(time.Millisecond) + } + return "", errUpstreamStillDown + } + + h.prog.handleRecovery(RecoveryReasonNetworkChange) + + events := h.snapshot() + if len(events) < 2 || events[0] != "reset" || events[1] != "read" { + t.Fatalf("events = %v, want the DNS reset followed by a resolver read", events) + } + if readsAtWaitStart != 1 { + t.Fatalf("resolver reads before the wait = %d, want 1", readsAtWaitStart) + } + if got := h.reads(); got < readsAtWaitStart+3 { + t.Fatalf("resolver reads during the wait = %d, want at least 3", got-readsAtWaitStart) + } + + // The refresh ends with the wait. + after := h.reads() + time.Sleep(10 * refreshTestInterval) + if got := h.reads(); got != after { + t.Fatalf("resolver reads went on after the wait: %d -> %d", after, got) + } +} + +// A recovery for an upstream failure keeps the network it is on, so it does +// not re-read the resolvers, as before. +func TestUpstreamFailureRecoveryKeepsOSResolver(t *testing.T) { + h := newStandardRecoveryHarness(t) + waitForUpstreamRecoveryFn = func(_ *prog, _ context.Context, _ map[string]*ctrld.UpstreamConfig, _ *recoveryDiagnostic) (string, error) { + time.Sleep(10 * refreshTestInterval) + return "", errUpstreamStillDown + } + + h.prog.handleRecovery(RecoveryReasonRegularFailure) + + if got := h.reads(); got != 0 { + t.Fatalf("resolver reads = %d (events %v), want none", got, h.snapshot()) + } +} + +// Once an upstream answers, the refresher stops before the completion re-reads +// the resolvers and re-applies the DNS settings, so no read races the restore. +func TestNetworkChangeRecoveryStopsRefreshBeforeCompletion(t *testing.T) { + h := newStandardRecoveryHarness(t) + waitForUpstreamRecoveryFn = func(_ *prog, _ context.Context, _ map[string]*ctrld.UpstreamConfig, _ *recoveryDiagnostic) (string, error) { + readsAtWaitStart := h.reads() + deadline := time.Now().Add(2 * time.Second) + for h.reads() < readsAtWaitStart+2 { + if time.Now().After(deadline) { + return "", errors.New("the resolvers were not re-read during the wait") + } + time.Sleep(time.Millisecond) + } + h.record("recovered") + return h.recovered, nil + } + // The restore outlasts several refresh gaps, so a refresher still running + // would read during it. + recoverySetDNSFn = func(*prog, []string) { + h.record("setDNS") + time.Sleep(10 * refreshTestInterval) + } + + h.prog.handleRecovery(RecoveryReasonNetworkChange) + + events := h.snapshot() + i := slices.Index(events, "recovered") + if i < 0 { + t.Fatalf("events = %v, want the upstream to recover", events) + } + if got, want := events[i+1:], []string{"read", "setDNS"}; !slices.Equal(got, want) { + t.Fatalf("events after the recovery = %v, want %v (all: %v)", got, want, events) + } +} + +// The refresher reads at the configured gaps and repeats the last one, so a +// long outage reads at the last, longest gap rather than at the first. +func TestNetworkChangeRecoveryRefreshBacksOff(t *testing.T) { + h := newStandardRecoveryHarness(t) + osResolverRecoveryRefreshIntervals = []time.Duration{refreshTestInterval, time.Hour} + waitForUpstreamRecoveryFn = func(_ *prog, _ context.Context, _ map[string]*ctrld.UpstreamConfig, _ *recoveryDiagnostic) (string, error) { + time.Sleep(20 * refreshTestInterval) + return "", errUpstreamStillDown + } + + h.prog.handleRecovery(RecoveryReasonNetworkChange) + + // The immediate read after the reset, then one at the first gap. + if got := h.reads(); got != 2 { + t.Fatalf("resolver reads = %d (events %v), want 2", got, h.snapshot()) + } +} + +// The read after the reset finds the new network's nameservers, so the OS +// resolver no longer counts as read before the network change. +func TestNetworkChangeRecoveryReadClearsStaleOSResolver(t *testing.T) { + h := newStandardRecoveryHarness(t) + h.prog.markOSResolverStale() + var staleAtWait bool + waitForUpstreamRecoveryFn = func(p *prog, _ context.Context, _ map[string]*ctrld.UpstreamConfig, _ *recoveryDiagnostic) (string, error) { + staleAtWait = p.osResolverStale() + return "", errUpstreamStillDown + } + + h.prog.handleRecovery(RecoveryReasonNetworkChange) + + if staleAtWait { + t.Fatal("OS resolver still stale after the post-reset read found nameservers") + } +} diff --git a/nameservers.go b/nameservers.go index 0aebf9e..460a387 100644 --- a/nameservers.go +++ b/nameservers.go @@ -4,10 +4,19 @@ type dnsFn func() []string // nameservers returns DNS nameservers from system settings. func nameservers() []string { + return nameserversFrom(dnsFns()) +} + +// quickNameservers is nameservers without waiting for the system to publish +// them: a source that finds none returns at once instead of retrying. +func quickNameservers() []string { + return nameserversFrom(quickDNSFns()) +} + +func nameserversFrom(fns []dnsFn) []string { var dns []string seen := make(map[string]bool) ch := make(chan []string) - fns := dnsFns() for _, fn := range fns { go func(fn dnsFn) { diff --git a/nameservers_darwin.go b/nameservers_darwin.go index 65ac705..d1f880a 100644 --- a/nameservers_darwin.go +++ b/nameservers_darwin.go @@ -22,9 +22,21 @@ func dnsFns() []dnsFn { return []dnsFn{dnsFromResolvConf, getDNSFromScutil, getAllDHCPNameservers} } +// quickDNSFns reads scutil once. While ctrld's own address is the configured +// DNS server, scutil reports nothing else, and retrying only delays the read. +func quickDNSFns() []dnsFn { + return []dnsFn{dnsFromResolvConf, func() []string { return getDNSFromScutilAttempts(1) }, getAllDHCPNameservers} +} + var scutilLocalAddresses = netmon.LocalAddresses func getDNSFromScutil() []string { + return getDNSFromScutilAttempts(10) +} + +// getDNSFromScutilAttempts reads scutil --dns up to maxRetries times, until it +// reports a nameserver that is not a local address. +func getDNSFromScutilAttempts(maxRetries int) []string { logger := *ProxyLogger.Load() // Skip scutil on mobile platforms - not available in sandbox @@ -33,10 +45,7 @@ func getDNSFromScutil() []string { return nil } - const ( - maxRetries = 10 - retryInterval = 100 * time.Millisecond - ) + const retryInterval = 100 * time.Millisecond regularIPs, loopbackIPs, _ := scutilLocalAddresses() diff --git a/nameservers_quick_other.go b/nameservers_quick_other.go new file mode 100644 index 0000000..fad0492 --- /dev/null +++ b/nameservers_quick_other.go @@ -0,0 +1,8 @@ +//go:build !darwin + +package ctrld + +// quickDNSFns is dnsFns: no other platform's sources retry. +func quickDNSFns() []dnsFn { + return dnsFns() +} diff --git a/nameservers_scutil_darwin_test.go b/nameservers_scutil_darwin_test.go index 55b5e0c..1ff8ac9 100644 --- a/nameservers_scutil_darwin_test.go +++ b/nameservers_scutil_darwin_test.go @@ -49,3 +49,40 @@ func TestScutilIPv6ProductionReader(t *testing.T) { t.Fatalf("local-address reads=%d, want 1", localReads) } } + +// While ctrld's own address is the only configured server, the full read +// retries scutil, and the quick read runs it once. +func TestScutilQuickReadDoesNotRetry(t *testing.T) { + old := scutilLocalAddresses + t.Cleanup(func() { scutilLocalAddresses = old }) + scutilLocalAddresses = func() ([]netip.Addr, []netip.Addr, error) { + return nil, []netip.Addr{netip.MustParseAddr("127.0.0.1")}, nil + } + dir := t.TempDir() + count := filepath.Join(dir, "count") + script := "#!/bin/sh\necho x >> '" + count + "'\nprintf '%s\\n' 'nameserver[0] : 127.0.0.1'\n" + if err := os.WriteFile(filepath.Join(dir, "scutil"), []byte(script), 0700); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", dir) + runs := func() int { + b, _ := os.ReadFile(count) + os.Remove(count) + return len(b) / 2 + } + for _, tc := range []struct { + name string + read func() []string + want int + }{ + {"quick", func() []string { return getDNSFromScutilAttempts(1) }, 1}, + {"full", getDNSFromScutil, 10}, + } { + if got := tc.read(); len(got) != 0 { + t.Fatalf("%s read = %v, want none", tc.name, got) + } + if got := runs(); got != tc.want { + t.Fatalf("%s read ran scutil %d times, want %d", tc.name, got, tc.want) + } + } +} diff --git a/resolver.go b/resolver.go index fb0f9e5..13c10d2 100644 --- a/resolver.go +++ b/resolver.go @@ -172,8 +172,9 @@ func defaultNameservers() []string { return nss } -// availableNameservers returns list of current available DNS servers of the system. -func availableNameservers() []string { +// availableNameservers returns list of current available DNS servers of the +// system, as read reports them. +func availableNameservers(read func() []string) []string { var nss []string // Ignore local addresses to prevent loop. regularIPs, loopbackIPs, _ := netmon.LocalAddresses() @@ -192,7 +193,7 @@ func availableNameservers() []string { "Added local IP to OS resolverexclusion map: %s", ipStr) } - systemNameservers := NameserversFn() + systemNameservers := read() logSystemNameservers(systemNameservers) for _, ns := range systemNameservers { @@ -303,13 +304,30 @@ func InitializeOsResolverWithSystemNameservers(guardAgainstNoNameservers bool) ( // InitializeOsResolverWithSystemNameservers with the reason that the journal // reports when the resolver set changes. func InitializeOsResolverWithSystemNameserversReason(guardAgainstNoNameservers bool, reason string) (effective, system []string) { + return initializeOsResolverFrom(NameserversFn, guardAgainstNoNameservers, reason) +} + +// InitializeOsResolverQuickWithReason is InitializeOsResolverWithReason, with +// guardAgainstNoNameservers set, that does not wait for the system to publish +// its nameservers. It suits a read that may run while ctrld's own address is +// the configured DNS server, where on macOS the full read retries scutil for +// about a second, holding the lock every OS resolver query and lookup takes. +func InitializeOsResolverQuickWithReason(reason string) []string { + effective, _ := initializeOsResolverFrom(quickNameserversFn, true, reason) + return effective +} + +// quickNameserversFn is NameserversFn for InitializeOsResolverQuickWithReason. +var quickNameserversFn = quickNameservers + +func initializeOsResolverFrom(read func() []string, guardAgainstNoNameservers bool, reason string) (effective, system []string) { resolverMutex.Lock() defer resolverMutex.Unlock() setOsResolverReason(reason) defer setOsResolverReason(osResolverReasonUnspecified) - system = availableNameservers() + system = availableNameservers(read) 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.