diff --git a/cmd/cli/dns_intercept_darwin.go b/cmd/cli/dns_intercept_darwin.go index 7c1cf3a..36836cc 100644 --- a/cmd/cli/dns_intercept_darwin.go +++ b/cmd/cli/dns_intercept_darwin.go @@ -1233,6 +1233,10 @@ func (p *prog) pfStabilizationLoop(ctx context.Context, stableRequired time.Dura p.pfStabilizing.Store(false) mainLog.Load().Info().Msgf("DNS intercept: pf stable for %s — restoring anchor rules", stableRequired) p.ensurePFAnchorActive() + routes, domainlessServers, exemptions := p.refreshDNSAfterVPNSettle("pf_stabilized") + if routes == 0 && domainlessServers == 0 && exemptions == 0 { + p.scheduleDNSAfterVPNSettleRefresh("pf_stabilized_followup", pfAnchorRecheckDelayLong) + } p.pfLastRestoreTime.Store(time.Now().UnixMilli()) return } @@ -1459,6 +1463,15 @@ func isResourceExhaustion(err error, output []byte) bool { strings.Contains(msg, "cannot allocate memory") } +func (p *prog) scheduleDNSAfterVPNSettleRefresh(reason string, delay time.Duration) { + time.AfterFunc(delay, func() { + if p.dnsInterceptState == nil { + return + } + p.refreshDNSAfterVPNSettle(reason) + }) +} + // pfWatchdog periodically checks that our pf anchor is still active. // Other programs (e.g., Windscribe desktop app, macOS configd) can replace // scheduleDelayedRechecks schedules delayed re-checks after a network change event. @@ -1826,7 +1839,14 @@ func (p *prog) forceReloadPFMainRuleset() { mainLog.Load().Error().Err(err).Msgf("DNS intercept: force reload — failed to load anchor (output: %s)", strings.TrimSpace(string(out))) } - // Reset upstream transports — pf reload flushes state table, killing DoH connections. + // Flush stale rdr/reply states after the forced ruleset + anchor reload. + // Without this, macOS can keep using pre-reload state and try to send + // redirected DNS replies directly from loopback to tunnel client addresses + // (for example, 127.0.0.1: -> 100.64.0.0/10), which fails with + // "sendmsg: can't assign requested address". + flushPFStates() + + // Reset upstream transports — pf reload/state flush kills existing DoH connections. p.resetUpstreamTransports() mainLog.Load().Info().Msg("DNS intercept: force reload — pf ruleset and anchor reloaded successfully") diff --git a/cmd/cli/dns_intercept_settle.go b/cmd/cli/dns_intercept_settle.go new file mode 100644 index 0000000..6d6ffaf --- /dev/null +++ b/cmd/cli/dns_intercept_settle.go @@ -0,0 +1,55 @@ +package cli + +import ( + "context" + + "github.com/Control-D-Inc/ctrld" +) + +var initializeOsResolver = ctrld.InitializeOsResolver + +func (p *prog) refreshDNSAfterVPNSettle(reason string) (routes, domainlessServers, exemptions int) { + mainLog.Load().Info().Msgf("DNS intercept: refreshing OS/VPN DNS route state after VPN settle (%s)", reason) + ctx := ctrld.LoggerCtx(context.Background(), mainLog.Load()) + ns := initializeOsResolver(ctx, true) + mainLog.Load().Debug().Msgf("DNS intercept: post-settle OS resolver nameservers: %v", ns) + + if p.vpnDNS == nil { + mainLog.Load().Debug().Msg("DNS intercept: post-settle VPN DNS route refresh skipped — manager unavailable") + return 0, 0, 0 + } + + beforeExemptions := p.vpnDNS.CurrentExemptions() + routes, domainlessServers, exemptions = p.vpnDNS.RefreshRoutesOnly() + afterExemptions := p.vpnDNS.CurrentExemptions() + + if vpnDNSExemptionsEqual(beforeExemptions, afterExemptions) { + mainLog.Load().Info().Msgf("DNS intercept: post-settle VPN DNS route refresh completed — %d routes, %d domainless servers, %d exemptions (pf unchanged)", + routes, domainlessServers, exemptions) + return routes, domainlessServers, exemptions + } + + if err := p.exemptVPNDNSServers(afterExemptions); err != nil { + mainLog.Load().Warn().Err(err).Msg("DNS intercept: post-settle VPN DNS exemption update failed") + } else { + mainLog.Load().Info().Msgf("DNS intercept: post-settle VPN DNS exemptions changed — updated pf/WFP with %d exemptions", len(afterExemptions)) + } + return routes, domainlessServers, exemptions +} + +func vpnDNSExemptionsEqual(a, b []vpnDNSExemption) bool { + if len(a) != len(b) { + return false + } + seen := make(map[vpnDNSExemption]int, len(a)) + for _, ex := range a { + seen[ex]++ + } + for _, ex := range b { + if seen[ex] == 0 { + return false + } + seen[ex]-- + } + return true +} diff --git a/cmd/cli/dns_intercept_settle_test.go b/cmd/cli/dns_intercept_settle_test.go new file mode 100644 index 0000000..b6c80be --- /dev/null +++ b/cmd/cli/dns_intercept_settle_test.go @@ -0,0 +1,49 @@ +package cli + +import ( + "context" + "testing" + + "github.com/Control-D-Inc/ctrld" +) + +func TestRefreshDNSAfterVPNSettleRefreshesOSResolverAndVPNRoutes(t *testing.T) { + oldInitialize := initializeOsResolver + defer func() { initializeOsResolver = oldInitialize }() + + var initialized []bool + initializeOsResolver = func(ctx context.Context, force bool) []string { + initialized = append(initialized, force) + return []string{"10.102.26.10:53"} + } + + var exemptionUpdates [][]vpnDNSExemption + p := &prog{} + p.vpnDNS = newVPNDNSManager(&mainLog, func(exemptions []vpnDNSExemption) error { + exemptionUpdates = append(exemptionUpdates, append([]vpnDNSExemption{}, exemptions...)) + return nil + }) + p.vpnDNS.discoverVPNDNS = func(context.Context) []ctrld.VPNDNSConfig { + return []ctrld.VPNDNSConfig{{ + InterfaceName: "utun4", + Servers: []string{"10.102.26.10"}, + Domains: []string{"bmwgroup.net"}, + }} + } + + routes, domainlessServers, exemptions := p.refreshDNSAfterVPNSettle("test") + + if routes != 1 || domainlessServers != 0 || exemptions != 1 { + t.Fatalf("expected 1 route, 0 domainless servers, 1 exemption, got routes=%d domainless=%d exemptions=%d", + routes, domainlessServers, exemptions) + } + if len(initialized) != 1 || !initialized[0] { + t.Fatalf("expected forced OS resolver refresh once, got %v", initialized) + } + if got := p.vpnDNS.UpstreamForDomain("jira.cc.bmwgroup.net."); len(got) != 1 || got[0] != "10.102.26.10" { + t.Fatalf("expected refreshed VPN DNS route, got %v", got) + } + if len(exemptionUpdates) != 0 { + t.Fatalf("expected route-only refresh to avoid pf exemption updates, got %+v", exemptionUpdates) + } +} diff --git a/cmd/cli/dns_intercept_windows.go b/cmd/cli/dns_intercept_windows.go index 91b368d..b35d3fd 100644 --- a/cmd/cli/dns_intercept_windows.go +++ b/cmd/cli/dns_intercept_windows.go @@ -683,8 +683,11 @@ func (p *prog) startDNSIntercept() error { // server, ctrld may have fallen back to 127.0.0.x:53 instead of 127.0.0.1:53. // NRPT must point to whichever address ctrld is actually listening on. listenerIP := "127.0.0.1" - if lc := p.cfg.FirstListener(); lc != nil && lc.IP != "" { + if lc := p.cfg.FirstListener(); lc != nil && lc.IP != "" && lc.IP != "0.0.0.0" && lc.IP != "::" { listenerIP = lc.IP + } else if lc != nil && (lc.IP == "0.0.0.0" || lc.IP == "::") { + mainLog.Load().Warn().Str("configured_ip", lc.IP). + Msg("DNS intercept: listener configured with wildcard IP, using 127.0.0.1 for NRPT rules") } state := &wfpState{ diff --git a/cmd/cli/main_test.go b/cmd/cli/main_test.go index d0a1149..55de3b0 100644 --- a/cmd/cli/main_test.go +++ b/cmd/cli/main_test.go @@ -2,6 +2,7 @@ package cli import ( "os" + "os/exec" "strings" "testing" @@ -28,5 +29,20 @@ func TestMain(m *testing.M) { l := zap.New(core) mainLog.Store(&ctrld.Logger{Logger: l}) + + // Stub the self-upgrade command builder for the whole test binary. The real + // builder execs os.Executable() — which under `go test` IS this test binary + // — with positional args ("upgrade", ...). `go test` stops flag parsing at + // the first positional arg and ignores the rest, so the child just re-runs + // the entire suite, hits the upgrade tests again, and spawns more children: + // a fork bomb of detached processes that stalls the host and (on Windows) + // holds the test binary's image locked, breaking CI artifact cleanup. + // Point it at the test binary with a no-match -test.run so any test that + // reaches performUpgrade still exercises the cmd.Start() success path while + // the child exits immediately without recursing. + newUpgradeCmd = func(exe string) *exec.Cmd { + return exec.Command(exe, "-test.run=^$") + } + os.Exit(m.Run()) } diff --git a/cmd/cli/prog.go b/cmd/cli/prog.go index dcad439..6ca5a51 100644 --- a/cmd/cli/prog.go +++ b/cmd/cli/prog.go @@ -1659,6 +1659,19 @@ func shouldUpgrade(vt string, cv *semver.Version, logger *ctrld.Logger) bool { return true } +// newUpgradeCmd builds the detached command used to self-upgrade. It is a +// package-level variable so tests can stub it. With the real implementation a +// *test* binary would re-exec itself — os.Executable() is the test binary, and +// because `go test` stops flag parsing at the first positional arg ("upgrade") +// it ignores the args and re-runs the entire suite. That child hits the same +// upgrade test and spawns another child, recursively: a fork bomb of detached +// processes that pins the host and locks the test binary's image file. +var newUpgradeCmd = func(exe string) *exec.Cmd { + cmd := exec.Command(exe, "upgrade", "prod", "-vv") + cmd.SysProcAttr = sysProcAttrForDetachedChildProcess() + return cmd +} + // performUpgrade executes the self-upgrade command. // Returns true if upgrade was initiated successfully, false otherwise. func performUpgrade(vt string, logger *ctrld.Logger) bool { @@ -1667,8 +1680,7 @@ func performUpgrade(vt string, logger *ctrld.Logger) bool { logger.Error().Err(err).Msg("Failed to get executable path, skipped self-upgrade") return false } - cmd := exec.Command(exe, "upgrade", "prod", "-vv") - cmd.SysProcAttr = sysProcAttrForDetachedChildProcess() + cmd := newUpgradeCmd(exe) if err := cmd.Start(); err != nil { logger.Error().Err(err).Msg("Failed to start self-upgrade") return false diff --git a/cmd/cli/prog_test.go b/cmd/cli/prog_test.go index 2787622..bb56f76 100644 --- a/cmd/cli/prog_test.go +++ b/cmd/cli/prog_test.go @@ -283,6 +283,8 @@ func Test_performUpgrade(t *testing.T) { }, } + // newUpgradeCmd is stubbed in TestMain so performUpgrade does not re-exec + // (and fork-bomb) the test binary; see the comment there. for _, tc := range tests { tc := tc t.Run(tc.name, func(t *testing.T) { diff --git a/cmd/cli/vpn_dns.go b/cmd/cli/vpn_dns.go index b3c20da..0143fa4 100644 --- a/cmd/cli/vpn_dns.go +++ b/cmd/cli/vpn_dns.go @@ -110,6 +110,8 @@ func (m *vpnDNSManager) Refresh(ctx context.Context, guardAgainstNoNameservers . m.mu.Lock() defer m.mu.Unlock() + previousExemptions := m.currentExemptionsLocked() + if vpnDNSSettlingEnabled && len(configs) == 0 && guardedRefresh && m.hasVPNDNSStateLocked() { if !m.retainedAfterEmptyDiscovery { exemptions := m.currentExemptionsLocked() @@ -198,14 +200,85 @@ func (m *vpnDNSManager) Refresh(ctx context.Context, guardAgainstNoNameservers . ctrld.Log(ctx, logger.Debug(), "VPN DNS refresh completed: %d configs, %d routes, %d domainless servers, %d unique exemptions", len(m.configs), len(m.routes), len(m.domainlessServers), len(exemptions)) - // Update intercept rules to permit VPN DNS traffic. - // Always call onServersChanged — including when exemptions is empty — so that - // stale exemptions from a previous VPN session get cleared on disconnect. - if m.onServersChanged != nil { - if err := m.onServersChanged(exemptions); err != nil { - ctrld.Log(ctx, logger.Error().Err(err), "Failed to update intercept exemptions for VPN DNS servers") + // Update intercept rules to permit VPN DNS traffic only when the exemption set + // actually changes. Network-change events can fire repeatedly while macOS/VPN + // state is otherwise identical; rewriting pf for identical exemptions can feed + // a self-triggering network-change loop. Empty exemptions are still applied + // when they differ from the previous set, so stale VPN exemptions are cleared + // on disconnect. + m.updateInterceptExemptionsIfChanged(ctx, logger, previousExemptions, exemptions, "VPN DNS") +} + +func (m *vpnDNSManager) updateInterceptExemptionsIfChanged(ctx context.Context, logger *ctrld.Logger, before, after []vpnDNSExemption, reason string) { + if m.onServersChanged == nil { + return + } + if vpnDNSExemptionsEqual(before, after) { + ctrld.Log(ctx, logger.Debug(), "VPN DNS exemptions unchanged after %s refresh; skipping intercept rule update", reason) + return + } + if err := m.onServersChanged(after); err != nil { + ctrld.Log(ctx, logger.Error().Err(err), "Failed to update intercept exemptions for VPN DNS servers") + } +} + +// RefreshRoutesOnly re-discovers VPN DNS configs and updates only ctrld's +// in-memory split-DNS routes. It intentionally does not call onServersChanged, +// so it does not rewrite/reload pf/WFP rules. Use this for post-settle discovery +// checks where we only need to learn late-published VPN search domains. +func (m *vpnDNSManager) RefreshRoutesOnly() (routes, domainlessServers, exemptions int) { + logger := mainLog.Load() + + logger.Debug().Msg("Refreshing VPN DNS route state only") + discoverVPNDNS := m.discoverVPNDNS + if discoverVPNDNS == nil { + discoverVPNDNS = ctrld.DiscoverVPNDNS + } + configs := discoverVPNDNS(context.Background()) + + if dri, err := netmon.DefaultRouteInterface(); err == nil && dri != "" { + for i := range configs { + if configs[i].InterfaceName == dri { + configs[i].IsExitMode = true + } } } + + m.mu.Lock() + defer m.mu.Unlock() + + m.retainedAfterEmptyDiscovery = false + m.configs = configs + m.routes = make(map[string][]string) + + for _, config := range configs { + for _, domain := range config.Domains { + domain = strings.TrimPrefix(domain, "~") + domain = strings.TrimPrefix(domain, ".") + domain = strings.ToLower(domain) + if domain != "" { + m.routes[domain] = append([]string{}, config.Servers...) + } + } + } + + var domainless []string + seenDomainless := make(map[string]bool) + for _, config := range configs { + if len(config.Domains) == 0 && len(config.Servers) > 0 { + for _, server := range config.Servers { + if !seenDomainless[server] { + seenDomainless[server] = true + domainless = append(domainless, server) + } + } + } + } + m.domainlessServers = domainless + + logger.Debug().Msgf("VPN DNS route-only refresh completed: %d configs, %d routes, %d domainless servers, %d exemptions", + len(m.configs), len(m.routes), len(m.domainlessServers), len(m.currentExemptionsLocked())) + return len(m.routes), len(m.domainlessServers), len(m.currentExemptionsLocked()) } func (m *vpnDNSManager) hasVPNDNSStateLocked() bool { diff --git a/cmd/cli/vpn_dns_test.go b/cmd/cli/vpn_dns_test.go index d215cd0..702b340 100644 --- a/cmd/cli/vpn_dns_test.go +++ b/cmd/cli/vpn_dns_test.go @@ -101,6 +101,31 @@ func TestVPNDNSRefreshClearsOnSecondGuardedEmptyDiscovery(t *testing.T) { } } +func TestVPNDNSRefreshSkipsUnchangedInterceptExemptions(t *testing.T) { + var updates [][]vpnDNSExemption + m := newVPNDNSManager(&mainLog, func(exemptions []vpnDNSExemption) error { + updates = append(updates, append([]vpnDNSExemption{}, exemptions...)) + return nil + }) + m.discoverVPNDNS = func(context.Context) []ctrld.VPNDNSConfig { + return []ctrld.VPNDNSConfig{{ + InterfaceName: "utun-test", + Servers: []string{"10.102.26.10"}, + Domains: []string{"example.internal"}, + }} + } + + m.Refresh(context.Background(), true) + m.Refresh(context.Background(), true) + + if len(updates) != 1 { + t.Fatalf("expected exactly one intercept exemption update for unchanged VPN DNS state, got %d", len(updates)) + } + if len(updates[0]) != 1 || updates[0][0].Server != "10.102.26.10" || updates[0][0].Interface != "utun-test" { + t.Fatalf("unexpected exemption update: %+v", updates[0]) + } +} + func TestVPNDNSTransportFailureSuppressesFallbackOnlyWhileRetainingState(t *testing.T) { withVPNDNSSettlingEnabled(t) m := newVPNDNSManager(&mainLog, nil)