From 462f912495c1593f20ed3e946025600272b22923 Mon Sep 17 00:00:00 2001 From: Dev Scribe Date: Thu, 24 Sep 2026 13:13:31 +0000 Subject: [PATCH] Harden DNS64, VPN routing and DNS recovery ownership --- cmd/cli/cli.go | 11 +- cmd/cli/dns64.go | 29 +++- cmd/cli/dns64_dnssec_test.go | 139 +++++++++++++++ cmd/cli/dns_intercept_darwin.go | 30 +++- cmd/cli/dns_proxy.go | 7 +- cmd/cli/dns_target_darwin.go | 100 +++++++++-- cmd/cli/dns_target_darwin_test.go | 3 + cmd/cli/dns_target_diagnostic_darwin_test.go | 13 +- cmd/cli/dns_target_ownership_darwin_test.go | 168 +++++++++++++++++++ cmd/cli/dns_target_stub.go | 2 + cmd/cli/listener_5353_darwin_test.go | 67 ++++++++ cmd/cli/listener_5353_linux_test.go | 32 ++++ cmd/cli/listener_5353_target_darwin_test.go | 41 +++++ cmd/cli/listener_5353_test.go | 34 ++++ cmd/cli/prog.go | 27 +++ cmd/cli/vpn_dns.go | 33 ++-- cmd/cli/vpn_dns_convergence_test.go | 133 +++++++++++++++ docs/619-target-ownership-limits.md | 17 ++ nameservers_darwin.go | 36 +--- nameservers_scutil.go | 45 +++++ nameservers_scutil_darwin_test.go | 51 ++++++ nameservers_scutil_test.go | 80 +++++++++ resolver.go | 9 +- 23 files changed, 1021 insertions(+), 86 deletions(-) create mode 100644 cmd/cli/dns64_dnssec_test.go create mode 100644 cmd/cli/dns_target_ownership_darwin_test.go create mode 100644 cmd/cli/listener_5353_darwin_test.go create mode 100644 cmd/cli/listener_5353_linux_test.go create mode 100644 cmd/cli/listener_5353_target_darwin_test.go create mode 100644 cmd/cli/listener_5353_test.go create mode 100644 cmd/cli/vpn_dns_convergence_test.go create mode 100644 docs/619-target-ownership-limits.md create mode 100644 nameservers_scutil.go create mode 100644 nameservers_scutil_darwin_test.go create mode 100644 nameservers_scutil_test.go diff --git a/cmd/cli/cli.go b/cmd/cli/cli.go index b6cf64f..1f5374f 100644 --- a/cmd/cli/cli.go +++ b/cmd/cli/cli.go @@ -1482,12 +1482,19 @@ func uninstall(p *prog, s service.Service) { } } +var restoreSavedStaticDNSInterfacesFn = withEachPhysicalInterfaces +var restoreSavedStaticDNSRestoreFn = restoreDNS + // restoreSavedStaticDNS restores DNS from saved static config files on physical interfaces. func restoreSavedStaticDNS(excludeIfaceName string, removeSaved bool) { - withEachPhysicalInterfaces(excludeIfaceName, "restore static DNS", func(i *net.Interface) error { + restoreSavedStaticDNSInterfacesFn(excludeIfaceName, "restore static DNS", func(i *net.Interface) error { + if !interceptTargetAllowsStaticRestore(i.Name) { + mainLog.Load().Debug().Msgf("Saved static DNS restore skipped on interface %s: intercept target cleanup is pending or ownership is unreadable", i.Name) + return nil + } file := savedStaticDnsSettingsFilePath(i) if _, err := os.Stat(file); err == nil { - if err := restoreDNS(i); err != nil { + if err := restoreSavedStaticDNSRestoreFn(i); err != nil { mainLog.Load().Error().Err(err).Msgf("Could not restore static DNS on interface %s", i.Name) } else { mainLog.Load().Debug().Msgf("Restored static DNS on interface %s successfully", i.Name) diff --git a/cmd/cli/dns64.go b/cmd/cli/dns64.go index b8d3989..8854575 100644 --- a/cmd/cli/dns64.go +++ b/cmd/cli/dns64.go @@ -175,6 +175,9 @@ func synthesizeAAAAFromA(req *dns.Msg, aAnswer *dns.Msg, prefix netip.Prefix) *d out.SetReply(req) out.Rcode = aAnswer.Rcode out.Compress = true + // An authenticated A RRset alone cannot authenticate synthesized AAAA. + // maybeDNS64 also checks the original AAAA denial before setting AD. + out.AuthenticatedData = false answers := make([]dns.RR, 0, len(aAnswer.Answer)) pb := prefix.Addr().As16() for _, rr := range aAnswer.Answer { @@ -228,10 +231,7 @@ func answerHasAAAA(answer *dns.Msg) bool { // rcodes are never synthesized (RFC 6147 ยง5.1.2: the name genuinely does // not exist or the query failed). func dns64Eligible(req, answer *dns.Msg) bool { - if req == nil || answer == nil || len(req.Question) == 0 || req.CheckingDisabled { - return false - } - if req.Question[0].Qtype != dns.TypeAAAA { + if !dns64RequestAllowed(req) || answer == nil { return false } if answer.Rcode != dns.RcodeSuccess { @@ -240,6 +240,13 @@ func dns64Eligible(req, answer *dns.Msg) bool { return !answerHasAAAA(answer) } +// Apply the same client gate to cold synthesis and fresh/stale variants. +// In particular, RFC 6147 section 5.5 forbids synthesis for CD+DO clients. +// Retain the existing conservative bypass for all CD requests. +func dns64RequestAllowed(req *dns.Msg) bool { + return req != nil && len(req.Question) > 0 && req.Question[0].Qtype == dns.TypeAAAA && !req.CheckingDisabled +} + // dns64Active reports whether synthesis should currently run, re-evaluating // network class and (if needed) kicking off background prefix discovery at // most every dns64RecheckInterval. @@ -307,7 +314,13 @@ func dns64CacheVariant(prefix netip.Prefix) string { } func dns64CacheKey(msg *dns.Msg, upstream string, prefix netip.Prefix) dnscache.Key { - return dnscache.NewVariantKey(msg, upstream, dns64CacheVariant(prefix)) + variant := dns64CacheVariant(prefix) + // DO changes the authentication and DNSSEC data in a synthesized reply. + // Do not reuse a non-DO result for a DO client (or the reverse). + if opt := msg.IsEdns0(); opt != nil && opt.Do() { + variant += ":do" + } + return dnscache.NewVariantKey(msg, upstream, variant) } func (p *prog) resetDNS64State() { @@ -476,6 +489,12 @@ func (p *prog) maybeDNS64(ctx context.Context, req *dns.Msg, answer *dns.Msg, re // DNS64 variant to avoid repeating both upstream lookups. return answer, prefix } + // RFC 6147 section 5.5: AD may describe synthesis only when both the + // AAAA denial and the A response were authenticated by our upstream. + // Without DO, synthesized responses must not assert AD. + if opt := req.IsEdns0(); opt != nil && opt.Do() { + synth.AuthenticatedData = answer.AuthenticatedData && aAnswer.AuthenticatedData + } ctrld.Log(ctx, mainLog.Load().Debug(), "dns64: synthesized AAAA from A records via NAT64 prefix %s", prefix) return synth, prefix } diff --git a/cmd/cli/dns64_dnssec_test.go b/cmd/cli/dns64_dnssec_test.go new file mode 100644 index 0000000..6c862db --- /dev/null +++ b/cmd/cli/dns64_dnssec_test.go @@ -0,0 +1,139 @@ +package cli + +import ( + "context" + "fmt" + "net" + "testing" + "time" + + "github.com/miekg/dns" + ctrld "github.com/Control-D-Inc/ctrld" + "github.com/Control-D-Inc/ctrld/internal/dnscache" +) + +func TestDNS64DNSSECCacheAndSynthesis(t *testing.T) { + captureDebugMainLog(t) + for _, do := range []bool{false, true} { + for _, denialAD := range []bool{false, true} { + for _, aAD := range []bool{false, true} { + t.Run(fmt.Sprintf("DO=%v/denialAD=%v/A_AD=%v", do, denialAD, aAD), func(t *testing.T) { + req := mkAAAAReq("dnssec.example.") + req.SetEdns0(1232, do) + denial := new(dns.Msg) + denial.SetReply(req) + denial.AuthenticatedData = denialAD + cache, err := dnscache.NewLRUCache(10) + if err != nil { + t.Fatal(err) + } + p := &prog{cache: cache} + p.dns64.active = true + p.dns64.prefix = dns64WellKnownPrefix + p.dns64.checkedAt = time.Now() + calls := 0 + resolveA := func(q *dns.Msg) *dns.Msg { + calls++ + if q.Question[0].Qtype != dns.TypeA { + t.Fatal("not an A lookup") + } + a := new(dns.Msg) + a.SetReply(q) + a.AuthenticatedData = aAD + a.Answer = []dns.RR{&dns.A{Hdr: dns.RR_Header{Name: q.Question[0].Name, Rrtype: dns.TypeA, Class: dns.ClassINET, Ttl: 60}, A: net.ParseIP("198.51.100.7")}} + return a + } + out, prefix := p.maybeDNS64(context.Background(), req, denial, resolveA) + wantAD := do && denialAD && aAD + if calls != 1 || !answerHasAAAA(out) || !prefix.IsValid() || out.AuthenticatedData != wantAD { + t.Fatalf("cold: calls=%d AAAA=%v prefix=%v AD=%v wantAD=%v", calls, answerHasAAAA(out), prefix, out.AuthenticatedData, wantAD) + } + now := time.Now() + for _, stale := range []bool{false, true} { + expiry := now.Add(time.Minute) + if stale { + expiry = now.Add(-time.Minute) + } + cache.Add(dns64CacheKey(req, "upstream.0", prefix), dnscache.NewValue(out, expiry)) + answer, old, hit, variant, _ := p.cachedResponse(req, "upstream.0", prefix, true, now) + got := answer + if stale { + got = old + } + if got == nil || !answerHasAAAA(got) || got.AuthenticatedData != wantAD || hit == stale || variant == stale { + t.Fatalf("normal cache stale=%v: answer=%v old=%v hit=%v variant=%v", stale, answer, old, hit, variant) + } + cd := req.Copy() + cd.CheckingDisabled = true + answer, old, hit, variant, _ = p.cachedResponse(cd, "upstream.0", prefix, true, now) + if answerHasAAAA(answer) || answerHasAAAA(old) || hit || variant { + t.Errorf("CD cache stale=%v leaked synthesis: answer=%v old=%v hit=%v variant=%v", stale, answer, old, hit, variant) + } + cold, coldPrefix := p.maybeDNS64(context.Background(), cd, denial, resolveA) + if cold != denial || coldPrefix.IsValid() || calls != 1 { + t.Fatal("CD cold path performed synthesis") + } + } + if denial.AuthenticatedData != denialAD { + t.Fatal("mutated denial") + } + }) + } + } + } +} + +func TestDNS64DNSSECOrdinaryCacheFallback(t *testing.T) { + req := mkAAAAReq("dnssec.example.") + req.CheckingDisabled = true + req.SetEdns0(1232, true) + ordinary := new(dns.Msg) + ordinary.SetReply(req) + cache, err := dnscache.NewLRUCache(10) + if err != nil { + t.Fatal(err) + } + p := &prog{cache: cache} + now := time.Now() + cache.Add(dnscache.NewKey(req, "upstream.0"), dnscache.NewValue(ordinary, now.Add(time.Minute))) + answer, _, hit, variant, bypass := p.cachedResponse(req, "upstream.0", dns64WellKnownPrefix, true, now) + if !hit || variant || bypass || answerHasAAAA(answer) { + t.Fatalf("ordinary cache: hit=%v variant=%v bypass=%v answer=%v", hit, variant, bypass, answer) + } +} + +func TestRecoveryOSOnlyCandidatePool(t *testing.T) { + for _, reason := range []RecoveryReason{RecoveryReasonNetworkChange, RecoveryReasonRegularFailure} { + p := &prog{cfg: &ctrld.Config{Upstream: map[string]*ctrld.UpstreamConfig{"0": {Type: ctrld.ResolverTypeOS}, "nil": nil}}} + pool := p.buildRecoveryUpstreams(reason) + if len(pool) != 1 || pool[upstreamOS] != osUpstreamConfig { + t.Errorf("reason=%v: OS-only recovery candidates=%v", reason, pool) + } + } +} + +func TestDNS64DNSSECCacheDOIsolation(t *testing.T) { + for _, do := range []bool{false, true} { + req := mkAAAAReq("dnssec.example.") + req.SetEdns0(1232, do) + response := new(dns.Msg) + response.SetReply(req) + response.AuthenticatedData = do + response.Answer = []dns.RR{&dns.AAAA{Hdr: dns.RR_Header{Name: req.Question[0].Name, Rrtype: dns.TypeAAAA, Class: dns.ClassINET, Ttl: 60}, AAAA: net.ParseIP("64:ff9b::c633:6407")}} + cache, err := dnscache.NewLRUCache(10) + if err != nil { + t.Fatal(err) + } + p := &prog{cache: cache} + now := time.Now() + other := req.Copy() + other.IsEdns0().SetDo(!do) + for _, expiry := range []time.Time{now.Add(time.Minute), now.Add(-time.Minute)} { + cache.Add(dns64CacheKey(req, "upstream.0", dns64WellKnownPrefix), dnscache.NewValue(response, expiry)) + answer, stale, hit, variant, _ := p.cachedResponse(other, "upstream.0", dns64WellKnownPrefix, true, now) + if answer != nil || stale != nil || hit || variant { + t.Fatalf("DO=%v result reused for DO=%v: answer=%v stale=%v", do, !do, answer, stale) + } + } + } +} diff --git a/cmd/cli/dns_intercept_darwin.go b/cmd/cli/dns_intercept_darwin.go index 68a7dbb..b4ddc8b 100644 --- a/cmd/cli/dns_intercept_darwin.go +++ b/cmd/cli/dns_intercept_darwin.go @@ -696,6 +696,21 @@ func flushPFStates() { } } +// pfNameserverAddress keeps socket zones out of group-scoped PF addresses. +// ctrld already has a blanket group exemption in the same anchor. +func pfNameserverAddress(server string) (string, string, bool) { + host, _, err := net.SplitHostPort(server) + if err != nil { + host = server + } + addr, err := netip.ParseAddr(host) + if err != nil { + return "", "", false + } + host = addr.WithZone("").Unmap().String() + return host, pfAddressFamily(host), true +} + func pfAddressFamily(ip string) string { if addr := net.ParseIP(ip); addr != nil && addr.To4() == nil { return "inet6" @@ -886,11 +901,10 @@ func (p *prog) buildPFAnchorRulesForTunnels(vpnExemptions []vpnDNSExemption, tun rules.WriteString("# Scoped to group " + pfGroupName + " so only ctrld's own queries are exempted,\n") rules.WriteString("# preventing other processes from bypassing the redirect by querying these IPs.\n") for _, ns := range osNS { - host, _, _ := net.SplitHostPort(ns) - if host == "" { - host = ns + host, af, ok := pfNameserverAddress(ns) + if !ok { + continue } - af := pfAddressFamily(host) rules.WriteString(fmt.Sprintf("pass out quick on ! lo0 %s proto { udp, tcp } from any to %s port 53 group %s\n", af, host, pfGroupName)) } rules.WriteString("\n") @@ -932,10 +946,10 @@ func (p *prog) buildPFAnchorRulesForTunnels(vpnExemptions []vpnDNSExemption, tun rules.WriteString("# Exempt VPN DNS servers: ctrld's own queries (group-scoped).\n") seen := make(map[string]bool) for _, ex := range vpnExemptions { - if !seen[ex.Server] { - seen[ex.Server] = true - af := pfAddressFamily(ex.Server) - rules.WriteString(fmt.Sprintf("pass out quick on ! lo0 %s proto { udp, tcp } from any to %s port 53 group %s\n", af, ex.Server, pfGroupName)) + host, af, ok := pfNameserverAddress(ex.Server) + if ok && !seen[host] { + seen[host] = true + rules.WriteString(fmt.Sprintf("pass out quick on ! lo0 %s proto { udp, tcp } from any to %s port 53 group %s\n", af, host, pfGroupName)) } } rules.WriteString("\n") diff --git a/cmd/cli/dns_proxy.go b/cmd/cli/dns_proxy.go index d2099ee..87e0b1b 100644 --- a/cmd/cli/dns_proxy.go +++ b/cmd/cli/dns_proxy.go @@ -124,7 +124,7 @@ func (p *prog) addCachedResponse(key dnscache.Key, answer *dns.Msg) { } func (p *prog) cachedResponse(req *dns.Msg, upstream string, dns64Prefix netip.Prefix, dns64Active bool, now time.Time) (answer, stale *dns.Msg, hit, dns64Hit, dns64Bypass bool) { - if dns64Active { + if dns64Active && dns64RequestAllowed(req) { if cachedValue := p.cache.Get(dns64CacheKey(req, upstream, dns64Prefix)); cachedValue != nil { answer = cachedValue.Msg.Copy() ctrld.SetCacheReply(answer, req, answer.Rcode) @@ -2719,6 +2719,11 @@ func (p *prog) buildRecoveryUpstreams(reason RecoveryReason) map[string]*ctrld.U upstreams[upstreamPrefix+k] = uc } } + // An OS-only configuration still needs a recovery worker. Do not add + // OS to mixed pools: their existing recovery boundaries stay intact. + if len(upstreams) == 0 { + upstreams[upstreamOS] = osUpstreamConfig + } } return upstreams } diff --git a/cmd/cli/dns_target_darwin.go b/cmd/cli/dns_target_darwin.go index 4f2fb32..95cb79b 100644 --- a/cmd/cli/dns_target_darwin.go +++ b/cmd/cli/dns_target_darwin.go @@ -8,6 +8,7 @@ import ( "errors" "net" "os" + "path/filepath" "slices" "tailscale.com/net/netmon" @@ -30,13 +31,38 @@ var ( interceptSaveStaticDNSSnapshotFn = func(iface *net.Interface, dns []string, owned string) error { return saveTargetStaticDNSSnapshot(savedStaticDnsSettingsFilePath(iface), dns, owned) } - interceptSaveCurrentStaticDNSFn = saveCurrentStaticDNS + interceptSaveCurrentStaticDNSFn = saveCurrentStaticDNS + interceptRemoveSavedDNSFn = func(iface *net.Interface) error { + err := os.Remove(savedStaticDnsSettingsFilePath(iface)) + if errors.Is(err, os.ErrNotExist) { + return nil + } + return err + } interceptSetDNSFn = setDNS interceptSavedStaticNameserversFn = savedStaticNameservers interceptResetDNSIgnoreUnusableIfaceFn = resetDnsIgnoreUnusableInterface interceptDHCPNameserversForInterfaceFn = readTargetDHCPNameservers ) +// A retained target record means cleanup was not completed. The broad saved +// static sweep must not bypass the target's ownership check or retry failures +// through an unguarded restore. Unknown state is conservatively left alone. +func interceptTargetAllowsStaticRestore(service string) bool { + data, err := os.ReadFile(interceptDNSTargetStatePathFn()) + if errors.Is(err, os.ErrNotExist) { + return true + } + if err != nil { + return false + } + var st interceptDNSTargetState + if json.Unmarshal(data, &st) != nil || st.Service == "" { + return false + } + return st.Service != service +} + type interceptDNSTargetState struct { Service string `json:"service"` Value string `json:"value"` @@ -64,23 +90,34 @@ func (p *prog) loadInterceptDNSTargetStateLocked() { // persistInterceptDNSTargetStateLocked writes (or clears) the state file to // match in-memory tracking. Callers must hold interceptDNSTargetMu. -func (p *prog) persistInterceptDNSTargetStateLocked() { +func (p *prog) persistInterceptDNSTargetStateLocked() error { file := interceptDNSTargetStatePathFn() if p.interceptDNSTargetService == "" { - _ = os.Remove(file) - return + if err := os.Remove(file); err != nil && !errors.Is(err, os.ErrNotExist) { + return err + } + return nil } data, err := json.Marshal(interceptDNSTargetState{Service: p.interceptDNSTargetService, Value: p.interceptDNSTargetSetValue}) if err != nil { - return + return err } - if err := os.WriteFile(file, data, 0600); err != nil { - mainLog.Load().Debug().Err(err).Msg("intercept DNS target: could not persist state file") + // Replace atomically; a failed write must not truncate prior ownership. + tmp, err := os.CreateTemp(filepath.Dir(file), ".intercept-target-*") + if err != nil { + return err } + defer os.Remove(tmp.Name()) + _, writeErr := tmp.Write(data) + err = errors.Join(writeErr, tmp.Close()) + if err != nil { + return err + } + return os.Rename(tmp.Name(), file) } -// ensureInterceptDNSTarget guarantees macOS always has an emittable DNS -// target while DNS intercept mode is active. +// ensureInterceptDNSTarget provides an emittable DNS target when ownership +// and discovery permit it; externally changed DNS is not permission to reinstall. // // Intercept mode deliberately never manages interface DNS: pf redirects DNS // packets in flight. But pf can only redirect packets macOS actually sends, @@ -192,6 +229,14 @@ func (p *prog) ensureInterceptDNSTarget(systemDiscovery []string) { return } } + // Tracking is not proof that the target is still installed. An external + // edit (including clearing DNS) is not permission to reinstall it. Retain + // the cleanup record until shutdown, preventing periodic reconciliation + // from reinterpreting the same edit as a new DNS-less admission. + if p.interceptDNSTargetService == iface.Name && !isInterceptDNSTargetOnly(snapshot, p.interceptDNSTargetSetValue) && !hasIPv4DNS(filterOwnTarget(snapshot, p.interceptDNSTargetSetValue)) { + diagnostic.failed(decision, "owned_target_changed", nil) + 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. @@ -256,12 +301,21 @@ func (p *prog) ensureInterceptDNSTarget(systemDiscovery []string) { mainLog.Load().Debug().Err(err).Msgf("intercept DNS target: could not save static DNS for %q", iface.Name) } } + p.setInterceptDNSTargetLocked(iface.Name, target) + if err := p.persistInterceptDNSTargetStateLocked(); err != nil { + p.setInterceptDNSTargetLocked("", "") + resolvedReason = "" + diagnostic.failed(decision, "ownership_persistence", err) + mainLog.Load().Warn().Err(err).Msg("intercept DNS target: not changing DNS without persisted ownership") + return + } if err := interceptSetDNSFn(iface, []string{target}); err != nil { + // A command error can follow a partial OS mutation. Keep the durable + // cleanup record; the next observation decides what still belongs to us. + resolvedReason = "" mainLog.Load().Warn().Err(err).Msgf("intercept DNS target: could not set %s on %q", target, iface.Name) return } - p.setInterceptDNSTargetLocked(iface.Name, target) - p.persistInterceptDNSTargetStateLocked() journal(mainLog.Load().Warn()).Str("service", iface.Name).Str("target", target). Bool("native_clat_fallback", nativeFallback). Str("reason", "dns_less_network"). @@ -295,7 +349,15 @@ func (p *prog) removeInterceptDNSTargetLocked(reason string) { return } if !isInterceptDNSTargetOnly(cur, val) { - p.clearInterceptDNSTargetStateLocked() + // The generic stop/uninstall sweep must not replay a stale snapshot + // over the external edit. On failure keep ownership so its guard skips. + if err := interceptRemoveSavedDNSFn(iface); err != nil { + mainLog.Load().Warn().Err(err).Msg("intercept DNS target: could not discard stale backup; retaining cleanup state") + return + } + if !p.clearInterceptDNSTargetStateLocked() { + return + } // ctrld owns the DNS of the service no longer, and a later outage // report needs the moment that ownership ended. journal(mainLog.Load().Info()).Str("service", svc).Str("target", val). @@ -312,7 +374,9 @@ func (p *prog) removeInterceptDNSTargetLocked(reason string) { mainLog.Load().Warn().Err(err).Msgf("intercept DNS target: could not reset DNS on %q; retaining cleanup state", svc) return } - p.clearInterceptDNSTargetStateLocked() + if !p.clearInterceptDNSTargetStateLocked() { + return + } journal(mainLog.Load().Info()).Str("service", svc).Str("target", val).Str("reason", reason). Msgf("intercept DNS target: removed %s from %q (%s)", val, svc, reason) } @@ -321,9 +385,15 @@ func (p *prog) interceptDNSTargetOwnershipLocked() dnsTargetOwnership { return dnsTargetOwnership{p.interceptDNSTargetService, p.interceptDNSTargetSetValue} } -func (p *prog) clearInterceptDNSTargetStateLocked() { +func (p *prog) clearInterceptDNSTargetStateLocked() bool { + service, value := p.interceptDNSTargetService, p.interceptDNSTargetSetValue p.setInterceptDNSTargetLocked("", "") - p.persistInterceptDNSTargetStateLocked() + if err := p.persistInterceptDNSTargetStateLocked(); err != nil { + p.setInterceptDNSTargetLocked(service, value) + mainLog.Load().Warn().Err(err).Msg("intercept DNS target: could not clear ownership; retaining cleanup state") + return false + } + return true } // setInterceptDNSTargetLocked stores the service that ctrld owns and the value diff --git a/cmd/cli/dns_target_darwin_test.go b/cmd/cli/dns_target_darwin_test.go index b22d9dd..c7e882c 100644 --- a/cmd/cli/dns_target_darwin_test.go +++ b/cmd/cli/dns_target_darwin_test.go @@ -41,6 +41,9 @@ func newInterceptTargetHarness(t *testing.T) *interceptTargetHarness { statePath: filepath.Join(t.TempDir(), interceptDNSTargetStateFile), } + origHome := homedir + homedir = t.TempDir() + t.Cleanup(func() { homedir = origHome }) origPath := interceptDNSTargetStatePathFn origRoute := interceptDefaultRouteInterfaceFn origIface := interceptInterfaceByNameFn diff --git a/cmd/cli/dns_target_diagnostic_darwin_test.go b/cmd/cli/dns_target_diagnostic_darwin_test.go index c83f298..2dd4892 100644 --- a/cmd/cli/dns_target_diagnostic_darwin_test.go +++ b/cmd/cli/dns_target_diagnostic_darwin_test.go @@ -240,14 +240,11 @@ func TestEnsureInterceptDNSTargetResolutionDoesNotClaimWriteSuccess(t *testing.T h.dhcpErr = nil h.setErr = errors.New("synthetic set failure") p.ensureInterceptDNSTarget([]string{}) - event := oneRecoveryEvent(t, logs, dnsTargetResolvedMessage) - wantField(t, event, "outcome", "decision_available") - wantField(t, event, "reason", "dns_less_network") - wantField(t, event, "action", "unchanged") - wantField(t, event, "ownership_before", false) - wantField(t, event, "ownership_after", false) - if len(h.setCalls) != 1 || p.interceptDNSTargetService != "" { - t.Fatal("failed target write was not exercised or acquired ownership") + if events := jsonLogEvents(t, logs, dnsTargetResolvedMessage); len(events) != 0 { + t.Fatal("failed target write reported a resolved decision") + } + if len(h.setCalls) != 1 || p.interceptDNSTargetService != "Wi-Fi" { + t.Fatal("failed target write did not retain cleanup state for possible partial mutation") } } diff --git a/cmd/cli/dns_target_ownership_darwin_test.go b/cmd/cli/dns_target_ownership_darwin_test.go new file mode 100644 index 0000000..aefaaa5 --- /dev/null +++ b/cmd/cli/dns_target_ownership_darwin_test.go @@ -0,0 +1,168 @@ +//go:build darwin + +package cli + +import ( + "errors" + "fmt" + "net" + "os" + "path/filepath" + "slices" + "testing" +) + +func TestInterceptDNSTargetOwnershipPersistenceBeforeMutation(t *testing.T) { + h := newInterceptTargetHarness(t) + h.nativeCLAT = true + h.dhcpErr = errors.New("no DHCPv4") + h.dns["Wi-Fi"] = []string{"2001:db8::53"} + h.statePath = filepath.Join(t.TempDir(), "absent", "ownership") + p := newInterceptTargetProg() + p.cfg.Listener["0"].IP = "127.0.0.9" + p.cfg.Listener["0"].Port = 5353 + p.ensureInterceptDNSTarget([]string{}) + if len(h.setCalls) != 0 || p.interceptDNSTargetService != "" || !slices.Equal(h.dns["Wi-Fi"], []string{"2001:db8::53"}) { + t.Fatalf("persistence failure mutated DNS: calls=%v dns=%v owner=%s", h.setCalls, h.dns["Wi-Fi"], p.interceptDNSTargetService) + } +} + +func TestInterceptDNSTargetExternalEditInvalidatesBackup(t *testing.T) { + h := newInterceptTargetHarness(t) + p := newInterceptTargetProg() + p.cfg.Listener["0"].IP = "127.0.0.9" + p.cfg.Listener["0"].Port = 5353 + p.ensureInterceptDNSTarget([]string{}) + target := p.interceptDNSTargetValue() + if !slices.Equal(h.dns["Wi-Fi"], []string{target}) { + t.Fatal("listener-aware target not installed") + } + backup := savedStaticDnsSettingsFilePath(&net.Interface{Name: "Wi-Fi"}) + if err := os.WriteFile(backup, []byte("2001:db8::old"), 0600); err != nil { + t.Fatal(err) + } + external := []string{"2001:db8::54"} + h.dns["Wi-Fi"] = slices.Clone(external) + for range 2 { + p.ensureInterceptDNSTarget([]string{}) + } + if !slices.Equal(h.dns["Wi-Fi"], external) || len(h.setCalls) != 1 { + t.Fatal("reconciliation overwrote external DNS") + } + p.removeInterceptDNSTarget("stop") + oldEach, oldRestore := restoreSavedStaticDNSInterfacesFn, restoreSavedStaticDNSRestoreFn + t.Cleanup(func() { restoreSavedStaticDNSInterfacesFn, restoreSavedStaticDNSRestoreFn = oldEach, oldRestore }) + restoreSavedStaticDNSInterfacesFn = func(_, _ string, f func(*net.Interface) error) { _ = f(&net.Interface{Name: "Wi-Fi"}) } + restoreSavedStaticDNSRestoreFn = func(*net.Interface) error { t.Fatal("full stop sweep replayed stale DNS"); return nil } + restoreSavedStaticDNS("", false) + if _, err := os.Stat(backup); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("stale backup remains available to stop sweep: %v", err) + } + if !slices.Equal(h.dns["Wi-Fi"], external) { + t.Fatal("stop overwrote external DNS") + } +} + +func TestInterceptDNSTargetFailedOwnershipClearRetainsState(t *testing.T) { + h := newInterceptTargetHarness(t) + p := newInterceptTargetProg() + p.ensureInterceptDNSTarget([]string{}) + if err := os.Remove(h.statePath); err != nil { + t.Fatal(err) + } + if err := os.Mkdir(h.statePath, 0700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(h.statePath, "busy"), nil, 0600); err != nil { + t.Fatal(err) + } + p.removeInterceptDNSTarget("stop") + if p.interceptDNSTargetService != "Wi-Fi" { + t.Fatal("failed ownership clear reported completed cleanup") + } +} + +func TestInterceptDNSTargetTrackedValueIsNotActualDNS(t *testing.T) { + for _, port := range []int{53, 5353} { + t.Run(fmt.Sprint(port), func(t *testing.T) { + logs := captureDebugMainLog(t) + h := newInterceptTargetHarness(t) + p := newInterceptTargetProg() + p.cfg.Listener["0"].IP = "127.0.0.9" + p.cfg.Listener["0"].Port = port + p.ensureInterceptDNSTarget([]string{}) + h.dns["Wi-Fi"] = nil + p.ensureInterceptDNSTarget([]string{}) + event := oneRecoveryEvent(t, logs, dnsTargetFailedMessage) + wantField(t, event, "stage", "owned_target_changed") + if len(h.setCalls) != 1 || len(h.dns["Wi-Fi"]) != 0 { + t.Fatal("reinstalled after external clear") + } + }) + } +} + +func TestInterceptDNSTargetFailedCleanupGuardsFullSweep(t *testing.T) { + h := newInterceptTargetHarness(t) + p := newInterceptTargetProg() + p.ensureInterceptDNSTarget([]string{}) + backup := savedStaticDnsSettingsFilePath(&net.Interface{Name: "Wi-Fi"}) + if err := os.WriteFile(backup, []byte("2001:db8::53"), 0600); err != nil { + t.Fatal(err) + } + h.resetErr = errors.New("reset failed") + p.removeInterceptDNSTarget("stop") + oldEach, oldRestore := restoreSavedStaticDNSInterfacesFn, restoreSavedStaticDNSRestoreFn + t.Cleanup(func() { restoreSavedStaticDNSInterfacesFn, restoreSavedStaticDNSRestoreFn = oldEach, oldRestore }) + restoreSavedStaticDNSInterfacesFn = func(_, _ string, f func(*net.Interface) error) { _ = f(&net.Interface{Name: "Wi-Fi"}) } + restoreSavedStaticDNSRestoreFn = func(*net.Interface) error { t.Fatal("sweep bypassed failed guarded cleanup"); return nil } + restoreSavedStaticDNS("", true) + if _, err := os.Stat(backup); err != nil { + t.Fatal("failed cleanup lost backup:", err) + } + if p.interceptDNSTargetService != "Wi-Fi" { + t.Fatal("failed cleanup lost state") + } +} + +func TestInterceptDNSTargetStaticRestoreSkipIsNotFailure(t *testing.T) { + for _, state := range []string{`{"service":"Wi-Fi","value":"127.0.0.53"}`, `invalid ownership`} { + t.Run(state, func(t *testing.T) { + h := newInterceptTargetHarness(t) + logs := captureDebugMainLog(t) + if err := os.WriteFile(h.statePath, []byte(state), 0600); err != nil { + t.Fatal(err) + } + backup := savedStaticDnsSettingsFilePath(&net.Interface{Name: "Wi-Fi"}) + const savedDNS = "2001:db8::53" + if err := os.WriteFile(backup, []byte(savedDNS), 0600); err != nil { + t.Fatal(err) + } + oldEach, oldRestore := restoreSavedStaticDNSInterfacesFn, restoreSavedStaticDNSRestoreFn + t.Cleanup(func() { restoreSavedStaticDNSInterfacesFn, restoreSavedStaticDNSRestoreFn = oldEach, oldRestore }) + calls := 0 + restoreSavedStaticDNSInterfacesFn = func(_, _ string, f func(*net.Interface) error) { + calls++ + if err := f(&net.Interface{Name: "Wi-Fi"}); err != nil { + t.Fatalf("ownership skip reported as restore failure: %v", err) + } + } + restoreSavedStaticDNSRestoreFn = func(*net.Interface) error { + t.Fatal("ownership skip restored DNS") + return nil + } + restoreSavedStaticDNS("", true) + if calls != 1 { + t.Fatalf("interface sweeps=%d, want 1", calls) + } + event := oneRecoveryEvent(t, logs, "Saved static DNS restore skipped on interface Wi-Fi: intercept target cleanup is pending or ownership is unreadable") + wantField(t, event, "level", "debug") + if data, err := os.ReadFile(backup); err != nil || string(data) != savedDNS { + t.Fatalf("ownership skip changed backup: %q, %v", data, err) + } + if data, err := os.ReadFile(h.statePath); err != nil || string(data) != state { + t.Fatalf("ownership skip changed state: %q, %v", data, err) + } + }) + } +} diff --git a/cmd/cli/dns_target_stub.go b/cmd/cli/dns_target_stub.go index 680ede3..459e7b2 100644 --- a/cmd/cli/dns_target_stub.go +++ b/cmd/cli/dns_target_stub.go @@ -2,6 +2,8 @@ package cli +func interceptTargetAllowsStaticRestore(_ string) bool { return true } + // 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 diff --git a/cmd/cli/listener_5353_darwin_test.go b/cmd/cli/listener_5353_darwin_test.go new file mode 100644 index 0000000..24abcc6 --- /dev/null +++ b/cmd/cli/listener_5353_darwin_test.go @@ -0,0 +1,67 @@ +//go:build darwin + +package cli + +import ( + "fmt" + ctrld "github.com/Control-D-Inc/ctrld" + "strings" + "testing" +) + +func TestPFScopedNameserverAddress(t *testing.T) { + for _, tc := range []struct{ server, host, family string }{ + {"[fe80::1%en0]:53", "fe80::1", "inet6"}, + {"2001:db8::53", "2001:db8::53", "inet6"}, + {"192.0.2.53:53", "192.0.2.53", "inet"}, + } { + host, family, ok := pfNameserverAddress(tc.server) + if !ok || host != tc.host || family != tc.family { + t.Fatalf("PF nameserver %q: %q %q %v", tc.server, host, family, ok) + } + } + if _, _, ok := pfNameserverAddress("not an address"); ok { + t.Fatal("invalid PF address accepted") + } +} + +func TestPFRecoveryScopedNameserverRules(t *testing.T) { + captureDebugMainLog(t) + p := &prog{cfg: &ctrld.Config{Listener: map[string]*ctrld.ListenerConfig{"0": {IP: "127.0.0.2", Port: 5353}}}} + // Recovery converts OS-discovered addresses into this exemption list. + rules := p.buildPFAnchorRulesForTunnels([]vpnDNSExemption{ + {Server: "fe80::1%en0"}, {Server: "fe80::1%en1"}, {Server: "192.0.2.53"}, + }, nil) + want := "pass out quick on ! lo0 inet6 proto { udp, tcp } from any to fe80::1 port 53 group " + pfGroupName + if strings.Count(rules, want) != 1 { + t.Fatalf("scoped recovery exemption missing or duplicated: %s", rules) + } + if strings.Contains(rules, "%en0") || strings.Contains(rules, "%en1") || strings.Contains(rules, "inet proto { udp, tcp } from any to fe80:") { + t.Fatal("socket zone or IPv4 family leaked into recovery PF rules") + } + if !strings.Contains(rules, "inet proto { udp, tcp } from any to 192.0.2.53 port 53 group "+pfGroupName) { + t.Fatal("IPv4 recovery exemption changed") + } +} + +func TestCustomListener5353PFRules(t *testing.T) { + captureDebugMainLog(t) + for _, ip := range []string{"127.0.0.1", "127.0.0.2", "127.0.0.53", "192.0.2.10"} { + t.Run(ip, func(t *testing.T) { + p := &prog{cfg: &ctrld.Config{Listener: map[string]*ctrld.ListenerConfig{"0": {IP: ip, Port: 5353}}}} + rules := p.buildPFAnchorRulesForTunnels(nil, []string{"utun9"}) + for _, proto := range []string{"udp", "tcp"} { + want := fmt.Sprintf("rdr on lo0 inet proto %s from any to ! %s port 53 -> %s port 5353", proto, ip, ip) + if !strings.Contains(rules, want) { + t.Errorf("missing redirect %s", want) + } + } + if strings.Contains(rules, "port 5354") { + t.Fatal("explicit port replaced with automatic fallback") + } + if p.interceptDNSTargetValue() == ip { + t.Fatal("target equals rdr exclusion: no port translation") + } + }) + } +} diff --git a/cmd/cli/listener_5353_linux_test.go b/cmd/cli/listener_5353_linux_test.go new file mode 100644 index 0000000..273cf53 --- /dev/null +++ b/cmd/cli/listener_5353_linux_test.go @@ -0,0 +1,32 @@ +//go:build linux + +package cli + +import ( + ctrld "github.com/Control-D-Inc/ctrld" + "net" + "testing" +) + +// Socket-only Linux control. Native macOS PF delivery is a separate QA gate. +func TestCustomListener5353Binding(t *testing.T) { + captureDebugMainLog(t) + for _, ip := range []string{"127.0.0.1", "127.0.0.2"} { + t.Run(ip, func(t *testing.T) { + cfg := &ctrld.Config{Listener: map[string]*ctrld.ListenerConfig{"0": {IP: ip, Port: 5353}}} + changed, ok := tryUpdateListenerConfigIntercept(cfg, func() {}, false) + if !ok || changed { + t.Fatalf("custom listener did not bind unchanged: changed=%v ok=%v", changed, ok) + } + occupied, err := net.Listen("tcp4", net.JoinHostPort(ip, "5353")) + if err != nil { + t.Fatal(err) + } + defer occupied.Close() + changed, ok = tryUpdateListenerConfigIntercept(cfg, func() {}, false) + if ok || changed || cfg.Listener["0"].IP != ip || cfg.Listener["0"].Port != 5353 { + t.Fatal("occupied explicit listener silently fell back") + } + }) + } +} diff --git a/cmd/cli/listener_5353_target_darwin_test.go b/cmd/cli/listener_5353_target_darwin_test.go new file mode 100644 index 0000000..8259e59 --- /dev/null +++ b/cmd/cli/listener_5353_target_darwin_test.go @@ -0,0 +1,41 @@ +//go:build darwin + +package cli + +import ( + "errors" + "slices" + "testing" +) + +func TestNativeCLATCustomListener5353Lifecycle(t *testing.T) { + for _, ip := range []string{"127.0.0.1", "127.0.0.2", "127.0.0.53", "192.0.2.10"} { + t.Run(ip, func(t *testing.T) { + h := newInterceptTargetHarness(t) + h.nativeCLAT = true + h.dhcpErr = errors.New("DHCPv4 intentionally absent") + p := newInterceptTargetProg() + p.cfg.Listener["0"].IP = ip + p.cfg.Listener["0"].Port = 5353 + target := p.interceptDNSTargetValue() + p.ensureInterceptDNSTarget([]string{}) + p.ensureInterceptDNSTarget([]string{}) + if !slices.Equal(h.dns["Wi-Fi"], []string{target}) || len(h.setCalls) != 1 { + t.Fatalf("target was not installed idempotently: %v writes %v", h.dns, h.setCalls) + } + if target == ip { + t.Fatal("OS DNS would hit port53 at listener instead of PF port5353 redirect") + } + h.dhcpErr = nil + h.dhcp = []string{"192.0.2.53"} + h.nativeCLAT = false + p.ensureInterceptDNSTarget([]string{"192.0.2.53"}) + if len(h.dns["Wi-Fi"]) != 0 || p.interceptDNSTargetService != "" { + t.Fatalf("DHCP return failed to remove target: %+v", h.dns) + } + if p.cfg.Listener["0"].IP != ip || p.cfg.Listener["0"].Port != 5353 { + t.Fatal("target lifecycle changed configured listener") + } + }) + } +} diff --git a/cmd/cli/listener_5353_test.go b/cmd/cli/listener_5353_test.go new file mode 100644 index 0000000..611b443 --- /dev/null +++ b/cmd/cli/listener_5353_test.go @@ -0,0 +1,34 @@ +package cli + +import ( + ctrld "github.com/Control-D-Inc/ctrld" + "testing" +) + +func TestCustomListener5353Contract(t *testing.T) { + for _, tc := range []struct{ ip, target string }{ + {"127.0.0.1", "127.0.0.53"}, {"127.0.0.2", "127.0.0.53"}, + {"127.0.0.53", "127.0.0.54"}, {"192.0.2.10", "127.0.0.53"}, + } { + t.Run(tc.ip, func(t *testing.T) { + if !isExplicitInterceptListener(tc.ip, 5353) { + t.Fatal("5353 was not explicit") + } + p := &prog{cfg: &ctrld.Config{Listener: map[string]*ctrld.ListenerConfig{"0": {IP: tc.ip, Port: 5353}}}} + if got := p.interceptDNSTargetValue(); got != tc.target { + t.Fatalf("target=%s want %s", got, tc.target) + } + current := p.cfg.Listener + generated := map[string]*ctrld.ListenerConfig{"0": {IP: "127.0.0.1", Port: 53}} + preserveBoundListeners(generated, current) + if generated["0"].IP != tc.ip || generated["0"].Port != 5353 { + t.Fatalf("generated reload lost bound listener: %+v", generated["0"]) + } + explicit := map[string]*ctrld.ListenerConfig{"0": {IP: tc.ip, Port: 5353}} + preserveBoundListeners(explicit, map[string]*ctrld.ListenerConfig{"0": {IP: "127.0.0.1", Port: 5354}}) + if explicit["0"].IP != tc.ip || explicit["0"].Port != 5353 { + t.Fatalf("explicit reload normalized: %+v", explicit["0"]) + } + }) + } +} diff --git a/cmd/cli/prog.go b/cmd/cli/prog.go index 0a3cc85..7957fa2 100644 --- a/cmd/cli/prog.go +++ b/cmd/cli/prog.go @@ -323,6 +323,8 @@ type prog struct { // VPN DNS manager for split DNS routing when intercept mode is active. vpnDNS *vpnDNSManager + // Serializes journal reads with publication after an intercept startup retry. + vpnDNSJournalMu sync.Mutex started chan struct{} onStartedDone chan struct{} @@ -1178,6 +1180,27 @@ func (p *prog) startNetworkJournal() func() { // drop. The search suffixes name the organization of the endpoint, so the // journal holds their number only. func (p *prog) logDNSConfigChanges(entries []dnsResolverEntry) { + // The poller emits one batch per changed snapshot (at most once per poll), + // not one discovery per resolver. Late DNS publication on an unchanged + // tunnel otherwise has no network event to refresh split routing. + // Join the existing shutdown fence before touching the manager or PF/WFP: + // closeNetMonitor drains this callback before restoring the host's DNS. + if len(entries) > 0 && p.beginNetworkActivity() { + defer p.netMonitorWG.Done() + // setDNS publishes the manager before signalling completion. The journal + // starts earlier; never race initial manager publication or initialize a + // manager in traditional/hard intercept mode. + select { + case <-p.csSetDnsDone: + p.vpnDNSJournalMu.Lock() + manager := p.vpnDNS + p.vpnDNSJournalMu.Unlock() + if manager != nil { + manager.Refresh(true) + } + default: + } + } for _, entry := range entries { journal(mainLog.Load().Info()). Strs("nameservers", entry.Nameservers). @@ -1546,8 +1569,10 @@ func (p *prog) setDNS(systemNameservers []string) { // Discovers search domains from virtual/VPN interfaces and forwards // matching queries to the DNS server on that interface. // Skipped in --intercept-mode hard where all DNS goes through ctrld. + p.vpnDNSJournalMu.Lock() p.vpnDNS = newVPNDNSManager(p.exemptVPNDNSServers) p.vpnDNS.Refresh(true) + p.vpnDNSJournalMu.Unlock() } setDnsOK = true @@ -1802,7 +1827,9 @@ func (p *prog) resetDNS(isStart bool, restoreStatic bool) { } // Clean up VPN DNS manager + p.vpnDNSJournalMu.Lock() p.vpnDNS = nil + p.vpnDNSJournalMu.Unlock() return } diff --git a/cmd/cli/vpn_dns.go b/cmd/cli/vpn_dns.go index 2c8516a..2bcf102 100644 --- a/cmd/cli/vpn_dns.go +++ b/cmd/cli/vpn_dns.go @@ -176,11 +176,7 @@ func (m *vpnDNSManager) refreshOnce(guardAgainstNoNameservers bool) { config.InterfaceName, len(config.Domains), len(config.Servers)) for _, domain := range config.Domains { - // Normalize domain: remove leading dot, Linux routing domain prefix (~), - // and convert to lowercase. - domain = strings.TrimPrefix(domain, "~") - domain = strings.TrimPrefix(domain, ".") - domain = strings.ToLower(domain) + domain = normalizeVPNDomain(domain) if domain != "" { m.routes[domain] = append([]string{}, config.Servers...) @@ -288,9 +284,7 @@ func (m *vpnDNSManager) RefreshRoutesOnly() (routes, domainlessServers, exemptio for _, config := range configs { for _, domain := range config.Domains { - domain = strings.TrimPrefix(domain, "~") - domain = strings.TrimPrefix(domain, ".") - domain = strings.ToLower(domain) + domain = normalizeVPNDomain(domain) if domain != "" { m.routes[domain] = append([]string{}, config.Servers...) } @@ -389,6 +383,14 @@ func (m *vpnDNSManager) VPNDNSReachable() { m.retainedAfterEmptyDiscovery = false } +// normalizeVPNDomain accepts OS suffix and Linux routing-domain spellings. +// The root alone is not a split-DNS route (including the Linux ~. catch-all). +func normalizeVPNDomain(domain string) string { + domain = strings.TrimPrefix(domain, "~") + domain = strings.TrimPrefix(domain, ".") + return strings.ToLower(strings.TrimSuffix(domain, ".")) +} + // UpstreamForDomain checks if the domain matches any VPN search domain. // Returns VPN DNS servers if matched, nil otherwise. func (m *vpnDNSManager) UpstreamForDomain(domain string) []string { @@ -402,14 +404,17 @@ func (m *vpnDNSManager) UpstreamForDomain(domain string) []string { domain = strings.TrimSuffix(domain, ".") domain = strings.ToLower(domain) - if servers, ok := m.routes[domain]; ok { - return append([]string{}, servers...) - } - - for vpnDomain, servers := range m.routes { - if strings.HasSuffix(domain, "."+vpnDomain) { + // Walk label boundaries from most to least specific. Map iteration order + // must never choose a parent VPN's resolver over a matching child zone. + for domain != "" { + if servers, ok := m.routes[domain]; ok { return append([]string{}, servers...) } + i := strings.IndexByte(domain, '.') + if i < 0 { + break + } + domain = domain[i+1:] } return nil diff --git a/cmd/cli/vpn_dns_convergence_test.go b/cmd/cli/vpn_dns_convergence_test.go new file mode 100644 index 0000000..d07cfba --- /dev/null +++ b/cmd/cli/vpn_dns_convergence_test.go @@ -0,0 +1,133 @@ +package cli + +import ( + "context" + "reflect" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/Control-D-Inc/ctrld" +) + +func TestVPNDNSLongestNormalizedSuffixAcrossRefreshes(t *testing.T) { + for _, routeOnly := range []bool{false, true} { + t.Run(map[bool]string{false: "full", true: "route-only"}[routeOnly], func(t *testing.T) { + m := newVPNDNSManager(nil) + m.discoverVPNDNS = func(context.Context) []ctrld.VPNDNSConfig { + return []ctrld.VPNDNSConfig{ + {Domains: []string{".EXAMPLE.com."}, Servers: []string{"10.0.0.1"}}, + {Domains: []string{"~Child.Example.COM."}, Servers: []string{"10.0.0.2", "10.0.0.3"}}, + {Domains: []string{".", "~."}, Servers: []string{"10.0.0.4"}}, + } + } + if routeOnly { + m.RefreshRoutesOnly() + } else { + m.Refresh(true) + } + for query, want := range map[string][]string{ + "example.com.": {"10.0.0.1"}, "other.example.com": {"10.0.0.1"}, + "child.example.com.": {"10.0.0.2", "10.0.0.3"}, + "HOST.Deep.Child.Example.COM.": {"10.0.0.2", "10.0.0.3"}, + "notexample.com.": nil, "example.com.other.": nil, "public.test.": nil, + } { + for i := 0; i < 1000; i++ { + if got := m.UpstreamForDomain(query); !reflect.DeepEqual(got, want) { + t.Fatalf("%s: got %v want %v", query, got, want) + } + } + } + got := m.UpstreamForDomain("child.example.com") + got[0] = "mutated" + if m.UpstreamForDomain("child.example.com")[0] != "10.0.0.2" { + t.Fatal("lookup leaked mutable slice") + } + }) + } +} + +// Drive the real poller diff -> journal callback -> manager discovery path. +// Both OS boundaries are fixtures, so this test cannot touch host DNS or PF. +func TestVPNDNSJournalLatePublicationConverges(t *testing.T) { + m := newVPNDNSManager(nil) + var discoveryCalls, updates int + var snapshot []ctrld.VPNDNSConfig + m.discoverVPNDNS = func(context.Context) []ctrld.VPNDNSConfig { discoveryCalls++; return snapshot } + m.onServersChanged = func([]vpnDNSExemption) error { updates++; return nil } + p := &prog{vpnDNS: m, csSetDnsDone: make(chan struct{})} + close(p.csSetDnsDone) + var table []dnsResolverEntry + poller := newDNSConfigPoller(func() ([]dnsResolverEntry, error) { return table, nil }) + emit := func() { p.logDNSConfigChanges(poller.pollOnce(time.Now())) } + emit() // empty startup baseline + for _, server := range []string{"10.0.0.1", "10.0.0.2"} { + snapshot = []ctrld.VPNDNSConfig{{InterfaceName: "utun-fixture", Domains: []string{"corp.example."}, Servers: []string{server}}} + table = []dnsResolverEntry{{Interface: "utun-fixture", Nameservers: []string{server}}} + emit() + if got := m.UpstreamForDomain("host.corp.example."); len(got) != 1 || got[0] != server { + t.Fatalf("late DNS-only change not published: %v", got) + } + for i := 0; i < 100; i++ { + emit() + } + } + if discoveryCalls != 2 || updates != 2 { + t.Fatalf("unchanged tables caused refresh storm: discoveries=%d updates=%d", discoveryCalls, updates) + } + // Removal is a DNS-only delta too; keep Windows' one-empty-snapshot guard. + snapshot = nil + table = nil + emit() + if vpnDNSSettlingEnabled { // Preserve Windows' one-empty-snapshot guard. + m.Refresh(true) + } + if got := m.UpstreamForDomain("host.corp.example"); len(got) != 0 { + t.Fatalf("removed DNS retained: %v", got) + } + p.closeNetMonitor() +} + +func TestVPNDNSJournalShutdownDrainsAndRejectsRefresh(t *testing.T) { + m := newVPNDNSManager(nil) + entered, release := make(chan struct{}), make(chan struct{}) + var calls, updates atomic.Int32 + m.discoverVPNDNS = func(context.Context) []ctrld.VPNDNSConfig { + calls.Add(1) + close(entered) + <-release + return []ctrld.VPNDNSConfig{{Domains: []string{"corp.example"}, Servers: []string{"10.0.0.1"}}} + } + m.onServersChanged = func([]vpnDNSExemption) error { updates.Add(1); return nil } + p := &prog{vpnDNS: m, csSetDnsDone: make(chan struct{})} + entries := []dnsResolverEntry{{Action: resolverActionChanged}} + p.logDNSConfigChanges(entries) + if calls.Load() != 0 { + t.Fatal("discovery ran before DNS initialization") + } + close(p.csSetDnsDone) + callbackDone := make(chan struct{}) + go func() { defer close(callbackDone); p.logDNSConfigChanges(entries) }() + <-entered + stopDone := make(chan struct{}) + go func() { p.closeNetMonitor(); close(stopDone) }() + <-p.networkActivityDone() + select { + case <-stopDone: + t.Fatal("shutdown did not drain in-flight refresh") + default: + } + close(release) + <-callbackDone + <-stopDone + var wg sync.WaitGroup + for i := 0; i < 100; i++ { + wg.Add(1) + go func() { defer wg.Done(); p.logDNSConfigChanges(entries) }() + } + wg.Wait() + if calls.Load() != 1 || updates.Load() != 1 { + t.Fatalf("post-stop mutation: discovery=%d updates=%d", calls.Load(), updates.Load()) + } +} diff --git a/docs/619-target-ownership-limits.md b/docs/619-target-ownership-limits.md new file mode 100644 index 0000000..7ccad15 --- /dev/null +++ b/docs/619-target-ownership-limits.md @@ -0,0 +1,17 @@ +# Target ownership safety (relates to #619) + +The target cleanup record is not proof of actual service DNS. Reconciliation now reports a tracked/actual mismatch instead of returning an implicit already-installed decision. An external clear or IPv6-only edit does not authorize reinstalling a target; the record is retained to prevent repeated watchdog admission. Usable external IPv4 keeps the existing guarded cleanup path. + +Ownership is written by atomic replacement before target installation. Failure refuses installation, rather than depending on a future restart to recover an untracked target. A failed DNS command may have partly mutated OS state, so its cleanup record is retained. Failed ownership-file removal also retains memory state and does not emit the target-removed success event. + +On external edits, cleanup discards the stale saved-static backup before releasing the record. The full saved-static sweep skips a service with retained target cleanup state; unreadable/malformed ownership state conservatively skips the sweep. No unbounded retry, fatal exit, or machine-stop policy is introduced. + +## Evidence and limits + +- Linux-adapted Darwin tests execute the real target lifecycle and saved-static sweep with only OS boundaries replaced. New regression tests cover persistence failure before mutation, external edit through reconciliation/cleanup/full sweep, ownership clear failure, actual-vs-tracked discrepancy, and failed-cleanup sweep guards. Custom listener values are configured and assertions derive the target from the production selector. +- This is not native macOS PF/CLAT proof. Existing native CLAT admission/removal behavior still needs native validation on the integrated artifact; no PF rules or target selector were changed here. +- A missing target is not automatically repaired: external clearing and accidental loss cannot safely be distinguished from these observations. The diagnostic reports the uncertainty; it does not claim resolution. +- Once explicit cleanup releases a record, a later new admission uses the existing policy for IPv6-only static DNS. This is not a persistent per-service operator opt-out mechanism. +- Retaining cleanup state does not guarantee another cleanup attempt after process exit. The void shutdown API still cannot report aggregate stop failure to its caller; no guaranteed retry or issue closure is claimed. +- Startup/running-interface reset and callback shutdown ordering live in prog.go and are outside this patch. Persistence failure was not a universal restart orphan on the baseline: startup can rescue the current interface, while old/noncurrent-service cases are conditional. +- Network configuration commands are not compare-and-swap transactions. A concurrent external edit after the last read remains a native race; these tests do not prove otherwise. Atomic file replacement does not claim power-loss durability. diff --git a/nameservers_darwin.go b/nameservers_darwin.go index 2ae1c29..65ac705 100644 --- a/nameservers_darwin.go +++ b/nameservers_darwin.go @@ -22,6 +22,8 @@ func dnsFns() []dnsFn { return []dnsFn{dnsFromResolvConf, getDNSFromScutil, getAllDHCPNameservers} } +var scutilLocalAddresses = netmon.LocalAddresses + func getDNSFromScutil() []string { logger := *ProxyLogger.Load() @@ -36,7 +38,7 @@ func getDNSFromScutil() []string { retryInterval = 100 * time.Millisecond ) - regularIPs, loopbackIPs, _ := netmon.LocalAddresses() + regularIPs, loopbackIPs, _ := scutilLocalAddresses() var nameservers []string for attempt := 0; attempt < maxRetries; attempt++ { @@ -51,36 +53,8 @@ func getDNSFromScutil() []string { continue } - var localDNS []string - seen := make(map[string]bool) - - scanner := bufio.NewScanner(bytes.NewReader(output)) - for scanner.Scan() { - line := strings.TrimSpace(scanner.Text()) - if strings.HasPrefix(line, "nameserver[") { - parts := strings.Split(line, ":") - if len(parts) == 2 { - ns := strings.TrimSpace(parts[1]) - if ip := net.ParseIP(ns); ip != nil { - // skip loopback IPs - isLocal := false - for _, v := range slices.Concat(regularIPs, loopbackIPs) { - ipStr := v.String() - if ip.String() == ipStr { - isLocal = true - break - } - } - if !isLocal && !seen[ip.String()] { - seen[ip.String()] = true - localDNS = append(localDNS, ip.String()) - } - } - } - } - } - - if err := scanner.Err(); err != nil { + localDNS, err := parseScutilNameservers(output, slices.Concat(regularIPs, loopbackIPs)) + if err != nil { Log(context.Background(), logger.Error(), "error scanning scutil output (attempt %d/%d): %v", attempt+1, maxRetries, err) continue } diff --git a/nameservers_scutil.go b/nameservers_scutil.go new file mode 100644 index 0000000..cf74591 --- /dev/null +++ b/nameservers_scutil.go @@ -0,0 +1,45 @@ +package ctrld + +import ( + "bufio" + "bytes" + "net/netip" + "strings" +) + +// Link-local addresses identify a host only within their interface's zone. +// In particular, an unzoned local fe80::1 must not exclude fe80::1%en0. +func scutilLocalAddressKey(addr netip.Addr) netip.Addr { + addr = addr.Unmap() + if addr.IsLinkLocalUnicast() { + return addr + } + return addr.WithZone("") +} + +// parseScutilNameservers retains IPv6 zones for the eventual socket dial. +func parseScutilNameservers(output []byte, local []netip.Addr) ([]string, error) { + excluded := make(map[netip.Addr]bool, len(local)) + for _, addr := range local { + excluded[scutilLocalAddressKey(addr)] = true + } + var servers []string + seen := make(map[netip.Addr]bool) + scanner := bufio.NewScanner(bytes.NewReader(output)) + for scanner.Scan() { + key, value, ok := strings.Cut(strings.TrimSpace(scanner.Text()), ":") + if !ok || !strings.HasPrefix(key, "nameserver[") { + continue + } + addr, err := netip.ParseAddr(strings.TrimSpace(value)) + if err != nil || excluded[scutilLocalAddressKey(addr)] || seen[addr] { + continue + } + seen[addr] = true + servers = append(servers, addr.String()) + } + if err := scanner.Err(); err != nil { + return nil, err + } + return servers, nil +} diff --git a/nameservers_scutil_darwin_test.go b/nameservers_scutil_darwin_test.go new file mode 100644 index 0000000..55b5e0c --- /dev/null +++ b/nameservers_scutil_darwin_test.go @@ -0,0 +1,51 @@ +//go:build darwin + +package ctrld + +import ( + "net" + "net/netip" + "os" + "path/filepath" + "slices" + "testing" +) + +func TestScopedOSResolverDoesNotBindDefaultSource(t *testing.T) { + old4, old6 := GetDefaultLocalIPv4(), GetDefaultLocalIPv6() + SetDefaultLocalIPv4(net.ParseIP("192.0.2.10")) + SetDefaultLocalIPv6(net.ParseIP("2001:db8::10")) + t.Cleanup(func() { SetDefaultLocalIPv4(old4); SetDefaultLocalIPv6(old6) }) + if got := defaultLocalIPForServer("[fe80::1%en0]:53"); got != nil { + t.Fatalf("scoped resolver bound unrelated source %v", got) + } + if got := defaultLocalIPForServer("[2001:db8::53]:53"); !got.Equal(net.ParseIP("2001:db8::10")) { + t.Fatalf("global IPv6 source changed: %v", got) + } + if got := defaultLocalIPForServer("192.0.2.53:53"); !got.Equal(net.ParseIP("192.0.2.10")) { + t.Fatalf("IPv4 source changed: %v", got) + } +} + +func TestScutilIPv6ProductionReader(t *testing.T) { + old := scutilLocalAddresses + t.Cleanup(func() { scutilLocalAddresses = old }) + localReads := 0 + scutilLocalAddresses = func() ([]netip.Addr, []netip.Addr, error) { + localReads++ + return []netip.Addr{netip.MustParseAddr("2001:db8::10"), netip.MustParseAddr("fe80::2%en0")}, + []netip.Addr{netip.MustParseAddr("::1"), netip.MustParseAddr("fe80::1")}, nil + } + dir := t.TempDir() + script := "#!/bin/sh\nprintf '%s\\n' 'nameserver[0] : 2001:db8::53' 'nameserver[1] : fe80::1%en0' 'nameserver[2] : fe80::2%en0' 'nameserver[3] : ::1' 'nameserver[4] : 2001:db8::10'\n" + if err := os.WriteFile(filepath.Join(dir, "scutil"), []byte(script), 0700); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", dir) + if got := getDNSFromScutil(); !slices.Equal(got, []string{"2001:db8::53", "fe80::1%en0"}) { + t.Fatalf("production reader lost IPv6: %v", got) + } + if localReads != 1 { + t.Fatalf("local-address reads=%d, want 1", localReads) + } +} diff --git a/nameservers_scutil_test.go b/nameservers_scutil_test.go new file mode 100644 index 0000000..7c2b090 --- /dev/null +++ b/nameservers_scutil_test.go @@ -0,0 +1,80 @@ +package ctrld + +import ( + "net/netip" + "slices" + "strings" + "testing" +) + +func TestScutilNameserversIPv6AndZones(t *testing.T) { + output := []byte(`DNS configuration +resolver #1 + nameserver[0] : 192.0.2.53 + nameserver[1] : 2001:db8::53 + nameserver[2] : fe80::1%en0 + nameserver[3] : 2001:db8::53 + nameserver[4] : fe80::1%en1 + nameserver[5] : 127.0.0.1 + nameserver[6] : ::1 + nameserver[7] : fe80::2%en0 + nameserver[8] : not-an-address + search domain[0] : internal.example +`) + local := []netip.Addr{netip.MustParseAddr("127.0.0.1"), netip.MustParseAddr("::1"), netip.MustParseAddr("fe80::2%en0")} + got, err := parseScutilNameservers(output, local) + want := []string{"192.0.2.53", "2001:db8::53", "fe80::1%en0", "fe80::1%en1"} + if err != nil || !slices.Equal(got, want) { + t.Fatalf("nameservers=%v err=%v", got, err) + } + effective := initializeOsResolver(got) + r := newResolverWithNameserver(effective) + if !slices.Contains(*r.lanServers.Load(), "[fe80::1%en0]:53") || !slices.Contains(*r.lanServers.Load(), "[fe80::1%en1]:53") { + t.Fatalf("zones lost in effective resolver: %v", effective) + } + if !slices.Contains(*r.publicServers.Load(), "[2001:db8::53]:53") { + t.Fatalf("global IPv6 lost: %v", effective) + } +} + +func TestScutilNameserversLocalExclusion(t *testing.T) { + for _, tt := range []struct { + name, local, server string + excluded bool + }{ + {"unzoned link-local does not exclude scoped router", "fe80::1", "fe80::1%en0", false}, + {"loopback link-local does not exclude router", "fe80::1%lo0", "fe80::1%en0", false}, + {"other link does not exclude router", "fe80::1%en1", "fe80::1%en0", false}, + {"same link excludes local address", "fe80::1%en0", "fe80::1%en0", true}, + {"unzoned link-local matches unzoned local", "fe80::1", "fe80::1", true}, + {"scoped local does not exclude unzoned candidate", "fe80::1%en0", "fe80::1", false}, + {"global ignores zones", "2001:db8::1%en0", "2001:db8::1%en1", true}, + {"loopback ignores zones", "::1%lo0", "::1", true}, + {"IPv4 mapped local", "::ffff:192.0.2.1", "192.0.2.1", true}, + {"IPv4 mapped candidate", "192.0.2.1", "::ffff:192.0.2.1", true}, + {"IPv4 link-local mapped candidate", "169.254.1.1", "::ffff:169.254.1.1", true}, + {"other global is retained", "2001:db8::1", "2001:db8::53", false}, + } { + t.Run(tt.name, func(t *testing.T) { + got, err := parseScutilNameservers([]byte("nameserver[0] : "+tt.server+"\n"), []netip.Addr{netip.MustParseAddr(tt.local)}) + if err != nil { + t.Fatal(err) + } + var want []string + if !tt.excluded { + want = []string{tt.server} + } + if !slices.Equal(got, want) { + t.Fatalf("nameservers=%v, want %v", got, want) + } + }) + } +} + +func TestScutilNameserversRejectsPartialScan(t *testing.T) { + output := []byte("nameserver[0] : 192.0.2.53\n" + strings.Repeat("x", 128*1024)) + got, err := parseScutilNameservers(output, nil) + if err == nil || got != nil { + t.Fatalf("partial scan accepted: %v, %v", got, err) + } +} diff --git a/resolver.go b/resolver.go index 959d57a..38e48e7 100644 --- a/resolver.go +++ b/resolver.go @@ -522,8 +522,13 @@ func defaultLocalIPForServer(server string) net.IP { if err != nil { return nil } - ip := net.ParseIP(host) - if ip != nil && ip.To4() == nil { + addr, err := netip.ParseAddr(host) + if err == nil && addr.Zone() != "" { + // Scoped resolvers select their interface via the destination zone. + // A cached default-interface source can have the wrong family or scope. + return nil + } + if err == nil && addr.Is6() && !addr.Is4In6() { return GetDefaultLocalIPv6() } return GetDefaultLocalIPv4()