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.
2026-07-16 13:17:19 +02:00 · 2026-05-25 06:08:46 -04:00
parent 5dd5846cca
commit 4395efcb22
4 changed files with 265 additions and 28 deletions
@@ -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)
 		}
@@ -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.
@@ -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")
+	}
+}