From 53d3d3d44a22b819399e38d5568ba5a66b66b924 Mon Sep 17 00:00:00 2001 From: Cuong Manh Le Date: Wed, 8 Jul 2026 15:00:23 +0700 Subject: [PATCH 01/11] cmd/cli: preserve fallback listener port across reload in DNS intercept On macOS DNS-intercept mode, when mDNSResponder owns *:53 ctrld falls back to listening on 127.0.0.1:5354, and the pf rdr rules correctly redirect DNS to the bound port at startup. However, DNS resolution later breaks with an endless watchdog "anchor intact but probe FAILED -> force reload" loop and `dig @127.0.0.1` timeouts. Root cause is config reload. In CD mode, apiConfigReload refetches the generated config (which always declares port 53) every hour and the reload merge in runWait only inherits the running port when the new port is 0. The generated config explicitly says 53, so `*p.cfg = *newCfg` reverts p.cfg to port 53. The DNS listener goroutines are started only when !reload, so they are never re-bound and stay on 5354. Every subsequent pf rebuild reads p.cfg and targets the dead port 53. Fix: after applying the reloaded config in DNS-intercept mode on darwin, restore the actual bound listener IP/Port into the in-memory config via the new preserveBoundListeners helper, logging the configured-vs-actual divergence. A reload cannot move the running listener anyway, so this keeps p.cfg consistent with reality; all pf rdr rules and the watchdog probe then target the live port. The on-disk generated config is intentionally left unchanged (still 53), so no generated-config change is required. --- cmd/cli/cli_intercept_listener_test.go | 90 +++++++++++++++++++++++++- cmd/cli/dns_intercept_darwin_test.go | 29 +++++++++ cmd/cli/prog.go | 45 +++++++++++++ 3 files changed, 163 insertions(+), 1 deletion(-) diff --git a/cmd/cli/cli_intercept_listener_test.go b/cmd/cli/cli_intercept_listener_test.go index 489b7ac..6787d1e 100644 --- a/cmd/cli/cli_intercept_listener_test.go +++ b/cmd/cli/cli_intercept_listener_test.go @@ -1,6 +1,10 @@ package cli -import "testing" +import ( + "testing" + + "github.com/Control-D-Inc/ctrld" +) func TestIsExplicitInterceptListener(t *testing.T) { tests := []struct { @@ -26,3 +30,87 @@ func TestIsExplicitInterceptListener(t *testing.T) { }) } } + +// TestPreserveBoundListeners is a regression test for #551: on reload, the on-disk +// generated config still declares 127.0.0.1:53, but the running listener has fallen back +// to 127.0.0.1:5354. preserveBoundListeners must keep the in-memory config on the actual +// bound port so pf rdr rules and probes do not target the dead default port. +func TestPreserveBoundListeners(t *testing.T) { + // cur = actual running listener (fell back to 5354); newCfg = freshly read from disk (53). + cur := map[string]*ctrld.ListenerConfig{"0": {IP: "127.0.0.1", Port: 5354}} + newListeners := map[string]*ctrld.ListenerConfig{"0": {IP: "127.0.0.1", Port: 53}} + + preserveBoundListeners(newListeners, cur) + + if got := newListeners["0"].Port; got != 5354 { + t.Errorf("listener port after reload = %d, want 5354 (actual bound port)", got) + } + if got := newListeners["0"].IP; got != "127.0.0.1" { + t.Errorf("listener IP after reload = %q, want 127.0.0.1", got) + } +} + +// TestPreserveBoundListeners_NoChange verifies that when the on-disk config matches the +// running listener, the config is left untouched (a legitimate reload with the same port). +func TestPreserveBoundListeners_NoChange(t *testing.T) { + cur := map[string]*ctrld.ListenerConfig{"0": {IP: "127.0.0.1", Port: 5354}} + newListeners := map[string]*ctrld.ListenerConfig{"0": {IP: "127.0.0.1", Port: 5354}} + + preserveBoundListeners(newListeners, cur) + + if got := newListeners["0"].Port; got != 5354 { + t.Errorf("listener port = %d, want 5354", got) + } +} + +// TestPreserveBoundListeners_MissingCurrent verifies that a listener present on disk but not +// in the current running set (e.g. newly added) is left as configured. +func TestPreserveBoundListeners_MissingCurrent(t *testing.T) { + cur := map[string]*ctrld.ListenerConfig{"0": {IP: "127.0.0.1", Port: 5354}} + newListeners := map[string]*ctrld.ListenerConfig{ + "0": {IP: "127.0.0.1", Port: 53}, + "1": {IP: "127.0.0.1", Port: 5355}, + } + + preserveBoundListeners(newListeners, cur) + + if got := newListeners["0"].Port; got != 5354 { + t.Errorf("listener 0 port = %d, want 5354 (preserved)", got) + } + if got := newListeners["1"].Port; got != 5355 { + t.Errorf("listener 1 port = %d, want 5355 (unchanged, no current binding)", got) + } +} + +// TestPreserveBoundListeners_ExplicitChangeNotMasked verifies that an explicit, non-default +// listener in the reloaded config is applied rather than reverted to the old bound listener. +// Reverting an explicit change would make the control-server reload comparison return 200 +// instead of 201, silently dropping the new listener. Regression guard for #551 review. +func TestPreserveBoundListeners_ExplicitChangeNotMasked(t *testing.T) { + // Running listener fell back to 5354; user reloads with an explicit new listener. + cur := map[string]*ctrld.ListenerConfig{"0": {IP: "127.0.0.1", Port: 5354}} + newListeners := map[string]*ctrld.ListenerConfig{"0": {IP: "127.0.0.2", Port: 5399}} + + preserveBoundListeners(newListeners, cur) + + if got := newListeners["0"].IP; got != "127.0.0.2" { + t.Errorf("explicit listener IP = %q, want 127.0.0.2 (not reverted)", got) + } + if got := newListeners["0"].Port; got != 5399 { + t.Errorf("explicit listener port = %d, want 5399 (not reverted)", got) + } +} + +// TestPreserveBoundListeners_ExplicitDefaultPreserved verifies that the default +// 127.0.0.1:53 listener remains fallback-eligible: when it diverges from the running +// fallback port it is still preserved (isExplicitInterceptListener treats :53 as non-explicit). +func TestPreserveBoundListeners_ExplicitDefaultPreserved(t *testing.T) { + cur := map[string]*ctrld.ListenerConfig{"0": {IP: "127.0.0.1", Port: 5354}} + newListeners := map[string]*ctrld.ListenerConfig{"0": {IP: "127.0.0.1", Port: 53}} + + preserveBoundListeners(newListeners, cur) + + if got := newListeners["0"].Port; got != 5354 { + t.Errorf("default listener port = %d, want 5354 (preserved fallback)", got) + } +} diff --git a/cmd/cli/dns_intercept_darwin_test.go b/cmd/cli/dns_intercept_darwin_test.go index c8d90b0..3ab810a 100644 --- a/cmd/cli/dns_intercept_darwin_test.go +++ b/cmd/cli/dns_intercept_darwin_test.go @@ -123,6 +123,35 @@ func TestPFBuildAnchorRules_Ordering(t *testing.T) { } } +// TestPFBuildAnchorRules_FallbackPort verifies that when the listener falls back +// to an alternate local port (e.g. 5354 because mDNSResponder owns *:53), the pf +// rdr rules redirect DNS to the ACTUAL bound port, not the configured default 53. +// Regression test for #551: pf redirected to a dead port after listener fallback. +func TestPFBuildAnchorRules_FallbackPort(t *testing.T) { + // Configured/generated listener is 127.0.0.1:53, but the runtime bound port is 5354. + p := &prog{cfg: &ctrld.Config{Listener: map[string]*ctrld.ListenerConfig{"0": {IP: "127.0.0.1", Port: 5354}}}} + rules := p.buildPFAnchorRules(nil) + + // rdr must redirect to the actual bound port 5354. + if !strings.Contains(rules, "rdr on lo0 inet proto udp from any to ! 127.0.0.1 port 53 -> 127.0.0.1 port 5354") { + t.Errorf("UDP rdr must redirect to bound port 5354, got:\n%s", rules) + } + if !strings.Contains(rules, "rdr on lo0 inet proto tcp from any to ! 127.0.0.1 port 53 -> 127.0.0.1 port 5354") { + t.Errorf("TCP rdr must redirect to bound port 5354, got:\n%s", rules) + } + + // The rdr redirect target must NOT point at the dead default port 53. + // Match the exact port at line end so "port 5354" is not a false positive. + if strings.Contains(rules, "-> 127.0.0.1 port 53\n") { + t.Errorf("rdr must not redirect to dead port 53 after fallback, got:\n%s", rules) + } + + // The inbound accept rule must also target the actual bound port. + if !strings.Contains(rules, "127.0.0.1 port 5354") { + t.Errorf("pass in rule must reference bound port 5354, got:\n%s", rules) + } +} + // TestPFAddressFamily tests the pfAddressFamily helper. func TestPFAddressFamily(t *testing.T) { tests := []struct { diff --git a/cmd/cli/prog.go b/cmd/cli/prog.go index 5da2490..27302f6 100644 --- a/cmd/cli/prog.go +++ b/cmd/cli/prog.go @@ -337,6 +337,18 @@ func (p *prog) runWait() { p.mu.Lock() *p.cfg = *newCfg + // In DNS-intercept mode on macOS, the DNS listener is bound once at startup and is + // NOT re-bound on reload (see prog.run: serveDNS is started only when !reload). When + // the configured/generated port (e.g. 127.0.0.1:53) is unavailable at startup because + // mDNSResponder owns *:53, ctrld falls back to an alternate local port (e.g. 5354). + // The on-disk config still declares 53, so adopting it here would revert p.cfg to a + // port nothing is listening on, and the pf rdr rules/probes rebuilt from p.cfg would + // target a dead port. Since a reload cannot move the running listener anyway, keep + // p.cfg pointing at the actual bound listener. The on-disk config (written above) is + // left unchanged. See #551. + if dnsIntercept && runtime.GOOS == "darwin" { + preserveBoundListeners(p.cfg.Listener, curListener) + } p.mu.Unlock() logger.Notice().Msg("reloading config successfully") @@ -348,6 +360,39 @@ func (p *prog) runWait() { } } +// preserveBoundListeners overrides the IP/Port of each listener in newListeners with the +// actual bound address from curListeners when they differ, logging the divergence. It is used +// on config reload in DNS-intercept mode where the running listener is never re-bound, so a +// port change on disk (e.g. reverting a fallback 5354 back to the generated 53) must not be +// applied to the in-memory config that drives pf rdr rules and probes. +// +// Preservation is limited to fallback-eligible (default/unset, i.e. 127.0.0.1:53) listeners. +// An explicit, non-default listener in the reloaded config is an intentional change that must +// be applied: tryUpdateListenerConfigIntercept binds explicit listeners exactly (no fallback), +// and the control-server reload handler detects the IP/port diff to trigger a restart that +// re-binds. Reverting an explicit change here would make that comparison return 200 instead of +// 201, silently dropping the new listener. See #551. +func preserveBoundListeners(newListeners, curListeners map[string]*ctrld.ListenerConfig) { + for n, curLc := range curListeners { + newLc := newListeners[n] + if newLc == nil || curLc == nil { + continue + } + if newLc.IP == curLc.IP && newLc.Port == curLc.Port { + continue + } + if isExplicitInterceptListener(newLc.IP, newLc.Port) { + continue + } + mainLog.Load().Info(). + Str("configured", net.JoinHostPort(newLc.IP, strconv.Itoa(newLc.Port))). + Str("actual", net.JoinHostPort(curLc.IP, strconv.Itoa(curLc.Port))). + Msg("DNS intercept: preserving actual bound listener across reload; on-disk config port not applied to running listener") + newLc.IP = curLc.IP + newLc.Port = curLc.Port + } +} + func (p *prog) preRun() { if iface == "auto" { iface = defaultIfaceName() From 836c9ccf124ab807f766ed2204b22d8059ed5f30 Mon Sep 17 00:00:00 2001 From: Cuong Manh Le Date: Thu, 9 Jul 2026 16:59:54 +0700 Subject: [PATCH 02/11] fix: treat 464XLAT CLAT source as local, not WAN On networks using 464XLAT (common on IPv6-only cellular carriers and iPhone hotspots), the local machine's DNS queries can reach ctrld's listener with a source address in the RFC 7335 IPv4 Service Continuity Prefix (192.0.0.0/29, e.g. 192.0.0.2 on the CLAT/host side). isWanClient classified 192.0.0.x as a WAN client, so with allow_wan_clients unset (the default) the query was refused, breaking DNS resolution entirely on the affected connection even though it originated from the local host. Recognize the IPv4 Service Continuity Prefix as a local range, mirroring the existing CGNAT special case: - Add ipv4ServiceContinuityPrefix (192.0.0.0/29) and an isServiceContinuityAddr helper (single definition, reused by both call sites). - isWanClient excludes the range, so 464XLAT/CLAT queries are served normally. - isPrivatePtrLookup treats the range as private so reverse lookups are handled consistently. Scoped to 192.0.0.0/29 (the exact 464XLAT range); this does not weaken allow_wan_clients since no globally routable remote client can appear from it. --- cmd/cli/dns_proxy.go | 20 ++++++++++++++++++-- cmd/cli/dns_proxy_test.go | 7 +++++++ 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/cmd/cli/dns_proxy.go b/cmd/cli/dns_proxy.go index 4c08083..00cbb5b 100644 --- a/cmd/cli/dns_proxy.go +++ b/cmd/cli/dns_proxy.go @@ -1299,7 +1299,8 @@ func isPrivatePtrLookup(m *dns.Msg) bool { return addr.IsPrivate() || addr.IsLoopback() || addr.IsLinkLocalUnicast() || - tsaddr.CGNATRange().Contains(addr) + tsaddr.CGNATRange().Contains(addr) || + isServiceContinuityAddr(addr) } } return false @@ -1337,6 +1338,20 @@ func isLanHostname(name string) bool { strings.HasSuffix(name, ".local") } +// ipv4ServiceContinuityPrefix is the RFC 7335 IPv4 Service Continuity Prefix +// (192.0.0.0/29), used by the CLAT in 464XLAT/DS-Lite transition setups. On such +// networks (common on IPv6-only cellular carriers and iPhone hotspots) the local +// machine's DNS queries reach ctrld with a source in this range (e.g. 192.0.0.2), +// so they must be treated as local, not WAN. Go's netip.IsPrivate does not cover +// this range — the same reason the CGNAT range is special-cased below. See #552. +var ipv4ServiceContinuityPrefix = netip.MustParsePrefix("192.0.0.0/29") + +// isServiceContinuityAddr reports whether ip is in the RFC 7335 IPv4 Service +// Continuity Prefix (464XLAT/DS-Lite CLAT). +func isServiceContinuityAddr(ip netip.Addr) bool { + return ipv4ServiceContinuityPrefix.Contains(ip) +} + // isWanClient reports whether the input is a WAN address. func isWanClient(na net.Addr) bool { var ip netip.Addr @@ -1347,7 +1362,8 @@ func isWanClient(na net.Addr) bool { !ip.IsPrivate() && !ip.IsLinkLocalUnicast() && !ip.IsLinkLocalMulticast() && - !tsaddr.CGNATRange().Contains(ip) + !tsaddr.CGNATRange().Contains(ip) && + !isServiceContinuityAddr(ip) } // isIPv6LoopbackListener reports whether the listener address is [::1]. diff --git a/cmd/cli/dns_proxy_test.go b/cmd/cli/dns_proxy_test.go index 7d94dbd..e8a4902 100644 --- a/cmd/cli/dns_proxy_test.go +++ b/cmd/cli/dns_proxy_test.go @@ -405,6 +405,8 @@ func Test_isPrivatePtrLookup(t *testing.T) { {"CGNAT", newDnsMsgPtr("100.66.27.28", t), true}, {"Loopback", newDnsMsgPtr("127.0.0.1", t), true}, {"Link Local Unicast", newDnsMsgPtr("fe80::69f6:e16e:8bdb:433f", t), true}, + // RFC 7335 IPv4 Service Continuity Prefix (464XLAT/DS-Lite CLAT), see #552. + {"464XLAT CLAT host", newDnsMsgPtr("192.0.0.2", t), true}, {"Public IP", newDnsMsgPtr("8.8.8.8", t), false}, } for _, tc := range tests { @@ -452,6 +454,11 @@ func Test_isWanClient(t *testing.T) { {"CGNAT", &net.UDPAddr{IP: net.ParseIP("100.66.27.28")}, false}, {"Loopback", &net.UDPAddr{IP: net.ParseIP("127.0.0.1")}, false}, {"Link Local Unicast", &net.UDPAddr{IP: net.ParseIP("fe80::69f6:e16e:8bdb:433f")}, false}, + // RFC 7335 IPv4 Service Continuity Prefix (464XLAT/DS-Lite CLAT), see #552. + {"464XLAT PLAT side", &net.UDPAddr{IP: net.ParseIP("192.0.0.1")}, false}, + {"464XLAT CLAT host", &net.UDPAddr{IP: net.ParseIP("192.0.0.2")}, false}, + // Outside the /29 but inside 192.0.0.0/24: still WAN (fix is scoped to /29). + {"192.0.0.0/24 outside /29", &net.UDPAddr{IP: net.ParseIP("192.0.0.100")}, true}, {"Public", &net.UDPAddr{IP: net.ParseIP("8.8.8.8")}, true}, } for _, tc := range tests { From d29e7d131e85d55e57468df56dd386cd65857498 Mon Sep 17 00:00:00 2001 From: Cuong Manh Le Date: Mon, 13 Jul 2026 20:52:57 +0700 Subject: [PATCH 03/11] fix: wrong DoQ resolver rewriting upstream responses with SetReply The DoQ resolver called SetReply on the already-unpacked upstream response. SetReply is meant to build a reply from a request, so it forces the RCODE to NOERROR and overwrites the Question with the request's question. This masked upstream failures from the proxy's failover logic (a SERVFAIL looked like a successful empty response) and corrupted the Question section of the response served to clients. Restore only the downstream transaction ID instead (RFC 9250 section 4.2.1 puts the DNS Message ID at 0 on the wire), preserving the upstream RCODE, Question, and answer sections untouched. This matches how the DoH and DoT resolvers return unpacked upstream responses. Refs github.com/Control-D-Inc/ctrld/issues/322 --- doq.go | 10 +++++- doq_test.go | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 111 insertions(+), 1 deletion(-) diff --git a/doq.go b/doq.go index a79cba9..6844895 100644 --- a/doq.go +++ b/doq.go @@ -242,7 +242,15 @@ func (p *doqConnPool) doResolve(ctx context.Context, msg *dns.Msg) (*dns.Msg, er if err := answer.Unpack(buf[2 : 2+respLen]); err != nil { return nil, err } - answer.SetReply(msg) + // RFC 9250 section 4.2.1 requires the DNS Message ID to be 0 on the wire, + // so restore the downstream transaction ID for the client. Do NOT use + // SetReply here: it rewrites the RCODE to NOERROR and overwrites the + // Question with the request's, which would mask upstream failures from the + // failover logic (a SERVFAIL would look like success) and let a + // wrong-question answer pass validation and poison the cache. Preserve the + // upstream RCODE, Question, and answer sections untouched so the proxy can + // evaluate them. See github.com/Control-D-Inc/ctrld/issues/322. + answer.Id = msg.Id return answer, nil } diff --git a/doq_test.go b/doq_test.go index 405b05b..98174f3 100644 --- a/doq_test.go +++ b/doq_test.go @@ -714,3 +714,105 @@ func TestDoQResolve_OversizedResponse_Rejected(t *testing.T) { t.Fatalf("Resolve returned non-nil answer alongside error: %v", answer) } } + +// frameDoQResponse packs msg and prepends the RFC 9250 2-octet length prefix, +// producing the exact bytes a DoQ server writes on the wire. +func frameDoQResponse(t *testing.T, msg *dns.Msg) []byte { + t.Helper() + b, err := msg.Pack() + if err != nil { + t.Fatalf("pack response: %v", err) + } + n := uint16(len(b)) + return append([]byte{byte(n >> 8), byte(n & 0xFF)}, b...) +} + +// TestDoQResolve_PreservesRcode locks in the fix for +// github.com/Control-D-Inc/ctrld/issues/322: the DoQ resolver must not rewrite +// an upstream response with SetReply, which would clobber a SERVFAIL into +// NOERROR and hide the failure from the proxy's failover logic. The upstream +// RCODE must survive; only the transaction ID is restored for the client. +func TestDoQResolve_PreservesRcode(t *testing.T) { + t.Parallel() + + // RFC 9250 puts the DNS Message ID at 0 on the wire. + resp := new(dns.Msg) + resp.SetQuestion("example.com.", dns.TypeA) + resp.Response = true + resp.Id = 0 + resp.Rcode = dns.RcodeServerFailure + + server := newMalformedDoQServer(t, frameDoQResponse(t, resp)) + uc := newMalformedDoQUpstream(t, server.cert, server.addr) + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + pool := newDOQConnPool(uc, []string{"127.0.0.1"}) + t.Cleanup(pool.CloseIdleConnections) + + msg := new(dns.Msg) + msg.SetQuestion("example.com.", dns.TypeA) + msg.RecursionDesired = true + + answer, err := pool.Resolve(ctx, msg) + if err != nil { + t.Fatalf("Resolve failed: %v", err) + } + if answer.Rcode != dns.RcodeServerFailure { + t.Fatalf("upstream SERVFAIL was rewritten to %s; failover would be bypassed", + dns.RcodeToString[answer.Rcode]) + } + if answer.Id != msg.Id { + t.Fatalf("transaction ID not restored: got %d, want %d", answer.Id, msg.Id) + } +} + +// TestDoQResolve_PreservesWrongQuestion locks in the fix for +// github.com/Control-D-Inc/ctrld/issues/322: when an upstream answers a +// different name than asked, the resolver must preserve the upstream's +// question rather than rewriting it to the request's question (as SetReply +// did). Rewriting would hide the mismatch and let wrong-domain records poison +// the shared cache. +func TestDoQResolve_PreservesWrongQuestion(t *testing.T) { + t.Parallel() + + resp := new(dns.Msg) + resp.SetQuestion("attacker.example.", dns.TypeA) + resp.Response = true + resp.Id = 0 + resp.Answer = append(resp.Answer, &dns.A{ + Hdr: dns.RR_Header{ + Name: "attacker.example.", + Rrtype: dns.TypeA, + Class: dns.ClassINET, + Ttl: 300, + }, + A: net.ParseIP("192.0.2.1"), + }) + + server := newMalformedDoQServer(t, frameDoQResponse(t, resp)) + uc := newMalformedDoQUpstream(t, server.cert, server.addr) + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + pool := newDOQConnPool(uc, []string{"127.0.0.1"}) + t.Cleanup(pool.CloseIdleConnections) + + msg := new(dns.Msg) + msg.SetQuestion("victim.example.", dns.TypeA) + msg.RecursionDesired = true + + answer, err := pool.Resolve(ctx, msg) + if err != nil { + t.Fatalf("Resolve failed: %v", err) + } + if len(answer.Question) == 0 || !strings.EqualFold(answer.Question[0].Name, "attacker.example.") { + t.Fatalf("upstream question was rewritten; got %v, want the upstream's attacker.example.", + answer.Question) + } + if answer.Id != msg.Id { + t.Fatalf("transaction ID not restored: got %d, want %d", answer.Id, msg.Id) + } +} From b79098658a3edac2efed9725432d350e7c32cb8d Mon Sep 17 00:00:00 2001 From: Dev Scribe Date: Sat, 4 Jul 2026 15:56:26 -0400 Subject: [PATCH 04/11] fix: restore DNS during invalid-device uninstall --- cmd/cli/cli.go | 75 ++++++++++++++++++------------- cmd/cli/commands.go | 16 +++---- cmd/cli/main.go | 3 ++ cmd/cli/prog.go | 2 +- cmd/cli/uninstall_invalid_test.go | 61 +++++++++++++++++++++++++ 5 files changed, 118 insertions(+), 39 deletions(-) create mode 100644 cmd/cli/uninstall_invalid_test.go diff --git a/cmd/cli/cli.go b/cmd/cli/cli.go index b75856f..4428d10 100644 --- a/cmd/cli/cli.go +++ b/cmd/cli/cli.go @@ -451,18 +451,7 @@ func run(appCallback *AppCallback, stopCh chan struct{}) { p.onStopped = append(p.onStopped, func() { // restore static DNS settings or DHCP p.resetDNS(false, true) - // Iterate over all physical interfaces and restore static DNS if a saved static config exists. - withEachPhysicalInterfaces("", "restore static DNS", func(i *net.Interface) error { - file := savedStaticDnsSettingsFilePath(i) - if _, err := os.Stat(file); err == nil { - if err := restoreDNS(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) - } - } - return nil - }) + restoreSavedStaticDNS("", false) }) close(waitCh) @@ -826,7 +815,7 @@ func processLogAndCacheFlags(v *viper.Viper, cfg *ctrld.Config) { } func netInterface(ifaceName string) (*net.Interface, error) { - if ifaceName == "auto" { + if ifaceName == autoIface { ifaceName = defaultIfaceName() } var iface *net.Interface @@ -1112,23 +1101,7 @@ func uninstall(p *prog, s service.Service) { } // restore static DNS settings or DHCP p.resetDNS(false, true) - - // Iterate over all physical interfaces and restore DNS if a saved static config exists. - withEachPhysicalInterfaces(p.runningIface, "restore static DNS", func(i *net.Interface) error { - file := savedStaticDnsSettingsFilePath(i) - if _, err := os.Stat(file); err == nil { - if err := restoreDNS(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) - err = os.Remove(file) - if err != nil { - mainLog.Load().Debug().Err(err).Msgf("Could not remove saved static DNS file for interface %s", i.Name) - } - } - } - return nil - }) + restoreSavedStaticDNS(p.runningIface, true) if router.Name() != "" { mainLog.Load().Debug().Msg("Router cleanup") @@ -1141,6 +1114,26 @@ func uninstall(p *prog, s service.Service) { } } +// 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 { + file := savedStaticDnsSettingsFilePath(i) + if _, err := os.Stat(file); err == nil { + if err := restoreDNS(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) + if removeSaved { + if err := os.Remove(file); err != nil { + mainLog.Load().Debug().Err(err).Msgf("Could not remove saved static DNS file for interface %s", i.Name) + } + } + } + } + return nil + }) +} + func validateConfig(cfg *ctrld.Config) error { if err := ctrld.ValidateConfig(validator.New(), cfg); err != nil { var ve validator.ValidationErrors @@ -2069,6 +2062,23 @@ func doValidateCdRemoteConfig(cdUID string, fatal bool) error { return nil } +// ensureRunningIfaceForInvalidUninstall populates p.runningIface before the +// invalid-device self-uninstall resets DNS. This path can run early during +// service startup (e.g. right after a reboot) before the running interface is +// otherwise known. resetDNS, via resetDNSForRunningIface, silently skips DNS +// restoration when p.runningIface is empty, which would leave the OS pointed at +// ctrld's local listener after the service is removed. See issue-556. +func ensureRunningIfaceForInvalidUninstall(p *prog, s service.Service) { + if iface == "" { + iface = autoIface + } + p.preRun() + if ir := runningIface(s); ir != nil { + p.runningIface = ir.Name + p.requiredMultiNICsConfig = ir.All + } +} + // uninstallInvalidCdUID performs self-uninstallation because the ControlD device does not exist. func uninstallInvalidCdUID(p *prog, logger zerolog.Logger, doStop bool) bool { s, err := newService(p, svcConfig) @@ -2076,8 +2086,13 @@ func uninstallInvalidCdUID(p *prog, logger zerolog.Logger, doStop bool) bool { logger.Warn().Err(err).Msg("failed to create new service") return false } + ensureRunningIfaceForInvalidUninstall(p, s) // restore static DNS settings or DHCP p.resetDNS(false, true) + // The invalid-device path may run early during service startup before runningIface + // is known. Restore every saved static DNS file so uninstalling does not leave the + // OS pointed at ctrld's local listener after the service is removed. + restoreSavedStaticDNS("", true) tasks := []task{{s.Uninstall, true, "Uninstall"}} if doTasks(tasks) { diff --git a/cmd/cli/commands.go b/cmd/cli/commands.go index a03745f..ec7b47c 100644 --- a/cmd/cli/commands.go +++ b/cmd/cli/commands.go @@ -399,7 +399,7 @@ NOTE: running "ctrld start" without any arguments will start already installed c reportSetDnsOk := func(sockDir string) { if cc := newSocketControlClient(ctx, s, sockDir); cc != nil { if resp, _ := cc.post(ifacePath, nil); resp != nil && resp.StatusCode == http.StatusOK { - if iface == "auto" { + if iface == autoIface { iface = defaultIfaceName() } res := &ifaceResponse{} @@ -748,7 +748,7 @@ NOTE: running "ctrld start" without any arguments will start already installed c startCmd.Run(cmd, args) }, } - startCmdAlias.Flags().StringVarP(&ifaceStartStop, "iface", "", "auto", `Update DNS setting for iface, "auto" means the default interface gateway`) + startCmdAlias.Flags().StringVarP(&ifaceStartStop, "iface", "", autoIface, `Update DNS setting for iface, "auto" means the default interface gateway`) startCmdAlias.Flags().AddFlagSet(startCmd.Flags()) rootCmd.AddCommand(startCmdAlias) @@ -833,7 +833,7 @@ func initStopCmd() *cobra.Command { stopCmd.Run(cmd, args) }, } - stopCmdAlias.Flags().StringVarP(&ifaceStartStop, "iface", "", "auto", `Reset DNS setting for iface, "auto" means the default interface gateway`) + stopCmdAlias.Flags().StringVarP(&ifaceStartStop, "iface", "", autoIface, `Reset DNS setting for iface, "auto" means the default interface gateway`) stopCmdAlias.Flags().AddFlagSet(stopCmd.Flags()) rootCmd.AddCommand(stopCmdAlias) @@ -865,7 +865,7 @@ func initRestartCmd() *cobra.Command { return } if iface == "" { - iface = "auto" + iface = autoIface } p.preRun() if ir := runningIface(s); ir != nil { @@ -1111,7 +1111,7 @@ NOTE: Uninstalling will set DNS to values provided by DHCP.`, return } if iface == "" { - iface = "auto" + iface = autoIface } p.preRun() if ir := runningIface(s); ir != nil { @@ -1207,7 +1207,7 @@ NOTE: Uninstalling will set DNS to values provided by DHCP.`, uninstallCmd.Run(cmd, args) }, } - uninstallCmdAlias.Flags().StringVarP(&ifaceStartStop, "iface", "", "auto", `Reset DNS setting for iface, "auto" means the default interface gateway`) + uninstallCmdAlias.Flags().StringVarP(&ifaceStartStop, "iface", "", autoIface, `Reset DNS setting for iface, "auto" means the default interface gateway`) uninstallCmdAlias.Flags().AddFlagSet(uninstallCmd.Flags()) rootCmd.AddCommand(uninstallCmdAlias) @@ -1400,7 +1400,7 @@ func initUpgradeCmd() *cobra.Command { return } if iface == "" { - iface = "auto" + iface = autoIface } p.preRun() if ir := runningIface(s); ir != nil { @@ -1595,7 +1595,7 @@ func onlyInterceptFlags(args []string) bool { } else { return false } - case arg == "--iface=auto" || arg == "--iface" || arg == "auto": + case arg == "--iface="+autoIface || arg == "--iface" || arg == autoIface: // Auto-added by startCmdAlias or its value; safe to ignore. continue default: diff --git a/cmd/cli/main.go b/cmd/cli/main.go index 972c308..762777d 100644 --- a/cmd/cli/main.go +++ b/cmd/cli/main.go @@ -56,6 +56,9 @@ const ( cdOrgFlagName = "cd-org" customHostnameFlagName = "custom-hostname" nextdnsFlagName = "nextdns" + + // autoIface is the sentinel --iface value meaning "use the default gateway interface". + autoIface = "auto" ) func init() { diff --git a/cmd/cli/prog.go b/cmd/cli/prog.go index 27302f6..6272934 100644 --- a/cmd/cli/prog.go +++ b/cmd/cli/prog.go @@ -394,7 +394,7 @@ func preserveBoundListeners(newListeners, curListeners map[string]*ctrld.Listene } func (p *prog) preRun() { - if iface == "auto" { + if iface == autoIface { iface = defaultIfaceName() p.requiredMultiNICsConfig = requiredMultiNICsConfig() } diff --git a/cmd/cli/uninstall_invalid_test.go b/cmd/cli/uninstall_invalid_test.go new file mode 100644 index 0000000..3ba3657 --- /dev/null +++ b/cmd/cli/uninstall_invalid_test.go @@ -0,0 +1,61 @@ +package cli + +import "testing" + +// Test_ensureRunningIfaceForInvalidUninstall is a regression test for issue-556: +// after a reboot, the invalid-device self-uninstall path could run before the +// running interface was known. Because resetDNS (via resetDNSForRunningIface) +// silently skips DNS restoration when p.runningIface is empty, the OS was left +// pointed at ctrld's local listener with no internet after the service was +// removed. ensureRunningIfaceForInvalidUninstall must populate p.runningIface +// before resetDNS runs. +func Test_ensureRunningIfaceForInvalidUninstall(t *testing.T) { + // preRun mutates the package-level iface global; restore it after the test. + origIface := iface + t.Cleanup(func() { iface = origIface }) + + // newService needs the package service config; it is safe to build here + // because ensureRunningIfaceForInvalidUninstall only queries the (absent) + // control socket via runningIface, which returns nil when ctrld is not + // running, and performs no DNS or service mutation. + s, err := newService(&prog{}, svcConfig) + if err != nil { + t.Fatalf("newService: %v", err) + } + + t.Run("iface flag already resolved but not yet copied", func(t *testing.T) { + iface = "eth-test" + p := &prog{} + + // Precondition mirrors the buggy post-reboot state: an empty running + // interface would make resetDNS skip restoration entirely. + if p.runningIface != "" { + t.Fatalf("precondition: runningIface = %q, want empty", p.runningIface) + } + + ensureRunningIfaceForInvalidUninstall(p, s) + + if p.runningIface == "" { + t.Fatal("runningIface still empty after prepare: resetDNS would skip DNS " + + "restoration and leave the OS pointed at ctrld's local listener") + } + if p.runningIface != "eth-test" { + t.Fatalf("runningIface = %q, want the resolved iface %q", p.runningIface, "eth-test") + } + }) + + t.Run("iface unset falls back to auto-detected interface", func(t *testing.T) { + iface = "" + p := &prog{} + + ensureRunningIfaceForInvalidUninstall(p, s) + + // With iface unset the prep resolves "auto" to the default interface + // (defaultIfaceName never returns empty on the supported platforms), so + // resetDNS has a concrete interface to restore. + if p.runningIface == "" { + t.Fatal("runningIface still empty after prepare with iface unset: " + + "resetDNS would skip DNS restoration") + } + }) +} From a4cfd4e479b167bad38eb321f96ebff28fbd5ab4 Mon Sep 17 00:00:00 2001 From: Cuong Manh Le Date: Tue, 14 Jul 2026 03:45:19 +0700 Subject: [PATCH 05/11] cmd/cli: discard upstream answers whose question mismatches the request Defense in depth against cache poisoning: a compromised or misbehaving upstream can return an answer for a different name than was asked (e.g. records for attacker.example in response to a query for victim.example). Such an answer would be cached under the legitimate request key and served to subsequent queries. Validate that the upstream answer echoes the request's question (case-insensitive name plus Qtype/Qclass, per RFC 1035 section 4.1.2) before serving or caching it. A mismatch is logged at debug level and the upstream is skipped, failing safe to the next upstream or SERVFAIL. Refs github.com/Control-D-Inc/ctrld/issues/322 --- cmd/cli/dns_proxy.go | 38 ++++++++++++++++++++++++++++++++++++++ cmd/cli/dns_proxy_test.go | 30 ++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+) diff --git a/cmd/cli/dns_proxy.go b/cmd/cli/dns_proxy.go index 00cbb5b..9992906 100644 --- a/cmd/cli/dns_proxy.go +++ b/cmd/cli/dns_proxy.go @@ -736,6 +736,17 @@ func (p *prog) proxy(ctx context.Context, req *proxyRequest) *proxyResponse { } continue } + // Reject an answer whose question does not match the request before it + // can be served or cached. A mismatched question means the upstream + // answered a different name/type than asked; caching it would poison + // the shared cache with wrong-domain records for the requested name. + // See github.com/Control-D-Inc/ctrld/issues/322. + if !sameQuestion(req.msg, answer) { + ctrld.Log(ctx, mainLog.Load().Debug(), + "discarding answer from %s: question mismatch (asked %q, got %q)", + upstreams[n], questionString(req.msg), questionString(answer)) + continue + } // We are doing LAN/PTR lookup using private resolver, so always process next one. // Except for the last, we want to send response instead of saying all upstream failed. if answer.Rcode != dns.RcodeSuccess && isLanOrPtrQuery && n != len(upstreamConfigs)-1 { @@ -899,6 +910,33 @@ func containRcode(rcodes []int, rcode int) bool { return false } +// sameQuestion reports whether the upstream answer echoes the request's +// question. A well-behaved resolver always copies the question section from +// the query (RFC 1035 section 4.1.2); names are compared case-insensitively +// because DNS names are case-insensitive. A mismatch means the upstream +// answered a different name/type than asked - malformed or malicious - and the +// answer must not be served or cached, or it would poison the shared cache with +// wrong-domain records. See github.com/Control-D-Inc/ctrld/issues/322. +func sameQuestion(req, answer *dns.Msg) bool { + if req == nil || answer == nil { + return false + } + if len(req.Question) == 0 || len(answer.Question) == 0 { + return false + } + rq, aq := req.Question[0], answer.Question[0] + return rq.Qtype == aq.Qtype && rq.Qclass == aq.Qclass && strings.EqualFold(rq.Name, aq.Name) +} + +// questionString renders a message's first question as "name/type" for logging. +func questionString(msg *dns.Msg) string { + if msg == nil || len(msg.Question) == 0 { + return "" + } + q := msg.Question[0] + return q.Name + "/" + dns.TypeToString[q.Qtype] +} + func setCachedAnswerTTL(answer *dns.Msg, now, expiredTime time.Time) { ttlSecs := expiredTime.Sub(now).Seconds() if ttlSecs < 0 { diff --git a/cmd/cli/dns_proxy_test.go b/cmd/cli/dns_proxy_test.go index e8a4902..5a82a1a 100644 --- a/cmd/cli/dns_proxy_test.go +++ b/cmd/cli/dns_proxy_test.go @@ -481,3 +481,33 @@ func Test_prog_queryFromSelf(t *testing.T) { p.queryFromSelf("foo") }) } + +func Test_sameQuestion(t *testing.T) { + mk := func(name string, qtype uint16) *dns.Msg { + m := new(dns.Msg) + m.SetQuestion(name, qtype) + return m + } + tests := []struct { + name string + req *dns.Msg + answer *dns.Msg + want bool + }{ + {"identical", mk("example.com.", dns.TypeA), mk("example.com.", dns.TypeA), true}, + {"case insensitive", mk("Example.COM.", dns.TypeA), mk("example.com.", dns.TypeA), true}, + {"different name", mk("victim.example.", dns.TypeA), mk("attacker.example.", dns.TypeA), false}, + {"different type", mk("example.com.", dns.TypeA), mk("example.com.", dns.TypeAAAA), false}, + {"nil req", nil, mk("example.com.", dns.TypeA), false}, + {"nil answer", mk("example.com.", dns.TypeA), nil, false}, + {"empty answer question", mk("example.com.", dns.TypeA), new(dns.Msg), false}, + } + for _, tc := range tests { + tc := tc + t.Run(tc.name, func(t *testing.T) { + if got := sameQuestion(tc.req, tc.answer); got != tc.want { + t.Errorf("sameQuestion() = %v, want %v", got, tc.want) + } + }) + } +} From c596ef586b699563058412801a915004b3ea1bdd Mon Sep 17 00:00:00 2001 From: Cuong Manh Le Date: Wed, 15 Jul 2026 00:58:58 +0700 Subject: [PATCH 06/11] fix: skip internal logging in silent mode Running with --silent still created and grew log files in cd mode. needInternalLogging() only checked cdUID and Service.LogPath, so a --silent flag enabled internal logging, persisted it to disk, and reset the global log level back to debug, overriding the NoLevel that --silent had set. Return false from needInternalLogging() when silent is set, so ctrld neither creates the internal log file nor writes debug logs. Add regression tests asserting needInternalLogging() is false in silent mode and that initInternalLogging() creates no log file. Refs https://github.com/Control-D-Inc/ctrld/issues/320 --- cmd/cli/log_writer.go | 6 +++ cmd/cli/log_writer_silent_test.go | 66 +++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+) create mode 100644 cmd/cli/log_writer_silent_test.go diff --git a/cmd/cli/log_writer.go b/cmd/cli/log_writer.go index 609b1d6..201968a 100644 --- a/cmd/cli/log_writer.go +++ b/cmd/cli/log_writer.go @@ -318,6 +318,12 @@ func (p *prog) initInternalLogging(writers []io.Writer) { // needInternalLogging reports whether prog needs to run internal logging. func (p *prog) needInternalLogging() bool { + // Do not run in silent mode: the user explicitly asked for no logging, so + // ctrld must not create or write the persisted internal log file (nor reset + // the global level back to debug). See https://github.com/Control-D-Inc/ctrld/issues/320. + if silent { + return false + } // Do not run in non-cd mode. if cdUID == "" { return false diff --git a/cmd/cli/log_writer_silent_test.go b/cmd/cli/log_writer_silent_test.go new file mode 100644 index 0000000..e40f779 --- /dev/null +++ b/cmd/cli/log_writer_silent_test.go @@ -0,0 +1,66 @@ +package cli + +import ( + "os" + "path/filepath" + "testing" + + "github.com/Control-D-Inc/ctrld" +) + +// Test_needInternalLogging_silent is a regression test for +// https://github.com/Control-D-Inc/ctrld/issues/320: running with --silent must +// not enable internal logging, otherwise ctrld creates and writes +// /ctrld.log (and, when verbose==0, resets the global level back to +// debug) despite the user asking for silence. +func Test_needInternalLogging_silent(t *testing.T) { + origSilent, origCdUID := silent, cdUID + t.Cleanup(func() { silent, cdUID = origSilent, origCdUID }) + + tests := []struct { + name string + silent bool + cdUID string + logPath string + want bool + }{ + {"silent suppresses internal logging in cd mode", true, "test-uid", "", false}, + {"cd mode enables internal logging", false, "test-uid", "", true}, + {"non-cd mode disabled", false, "", "", false}, + {"explicit log path disables internal logging", false, "test-uid", "/var/log/ctrld.log", false}, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + silent = tt.silent + cdUID = tt.cdUID + p := &prog{cfg: &ctrld.Config{}} + p.cfg.Service.LogPath = tt.logPath + if got := p.needInternalLogging(); got != tt.want { + t.Fatalf("needInternalLogging() = %v, want %v", got, tt.want) + } + }) + } +} + +// Test_initInternalLogging_silentCreatesNoFile drives the real initInternalLogging +// path and asserts that a --silent --cd run does not create /ctrld.log, +// which is the observable failure reported in +// https://github.com/Control-D-Inc/ctrld/issues/320. +func Test_initInternalLogging_silentCreatesNoFile(t *testing.T) { + origSilent, origCdUID, origHomedir := silent, cdUID, homedir + t.Cleanup(func() { silent, cdUID, homedir = origSilent, origCdUID, origHomedir }) + + dir := t.TempDir() + homedir = dir + cdUID = "test-uid" // cd mode, which would otherwise enable internal logging + silent = true + + p := &prog{cfg: &ctrld.Config{}} + p.initInternalLogging(nil) + + logPath := filepath.Join(dir, logFileName) + if _, err := os.Stat(logPath); !os.IsNotExist(err) { + t.Fatalf("silent mode must not create %s (stat err = %v)", logPath, err) + } +} From fa074f1f5e2d82cb4a3a470364317c33eb545295 Mon Sep 17 00:00:00 2001 From: Cuong Manh Le Date: Wed, 15 Jul 2026 22:48:39 +0700 Subject: [PATCH 07/11] fix: count DoQ/UDP test server call before writing reply countHandler incremented its call counter after w.WriteMsg, but the DNS client returns as soon as it receives the reply. A test reading the counter right after Resolve returned could therefore observe a stale zero, e.g. Test_Edns0_CacheReply intermittently failing on CI with "cache not hit, server was called: 0" while passing on retry. Increment the counter before writing the reply so it is guaranteed visible once the client has the response. Verified by widening the post-write window to reproduce the failure deterministically, then confirming the reordered handler passes 500x and under -race. --- resolver_test.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/resolver_test.go b/resolver_test.go index f030739..f45c16a 100644 --- a/resolver_test.go +++ b/resolver_test.go @@ -383,6 +383,11 @@ func nonSuccessHandlerWithRcode(rcode int) dns.HandlerFunc { func countHandler(call *atomic.Int64) dns.HandlerFunc { return func(w dns.ResponseWriter, msg *dns.Msg) { + // Count the call before writing the reply. The client returns as soon + // as it receives the response, so a caller that reads this counter right + // after Resolve returns would race an increment done after WriteMsg and + // could observe a stale zero. + call.Add(1) m := new(dns.Msg) m.SetRcode(msg, dns.RcodeSuccess) if cookie := getEdns0Cookie(msg.IsEdns0()); cookie != nil { @@ -395,7 +400,6 @@ func countHandler(call *atomic.Int64) dns.HandlerFunc { m.IsEdns0().Option = append(m.IsEdns0().Option, cookieOption) } w.WriteMsg(m) - call.Add(1) } } From 737fc79b58b825de484737cc5b12fad51bd73657 Mon Sep 17 00:00:00 2001 From: Cuong Manh Le Date: Thu, 16 Jul 2026 21:53:23 +0700 Subject: [PATCH 08/11] fix: harden DoH oversized-body tests against server write timing TestDoHResolve_{OversizedBody_Rejected,NonOKStatus_BoundedErrorBody, OversizedBody_DoH3} asserted how many bytes the test server managed to write before the client tore down the connection. That count reflects kernel socket send buffers and HTTP/2 flow-control windows, which vary by OS and load, so the server could buffer the whole body before teardown and fail the assertion. It flaked on the Windows CI runner, but reproduces on Linux too. Replace the server-side byte counter with a deterministic synchronization point. The handler writes exactly the read cap (dohMaxResponseSize+1 for the body, dohMaxErrorBodySize for the error path), flushes, then blocks without ever returning, so the response stream never gets an EOF. The test then requires Resolve to return the size/status error before the handler is released: ctrld's bounded read (io.LimitReader) returns after the capped prefix, while a read to EOF would block on the withheld stream and trip the deadline. This removes the socket-buffer timing dependence and, unlike asserting on the returned error alone, still fails if the caps are removed -- verified by reverting both reads in doh.go to io.ReadAll(resp.Body), which makes all three tests time out. --- doh_test.go | 147 +++++++++++++++++++++++++--------------------------- 1 file changed, 70 insertions(+), 77 deletions(-) diff --git a/doh_test.go b/doh_test.go index 6d1e726..96d35a4 100644 --- a/doh_test.go +++ b/doh_test.go @@ -12,7 +12,6 @@ import ( "net/url" "runtime" "strings" - "sync/atomic" "testing" "time" @@ -268,33 +267,62 @@ func newTestHTTP3Server(t *testing.T, handler http.Handler) *testHTTP3Server { return h3Server } -// oversizedDoHHandler streams `bodyBytes` bytes of zeros with the given -// HTTP status. The atomic counter records bytes the handler actually -// wrote, so tests can confirm the client tore down the stream before -// consuming the whole attacker-controlled body. -func oversizedDoHHandler(status int, bodyBytes int64, written *atomic.Int64) http.HandlerFunc { +// blockingBodyHandler writes exactly nbytes of body with the given status, +// flushes them, then blocks until release is closed WITHOUT ever returning. +// Because the handler does not return, the response stream is never terminated +// (no EOF/FIN). A client that stops after a bounded prefix therefore completes, +// while a client that reads to EOF blocks. Tests set nbytes to the exact read +// cap so the client consumes the whole written body (no half-written frame is +// left blocking on flow control) yet still never sees EOF. +func blockingBodyHandler(status, nbytes int, release <-chan struct{}) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", headerApplicationDNS) w.WriteHeader(status) - chunk := make([]byte, 64*1024) - var sent int64 - for sent < bodyBytes { - n := int64(len(chunk)) - if remaining := bodyBytes - sent; remaining < n { - n = remaining - } - m, err := w.Write(chunk[:n]) - if err != nil { - return - } - sent += int64(m) - if written != nil { - written.Add(int64(m)) - } - if f, ok := w.(http.Flusher); ok { - f.Flush() - } + if _, err := w.Write(make([]byte, nbytes)); err != nil { + return } + if f, ok := w.(http.Flusher); ok { + f.Flush() + } + <-release + } +} + +// requireBoundedResolve asserts that r.Resolve returns the expected size/status +// error while the server is still withholding EOF (the handler is blocked in +// blockingBodyHandler). Returning under those conditions proves ctrld read only +// a bounded prefix of the body: a resolver that instead read to EOF would block +// on the withheld stream and trip the deadline. This is the deterministic +// regression guard for the issue-312 OOM protections, replacing the earlier +// flaky server-side byte counter (issue-561). +func requireBoundedResolve(t *testing.T, r Resolver, msg *dns.Msg, wantErrSubstr string) { + t.Helper() + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + type result struct { + answer *dns.Msg + err error + } + done := make(chan result, 1) + go func() { + answer, err := r.Resolve(ctx, msg) + done <- result{answer, err} + }() + + select { + case res := <-done: + if res.err == nil { + t.Fatalf("Resolve unexpectedly succeeded; answer=%v", res.answer) + } + if !strings.Contains(res.err.Error(), wantErrSubstr) { + t.Fatalf("error %q does not contain %q", res.err, wantErrSubstr) + } + if res.answer != nil { + t.Fatalf("Resolve returned non-nil answer alongside error: %v", res.answer) + } + case <-time.After(5 * time.Second): + t.Fatal("Resolve did not return while the server withheld EOF: the body is being read to EOF instead of a bounded prefix (issue-312 OOM protection missing)") } } @@ -348,9 +376,12 @@ func doh3UpstreamForAddr(t *testing.T, addr string, cert *x509.Certificate) *Ups // returning a body larger than the DNS protocol allows must be rejected // with an explicit size error rather than buffered into ctrld memory. func TestDoHResolve_OversizedBody_Rejected(t *testing.T) { - const oversized = 2 * 1024 * 1024 // far past dohMaxResponseSize (~64 KiB) - var written atomic.Int64 - srv := httptest.NewUnstartedServer(oversizedDoHHandler(http.StatusOK, oversized, &written)) + // Write exactly the LimitReader cap, then withhold EOF. ctrld's bounded + // read (io.LimitReader of dohMaxResponseSize+1) returns after this prefix; + // an unbounded read would block on the missing EOF and trip the deadline. + release := make(chan struct{}) + defer close(release) + srv := httptest.NewUnstartedServer(blockingBodyHandler(http.StatusOK, dohMaxResponseSize+1, release)) testCert := generateTestCertificate(t) srv.TLS = &tls.Config{ Certificates: []tls.Certificate{testCert.tlsCert}, @@ -370,31 +401,19 @@ func TestDoHResolve_OversizedBody_Rejected(t *testing.T) { msg.SetQuestion("example.com.", dns.TypeA) msg.RecursionDesired = true - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) - defer cancel() - - answer, err := r.Resolve(ctx, msg) - if err == nil { - t.Fatalf("Resolve unexpectedly succeeded; answer=%v", answer) - } - if !strings.Contains(err.Error(), "maximum DNS message size") { - t.Fatalf("error %q does not mention the size cap", err) - } - if answer != nil { - t.Fatalf("Resolve returned non-nil answer alongside error: %v", answer) - } - if got := written.Load(); got >= int64(oversized) { - t.Fatalf("server wrote the entire %d-byte body before client tore down (wrote=%d) — cap not effective", oversized, got) - } + requireBoundedResolve(t, r, msg, "maximum DNS message size") } // TestDoHResolve_NonOKStatus_BoundedErrorBody locks in that a non-200 // response with a huge body does not pull the body fully into ctrld // memory just to format an error string. func TestDoHResolve_NonOKStatus_BoundedErrorBody(t *testing.T) { - const huge = 8 * 1024 * 1024 - var written atomic.Int64 - srv := httptest.NewUnstartedServer(oversizedDoHHandler(http.StatusBadGateway, huge, &written)) + // Same synchronization as the oversized-body test, but at the error-body + // cap: the non-200 path reads through an io.LimitReader of + // dohMaxErrorBodySize, so it must return after this prefix without EOF. + release := make(chan struct{}) + defer close(release) + srv := httptest.NewUnstartedServer(blockingBodyHandler(http.StatusBadGateway, dohMaxErrorBodySize, release)) testCert := generateTestCertificate(t) srv.TLS = &tls.Config{ Certificates: []tls.Certificate{testCert.tlsCert}, @@ -414,36 +433,22 @@ func TestDoHResolve_NonOKStatus_BoundedErrorBody(t *testing.T) { msg.SetQuestion("example.com.", dns.TypeA) msg.RecursionDesired = true - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) - defer cancel() - - answer, err := r.Resolve(ctx, msg) - if err == nil { - t.Fatalf("Resolve unexpectedly succeeded; answer=%v", answer) - } - if !strings.Contains(err.Error(), "status: 502") { - t.Fatalf("error %q does not surface the upstream status", err) - } - if answer != nil { - t.Fatalf("Resolve returned non-nil answer alongside error: %v", answer) - } - if got := written.Load(); got > 1024*1024 { - t.Fatalf("server wrote %d bytes before client tore down — error path is reading too much body", got) - } + requireBoundedResolve(t, r, msg, "status: 502") } // TestDoHResolve_OversizedBody_DoH3 mirrors the DoH oversized-body check // on the HTTP/3 transport, since github-312 specifically reproduced the // OOM via DoH3. func TestDoHResolve_OversizedBody_DoH3(t *testing.T) { - const oversized = 2 * 1024 * 1024 + release := make(chan struct{}) + defer close(release) testCert := generateTestCertificate(t) udpConn, err := net.ListenUDP("udp", &net.UDPAddr{IP: net.ParseIP("127.0.0.1"), Port: 0}) if err != nil { t.Fatalf("udp listen: %v", err) } h3 := &http3.Server{ - Handler: oversizedDoHHandler(http.StatusOK, oversized, nil), + Handler: blockingBodyHandler(http.StatusOK, dohMaxResponseSize+1, release), TLSConfig: &tls.Config{ Certificates: []tls.Certificate{testCert.tlsCert}, NextProtos: []string{"h3"}, @@ -471,17 +476,5 @@ func TestDoHResolve_OversizedBody_DoH3(t *testing.T) { msg.SetQuestion("example.com.", dns.TypeA) msg.RecursionDesired = true - ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) - defer cancel() - - answer, err := r.Resolve(ctx, msg) - if err == nil { - t.Fatalf("Resolve unexpectedly succeeded; answer=%v", answer) - } - if !strings.Contains(err.Error(), "maximum DNS message size") { - t.Fatalf("error %q does not mention the size cap", err) - } - if answer != nil { - t.Fatalf("Resolve returned non-nil answer alongside error: %v", answer) - } + requireBoundedResolve(t, r, msg, "maximum DNS message size") } From d38538f5932e970342d1d5a7eb1c9e46cb1652bc Mon Sep 17 00:00:00 2001 From: Cuong Manh Le Date: Tue, 21 Jul 2026 15:54:02 +0700 Subject: [PATCH 09/11] fix: partition DNS cache by EDNS Client Subnet MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With cache_enable = true, one cache entry was shared by every client asking the same name against the same upstream: the cache key ({Qtype, Qclass, Name, Upstream}) and the osResolver hot-cache/singleflight key ("name:qtype:") both ignored the EDNS Client Subnet (ECS). A response tailored for subnet A was therefore served to subnet B. A cached answer's records are scoped to the network that generated them (RFC 7871 §7.3), so sharing them across subnets returns the wrong CDN/policy answer. Rewriting only the ECS option on the shared answer is worse: forwarders that validate the echoed ECS (e.g. dnsmasq with add-subnet) then accept the wrong-subnet answer instead of rejecting it as a mismatch. Partition both cache paths by a canonical ECS tuple (family, source-prefix, masked address) via the new dnscache.CanonicalECS: the LRU key gains an ECS field and the singleflight/hot-cache key appends the canonical ECS. Same subnet still shares an entry; different subnets (or address families) never do. Only a request with no ECS option collapses to the shared empty partition; a carried /0 keeps its own family-scoped token, since it is forwarded with an ECS option and must stay distinguishable from a no-ECS query (RFC 7871 §7.3.1). SetCacheReply no longer touches ECS and only reconciles the EDNS Cookie. Adds real cache-path regression tests (LRU and osResolver hot cache) that serve a different A record per subnet and verify the second subnet never receives the first's record. Fixes https://github.com/Control-D-Inc/ctrld/issues/324 --- dns.go | 6 ++ dns_test.go | 57 ++++++++++ internal/dnscache/cache.go | 61 ++++++++++- internal/dnscache/cache_test.go | 186 ++++++++++++++++++++++++++++++++ resolver.go | 8 +- resolver_test.go | 87 +++++++++++++++ 6 files changed, 402 insertions(+), 3 deletions(-) create mode 100644 dns_test.go create mode 100644 internal/dnscache/cache_test.go diff --git a/dns.go b/dns.go index f2b71a5..f89f824 100644 --- a/dns.go +++ b/dns.go @@ -14,6 +14,12 @@ func SetCacheReply(answer, msg *dns.Msg, code int) { // See https://datatracker.ietf.org/doc/html/rfc7873#section-4 sCookie.Cookie = cCookie.Cookie[:16] + sCookie.Cookie[16:] } + // NOTE: the answer's EDNS Client Subnet (ECS) is intentionally left as the + // upstream returned it. Correctness across clients is guaranteed by + // partitioning the cache and singleflight keys by ECS (see + // dnscache.CanonicalECS), so a cache hit only ever serves an answer that was + // resolved for the requester's own subnet. Rewriting the ECS option here + // without re-scoping the Answer records would violate RFC 7871 §7.3. } // getEdns0Cookie returns Edns0 cookie from *dns.OPT if present. diff --git a/dns_test.go b/dns_test.go new file mode 100644 index 0000000..9ca7608 --- /dev/null +++ b/dns_test.go @@ -0,0 +1,57 @@ +package ctrld + +import ( + "net" + "testing" + + "github.com/miekg/dns" +) + +// Test_SetCacheReply_DoesNotRewriteECS documents the post-#564 contract: cross-client +// correctness is guaranteed by partitioning the cache/singleflight keys by ECS +// (dnscache.CanonicalECS), NOT by rewriting the cached answer's ECS option. Rewriting the +// ECS metadata while leaving the Answer records scoped to another subnet would violate +// RFC 7871 §7.3 and make forwarders accept a wrong-subnet answer. SetCacheReply must +// therefore leave the answer's ECS untouched. +func Test_SetCacheReply_DoesNotRewriteECS(t *testing.T) { + answer := new(dns.Msg) + answer.SetQuestion(dns.Fqdn("controld.com"), dns.TypeA) + answer.SetEdns0(4096, false) + cachedSubnet := &dns.EDNS0_SUBNET{ + Code: dns.EDNS0SUBNET, + Family: 2, + SourceNetmask: 64, + SourceScope: 64, + Address: net.ParseIP("2001:db8:1::"), + } + answer.IsEdns0().Option = append(answer.IsEdns0().Option, cachedSubnet) + + req := new(dns.Msg) + req.SetQuestion(dns.Fqdn("controld.com"), dns.TypeA) + req.SetEdns0(4096, true) + req.IsEdns0().Option = append(req.IsEdns0().Option, &dns.EDNS0_SUBNET{ + Code: dns.EDNS0SUBNET, + Family: 2, + SourceNetmask: 64, + Address: net.ParseIP("2001:db8:2::"), + }) + + SetCacheReply(answer, req, dns.RcodeSuccess) + + var got *dns.EDNS0_SUBNET + for _, o := range answer.IsEdns0().Option { + if e, ok := o.(*dns.EDNS0_SUBNET); ok { + got = e + break + } + } + if got == nil { + t.Fatal("SetCacheReply dropped the answer's ECS option") + } + if want := net.ParseIP("2001:db8:1::"); !got.Address.Equal(want) { + t.Fatalf("SetCacheReply rewrote the answer ECS to the requester's subnet: got %v, want %v (unchanged)", got.Address, want) + } + if got.SourceScope != 64 { + t.Fatalf("SetCacheReply altered the answer ECS scope: got %d, want 64 (unchanged)", got.SourceScope) + } +} diff --git a/internal/dnscache/cache.go b/internal/dnscache/cache.go index af8883e..1437370 100644 --- a/internal/dnscache/cache.go +++ b/internal/dnscache/cache.go @@ -1,6 +1,8 @@ package dnscache import ( + "fmt" + "net" "strings" "time" @@ -16,11 +18,17 @@ type Cacher interface { } // Key is the caching key for DNS message. +// +// ECS partitions the cache by EDNS Client Subnet so an answer resolved for one +// subnet is never served to a client in a different subnet. Answer records are +// scoped to the network that generated them (RFC 7871 §7.3), so they must not +// be shared across subnets even when the question is otherwise identical. type Key struct { Qtype uint16 Qclass uint16 Name string Upstream string + ECS string } type Value struct { @@ -60,7 +68,58 @@ func NewLRUCache(size int) (*LRUCache, error) { // NewKey creates a new cache key for given DNS message. func NewKey(msg *dns.Msg, upstream string) Key { q := msg.Question[0] - return Key{Qtype: q.Qtype, Qclass: q.Qclass, Name: normalizeQname(q.Name), Upstream: upstream} + return Key{Qtype: q.Qtype, Qclass: q.Qclass, Name: normalizeQname(q.Name), Upstream: upstream, ECS: CanonicalECS(msg)} +} + +// CanonicalECS returns a canonical string form of the EDNS Client Subnet (ECS, +// EDNS option 8) carried by msg, suitable for partitioning cache and +// singleflight keys. A request with no ECS option returns "", so all ECS-less +// queries share one partition and behave as before. +// +// A request that DOES carry an ECS option is never mapped to "", even at SOURCE +// PREFIX-LENGTH 0: the /0 query is forwarded with an ECS option, may draw +// different upstream data than an ECS-less query, and its answer (with the +// echoed option) must not be served to a client that sent no ECS — RFC 7871 +// §7.3.1 requires /0-cached data to remain distinguishable. The family is part +// of the token, so an IPv4 /0 and an IPv6 /0 stay distinct too. +// +// The address is masked to its source prefix length so only the significant +// subnet bits contribute to the key: two clients in the same subnet share a +// partition, while different subnets (or address families) never do. The +// response-only SCOPE PREFIX-LENGTH is deliberately excluded — it is not part of +// what the client asked for. +func CanonicalECS(msg *dns.Msg) string { + opt := msg.IsEdns0() + if opt == nil { + return "" + } + for _, o := range opt.Option { + e, ok := o.(*dns.EDNS0_SUBNET) + if !ok { + continue + } + bits := int(e.SourceNetmask) + total := 128 + zero := net.IPv6zero + if e.Family == 1 { + total = 32 + zero = net.IPv4zero + } + addr := e.Address + if len(addr) == 0 { + // A /0 request commonly carries an empty address; normalize it to + // the family zero so its token is stable regardless of encoding. + addr = zero + } + masked := addr + if bits <= total { + if m := addr.Mask(net.CIDRMask(bits, total)); m != nil { + masked = m + } + } + return fmt.Sprintf("%d/%d/%s", e.Family, e.SourceNetmask, masked.String()) + } + return "" } // NewValue creates a new cache value for given DNS message. diff --git a/internal/dnscache/cache_test.go b/internal/dnscache/cache_test.go new file mode 100644 index 0000000..449ed1a --- /dev/null +++ b/internal/dnscache/cache_test.go @@ -0,0 +1,186 @@ +package dnscache + +import ( + "net" + "testing" + "time" + + "github.com/miekg/dns" +) + +// msgWithECS builds an A query for name carrying an EDNS Client Subnet option, or none +// when family == 0. +func msgWithECS(name string, family uint16, prefix uint8, addr string) *dns.Msg { + m := new(dns.Msg) + m.SetQuestion(dns.Fqdn(name), dns.TypeA) + if family == 0 { + return m + } + m.SetEdns0(4096, true) + m.IsEdns0().Option = append(m.IsEdns0().Option, &dns.EDNS0_SUBNET{ + Code: dns.EDNS0SUBNET, + Family: family, + SourceNetmask: prefix, + Address: net.ParseIP(addr), + }) + return m +} + +// answerWithA builds a cached answer holding a single A record with the given address. +func answerWithA(name, a string) *dns.Msg { + m := new(dns.Msg) + m.SetQuestion(dns.Fqdn(name), dns.TypeA) + rr, err := dns.NewRR(dns.Fqdn(name) + " 300 IN A " + a) + if err != nil { + panic(err) + } + m.Answer = []dns.RR{rr} + return m +} + +func firstA(msg *dns.Msg) string { + for _, rr := range msg.Answer { + if a, ok := rr.(*dns.A); ok { + return a.A.String() + } + } + return "" +} + +// TestCanonicalECS covers the key-partitioning helper: only a request with no ECS option +// collapses to the shared empty partition, while a carried /0 keeps its own family-scoped +// token (distinct from no-ECS, per RFC 7871 §7.3.1); host bits below the source prefix do +// not fragment the key, and different subnets / families produce different keys. +func TestCanonicalECS(t *testing.T) { + tests := []struct { + name string + msg *dns.Msg + want string + }{ + {"no ecs", msgWithECS("controld.com", 0, 0, ""), ""}, + // A carried ECS option is never "" (RFC 7871 §7.3.1), and IPv4 /0 vs + // IPv6 /0 stay distinct; the address encoding must not change the token. + {"ipv4 /0", msgWithECS("controld.com", 1, 0, "0.0.0.0"), "1/0/0.0.0.0"}, + {"ipv4 /0 empty addr", msgWithECS("controld.com", 1, 0, ""), "1/0/0.0.0.0"}, + {"ipv6 /0", msgWithECS("controld.com", 2, 0, "::"), "2/0/::"}, + {"ipv6 /0 empty addr", msgWithECS("controld.com", 2, 0, ""), "2/0/::"}, + {"ipv4 /24", msgWithECS("controld.com", 1, 24, "203.0.113.0"), "1/24/203.0.113.0"}, + {"ipv4 host bits masked", msgWithECS("controld.com", 1, 24, "203.0.113.7"), "1/24/203.0.113.0"}, + {"ipv6 /64", msgWithECS("controld.com", 2, 64, "2001:db8:1::"), "2/64/2001:db8:1::"}, + {"ipv6 host bits masked", msgWithECS("controld.com", 2, 64, "2001:db8:1::dead:beef"), "2/64/2001:db8:1::"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := CanonicalECS(tt.msg); got != tt.want { + t.Fatalf("CanonicalECS = %q, want %q", got, tt.want) + } + }) + } +} + +// TestNewKey_ECSPartition verifies the cache key distinguishes subnets while collapsing +// same-subnet requests, so the LRU cache cannot serve one subnet's records to another. +func TestNewKey_ECSPartition(t *testing.T) { + const up = "https://dns.example/dns-query" + subnetA := NewKey(msgWithECS("controld.com", 2, 64, "2001:db8:1::"), up) + subnetB := NewKey(msgWithECS("controld.com", 2, 64, "2001:db8:2::"), up) + subnetAHost := NewKey(msgWithECS("controld.com", 2, 64, "2001:db8:1::5"), up) + ipv4 := NewKey(msgWithECS("controld.com", 1, 24, "203.0.113.0"), up) + noECS := NewKey(msgWithECS("controld.com", 0, 0, ""), up) + + if subnetA == subnetB { + t.Fatal("different subnets must not share a cache key") + } + if subnetA != subnetAHost { + t.Fatal("same subnet (different host bits) must share a cache key") + } + if subnetA == ipv4 || subnetA == noECS || ipv4 == noECS { + t.Fatal("different families / no-ECS must not collide") + } +} + +// TestLRUCache_ECSZeroPrefixDistinctFromNoECS is the regression test for the /0-vs-no-ECS +// collision (RFC 7871 §7.3.1). A query carrying an ECS /0 option is forwarded WITH ECS and +// may draw different upstream data, so its cached answer must never be served to a client +// that sent no ECS at all, nor may IPv4 /0 and IPv6 /0 cross-serve each other. +func TestLRUCache_ECSZeroPrefixDistinctFromNoECS(t *testing.T) { + c, err := NewLRUCache(16) + if err != nil { + t.Fatalf("NewLRUCache: %v", err) + } + const up = "https://dns.example/dns-query" + expire := time.Now().Add(time.Minute) + + noECS := msgWithECS("controld.com", 0, 0, "") + zeroV4 := msgWithECS("controld.com", 1, 0, "0.0.0.0") + zeroV6 := msgWithECS("controld.com", 2, 0, "::") + + // Only the IPv4 /0 query's answer is cached. + c.Add(NewKey(zeroV4, up), NewValue(answerWithA("controld.com", "192.0.2.1"), expire)) + + // A no-ECS client must NOT receive the ECS /0 cached answer. + if got := c.Get(NewKey(noECS, up)); got != nil { + t.Fatalf("no-ECS client received the ECS /0 cached record %q", firstA(got.Msg)) + } + // An IPv6 /0 client must NOT receive the IPv4 /0 answer. + if got := c.Get(NewKey(zeroV6, up)); got != nil { + t.Fatalf("IPv6 /0 client received the IPv4 /0 cached record %q", firstA(got.Msg)) + } + // The IPv4 /0 client still hits its own entry. + if got := c.Get(NewKey(zeroV4, up)); got == nil || firstA(got.Msg) != "192.0.2.1" { + t.Fatalf("IPv4 /0 lost its own cached record: %v", got) + } + + // The no-ECS partition is independent and does not corrupt the /0 entry. + c.Add(NewKey(noECS, up), NewValue(answerWithA("controld.com", "198.51.100.1"), expire)) + if got := c.Get(NewKey(noECS, up)); got == nil || firstA(got.Msg) != "198.51.100.1" { + t.Fatalf("no-ECS partition wrong record: %v", got) + } + if got := c.Get(NewKey(zeroV4, up)); got == nil || firstA(got.Msg) != "192.0.2.1" { + t.Fatalf("IPv4 /0 record corrupted by no-ECS insert: %v", got) + } +} + +// TestLRUCache_ECSNoCrossSubnetServe is the real cache-path regression test for #564: +// an entry populated for subnet A must never be returned to a client in subnet B, even +// though the question (name/type/class/upstream) is identical. The two subnets carry +// different A records; the second client must get its own record or a miss, never A's. +func TestLRUCache_ECSNoCrossSubnetServe(t *testing.T) { + c, err := NewLRUCache(16) + if err != nil { + t.Fatalf("NewLRUCache: %v", err) + } + const up = "https://dns.example/dns-query" + expire := time.Now().Add(time.Minute) + + reqA := msgWithECS("controld.com", 2, 64, "2001:db8:1::") + reqB := msgWithECS("controld.com", 2, 64, "2001:db8:2::") + + // Only subnet A's answer (A record 192.0.2.1) is cached. + c.Add(NewKey(reqA, up), NewValue(answerWithA("controld.com", "192.0.2.1"), expire)) + + // A client in subnet B must NOT hit subnet A's entry. + if got := c.Get(NewKey(reqB, up)); got != nil { + t.Fatalf("subnet B received subnet A's cached record %q; cache is not ECS-partitioned", firstA(got.Msg)) + } + + // Subnet A still hits its own entry. + if got := c.Get(NewKey(reqA, up)); got == nil || firstA(got.Msg) != "192.0.2.1" { + t.Fatalf("subnet A lost its own cached record: %+v", got) + } + + // Now cache subnet B's distinct answer and confirm the two never cross. + c.Add(NewKey(reqB, up), NewValue(answerWithA("controld.com", "198.51.100.1"), expire)) + if got := c.Get(NewKey(reqB, up)); got == nil || firstA(got.Msg) != "198.51.100.1" { + t.Fatalf("subnet B got the wrong record: %v", got) + } + if got := c.Get(NewKey(reqA, up)); got == nil || firstA(got.Msg) != "192.0.2.1" { + t.Fatalf("subnet A record corrupted by subnet B insert: %v", got) + } + + // A second host within subnet A shares the partition (cache hit with A's record). + reqAHost := msgWithECS("controld.com", 2, 64, "2001:db8:1::9") + if got := c.Get(NewKey(reqAHost, up)); got == nil || firstA(got.Msg) != "192.0.2.1" { + t.Fatalf("same-subnet host missed the shared cache entry: %v", got) + } +} diff --git a/resolver.go b/resolver.go index 2284d04..a73bfa4 100644 --- a/resolver.go +++ b/resolver.go @@ -19,6 +19,8 @@ import ( "golang.org/x/sync/singleflight" "tailscale.com/net/netmon" "tailscale.com/net/tsaddr" + + "github.com/Control-D-Inc/ctrld/internal/dnscache" ) const ( @@ -378,8 +380,10 @@ func (o *osResolver) Resolve(ctx context.Context, msg *dns.Msg) (*dns.Msg, error domain := strings.TrimSuffix(msg.Question[0].Name, ".") qtype := msg.Question[0].Qtype - // Unique key for the singleflight group. - key := fmt.Sprintf("%s:%d:", domain, qtype) + // Unique key for the singleflight group. The EDNS Client Subnet is part of + // the key so subnet-specific answers are neither coalesced nor hot-cached + // across different subnets (RFC 7871 §7.3). + key := fmt.Sprintf("%s:%d:%s", domain, qtype, dnscache.CanonicalECS(msg)) // Checking the cache first. if val, ok := o.cache.Load(key); ok { diff --git a/resolver_test.go b/resolver_test.go index f45c16a..30a3af7 100644 --- a/resolver_test.go +++ b/resolver_test.go @@ -282,6 +282,93 @@ func Test_Edns0_CacheReply(t *testing.T) { } } +// ecsAnswerHandler returns a distinct A record per EDNS Client Subnet, so a test can +// prove one subnet never receives another subnet's cached record. It counts upstream +// calls to confirm the hot cache/singleflight is partitioned by ECS rather than shared. +func ecsAnswerHandler(call *atomic.Int64) dns.HandlerFunc { + return func(w dns.ResponseWriter, msg *dns.Msg) { + call.Add(1) + a := "203.0.113.1" // no/other subnet + if opt := msg.IsEdns0(); opt != nil { + for _, o := range opt.Option { + if e, ok := o.(*dns.EDNS0_SUBNET); ok { + switch { + case e.Address.Equal(net.ParseIP("2001:db8:1::")): + a = "192.0.2.1" + case e.Address.Equal(net.ParseIP("2001:db8:2::")): + a = "198.51.100.1" + } + } + } + } + m := new(dns.Msg) + m.SetReply(msg) + rr, _ := dns.NewRR(msg.Question[0].Name + " 300 IN A " + a) + m.Answer = []dns.RR{rr} + w.WriteMsg(m) + } +} + +// Test_osResolver_HotCache_ECSPartition is the real cache-path regression test for #564 on +// the osResolver hot cache / singleflight path: the upstream returns a different A record +// per subnet, and a client in subnet B must never be served subnet A's hot-cached record. +func Test_osResolver_HotCache_ECSPartition(t *testing.T) { + lanPC, err := net.ListenPacket("udp", "127.0.0.1:0") + if err != nil { + t.Fatalf("failed to listen on LAN address: %v", err) + } + call := &atomic.Int64{} + lanServer, lanAddr, err := runLocalPacketConnTestServer(t, lanPC, ecsAnswerHandler(call)) + if err != nil { + t.Fatalf("failed to run LAN test server: %v", err) + } + defer lanServer.Shutdown() + + or := newResolverWithNameserver([]string{lanAddr}) + query := func(subnet string) string { + m := new(dns.Msg) + m.SetQuestion(dns.Fqdn("controld.com"), dns.TypeA) + m.RecursionDesired = true + m.SetEdns0(4096, true) + m.IsEdns0().Option = append(m.IsEdns0().Option, &dns.EDNS0_SUBNET{ + Code: dns.EDNS0SUBNET, + Family: 2, + SourceNetmask: 64, + Address: net.ParseIP(subnet), + }) + answer, err := or.Resolve(context.Background(), m) + if err != nil { + t.Fatal(err) + } + for _, rr := range answer.Answer { + if a, ok := rr.(*dns.A); ok { + return a.A.String() + } + } + return "" + } + + // Subnet A populates the hot cache; a repeat hits it (upstream called once). + if got := query("2001:db8:1::"); got != "192.0.2.1" { + t.Fatalf("subnet A: got %q, want 192.0.2.1", got) + } + if got := query("2001:db8:1::"); got != "192.0.2.1" { + t.Fatalf("subnet A repeat: got %q, want 192.0.2.1", got) + } + if call.Load() != 1 { + t.Fatalf("subnet A repeat did not hit the hot cache: %d upstream calls", call.Load()) + } + + // Subnet B must get ITS OWN record, not subnet A's hot-cached one, and this + // requires a fresh upstream call (the cache is partitioned, not shared). + if got := query("2001:db8:2::"); got != "198.51.100.1" { + t.Fatalf("subnet B was served the wrong record %q (want 198.51.100.1); hot cache is not ECS-partitioned", got) + } + if call.Load() != 2 { + t.Fatalf("subnet B unexpectedly served from subnet A's cache: %d upstream calls, want 2", call.Load()) + } +} + // https://github.com/Control-D-Inc/ctrld/issues/255 func Test_legacyResolverWithBigExtraSection(t *testing.T) { lanPC, err := net.ListenPacket("udp", "127.0.0.1:0") // 127.0.0.1 is considered LAN (loopback) From 8ebe911b1a7b046b02c77bde11c5bbbbcd893290 Mon Sep 17 00:00:00 2001 From: Cuong Manh Le Date: Tue, 28 Jul 2026 16:15:14 +0700 Subject: [PATCH 10/11] Bump golang.org/x/text to v0.40.0 For fixing GO-2026-5970. --- go.mod | 8 ++++---- go.sum | 16 ++++++++-------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/go.mod b/go.mod index ebbde36..db654b9 100644 --- a/go.mod +++ b/go.mod @@ -37,7 +37,7 @@ require ( github.com/stretchr/testify v1.11.1 github.com/vishvananda/netlink v1.3.1 golang.org/x/net v0.56.0 - golang.org/x/sync v0.21.0 + golang.org/x/sync v0.22.0 golang.org/x/sys v0.46.0 golang.zx2c4.com/wireguard/windows v0.5.3 tailscale.com v1.74.0 @@ -94,9 +94,9 @@ require ( go4.org/netipx v0.0.0-20231129151722-fdeea329fbba // indirect golang.org/x/crypto v0.53.0 // indirect golang.org/x/exp v0.0.0-20240119083558-1b970713d09a // indirect - golang.org/x/mod v0.36.0 // indirect - golang.org/x/text v0.38.0 // indirect - golang.org/x/tools v0.45.0 // indirect + golang.org/x/mod v0.37.0 // indirect + golang.org/x/text v0.40.0 // indirect + golang.org/x/tools v0.47.0 // indirect google.golang.org/protobuf v1.33.0 // indirect gopkg.in/ini.v1 v1.67.0 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect diff --git a/go.sum b/go.sum index f9a9f11..b80b0cf 100644 --- a/go.sum +++ b/go.sum @@ -386,8 +386,8 @@ golang.org/x/mod v0.2.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/mod v0.3.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/mod v0.4.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/mod v0.4.1/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= -golang.org/x/mod v0.36.0 h1:JJjpVx6myfUsUdAzZuOSTTmRE0PfZeNWzzvKrP7amb4= -golang.org/x/mod v0.36.0/go.mod h1:moc6ELqsWcOw5Ef3xVprK5ul/MvtVvkIXLziUOICjUQ= +golang.org/x/mod v0.37.0 h1:vF1DjpVEshcIqoEaauuHebaLk1O1forxjxBaVn884JQ= +golang.org/x/mod v0.37.0/go.mod h1:m8S8VeM9r4dzDwjrKO0a1sZP3YjeMamRRlD+fmR2Q/0= golang.org/x/net v0.0.0-20180724234803-3673e40ba225/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= golang.org/x/net v0.0.0-20180826012351-8a410e7b638d/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= golang.org/x/net v0.0.0-20190108225652-1e06a53dbb7e/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= @@ -441,8 +441,8 @@ golang.org/x/sync v0.0.0-20200317015054-43a5402ce75a/go.mod h1:RxMgew5VJxzue5/jJ golang.org/x/sync v0.0.0-20200625203802-6e8e738ad208/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20201020160332-67f06af15bc9/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20201207232520-09787c993a3a/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= -golang.org/x/sync v0.21.0 h1:HLII4xRRTtCRkxYp4HNFF0Js/Og6q2i++KXbg0gHCwM= -golang.org/x/sync v0.21.0/go.mod h1:9xrNwdLfx4jkKbNva9FpL6vEN7evnE43NNNJQ2LF3+0= +golang.org/x/sync v0.22.0 h1:SZjpbeLmrCk4xhRSZFNZW5gFUeCeFgjekvI/+gfScek= +golang.org/x/sync v0.22.0/go.mod h1:9xrNwdLfx4jkKbNva9FpL6vEN7evnE43NNNJQ2LF3+0= golang.org/x/sys v0.0.0-20180830151530-49385e6e1522/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20190312061237-fead79001313/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= @@ -504,8 +504,8 @@ golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= golang.org/x/text v0.3.4/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= golang.org/x/text v0.3.6/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= golang.org/x/text v0.3.7/go.mod h1:u+2+/6zg+i71rQMx5EYifcz6MCKuco9NR6JIITiCfzQ= -golang.org/x/text v0.38.0 h1:sXmwo9DwP3OK9EZ7PqAdaooSGozfl/3a6/xJcbzPRhE= -golang.org/x/text v0.38.0/go.mod h1:YXZt3QhHUKYT53r2lLKFIVi6Ao1jdzrTR/KQ09qyxF4= +golang.org/x/text v0.40.0 h1:Ub2Z6/xjgF1WrYQz2nuITOEegKFtiIy+rieRJ5lHZKs= +golang.org/x/text v0.40.0/go.mod h1:hpnzDAfGV753zIKo+wk3u1bVKCGPbrnF7+7LBF/UHVY= golang.org/x/time v0.0.0-20181108054448-85acf8d2951c/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ= golang.org/x/time v0.0.0-20190308202827-9d24e82272b4/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ= golang.org/x/time v0.0.0-20191024005414-555d28b269f0/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ= @@ -556,8 +556,8 @@ golang.org/x/tools v0.0.0-20201208233053-a543418bbed2/go.mod h1:emZCQorbCU4vsT4f golang.org/x/tools v0.0.0-20210105154028-b0ab187a4818/go.mod h1:emZCQorbCU4vsT4fOWvOPXz4eW1wZW4PmDk9uLelYpA= golang.org/x/tools v0.0.0-20210108195828-e2f9c7f1fc8e/go.mod h1:emZCQorbCU4vsT4fOWvOPXz4eW1wZW4PmDk9uLelYpA= golang.org/x/tools v0.1.0/go.mod h1:xkSsbof2nBLbhDlRMhhhyNLN/zl3eTqcnHD5viDpcZ0= -golang.org/x/tools v0.45.0 h1:18qN3FAooORvApf5XjCXgsuayZOEtXf6JK18I3+ONa8= -golang.org/x/tools v0.45.0/go.mod h1:LuUGqqaXcXMEFEruIVJVm5mgDD8vww/z/SR1gQ4uE/0= +golang.org/x/tools v0.47.0 h1:7Kn5x/d1svx/PzryTsqeoZN4TZwqeH5pGWjefhLi/1Q= +golang.org/x/tools v0.47.0/go.mod h1:dFHnyTvFWY212G+h7ZY4Vsp/K3U4/7W9TyVaAul8uCA= golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= From 959f49dae3c3b04c8d7595a5945d7bc39fad09b5 Mon Sep 17 00:00:00 2001 From: Ginder Singh Date: Thu, 2 Jul 2026 15:10:57 -0400 Subject: [PATCH 11/11] Add mobile sandbox optimizations for v1.5.3 - Skip systemd-resolved initialization on Android (ChromeOS crash fix) - Skip system DNS discovery commands on iOS mobile (sandbox restrictions) - Skip route-based DNS discovery on Android - Skip systemd resolver on Android - Add mobile platform checks to prevent sandbox access violations These changes ensure ctrld works correctly in mobile sandboxed environments where system commands and file access are restricted. --- cmd/cli/prog_linux.go | 3 +++ nameservers_darwin.go | 22 ++++++++++++++++++++++ nameservers_linux.go | 16 ++++++++++++++++ 3 files changed, 41 insertions(+) diff --git a/cmd/cli/prog_linux.go b/cmd/cli/prog_linux.go index 2e5c7c7..f566441 100644 --- a/cmd/cli/prog_linux.go +++ b/cmd/cli/prog_linux.go @@ -14,6 +14,9 @@ import ( ) func init() { + if isAndroid() { + return + } if r, err := newLoopbackOSConfigurator(); err == nil { useSystemdResolved = r.Mode() == "systemd-resolved" } diff --git a/nameservers_darwin.go b/nameservers_darwin.go index 1bf4574..ed97cb4 100644 --- a/nameservers_darwin.go +++ b/nameservers_darwin.go @@ -11,6 +11,7 @@ import ( "net" "os/exec" "regexp" + "runtime" "slices" "strings" "time" @@ -25,6 +26,12 @@ func dnsFns() []dnsFn { func getDNSFromScutil() []string { logger := *ProxyLogger.Load() + // Skip scutil on mobile platforms - not available in sandbox + if isMobile() { + Log(context.Background(), logger.Debug(), "skipping scutil DNS discovery on mobile platform") + return nil + } + const ( maxRetries = 10 retryInterval = 100 * time.Millisecond @@ -89,6 +96,11 @@ func getDNSFromScutil() []string { } func getDHCPNameservers(iface string) ([]string, error) { + // Skip ipconfig on mobile platforms - not available in sandbox + if isMobile() { + return nil, fmt.Errorf("ipconfig not available on mobile") + } + // Run the ipconfig command for the given interface. cmd := exec.Command("ipconfig", "getpacket", iface) output, err := cmd.Output() @@ -201,6 +213,11 @@ func getAllDHCPNameservers() []string { } func patchNetIfaceName(iface *net.Interface) (bool, error) { + // Skip networksetup on mobile platforms - not available in sandbox + if isMobile() { + return false, nil + } + b, err := exec.Command("networksetup", "-listnetworkserviceorder").Output() if err != nil { return false, err @@ -234,3 +251,8 @@ func networkServiceName(ifaceName string, r io.Reader) string { } return "" } + +// isMobile reports whether the current OS is a mobile platform. +func isMobile() bool { + return runtime.GOOS == "ios" +} diff --git a/nameservers_linux.go b/nameservers_linux.go index 37a9ed2..3534897 100644 --- a/nameservers_linux.go +++ b/nameservers_linux.go @@ -7,6 +7,7 @@ import ( "net" "net/netip" "os" + "runtime" "strings" "tailscale.com/net/netmon" @@ -24,6 +25,11 @@ func dnsFns() []dnsFn { } func dns4() []string { + // Skip route-based DNS discovery on Android + if runtime.GOOS == "android" { + return nil + } + f, err := os.Open(v4RouteFile) if err != nil { return nil @@ -64,6 +70,11 @@ func dns4() []string { } func dns6() []string { + // Skip route-based DNS discovery on Android + if runtime.GOOS == "android" { + return nil + } + f, err := os.Open(v6RouteFile) if err != nil { return nil @@ -98,6 +109,11 @@ func dns6() []string { } func dnsFromSystemdResolver() []string { + // Skip systemd resolver on Android + if runtime.GOOS == "android" { + return nil + } + c, err := resolvconffile.ParseFile("/run/systemd/resolve/resolv.conf") if err != nil { return nil