diff --git a/cmd/cli/cli.go b/cmd/cli/cli.go index 0ee8036..de78e8e 100644 --- a/cmd/cli/cli.go +++ b/cmd/cli/cli.go @@ -1287,7 +1287,7 @@ func tryUpdateListenerConfigIntercept(cfg *ctrld.Config, notifyFunc func(), fata return false, true } - hasExplicitConfig := lc.IP != "" && lc.IP != "0.0.0.0" && lc.Port != 0 + hasExplicitConfig := isExplicitInterceptListener(lc.IP, lc.Port) if !hasExplicitConfig { // Set defaults for intercept mode if lc.IP == "" || lc.IP == "0.0.0.0" { @@ -1345,6 +1345,16 @@ func tryUpdateListenerConfigIntercept(cfg *ctrld.Config, notifyFunc func(), fata return updated, false } +func isExplicitInterceptListener(ip string, port int) bool { + if ip == "" || ip == "0.0.0.0" || port == 0 { + return false + } + // 127.0.0.1:53 is the default macOS DNS-intercept listener. It can appear + // in generated/custom Control D configs, but it should still be allowed to + // fall back to 127.0.0.1:5354 when mDNSResponder already owns port 53. + return !(ip == "127.0.0.1" && port == 53) +} + // tryUpdateListenerConfig tries updating listener config with a working one. // If fatal is true, and there's listen address conflicted, the function do // fatal error. diff --git a/cmd/cli/cli_intercept_listener_test.go b/cmd/cli/cli_intercept_listener_test.go new file mode 100644 index 0000000..6787d1e --- /dev/null +++ b/cmd/cli/cli_intercept_listener_test.go @@ -0,0 +1,116 @@ +package cli + +import ( + "testing" + + "github.com/Control-D-Inc/ctrld" +) + +func TestIsExplicitInterceptListener(t *testing.T) { + tests := []struct { + name string + ip string + port int + want bool + }{ + {name: "empty", ip: "", port: 0, want: false}, + {name: "wildcard", ip: "0.0.0.0", port: 53, want: false}, + {name: "zero port", ip: "127.0.0.1", port: 0, want: false}, + {name: "default intercept listener", ip: "127.0.0.1", port: 53, want: false}, + {name: "fallback port explicit", ip: "127.0.0.1", port: 5354, want: true}, + {name: "custom loopback explicit", ip: "127.0.0.2", port: 53, want: true}, + {name: "custom address explicit", ip: "192.0.2.10", port: 53, want: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := isExplicitInterceptListener(tt.ip, tt.port); got != tt.want { + t.Fatalf("isExplicitInterceptListener(%q, %d) = %v, want %v", tt.ip, tt.port, got, tt.want) + } + }) + } +} + +// 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 200a125..a13ab1e 100644 --- a/cmd/cli/prog.go +++ b/cmd/cli/prog.go @@ -326,6 +326,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() p.Notice().Msg("Reloading config successfully") @@ -338,6 +350,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()