From 4395efcb22eb9129c80f3b6b0f39ef322d5f938f Mon Sep 17 00:00:00 2001 From: Codescribe Date: Mon, 25 May 2026 06:08:46 -0400 Subject: [PATCH 1/2] fix: stabilize Windows VPN DNS during adapter settling Fixes Windows DNS-intercept behavior for AD/internal split-rule domains during sleep/wake or VPN adapter settling without relying on a fixed timeout. --- cmd/cli/dns_proxy.go | 38 +++++++++++++- cmd/cli/vpn_dns.go | 107 +++++++++++++++++++++++++++++++++------- cmd/cli/vpn_dns_test.go | 87 ++++++++++++++++++++++++++++++++ nameservers_windows.go | 61 +++++++++++++++++++---- 4 files changed, 265 insertions(+), 28 deletions(-) create mode 100644 cmd/cli/vpn_dns_test.go diff --git a/cmd/cli/dns_proxy.go b/cmd/cli/dns_proxy.go index d3ddb04d..fe37f609 100644 --- a/cmd/cli/dns_proxy.go +++ b/cmd/cli/dns_proxy.go @@ -548,6 +548,7 @@ func (p *prog) proxy(ctx context.Context, req *proxyRequest) *proxyResponse { if vpnServers := p.vpnDNS.UpstreamForDomain(domain); len(vpnServers) > 0 { ctrld.Log(ctx, mainLog.Load().Debug(), "VPN DNS route matched for domain %s, using servers: %v", domain, vpnServers) + var gotTransportFailure bool for _, server := range vpnServers { upstreamConfig := p.vpnDNS.upstreamConfigFor(server) ctrld.Log(ctx, mainLog.Load().Debug(), "Querying VPN DNS server: %s", server) @@ -561,6 +562,7 @@ func (p *prog) proxy(ctx context.Context, req *proxyRequest) *proxyResponse { answer, err := dnsResolver.Resolve(resolveCtx, req.msg) cancel() if answer != nil { + p.vpnDNS.VPNDNSReachable() ctrld.Log(ctx, mainLog.Load().Debug(), "VPN DNS query successful") if p.cache != nil { ttl := 60 * time.Second @@ -573,9 +575,22 @@ func (p *prog) proxy(ctx context.Context, req *proxyRequest) *proxyResponse { } return &proxyResponse{answer: answer} } + gotTransportFailure = true ctrld.Log(ctx, mainLog.Load().Debug().Err(err), "VPN DNS server %s failed", server) } + // Explicit VPN DNS routes are authoritative for their suffix. If all + // routed servers fail at the transport layer while Windows is serving + // retained VPN DNS state, fail closed instead of leaking VPN/internal + // names to normal upstreams. + if gotTransportFailure && p.vpnDNS.ShouldFailClosedAfterVPNDNSTransportFailure(domain, vpnServers) { + ctrld.Log(ctx, mainLog.Load().Debug(), + "All VPN DNS servers had transport failures for %s; returning SERVFAIL while retained VPN DNS state is active", domain) + answer := new(dns.Msg) + answer.SetRcode(req.msg, dns.RcodeServerFailure) + return &proxyResponse{answer: answer} + } + ctrld.Log(ctx, mainLog.Load().Debug(), "All VPN DNS servers failed, falling back to normal upstreams") } } @@ -589,13 +604,15 @@ func (p *prog) proxy(ctx context.Context, req *proxyRequest) *proxyResponse { // polluting captive portal / DHCP flows. if dnsIntercept && p.vpnDNS != nil && req.ufr.matched && len(upstreams) > 0 && upstreams[0] == upstreamOS && - len(req.msg.Question) > 0 && !p.isAdDomainQuery(req.msg) { + len(req.msg.Question) > 0 { if dlServers := p.vpnDNS.DomainlessServers(); len(dlServers) > 0 { domain := req.msg.Question[0].Name ctrld.Log(ctx, mainLog.Load().Debug(), "Split-rule query %s going to upstream.os, trying %d domain-less VPN DNS servers first: %v", domain, len(dlServers), dlServers) + var gotDNSAnswer bool + var gotTransportFailure bool for _, server := range dlServers { upstreamCfg := p.vpnDNS.upstreamConfigFor(server) ctrld.Log(ctx, mainLog.Load().Debug(), "Querying domain-less VPN DNS server: %s", server) @@ -608,6 +625,10 @@ func (p *prog) proxy(ctx context.Context, req *proxyRequest) *proxyResponse { resolveCtx, cancel := upstreamCfg.Context(ctx) answer, err := dnsResolver.Resolve(resolveCtx, req.msg) cancel() + if answer != nil { + gotDNSAnswer = true + p.vpnDNS.VPNDNSReachable() + } if answer != nil && answer.Rcode == dns.RcodeSuccess { ctrld.Log(ctx, mainLog.Load().Debug(), "Domain-less VPN DNS server %s answered %s successfully", server, domain) @@ -618,10 +639,25 @@ func (p *prog) proxy(ctx context.Context, req *proxyRequest) *proxyResponse { "Domain-less VPN DNS server %s returned %s for %s, trying next", server, dns.RcodeToString[answer.Rcode], domain) } else { + gotTransportFailure = true ctrld.Log(ctx, mainLog.Load().Debug().Err(err), "Domain-less VPN DNS server %s failed for %s", server, domain) } } + + // If every domainless VPN DNS attempt failed before receiving a DNS + // packet while Windows is serving retained VPN DNS state, fail closed + // instead of asking LAN/public DNS about internal split-rule names and + // caching false negatives. Reachable negative DNS responses still fall + // through to the old OS fallback behavior below. + if !gotDNSAnswer && gotTransportFailure && p.vpnDNS.ShouldFailClosedAfterVPNDNSTransportFailure(domain, dlServers) { + ctrld.Log(ctx, mainLog.Load().Debug(), + "All domain-less VPN DNS servers had transport failures for %s; returning SERVFAIL while retained VPN DNS state is active", domain) + answer := new(dns.Msg) + answer.SetRcode(req.msg, dns.RcodeServerFailure) + return &proxyResponse{answer: answer} + } + ctrld.Log(ctx, mainLog.Load().Debug(), "All domain-less VPN DNS servers failed for %s, falling back to OS resolver", domain) } diff --git a/cmd/cli/vpn_dns.go b/cmd/cli/vpn_dns.go index bc4b1a13..6b94e0ab 100644 --- a/cmd/cli/vpn_dns.go +++ b/cmd/cli/vpn_dns.go @@ -3,6 +3,7 @@ package cli import ( "context" "net" + "runtime" "strings" "sync" @@ -11,6 +12,8 @@ import ( "github.com/Control-D-Inc/ctrld" ) +var vpnDNSSettlingEnabled = runtime.GOOS == "windows" + // vpnDNSExemption represents a VPN DNS server that needs pf/WFP exemption, // including the interface it was discovered on. The interface is used on macOS // to create interface-scoped pf exemptions that allow the VPN's local DNS @@ -38,6 +41,11 @@ type vpnDNSManager struct { // as additional nameservers for queries that match split-DNS rules // (from ctrld config, AD domain, or VPN suffix config). domainlessServers []string + // retainedAfterEmptyDiscovery means Windows reported an empty VPN DNS + // snapshot once while previous VPN DNS state existed. We keep that last-known + // state for one guarded refresh cycle because Windows can briefly report an + // intermediate empty adapter/DNS state after sleep/wake or reconnect. + retainedAfterEmptyDiscovery bool // Called when VPN DNS server list changes, to update intercept exemptions. onServersChanged vpnDNSExemptFunc } @@ -78,6 +86,29 @@ func (m *vpnDNSManager) Refresh(guardAgainstNoNameservers bool) { m.mu.Lock() defer m.mu.Unlock() + if vpnDNSSettlingEnabled && len(configs) == 0 && guardAgainstNoNameservers && m.hasVPNDNSStateLocked() { + if !m.retainedAfterEmptyDiscovery { + exemptions := m.currentExemptionsLocked() + m.retainedAfterEmptyDiscovery = true + logger.Debug().Msgf( + "VPN DNS discovery empty; retaining last-known VPN DNS state for one guarded refresh (%d domainless servers, %d exemptions)", + len(m.domainlessServers), len(exemptions)) + if m.onServersChanged != nil { + if err := m.onServersChanged(exemptions); err != nil { + logger.Error().Err(err).Msg("Failed to re-apply retained VPN DNS exemptions") + } + } + return + } + logger.Debug().Msgf( + "VPN DNS discovery still empty on next guarded refresh; clearing retained VPN DNS state (%d domainless servers)", + len(m.domainlessServers)) + } + + // Any discovery path that does not return with retained state clears the + // settling marker: non-empty discovery replaces old servers immediately, and + // an unguarded/second empty discovery clears stale state below. + m.retainedAfterEmptyDiscovery = false m.configs = configs m.routes = make(map[string][]string) @@ -151,6 +182,63 @@ func (m *vpnDNSManager) Refresh(guardAgainstNoNameservers bool) { } } +func (m *vpnDNSManager) hasVPNDNSStateLocked() bool { + return len(m.configs) > 0 || len(m.routes) > 0 || len(m.domainlessServers) > 0 +} + +func (m *vpnDNSManager) currentExemptionsLocked() []vpnDNSExemption { + type key struct{ server, iface string } + seen := make(map[key]bool) + var exemptions []vpnDNSExemption + for _, config := range m.configs { + for _, server := range config.Servers { + k := key{server, config.InterfaceName} + if seen[k] { + continue + } + seen[k] = true + exemptions = append(exemptions, vpnDNSExemption{ + Server: server, + Interface: config.InterfaceName, + IsExitMode: config.IsExitMode, + }) + } + } + return exemptions +} + +// ShouldFailClosedAfterVPNDNSTransportFailure reports whether split-rule +// queries should fail closed instead of falling back to OS/public DNS after +// every candidate VPN DNS server failed before returning a DNS packet. This is +// Windows-only and only active while serving retained VPN DNS state from a +// guarded empty discovery, which is the short window where Windows can report +// VPN DNS before routes to those servers are usable after wake/reconnect. +func (m *vpnDNSManager) ShouldFailClosedAfterVPNDNSTransportFailure(domain string, servers []string) bool { + m.mu.RLock() + defer m.mu.RUnlock() + + if !vpnDNSSettlingEnabled || len(servers) == 0 || !m.retainedAfterEmptyDiscovery || !m.hasVPNDNSStateLocked() { + return false + } + + mainLog.Load().Debug().Msgf( + "VPN DNS transport failed for %s while retained VPN DNS state is active; suppressing OS fallback for this query (servers=%v)", + domain, servers) + return true +} + +// VPNDNSReachable records that a VPN DNS server returned a DNS response. The +// response may be negative (NXDOMAIN/SERVFAIL); the important signal is that +// the VPN DNS transport is reachable again. +func (m *vpnDNSManager) VPNDNSReachable() { + m.mu.Lock() + defer m.mu.Unlock() + if m.retainedAfterEmptyDiscovery { + mainLog.Load().Debug().Msg("VPN DNS transport recovered; clearing retained-empty-discovery state") + } + m.retainedAfterEmptyDiscovery = false +} + // UpstreamForDomain checks if the domain matches any VPN search domain. // Returns VPN DNS servers if matched, nil otherwise. func (m *vpnDNSManager) UpstreamForDomain(domain string) []string { @@ -208,24 +296,7 @@ func (m *vpnDNSManager) CurrentServers() []string { func (m *vpnDNSManager) CurrentExemptions() []vpnDNSExemption { m.mu.RLock() defer m.mu.RUnlock() - - type key struct{ server, iface string } - seen := make(map[key]bool) - var exemptions []vpnDNSExemption - for _, config := range m.configs { - for _, server := range config.Servers { - k := key{server, config.InterfaceName} - if !seen[k] { - seen[k] = true - exemptions = append(exemptions, vpnDNSExemption{ - Server: server, - Interface: config.InterfaceName, - IsExitMode: config.IsExitMode, - }) - } - } - } - return exemptions + return m.currentExemptionsLocked() } // Routes returns a copy of the current VPN DNS routes for debugging. diff --git a/cmd/cli/vpn_dns_test.go b/cmd/cli/vpn_dns_test.go new file mode 100644 index 00000000..cda57163 --- /dev/null +++ b/cmd/cli/vpn_dns_test.go @@ -0,0 +1,87 @@ +package cli + +import ( + "testing" + + "github.com/Control-D-Inc/ctrld" +) + +func withVPNDNSSettlingEnabled(t *testing.T) { + t.Helper() + old := vpnDNSSettlingEnabled + vpnDNSSettlingEnabled = true + t.Cleanup(func() { vpnDNSSettlingEnabled = old }) +} + +func TestVPNDNSRefreshRetainsStateForOneGuardedEmptyDiscovery(t *testing.T) { + withVPNDNSSettlingEnabled(t) + var gotExemptions []vpnDNSExemption + m := newVPNDNSManager(func(exemptions []vpnDNSExemption) error { + gotExemptions = exemptions + return nil + }) + m.configs = []ctrld.VPNDNSConfig{{ + InterfaceName: "Ethernet 6", + Servers: []string{"10.25.37.21", "10.25.37.22"}, + }} + m.domainlessServers = []string{"10.25.37.21", "10.25.37.22"} + + m.Refresh(true) + + if got := m.DomainlessServers(); len(got) != 2 { + t.Fatalf("expected retained domainless servers, got %v", got) + } + if len(gotExemptions) != 2 { + t.Fatalf("expected retained exemptions to be re-applied, got %v", gotExemptions) + } + if !m.retainedAfterEmptyDiscovery { + t.Fatal("expected empty discovery retention to be marked") + } +} + +func TestVPNDNSRefreshClearsOnSecondGuardedEmptyDiscovery(t *testing.T) { + withVPNDNSSettlingEnabled(t) + var gotExemptions []vpnDNSExemption + m := newVPNDNSManager(func(exemptions []vpnDNSExemption) error { + gotExemptions = exemptions + return nil + }) + m.configs = []ctrld.VPNDNSConfig{{ + InterfaceName: "Ethernet 6", + Servers: []string{"10.25.37.21"}, + }} + m.domainlessServers = []string{"10.25.37.21"} + m.retainedAfterEmptyDiscovery = true + + m.Refresh(true) + + if got := m.DomainlessServers(); len(got) != 0 { + t.Fatalf("expected domainless servers to be cleared on second empty discovery, got %v", got) + } + if len(gotExemptions) != 0 { + t.Fatalf("expected empty exemptions after clearing stale state, got %v", gotExemptions) + } + if m.retainedAfterEmptyDiscovery { + t.Fatal("expected retained empty-discovery marker to be cleared with stale state") + } +} + +func TestVPNDNSTransportFailureSuppressesFallbackOnlyWhileRetainingState(t *testing.T) { + withVPNDNSSettlingEnabled(t) + m := newVPNDNSManager(nil) + m.domainlessServers = []string{"10.25.37.21"} + + if m.ShouldFailClosedAfterVPNDNSTransportFailure("splunk.aws.arena.net.", []string{"10.25.37.21"}) { + t.Fatal("did not expect transport failure to suppress OS fallback outside retained empty-discovery state") + } + + m.retainedAfterEmptyDiscovery = true + if !m.ShouldFailClosedAfterVPNDNSTransportFailure("splunk.aws.arena.net.", []string{"10.25.37.21"}) { + t.Fatal("expected transport failure to suppress OS fallback while retained state is active") + } + + m.VPNDNSReachable() + if m.retainedAfterEmptyDiscovery { + t.Fatal("expected reachable DNS response to clear retained empty-discovery state") + } +} diff --git a/nameservers_windows.go b/nameservers_windows.go index 75fdf8e0..2867aa73 100644 --- a/nameservers_windows.go +++ b/nameservers_windows.go @@ -56,6 +56,8 @@ const ( var ( dcRetryMu sync.Mutex dcRetryCancel context.CancelFunc + dcRetryDomain string + dcRetryID uint64 // Lazy-loaded netapi32 for DsGetDcNameW calls. netapi32DLL = windows.NewLazySystemDLL("netapi32.dll") @@ -133,9 +135,6 @@ func dnsFromAdapter() []string { func getDNSServers(ctx context.Context) ([]string, error) { logger := *ProxyLogger.Load() - // Cancel any in-flight DC retry from a previous network state. - cancelDCRetry() - // Check context before making the call if ctx.Err() != nil { return nil, ctx.Err() @@ -157,6 +156,9 @@ func getDNSServers(ctx context.Context) ([]string, error) { var dcServers []string var adDomain string isDomain := checkDomainJoined() + if !isDomain { + cancelDCRetry() + } if isDomain { domainName, err := system.GetActiveDirectoryDomain() if err != nil { @@ -164,6 +166,7 @@ func getDNSServers(ctx context.Context) ([]string, error) { "Failed to get local AD domain: %v", err) } else { adDomain = domainName + cancelDCRetryForOtherDomain(domainName) // Load netapi32.dll var info *DomainControllerInfo flags := uint32(DS_RETURN_DNS_NAME | DS_IP_REQUIRED | DS_IS_DNS_NAME) @@ -205,7 +208,7 @@ func getDNSServers(ctx context.Context) ([]string, error) { // Start background retry for transient DC errors. if isTransientDCError(ret) { Log(ctx, logger.Info(), - "AD DC detection failed with transient error %d for %s, starting background retry", ret, domainName) + "AD DC detection failed with retryable error %d for %s, ensuring background retry", ret, domainName) startDCRetry(domainName) } } else if info != nil { @@ -219,6 +222,7 @@ func getDNSServers(ctx context.Context) ([]string, error) { if ip := net.ParseIP(dcAddr); ip != nil { dcServers = append(dcServers, ip.String()) + cancelDCRetry() Log(ctx, logger.Debug(), "Added domain controller DNS servers: %v", dcServers) } @@ -387,10 +391,13 @@ func checkDomainJoined() bool { } // isTransientDCError returns true if the DsGetDcName error code indicates -// a transient failure that may succeed on retry. +// a transient failure that may succeed on retry. ERROR_NO_SUCH_DOMAIN is +// retryable here because we only call this path after Windows already reported +// the machine is domain joined and returned a local AD domain name; during VPN +// DNS churn, DC locator can temporarily fail to resolve that known domain. func isTransientDCError(code uintptr) bool { switch code { - case errConnReset, errRPCUnavailable, errNoLogonServers, errDCNotFound, errNetUnreachable: + case errNoSuchDomain, errConnReset, errRPCUnavailable, errNoLogonServers, errDCNotFound, errNetUnreachable: return true default: return false @@ -404,23 +411,49 @@ func cancelDCRetry() { if dcRetryCancel != nil { dcRetryCancel() dcRetryCancel = nil + dcRetryDomain = "" + dcRetryID = 0 + } +} + +// cancelDCRetryForOtherDomain keeps an existing retry alive during noisy +// network-change refreshes, but stops it if Windows reports a different AD +// domain. This avoids the start/cancel storm seen when DsGetDcName briefly +// returns ERROR_NO_SUCH_DOMAIN while VPN DNS is still settling. +func cancelDCRetryForOtherDomain(domainName string) { + dcRetryMu.Lock() + defer dcRetryMu.Unlock() + if dcRetryCancel == nil || dcRetryDomain == "" || strings.EqualFold(dcRetryDomain, domainName) { + return } + dcRetryCancel() + dcRetryCancel = nil + dcRetryDomain = "" + dcRetryID = 0 } // startDCRetry spawns a background goroutine that retries DsGetDcName with // exponential backoff. On success it appends the DC IP to the OS resolver. func startDCRetry(domainName string) { dcRetryMu.Lock() - // Cancel any previous retry. + if dcRetryCancel != nil && strings.EqualFold(dcRetryDomain, domainName) { + ProxyLogger.Load().Debug().Msgf("AD DC retry already running for domain %s", domainName) + dcRetryMu.Unlock() + return + } if dcRetryCancel != nil { dcRetryCancel() } ctx, cancel := context.WithCancel(context.Background()) + dcRetryID++ + retryID := dcRetryID dcRetryCancel = cancel + dcRetryDomain = domainName dcRetryMu.Unlock() - go func() { + go func(retryID uint64) { logger := *ProxyLogger.Load() + defer clearDCRetryIfCurrent(domainName, retryID) delay := dcRetryInitialDelay for attempt := 1; attempt <= dcRetryMaxAttempts; attempt++ { @@ -472,7 +505,17 @@ func startDCRetry(domainName string) { Log(ctx, logger.Warn(), "AD DC retry exhausted %d attempts for domain %s", dcRetryMaxAttempts, domainName) - }() + }(retryID) +} + +func clearDCRetryIfCurrent(domainName string, retryID uint64) { + dcRetryMu.Lock() + defer dcRetryMu.Unlock() + if dcRetryCancel != nil && dcRetryID == retryID && strings.EqualFold(dcRetryDomain, domainName) { + dcRetryCancel = nil + dcRetryDomain = "" + dcRetryID = 0 + } } // tryGetDCAddress attempts a single DsGetDcName call and returns the DC IP on success, From 75e8447c75f69dd0d98b21ec9bca32b8c4c3d1c2 Mon Sep 17 00:00:00 2001 From: Cuong Manh Le Date: Mon, 1 Jun 2026 16:30:34 +0700 Subject: [PATCH 2/2] test: isolate VPN DNS settling tests from host adapters --- cmd/cli/vpn_dns.go | 10 +++++++++- cmd/cli/vpn_dns_test.go | 3 +++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/cmd/cli/vpn_dns.go b/cmd/cli/vpn_dns.go index 6b94e0ab..f671d2d1 100644 --- a/cmd/cli/vpn_dns.go +++ b/cmd/cli/vpn_dns.go @@ -46,6 +46,9 @@ type vpnDNSManager struct { // state for one guarded refresh cycle because Windows can briefly report an // intermediate empty adapter/DNS state after sleep/wake or reconnect. retainedAfterEmptyDiscovery bool + // discoverVPNDNS is injected for tests so Refresh does not depend on the + // runner host's real VPN/virtual adapter state. + discoverVPNDNS func(context.Context) []ctrld.VPNDNSConfig // Called when VPN DNS server list changes, to update intercept exemptions. onServersChanged vpnDNSExemptFunc } @@ -56,6 +59,7 @@ type vpnDNSManager struct { func newVPNDNSManager(exemptFunc vpnDNSExemptFunc) *vpnDNSManager { return &vpnDNSManager{ routes: make(map[string][]string), + discoverVPNDNS: ctrld.DiscoverVPNDNS, onServersChanged: exemptFunc, } } @@ -66,7 +70,11 @@ func (m *vpnDNSManager) Refresh(guardAgainstNoNameservers bool) { logger := mainLog.Load() logger.Debug().Msg("Refreshing VPN DNS configurations") - configs := ctrld.DiscoverVPNDNS(context.Background()) + discoverVPNDNS := m.discoverVPNDNS + if discoverVPNDNS == nil { + discoverVPNDNS = ctrld.DiscoverVPNDNS + } + configs := discoverVPNDNS(context.Background()) // Detect exit mode: if the default route goes through a VPN DNS interface, // the VPN is routing ALL traffic (exit node / full tunnel). This is more diff --git a/cmd/cli/vpn_dns_test.go b/cmd/cli/vpn_dns_test.go index cda57163..e8c03e67 100644 --- a/cmd/cli/vpn_dns_test.go +++ b/cmd/cli/vpn_dns_test.go @@ -1,6 +1,7 @@ package cli import ( + "context" "testing" "github.com/Control-D-Inc/ctrld" @@ -20,6 +21,7 @@ func TestVPNDNSRefreshRetainsStateForOneGuardedEmptyDiscovery(t *testing.T) { gotExemptions = exemptions return nil }) + m.discoverVPNDNS = func(context.Context) []ctrld.VPNDNSConfig { return nil } m.configs = []ctrld.VPNDNSConfig{{ InterfaceName: "Ethernet 6", Servers: []string{"10.25.37.21", "10.25.37.22"}, @@ -46,6 +48,7 @@ func TestVPNDNSRefreshClearsOnSecondGuardedEmptyDiscovery(t *testing.T) { gotExemptions = exemptions return nil }) + m.discoverVPNDNS = func(context.Context) []ctrld.VPNDNSConfig { return nil } m.configs = []ctrld.VPNDNSConfig{{ InterfaceName: "Ethernet 6", Servers: []string{"10.25.37.21"},