From b6d5abfae8e076bce00a51b1670aaeca638adc17 Mon Sep 17 00:00:00 2001 From: Cuong Manh Le Date: Tue, 13 Oct 2026 17:43:57 +0700 Subject: [PATCH] cli: hold NAT64 discovery while the OS resolver predates a network change After a network change, the OS resolver keeps the previous network's nameservers until a read finds the new ones. In Standard mode that usually waits for the recovery to remove ctrld's DNS settings: while 127.0.0.1 is configured, the system reports nothing else, and on macOS an RDNSS-only IPv6 network has no other source. macOS resolves ipv4only.arpa through ctrld right after the change to discover the NAT64 prefix; a fast SERVFAIL from the stale set ends that discovery, macOS never starts its CLAT, and an IPv4-only upstream never recovers. On every accepted network change, on all platforms and in both Standard and intercept mode, mark the OS resolver stale and read the nameservers once before the re-bootstrap, without the scutil retry loop. While the set is stale, a failed ipv4only.arpa AAAA lookup gets no reply, so the host retries it. A read that finds nameservers clears the mark, and the mark expires after 30s. Network-change recovery also re-reads the nameservers right after the reset and then at 1s, 2s, 5s and every 30s while it waits, for networks that publish their resolvers late. --- cmd/cli/dns_intercept_settle.go | 7 + cmd/cli/dns_proxy.go | 99 +++++++++++++- cmd/cli/network_transition.go | 2 + cmd/cli/network_transition_test.go | 4 + cmd/cli/os_resolver_stale.go | 74 +++++++++++ cmd/cli/os_resolver_stale_test.go | 174 +++++++++++++++++++++++++ cmd/cli/prog.go | 5 + cmd/cli/recovery_os_refresh_test.go | 194 ++++++++++++++++++++++++++++ nameservers.go | 11 +- nameservers_darwin.go | 17 ++- nameservers_quick_other.go | 8 ++ nameservers_scutil_darwin_test.go | 37 ++++++ resolver.go | 26 +++- 13 files changed, 642 insertions(+), 16 deletions(-) create mode 100644 cmd/cli/os_resolver_stale.go create mode 100644 cmd/cli/os_resolver_stale_test.go create mode 100644 cmd/cli/recovery_os_refresh_test.go create mode 100644 nameservers_quick_other.go 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.