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()