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/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/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/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/dns_proxy.go b/cmd/cli/dns_proxy.go index 4c08083..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 { @@ -1299,7 +1337,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 +1376,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 +1400,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..5a82a1a 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 { @@ -474,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) + } + }) + } +} 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) + } +} 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 5da2490..6272934 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,8 +360,41 @@ 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" { + if iface == autoIface { iface = defaultIfaceName() p.requiredMultiNICsConfig = requiredMultiNICsConfig() } 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/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") + } + }) +} 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/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") } 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) + } +} 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= 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/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 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 f030739..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) @@ -383,6 +470,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 +487,6 @@ func countHandler(call *atomic.Int64) dns.HandlerFunc { m.IsEdns0().Option = append(m.IsEdns0().Option, cookieOption) } w.WriteMsg(m) - call.Add(1) } }