From 9399f4590b91003489153a6de203a0edb3174b7d Mon Sep 17 00:00:00 2001 From: Dev Scribe Date: Fri, 12 Jun 2026 07:50:20 -0400 Subject: [PATCH] fix: refresh macOS VPN DNS after pf stabilization --- cmd/cli/dns_intercept_darwin.go | 13 +++++ cmd/cli/dns_intercept_settle.go | 55 ++++++++++++++++++ cmd/cli/dns_intercept_settle_test.go | 49 ++++++++++++++++ cmd/cli/dns_intercept_windows.go | 3 - cmd/cli/vpn_dns.go | 85 ++++++++++++++++++++++++++-- cmd/cli/vpn_dns_test.go | 25 ++++++++ 6 files changed, 221 insertions(+), 9 deletions(-) create mode 100644 cmd/cli/dns_intercept_settle.go create mode 100644 cmd/cli/dns_intercept_settle_test.go diff --git a/cmd/cli/dns_intercept_darwin.go b/cmd/cli/dns_intercept_darwin.go index 586ea42..8f96f38 100644 --- a/cmd/cli/dns_intercept_darwin.go +++ b/cmd/cli/dns_intercept_darwin.go @@ -1221,6 +1221,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 } @@ -1382,6 +1386,15 @@ func (p *prog) ensurePFAnchorActive() bool { return true } +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. 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 708ee30..6e0e7d4 100644 --- a/cmd/cli/dns_intercept_windows.go +++ b/cmd/cli/dns_intercept_windows.go @@ -1164,8 +1164,6 @@ func (p *prog) cleanupWFPFilters(state *wfpState) { // installs WFP block filters via block-outside-dns. The hard permit (with // FWPM_FILTER_FLAG_CLEAR_ACTION_RIGHT) in a max-weight sublayer overrides the // third-party blocks without affecting their protection for non-loopback DNS. -// -// See: https://gitlab.int.windscribe.com/controld/clients/ctrld/-/issues/526 func (p *prog) activateLoopbackWFPProtect(state *wfpState) error { state.mu.Lock() defer state.mu.Unlock() @@ -1861,7 +1859,6 @@ func (p *prog) nrptProbeAndHeal() { // installed WFP filters that block DNS to non-tunnel interfaces, including // loopback. A high-priority "hard permit" for localhost DNS overrides these // blocks and restores NRPT routing to ctrld's listener. - // See: https://gitlab.int.windscribe.com/controld/clients/ctrld/-/issues/526 loopbackState, ok := p.dnsInterceptState.(*wfpState) if !ok || loopbackState == nil { mainLog.Load().Error().Msg("DNS intercept: no state available for loopback WFP protect") diff --git a/cmd/cli/vpn_dns.go b/cmd/cli/vpn_dns.go index 50d81bd..93569e4 100644 --- a/cmd/cli/vpn_dns.go +++ b/cmd/cli/vpn_dns.go @@ -102,6 +102,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() @@ -190,14 +192,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 32f6c24..86d4602 100644 --- a/cmd/cli/vpn_dns_test.go +++ b/cmd/cli/vpn_dns_test.go @@ -69,6 +69,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)