diff --git a/cmd/cli/dns_intercept_darwin.go b/cmd/cli/dns_intercept_darwin.go index 708c87f..b15836e 100644 --- a/cmd/cli/dns_intercept_darwin.go +++ b/cmd/cli/dns_intercept_darwin.go @@ -47,6 +47,15 @@ const ( // can turn a pf ruleset race into a fork/file-descriptor exhaustion loop. pfExecFailureBackoff = 5 * time.Second + // pfStabilizationMaxWait bounds the total period ordinary repair can be + // suppressed if PF never becomes stable or pfctl keeps failing. + pfStabilizationMaxWait = 90 * time.Second + + // pfIgnoredChangeReconcileInterval bounds leading-edge pf/VPN-DNS work + // during continuous ignored network-change storms. The 2s/4s delayed checks + // still provide trailing reconciliation once the storm stops. + pfIgnoredChangeReconcileInterval = pfAnchorRecheckDelayLong + // pfVPNInterfacePrefixes lists interface name prefixes that indicate VPN/tunnel // interfaces on macOS. Used to add interface-specific DNS intercept rules so that // VPN software with "pass out quick on " rules cannot bypass our intercept. @@ -241,6 +250,11 @@ func (p *prog) startDNSIntercept() error { if err := p.validateDNSIntercept(); err != nil { return err } + p.pfStabilizing.Store(false) + p.pfLastRestoreTime.Store(0) + p.pfBackoffMultiplier.Store(0) + p.pfExecBackoffUntil.Store(0) + p.pfIgnoredChangeLastReconcile.Store(0) // Set up _ctrld group for pf exemption scoping. This ensures that only ctrld's // own DNS queries (matching "group _ctrld" in pf rules) can bypass the redirect. @@ -299,7 +313,7 @@ func (p *prog) startDNSIntercept() error { out, err := exec.Command("pfctl", "-a", pfAnchorName, "-f", pfAnchorFile).CombinedOutput() if err != nil { - os.Remove(pfAnchorFile) + p.rollbackDNSInterceptStart() return fmt.Errorf("dns intercept: failed to load pf anchor: %w (output: %s)", err, strings.TrimSpace(string(out))) } // Baseline the reconcile snapshot: the anchor just loaded already contains this @@ -308,14 +322,16 @@ func (p *prog) startDNSIntercept() error { mainLog.Load().Debug().Msgf("DNS intercept: loaded pf anchor %q from %s", pfAnchorName, pfAnchorFile) if err := p.ensurePFAnchorReference(); err != nil { - mainLog.Load().Warn().Err(err).Msg("DNS intercept: could not add anchor references to running pf ruleset — anchor may not be active") + p.rollbackDNSInterceptStart() + return fmt.Errorf("dns intercept: failed to activate pf anchor references: %w", err) } out, err = exec.Command("pfctl", "-e").CombinedOutput() if err != nil { outStr := strings.TrimSpace(string(out)) if !strings.Contains(outStr, "already enabled") { - mainLog.Load().Warn().Msgf("DNS intercept: pfctl -e returned: %s (err: %v) — pf may not be enabled", outStr, err) + p.rollbackDNSInterceptStart() + return fmt.Errorf("dns intercept: failed to enable pf: %w (output: %s)", err, outStr) } } @@ -333,8 +349,12 @@ func (p *prog) startDNSIntercept() error { mainLog.Load().Debug().Msgf("DNS intercept: active pf NAT/redirect rules:\n%s", strings.TrimSpace(string(out))) } - // Post-load verification: confirm everything actually took effect. - p.verifyPFState() + // Post-load verification: do not publish intercept mode unless PF is + // authoritatively active; the caller can then use interface-DNS fallback. + if !p.verifyPFState() { + p.rollbackDNSInterceptStart() + return fmt.Errorf("dns intercept: post-load PF verification failed") + } p.dnsInterceptState = &pfState{ anchorFile: pfAnchorFile, @@ -343,7 +363,9 @@ func (p *prog) startDNSIntercept() error { // Store the initial set of tunnel interfaces so we can detect changes later. p.mu.Lock() - p.lastTunnelIfaces = discoverTunnelInterfaces() + p.lastTunnelIfaces = discoverTunnelInterfacesForReconcile() + p.pendingTunnelIfaces = nil + p.hasPendingTunnelIfaces = false p.mu.Unlock() lc := p.cfg.FirstListener() @@ -360,6 +382,18 @@ func (p *prog) startDNSIntercept() error { return nil } +func (p *prog) rollbackDNSInterceptStart() { + if out, err := exec.Command("pfctl", "-a", pfAnchorName, "-F", "all").CombinedOutput(); err != nil { + mainLog.Load().Warn().Err(err).Msgf("DNS intercept: startup rollback could not flush anchor (output: %s)", strings.TrimSpace(string(out))) + } + if err := os.Remove(pfAnchorFile); err != nil && !os.IsNotExist(err) { + mainLog.Load().Warn().Err(err).Msg("DNS intercept: startup rollback could not remove anchor file") + } + if err := p.removePFAnchorReference(); err != nil { + mainLog.Load().Warn().Err(err).Msg("DNS intercept: startup rollback could not remove anchor references") + } +} + // ensurePFAnchorReference ensures the running pf ruleset includes our anchor // declarations. We dump the RUNNING ruleset via "pfctl -sr" (filter+scrub rules) // and "pfctl -sn" (NAT/rdr rules), check if our references exist, and if not, @@ -434,19 +468,19 @@ func (p *prog) ensurePFAnchorReference() error { pureFilterLines = append([]string{anchorRef}, pureFilterLines...) } - // Dump and clean pf options. VPN apps (e.g., Windscribe) set "set skip on { lo0 }" - // which disables pf processing on loopback, breaking our route-to + rdr mechanism. - // We strip lo0 and tunnel interfaces from the skip list before reloading. - cleanedOptions, hadLoopbackSkip := pfGetCleanedOptions() - if hadLoopbackSkip { - mainLog.Load().Info().Msg("DNS intercept: will reload pf options without lo0 in skip list") - } - - // Reassemble in pf's required order: options → scrub → translation → filtering. + // Reassemble in pf's required order: scrub → translation → filtering. + // + // KNOWN GAP: no options section is emitted, so this reload drops the running pf + // options - timeouts, limits and any third-party "set skip" directives - which by + // pf semantics revert to defaults. + // + // They cannot simply be carried across, because Apple's pfctl offers no way to read + // them: pfctl(8) accepts -s nat, queue, rules, Anchors, states, Sources, info, + // References, labels, timeouts, memory, Tables, osfp, Interfaces and all, with no + // options modifier. A fix therefore likely means not rebuilding the main ruleset + // from scratch at all. What macOS pf does to system options on a reload like this + // needs confirming on a host - tracked as follow-up to #573. var combined strings.Builder - if cleanedOptions != "" { - combined.WriteString(cleanedOptions) - } for _, line := range scrubLines { combined.WriteString(line + "\n") } @@ -488,139 +522,67 @@ func (p *prog) checkAnchorOrdering(filterLines []string, ourAnchorRef string) { } } -// pfGetCleanedOptions dumps the running pf options via "pfctl -sO" and returns -// them with lo0 removed from any "set skip on" directive. VPN apps like Windscribe -// set "set skip on { lo0 }" which tells pf to bypass ALL processing on -// loopback — this breaks our route-to + rdr interception mechanism which depends on -// lo0. We strip lo0 (and any known VPN tunnel interfaces) from the skip list so our -// rdr rules on lo0 can fire. Other options (timeouts, limits, etc.) are preserved. -// -// Returns the cleaned options as a string suitable for prepending to a pfctl -f reload, -// and a boolean indicating whether lo0 was found in the skip list (i.e., we needed to fix it). -func pfGetCleanedOptions() (string, bool) { - out, err := exec.Command("pfctl", "-sO").CombinedOutput() - if err != nil { - mainLog.Load().Debug().Err(err).Msg("DNS intercept: could not dump pf options") - return "", false - } - - var cleaned strings.Builder - hadLoopbackSkip := false - - for _, line := range strings.Split(string(out), "\n") { - line = strings.TrimSpace(line) - if line == "" || strings.Contains(line, "ALTQ") { - continue - } - - // Parse "set skip on { lo0 ipsec0 }" or "set skip on lo0" - if strings.HasPrefix(line, "set skip on") { - // Extract interface list from the skip directive. - skipPart := strings.TrimPrefix(line, "set skip on") - skipPart = strings.TrimSpace(skipPart) - skipPart = strings.Trim(skipPart, "{}") - skipPart = strings.TrimSpace(skipPart) - - ifaces := strings.Fields(skipPart) - var kept []string - for _, iface := range ifaces { - if iface == "lo0" { - hadLoopbackSkip = true - continue // Remove lo0 — we need pf to process lo0 for our rdr rules. - } - // Also remove VPN tunnel interfaces — we have explicit intercept - // rules for them in our anchor, so skipping defeats the purpose. - isTunnel := false - for _, prefix := range strings.Split(pfVPNInterfacePrefixes, ",") { - if strings.HasPrefix(iface, strings.TrimSpace(prefix)) { - isTunnel = true - break - } - } - if isTunnel { - mainLog.Load().Debug().Msgf("DNS intercept: removing tunnel interface %q from pf skip list", iface) - continue - } - kept = append(kept, iface) - } - - if len(kept) > 0 { - cleaned.WriteString(fmt.Sprintf("set skip on { %s }\n", strings.Join(kept, " "))) - } - // If no interfaces left, omit the skip directive entirely. - continue - } - - // Preserve all other options (timeouts, limits, etc.). - cleaned.WriteString(line + "\n") - } - - if hadLoopbackSkip { - mainLog.Load().Warn().Msg("DNS intercept: detected 'set skip on lo0' — another program (likely VPN software) " + - "disabled pf processing on loopback, which breaks our DNS interception. Removing lo0 from skip list.") - } - - return cleaned.String(), hadLoopbackSkip -} - -// pfFilterRuleLines filters pfctl output into actual pf rule lines, -// stripping stderr warnings (e.g., "No ALTQ support in kernel") and empty lines. -func pfFilterRuleLines(output string) []string { - var rules []string - for _, line := range strings.Split(output, "\n") { - line = strings.TrimSpace(line) - if line == "" { - continue - } - // Skip pfctl stderr warnings that appear in CombinedOutput. - if strings.Contains(line, "ALTQ") { - continue - } - rules = append(rules, line) - } - return rules -} - -// pfContainsRule checks if any line in the slice contains the given rule string. -// Uses substring matching because pfctl may append extra tokens like " all" to rules -// (e.g., `rdr-anchor "com.controld.ctrld" all`), which would fail exact matching. -func pfContainsRule(lines []string, rule string) bool { - for _, line := range lines { - if strings.Contains(line, rule) { - return true - } - } - return false -} - // stopDNSIntercept removes all pf rules and cleans up the DNS interception. func (p *prog) stopDNSIntercept() error { - if p.dnsInterceptState == nil { + state, ok := p.dnsInterceptState.(*pfState) + if !ok || state == nil { mainLog.Load().Debug().Msg("DNS intercept: no pf state to clean up") return nil } - mainLog.Load().Info().Msg("DNS intercept: shutting down pf redirect") + // Revoke lifecycle state and cancel future owners before waiting for any + // in-flight PF mutation. A mutation that already owns the lane may finish, + // but final cleanup always runs after it and no new owner can start. + p.dnsInterceptState = nil + pfShutdownStateRevokedForTest() + p.mu.Lock() + if p.pfStabilizeCancel != nil { + p.pfStabilizeCancel() + p.pfStabilizeCancel = nil + } + p.lastTunnelIfaces = nil + p.pendingTunnelIfaces = nil + p.hasPendingTunnelIfaces = false + p.mu.Unlock() + p.pfStabilizing.Store(false) - out, err := exec.Command("pfctl", "-a", p.dnsInterceptState.(*pfState).anchorName, "-F", "all").CombinedOutput() + p.pfDelayedRecheckMu.Lock() + for _, timer := range p.pfDelayedRecheckTimers { + if timer != nil { + timer.Stop() + } + } + p.pfDelayedRecheckTimers = nil + p.pfDelayedRecheckMu.Unlock() + + for !p.pfEnsureRunning.CompareAndSwap(false, true) { + time.Sleep(10 * time.Millisecond) + } + defer p.pfEnsureRunning.Store(false) + + mainLog.Load().Info().Msg("DNS intercept: shutting down pf redirect") + out, err := exec.Command("pfctl", "-a", state.anchorName, "-F", "all").CombinedOutput() if err != nil { mainLog.Load().Warn().Msgf("DNS intercept: failed to flush pf anchor %q: %v (output: %s)", - p.dnsInterceptState.(*pfState).anchorName, err, strings.TrimSpace(string(out))) + state.anchorName, err, strings.TrimSpace(string(out))) } else { - mainLog.Load().Debug().Msgf("DNS intercept: flushed pf anchor %q", p.dnsInterceptState.(*pfState).anchorName) + mainLog.Load().Debug().Msgf("DNS intercept: flushed pf anchor %q", state.anchorName) } - if err := os.Remove(p.dnsInterceptState.(*pfState).anchorFile); err != nil && !os.IsNotExist(err) { - mainLog.Load().Warn().Msgf("DNS intercept: failed to remove anchor file %s: %v", p.dnsInterceptState.(*pfState).anchorFile, err) + if err := os.Remove(state.anchorFile); err != nil && !os.IsNotExist(err) { + mainLog.Load().Warn().Msgf("DNS intercept: failed to remove anchor file %s: %v", state.anchorFile, err) } else { - mainLog.Load().Debug().Msgf("DNS intercept: removed anchor file %s", p.dnsInterceptState.(*pfState).anchorFile) + mainLog.Load().Debug().Msgf("DNS intercept: removed anchor file %s", state.anchorFile) } if err := p.removePFAnchorReference(); err != nil { mainLog.Load().Warn().Err(err).Msg("DNS intercept: failed to remove anchor references from running pf ruleset") } - p.dnsInterceptState = nil + p.pfLastRestoreTime.Store(0) + p.pfBackoffMultiplier.Store(0) + p.pfExecBackoffUntil.Store(0) + p.pfIgnoredChangeLastReconcile.Store(0) mainLog.Load().Info().Msg("DNS intercept: pf shutdown complete") return nil } @@ -642,6 +604,14 @@ func (p *prog) removePFAnchorReference() error { return fmt.Errorf("failed to dump running filter rules: %w (output: %s)", err, strings.TrimSpace(string(filterOut))) } + // Nothing of ours in the running ruleset means nothing to remove - and the reload + // below would reset system-wide pf options on the way past. Startup rollback reaches + // here after failures that happen before the references are ever added. + if !pfAnchorReferencesPresent(string(natOut), string(filterOut), pfAnchorName) { + mainLog.Load().Debug().Msg("DNS intercept: no anchor references in the running pf ruleset — nothing to remove") + return nil + } + // Filter and remove our lines. natLines := pfFilterRuleLines(string(natOut)) filterLines := pfFilterRuleLines(string(filterOut)) @@ -742,6 +712,8 @@ func discoverTunnelInterfaces() []string { return tunnels } +var discoverTunnelInterfacesForReconcile = discoverTunnelInterfaces + // dnsInterceptSupported reports whether DNS intercept mode is supported on this platform. func dnsInterceptSupported() bool { _, err := exec.LookPath("pfctl") @@ -813,6 +785,14 @@ func (p *prog) buildPFAnchorRules(vpnExemptions []vpnDNSExemption) string { // had already taken effect. Passing the set in lets each path record exactly what pf // accepted. func (p *prog) buildPFAnchorRulesWith(vpnExemptions []vpnDNSExemption, forwardedSources []forwardedSource) string { + return p.buildPFAnchorRulesForState(vpnExemptions, discoverTunnelInterfacesForReconcile(), forwardedSources) +} + +func (p *prog) buildPFAnchorRulesForTunnels(vpnExemptions []vpnDNSExemption, tunnelIfaces []string) string { + return p.buildPFAnchorRulesForState(vpnExemptions, tunnelIfaces, p.currentForwardedSources()) +} + +func (p *prog) buildPFAnchorRulesForState(vpnExemptions []vpnDNSExemption, tunnelIfaces []string, forwardedSources []forwardedSource) string { // Read the actual listener address from config. In intercept mode, ctrld may // be on a non-standard port (e.g., 127.0.0.1:5354) if mDNSResponder holds *:53. // The pf rdr rules must redirect to wherever ctrld is actually listening. @@ -980,7 +960,20 @@ func (p *prog) buildPFAnchorRulesWith(vpnExemptions []vpnDNSExemption, forwarded } // --- Interface-specific VPN/tunnel intercept rules --- - tunnelIfaces := discoverTunnelInterfaces() + // VPN apps (e.g., Windscribe, Cisco AnyConnect) often add pf rules like: + // pass out quick on ipsec0 inet all flags S/SA keep state + // inside their own anchors. If their anchor is evaluated before ours, their + // "quick" match on the VPN interface captures DNS traffic before our generic + // "on ! lo0" rule can intercept it. To counter this, we add explicit intercept + // rules for each active tunnel interface. These use "quick" and match port 53 + // specifically, so they take priority over the VPN app's broader "all" rules + // regardless of anchor ordering. + // + // NOTE: If anchor ordering alone proves insufficient in the future, a "nuclear + // option" is available: inject DNS intercept rules directly into the MAIN pf + // ruleset (not inside our anchor). Main ruleset rules are evaluated before ALL + // anchors, making them impossible for another app to override without explicitly + // removing them. See docs/dns-intercept-mode.md for details. if len(tunnelIfaces) > 0 { rules.WriteString("# --- VPN/tunnel interface intercept rules ---\n") rules.WriteString("# Explicit intercept on tunnel interfaces prevents VPN apps from capturing\n") @@ -1046,7 +1039,7 @@ func (p *prog) buildPFAnchorRulesWith(vpnExemptions []vpnDNSExemption, forwarded // verifyPFState checks that the pf ruleset is correctly configured after loading. // It verifies both the anchor references in the main ruleset and the rules within // our anchor. Failures are logged at ERROR level to make them impossible to miss. -func (p *prog) verifyPFState() { +func (p *prog) verifyPFState() bool { rdrAnchorRef := fmt.Sprintf("rdr-anchor \"%s\"", pfAnchorName) anchorRef := fmt.Sprintf("anchor \"%s\"", pfAnchorName) verified := true @@ -1054,6 +1047,7 @@ func (p *prog) verifyPFState() { // Check main ruleset for anchor references. natOut, err := exec.Command("pfctl", "-sn").CombinedOutput() if err != nil { + p.pfBackoffResourceExhaustion(err, natOut, "verify NAT rules") mainLog.Load().Error().Err(err).Msg("DNS intercept: VERIFICATION FAILED — could not dump NAT rules") verified = false } else if !strings.Contains(string(natOut), rdrAnchorRef) { @@ -1063,6 +1057,7 @@ func (p *prog) verifyPFState() { filterOut, err := exec.Command("pfctl", "-sr").CombinedOutput() if err != nil { + p.pfBackoffResourceExhaustion(err, filterOut, "verify filter rules") mainLog.Load().Error().Err(err).Msg("DNS intercept: VERIFICATION FAILED — could not dump filter rules") verified = false } else if !strings.Contains(string(filterOut), anchorRef) { @@ -1073,38 +1068,44 @@ func (p *prog) verifyPFState() { // Check our anchor has rules loaded. anchorFilter, err := exec.Command("pfctl", "-a", pfAnchorName, "-sr").CombinedOutput() if err != nil { + p.pfBackoffResourceExhaustion(err, anchorFilter, "verify anchor filter rules") mainLog.Load().Error().Err(err).Msg("DNS intercept: VERIFICATION FAILED — could not dump anchor filter rules") verified = false - } else if len(strings.TrimSpace(string(anchorFilter))) == 0 { + } else if pfRulesetEmpty(string(anchorFilter)) { mainLog.Load().Error().Msg("DNS intercept: VERIFICATION FAILED — anchor has no filter rules loaded") verified = false } anchorNat, err := exec.Command("pfctl", "-a", pfAnchorName, "-sn").CombinedOutput() if err != nil { + p.pfBackoffResourceExhaustion(err, anchorNat, "verify anchor NAT rules") mainLog.Load().Error().Err(err).Msg("DNS intercept: VERIFICATION FAILED — could not dump anchor NAT rules") verified = false - } else if len(strings.TrimSpace(string(anchorNat))) == 0 { + } else if pfRulesetEmpty(string(anchorNat)) { mainLog.Load().Error().Msg("DNS intercept: VERIFICATION FAILED — anchor has no NAT/redirect rules loaded") verified = false } - // Check that lo0 is not in the skip list — if it is, our rdr rules are dead. - optOut, err := exec.Command("pfctl", "-sO").CombinedOutput() - if err == nil { - for _, line := range strings.Split(string(optOut), "\n") { - line = strings.TrimSpace(line) - if strings.HasPrefix(line, "set skip on") && strings.Contains(line, "lo0") { - mainLog.Load().Error().Msg("DNS intercept: VERIFICATION FAILED — 'set skip on lo0' is active, rdr rules on loopback will not fire") - verified = false - break - } - } - } + // There is deliberately no "set skip on lo0" check here, even though such a skip + // would make our rdr rules on loopback dead. + // + // macOS offers no way to query it as text: pfctl(8) accepts -s nat, queue, rules, + // Anchors, states, Sources, info, References, labels, timeouts, memory, Tables, + // osfp, Interfaces and all - there is no options or skip modifier. Since this + // verification gates whether intercept mode may publish, a check that cannot + // succeed here would roll back every start. + // + // probePFIntercept() detects the condition functionally instead: it sends a real + // query from outside the _ctrld group and confirms the listener received the + // redirect, which cannot succeed if pf is bypassing loopback. An explicit check is + // follow-up work - pfctl(8) documents "-s Interfaces -v" as listing which + // interfaces have skip rules activated, but its output shape needs confirming on a + // host that actually has a skip configured before anything parses it. if verified { mainLog.Load().Info().Msg("DNS intercept: post-load verification passed — all pf rules confirmed active") } + return verified } // resetUpstreamTransports forces all DoH/DoT/DoQ upstreams to re-bootstrap their @@ -1135,32 +1136,166 @@ func (p *prog) resetUpstreamTransports() { } } +var ( + ensurePFAnchorReferenceForRestore = func(p *prog) error { + return p.ensurePFAnchorReference() + } + rebuildPFAnchorRulesForReconcile = func(p *prog, exemptions []vpnDNSExemption) ([]string, error) { + return p.rebuildPFAnchorRules(exemptions) + } + restorePFAnchorForReconcile = func(p *prog, reason string) pfAnchorCheckResult { + return p.restorePFAnchor(reason) + } + pfShutdownStateRevokedForTest = func() {} +) + +func (p *prog) rebuildPFAnchorRules(vpnExemptions []vpnDNSExemption) ([]string, error) { + tunnelIfaces := append([]string(nil), discoverTunnelInterfacesForReconcile()...) + forwarded := p.currentForwardedSources() + rulesStr := p.buildPFAnchorRulesForState(vpnExemptions, tunnelIfaces, forwarded) + if err := writePFAnchorFile(rulesStr); err != nil { + return nil, fmt.Errorf("write anchor file: %w", err) + } + out, err := exec.Command("pfctl", "-a", pfAnchorName, "-f", pfAnchorFile).CombinedOutput() + if err != nil { + return nil, fmt.Errorf("load rebuilt anchor: %w (output: %s)", err, strings.TrimSpace(string(out))) + } + p.recordAppliedForwardedSources(forwarded) + flushPFStates() + return tunnelIfaces, nil +} + +func (p *prog) restorePFAnchor(reason string) pfAnchorCheckResult { + return p.restorePFAnchorWithTransportReset(reason, true) +} + +func (p *prog) restorePFAnchorWithTransportReset(reason string, resetTransports bool) pfAnchorCheckResult { + if p.dnsInterceptState == nil { + return pfAnchorCheckSkipped + } + var vpnExemptions []vpnDNSExemption + if p.vpnDNS != nil { + vpnExemptions = p.vpnDNS.CurrentExemptions() + } + mainLog.Load().Info().Str("reason", reason).Msg("DNS intercept: restoring pf anchor") + if err := ensurePFAnchorReferenceForRestore(p); err != nil { + p.pfBackoffResourceExhaustion(err, nil, "restore anchor references") + if resetTransports { + p.resetUpstreamTransports() + } + mainLog.Load().Error().Err(err).Msg("DNS intercept: failed to restore anchor references") + return pfAnchorCheckFailed + } + + tunnelIfaces, err := rebuildPFAnchorRulesForReconcile(p, vpnExemptions) + if err != nil { + p.pfBackoffResourceExhaustion(err, nil, "rebuild anchor") + if resetTransports { + p.resetUpstreamTransports() + } + mainLog.Load().Error().Err(err).Msg("DNS intercept: failed to rebuild anchor") + return pfAnchorCheckFailed + } + + // rebuildPFAnchorRules flushed PF states after the successful load. Reset + // transports even if the authoritative verification below fails, otherwise + // DoH can remain pinned to connections invalidated by that flush. + if resetTransports { + p.resetUpstreamTransports() + } + if !p.verifyPFState() { + mainLog.Load().Error().Str("reason", reason).Msg("DNS intercept: rebuilt anchor failed post-load verification") + return pfAnchorCheckFailed + } + if p.vpnDNS != nil { + p.vpnDNS.markInterceptExemptionsApplied(vpnExemptions) + } + p.commitPFReconcileState(tunnelIfaces) + p.pfLastRestoreTime.Store(time.Now().UnixMilli()) + mainLog.Load().Info().Str("reason", reason).Msg("DNS intercept: pf anchor restored successfully") + return pfAnchorCheckRestored +} + +func (p *prog) commitPFReconcileState(tunnelIfaces []string) { + p.mu.Lock() + defer p.mu.Unlock() + p.lastTunnelIfaces = append([]string(nil), tunnelIfaces...) + if p.hasPendingTunnelIfaces && stringSlicesEqual(p.pendingTunnelIfaces, tunnelIfaces) { + p.pendingTunnelIfaces = nil + p.hasPendingTunnelIfaces = false + } +} + +func (p *prog) clearPendingTunnelReconcile() { + p.mu.Lock() + p.pendingTunnelIfaces = nil + p.hasPendingTunnelIfaces = false + p.mu.Unlock() +} + +func (p *prog) hasPendingTunnelReconcile() bool { + p.mu.Lock() + defer p.mu.Unlock() + return p.hasPendingTunnelIfaces +} + +func (p *prog) reconcilePFAnchorForTunnelChange() pfAnchorCheckResult { + if p.pfExecBackoffActive() { + return pfAnchorCheckSkipped + } + if !p.pfEnsureRunning.CompareAndSwap(false, true) { + return pfAnchorCheckSkipped + } + defer p.pfEnsureRunning.Store(false) + return restorePFAnchorForReconcile(p, "tunnel interface change") +} + // checkTunnelInterfaceChanges compares the current set of active tunnel interfaces // against the last known set. If they differ (e.g., a VPN connected and created utun420), // it rebuilds and reloads the pf anchor rules to include interface-specific intercept // rules for the new interface. // -// Returns true if the anchor was rebuilt, false if no changes detected. -// This is called from the network change callback even when validInterfacesMap() -// reports no changes — because validInterfacesMap() only tracks physical hardware -// ports (en0, bridge0, etc.) and ignores tunnel interfaces (utun*, ipsec*, etc.). +// Returns true when a new tunnel state is observed or a pending state is +// successfully applied. Coalesced/deferred retries return false so ignored-event +// storms do not bypass the macOS VPN-DNS refresh limiter on every notification. func (p *prog) checkTunnelInterfaceChanges() bool { if p.dnsInterceptState == nil { return false } - current := discoverTunnelInterfaces() + current := append([]string(nil), discoverTunnelInterfacesForReconcile()...) p.mu.Lock() - prev := p.lastTunnelIfaces - changed := !stringSlicesEqual(prev, current) - if changed { - p.lastTunnelIfaces = current + prev := append([]string(nil), p.lastTunnelIfaces...) + stabilizing := p.pfStabilizing.Load() + newObservation := false + if p.hasPendingTunnelIfaces && stringSlicesEqual(p.pendingTunnelIfaces, current) { + if stabilizing { + p.mu.Unlock() + return false + } + if stringSlicesEqual(prev, current) { + p.pendingTunnelIfaces = nil + p.hasPendingTunnelIfaces = false + p.mu.Unlock() + return false + } + // The same pending state still differs from the applied baseline. Retry it + // after stabilization/failure instead of treating it as already applied. + } else { + if !p.hasPendingTunnelIfaces && stringSlicesEqual(prev, current) { + p.mu.Unlock() + return false + } + newObservation = true + p.pendingTunnelIfaces = append([]string(nil), current...) + p.hasPendingTunnelIfaces = true } p.mu.Unlock() - if !changed { - return false + if stringSlicesEqual(prev, current) { + p.clearPendingTunnelReconcile() + return true } // Detect NEW tunnel interfaces (not just any change). @@ -1177,41 +1312,26 @@ func (p *prog) checkTunnelInterfaceChanges() bool { } } + if stabilizing { + mainLog.Load().Debug().Msgf("DNS intercept: tunnel state changed during stabilization (was %v, now %v) — deferring anchor rebuild", prev, current) + return newObservation + } + if hasNewTunnel { - // A new VPN tunnel appeared. Enter stabilization mode — the VPN may be - // about to wipe our pf rules (Windscribe does this ~500ms after tunnel creation). - // We can't check pfAnchorIsWiped() here because the wipe hasn't happened yet. - // The stabilization loop will detect whether pf actually gets wiped: - // - If rules change (VPN touches pf): wait for stability, then restore. - // - If rules stay stable for the full wait (Tailscale): exit early and rebuild immediately. + // A new VPN tunnel appeared. The dedicated post-stabilization repair path + // rebuilds the anchor if no successful exemption update applies it first. p.pfStartStabilization() - return true + return newObservation } mainLog.Load().Info().Msgf("DNS intercept: tunnel interfaces changed (was %v, now %v) — rebuilding pf anchor rules", prev, current) - - // Rebuild anchor rules with the updated tunnel interface list. - // Pass current VPN DNS exemptions so they are preserved for still-active VPNs. - var vpnExemptions []vpnDNSExemption - if p.vpnDNS != nil { - vpnExemptions = p.vpnDNS.CurrentExemptions() + result := p.reconcilePFAnchorForTunnelChange() + if result != pfAnchorCheckRestored { + // Keep the desired tunnel set pending. The next event, delayed check, or + // watchdog tick retries it without confusing pending with applied state. + mainLog.Load().Debug().Msgf("DNS intercept: tunnel rule rebuild deferred/failed (result: %d)", result) } - forwarded := p.currentForwardedSources() - rulesStr := p.buildPFAnchorRulesWith(vpnExemptions, forwarded) - if err := writePFAnchorFile(rulesStr); err != nil { - mainLog.Load().Error().Err(err).Msg("DNS intercept: failed to write rebuilt anchor file") - return true - } - out, err := exec.Command("pfctl", "-a", pfAnchorName, "-f", pfAnchorFile).CombinedOutput() - if err != nil { - mainLog.Load().Error().Err(err).Msgf("DNS intercept: failed to reload rebuilt anchor (output: %s)", strings.TrimSpace(string(out))) - return true - } - p.recordAppliedForwardedSources(forwarded) - - flushPFStates() - mainLog.Load().Info().Msgf("DNS intercept: rebuilt pf anchor with %d tunnel interfaces", len(current)) - return true + return newObservation || result == pfAnchorCheckRestored } // stringSlicesEqual reports whether two string slices have the same elements in the same order. @@ -1231,11 +1351,9 @@ func stringSlicesEqual(a, b []string) bool { // until the VPN's ruleset stops changing. This prevents a death spiral where // ctrld and the VPN repeatedly overwrite each other's pf rules. func (p *prog) pfStartStabilization() { - if p.pfStabilizing.Load() { - // Already stabilizing — extending is handled by backoff. + if !p.pfStabilizing.CompareAndSwap(false, true) { return } - p.pfStabilizing.Store(true) multiplier := max(int(p.pfBackoffMultiplier.Load()), 1) baseStableTime := 6000 * time.Millisecond // 4 polls at 1.5s @@ -1260,9 +1378,17 @@ func (p *prog) pfStartStabilization() { // pfStabilizationLoop polls pfctl -sr hash until the ruleset is stable for the // required duration, then restores our anchor rules. func (p *prog) pfStabilizationLoop(ctx context.Context, stableRequired time.Duration) { + p.pfStabilizationLoopWithMaxWait(ctx, stableRequired, pfStabilizationMaxWait) +} + +func (p *prog) pfStabilizationLoopWithMaxWait(ctx context.Context, stableRequired, maxWaitDuration time.Duration) { defer p.pfStabilizing.Store(false) pollInterval := 1500 * time.Millisecond + pollTicker := time.NewTicker(pollInterval) + defer pollTicker.Stop() + maxWait := time.NewTimer(maxWaitDuration) + defer maxWait.Stop() var lastHash string stableSince := time.Time{} @@ -1273,12 +1399,20 @@ func (p *prog) pfStabilizationLoop(ctx context.Context, stableRequired time.Dura return case <-p.stopCh: return - case <-time.After(pollInterval): + case <-maxWait.C: + mainLog.Load().Warn().Msgf("DNS intercept: stabilization timed out after %s — returning ownership to delayed/watchdog recovery", maxWaitDuration) + p.scheduleDelayedRechecks() + return + case <-pollTicker.C: } + if p.pfExecBackoffActive() { + continue + } // Hash the current filter ruleset. out, err := exec.Command("pfctl", "-sr").CombinedOutput() if err != nil { + p.pfBackoffResourceExhaustion(err, out, "poll stabilization filter rules") continue } hash := fmt.Sprintf("%x", sha256.Sum256(out)) @@ -1297,65 +1431,65 @@ func (p *prog) pfStabilizationLoop(ctx context.Context, stableRequired time.Dura } if time.Since(stableSince) >= stableRequired { - // Stable long enough — restore our rules. - // Clear stabilizing flag BEFORE calling ensurePFAnchorActive so - // the guard inside that function doesn't suppress our restore. - p.pfStabilizing.Store(false) - mainLog.Load().Info().Msgf("DNS intercept: pf stable for %s — restoring anchor rules", stableRequired) - p.ensurePFAnchorActive() + // The active loop retains ownership until the dedicated post-stable + // repair finishes. It never re-enters stabilization recursively. + mainLog.Load().Info().Msgf("DNS intercept: pf stable for %s — reconciling anchor rules", stableRequired) + result := p.reconcilePFAnchorAfterStabilization() + if result != pfAnchorCheckRestored && result != pfAnchorCheckIntact { + p.scheduleDelayedRechecks() + } 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()) + if p.hasPendingTunnelReconcile() { + p.scheduleDelayedRechecks() + } return } } } -// ensurePFAnchorActive checks that our pf anchor references and rules are still -// present in the running ruleset. If anything is missing (e.g., another program -// like Windscribe desktop or macOS itself reloaded pf.conf), it restores them. -// -// Returns true if restoration was needed, false if everything was already intact. -// Called both on network changes (immediate) and by the periodic pfWatchdog. -func (p *prog) ensurePFAnchorActive() bool { +var runPFAnchorCheckCommand = func(args ...string) ([]byte, error) { + return exec.Command("pfctl", args...).CombinedOutput() +} + +// ensurePFAnchorActive checks live PF state and returns an explicit outcome for +// callers that must distinguish intact, restored, deferred, skipped, and failed. +func (p *prog) ensurePFAnchorActive() pfAnchorCheckResult { + return p.ensurePFAnchorActiveWithPolicy(true, false) +} + +// reconcilePFAnchorAfterStabilization is owned by the active stabilization loop. +// It bypasses ordinary recent-wipe deferral and forces a rebuild only when tunnel +// state is still pending. Otherwise an intact anchor is left alone, avoiding a +// redundant global PF state flush after an earlier successful exemption update. +func (p *prog) reconcilePFAnchorAfterStabilization() pfAnchorCheckResult { + return p.ensurePFAnchorActiveWithPolicy(false, p.hasPendingTunnelReconcile()) +} + +func (p *prog) ensurePFAnchorActiveWithPolicy(allowRecentWipeDeferral, forceRebuild bool) pfAnchorCheckResult { if p.dnsInterceptState == nil { - return false + return pfAnchorCheckSkipped + } + // Ordinary callers must not compete with the loop that owns stabilization. + // Check before acquiring the singleflight guard so the post-stable owner is + // not blocked by a watchdog callback that would immediately skip. + if allowRecentWipeDeferral && p.pfStabilizing.Load() { + mainLog.Load().Debug().Msg("DNS intercept watchdog: suppressed — VPN stabilization in progress") + return pfAnchorCheckSkipped } if !p.pfEnsureRunning.CompareAndSwap(false, true) { mainLog.Load().Debug().Msg("DNS intercept watchdog: check already running, skipping duplicate") - return false + return pfAnchorCheckSkipped } defer p.pfEnsureRunning.Store(false) if p.pfExecBackoffActive() { - return false + return pfAnchorCheckSkipped } - - // While stabilizing (VPN connecting), suppress all restores. - // The stabilization loop will restore once pf settles. - if p.pfStabilizing.Load() { - mainLog.Load().Debug().Msg("DNS intercept watchdog: suppressed — VPN stabilization in progress") - return false - } - - // Check if our last restore was very recent and got wiped again. - // This indicates a VPN reconnect cycle — enter stabilization with backoff. - if lastRestore := p.pfLastRestoreTime.Load(); lastRestore > 0 { - elapsed := time.Since(time.UnixMilli(lastRestore)) - if elapsed < 10*time.Second { - // Rules were wiped within 10s of our last restore — VPN is fighting us. - p.pfBackoffMultiplier.Add(1) - mainLog.Load().Warn().Msgf("DNS intercept: rules wiped %s after restore — entering stabilization (backoff multiplier: %d)", - elapsed, p.pfBackoffMultiplier.Load()) - p.pfStartStabilization() - return false - } - // Rules survived >10s — reset backoff - if p.pfBackoffMultiplier.Load() > 0 { - p.pfBackoffMultiplier.Store(0) - } + if allowRecentWipeDeferral && p.pfStabilizing.Load() { + return pfAnchorCheckSkipped } rdrAnchorRef := fmt.Sprintf("rdr-anchor \"%s\"", pfAnchorName) @@ -1363,13 +1497,11 @@ func (p *prog) ensurePFAnchorActive() bool { needsRestore := false // Check 1: anchor references in the main ruleset. - natOut, err := exec.Command("pfctl", "-sn").CombinedOutput() + natOut, err := runPFAnchorCheckCommand("-sn") if err != nil { - if p.pfBackoffResourceExhaustion(err, natOut, "dump NAT rules") { - return false - } + p.pfBackoffResourceExhaustion(err, natOut, "dump NAT rules") mainLog.Load().Warn().Err(err).Msg("DNS intercept watchdog: could not dump NAT rules") - return false + return pfAnchorCheckFailed } if !strings.Contains(string(natOut), rdrAnchorRef) { mainLog.Load().Warn().Msg("DNS intercept watchdog: rdr-anchor reference missing from running ruleset") @@ -1377,13 +1509,11 @@ func (p *prog) ensurePFAnchorActive() bool { } if !needsRestore { - filterOut, err := exec.Command("pfctl", "-sr").CombinedOutput() + filterOut, err := runPFAnchorCheckCommand("-sr") if err != nil { - if p.pfBackoffResourceExhaustion(err, filterOut, "dump filter rules") { - return false - } + p.pfBackoffResourceExhaustion(err, filterOut, "dump filter rules") mainLog.Load().Warn().Err(err).Msg("DNS intercept watchdog: could not dump filter rules") - return false + return pfAnchorCheckFailed } if !strings.Contains(string(filterOut), anchorRef) { mainLog.Load().Warn().Msg("DNS intercept watchdog: anchor reference missing from running filter rules") @@ -1397,100 +1527,88 @@ func (p *prog) ensurePFAnchorActive() bool { // Without rdr, route-to sends packets to lo0 but they never get redirected to 127.0.0.1:53, // causing an infinite packet loop on lo0 and complete DNS failure. if !needsRestore { - anchorFilter, err := exec.Command("pfctl", "-a", pfAnchorName, "-sr").CombinedOutput() - if err != nil || len(strings.TrimSpace(string(anchorFilter))) == 0 { - if p.pfBackoffResourceExhaustion(err, anchorFilter, "dump anchor filter rules") { - return false - } + anchorFilter, err := runPFAnchorCheckCommand("-a", pfAnchorName, "-sr") + if err != nil { + p.pfBackoffResourceExhaustion(err, anchorFilter, "dump anchor filter rules") + mainLog.Load().Warn().Err(err).Msg("DNS intercept watchdog: could not dump anchor filter rules") + return pfAnchorCheckFailed + } + if pfRulesetEmpty(string(anchorFilter)) { mainLog.Load().Warn().Msg("DNS intercept watchdog: anchor has no filter rules — content was flushed") needsRestore = true } } if !needsRestore { - anchorNat, err := exec.Command("pfctl", "-a", pfAnchorName, "-sn").CombinedOutput() - if err != nil || len(strings.TrimSpace(string(anchorNat))) == 0 { - if p.pfBackoffResourceExhaustion(err, anchorNat, "dump anchor NAT rules") { - return false - } + anchorNat, err := runPFAnchorCheckCommand("-a", pfAnchorName, "-sn") + if err != nil { + p.pfBackoffResourceExhaustion(err, anchorNat, "dump anchor NAT rules") + mainLog.Load().Warn().Err(err).Msg("DNS intercept watchdog: could not dump anchor NAT rules") + return pfAnchorCheckFailed + } + if pfRulesetEmpty(string(anchorNat)) { mainLog.Load().Warn().Msg("DNS intercept watchdog: anchor has no rdr rules — translation was flushed (will cause packet loop on lo0)") needsRestore = true } } - // Check 3: "set skip on lo0" — VPN apps (e.g., Windscribe) load a complete pf.conf - // with "set skip on { lo0 }" which disables ALL pf processing on loopback. - // Our entire interception mechanism (route-to lo0 + rdr on lo0) depends on lo0 being - // processed by pf. This check detects the skip and triggers a restore that removes it. - if !needsRestore { - optOut, err := exec.Command("pfctl", "-sO").CombinedOutput() - if err == nil { - optStr := string(optOut) - // Check if lo0 appears in any "set skip on" directive. - for _, line := range strings.Split(optStr, "\n") { - line = strings.TrimSpace(line) - if strings.HasPrefix(line, "set skip on") && strings.Contains(line, "lo0") { - mainLog.Load().Warn().Msg("DNS intercept watchdog: 'set skip on lo0' detected — loopback bypass breaks our rdr rules") - needsRestore = true - break - } - } + // There is deliberately no "set skip on lo0" check here - see the note in + // verifyPFState for why macOS cannot be asked for skip state. + // + // It matters more here than a missing check might suggest: anything that cannot + // conclude has to return pfAnchorCheckFailed, and the caller only runs + // probePFIntercept() and the forced-reload recovery on an intact result. A check + // that can never conclude would therefore keep this function from ever reporting + // intact, disabling the functional probe and the self-heal it drives. + // + // So the skip case is left to probePFIntercept(). An explicit check can return via + // "pfctl -s Interfaces -v" once its output shape is confirmed on a host that has a + // skip configured. + + // Only classify a recent event as a repeated wipe after the live ruleset + // actually proves that a reference or anchor rule is missing. Previously this + // timestamp check ran first, so every healthy callback for ten seconds after + // stabilization was mislabeled as another wipe. + if lastRestore := p.pfLastRestoreTime.Load(); lastRestore > 0 { + elapsed := time.Since(time.UnixMilli(lastRestore)) + if needsRestore && allowRecentWipeDeferral && elapsed < 10*time.Second { + p.pfBackoffMultiplier.Add(1) + mainLog.Load().Warn().Msgf("DNS intercept: rules wiped %s after restore — entering stabilization (backoff multiplier: %d)", + elapsed, p.pfBackoffMultiplier.Load()) + p.pfStartStabilization() + return pfAnchorCheckDeferred + } + if elapsed >= 10*time.Second && p.pfBackoffMultiplier.Load() > 0 { + p.pfBackoffMultiplier.Store(0) } } - if !needsRestore { + if !needsRestore && !forceRebuild { mainLog.Load().Debug().Msg("DNS intercept watchdog: pf anchor intact") - return false + return pfAnchorCheckIntact } - // Restore: re-inject anchor references into the main ruleset. - mainLog.Load().Info().Msg("DNS intercept watchdog: restoring pf anchor references") - if err := p.ensurePFAnchorReference(); err != nil { - p.pfBackoffResourceExhaustion(err, nil, "restore anchor references") - mainLog.Load().Error().Err(err).Msg("DNS intercept watchdog: failed to restore anchor references") - return true + reason := "missing pf anchor state" + if forceRebuild && !needsRestore { + reason = "post-stabilization rebuild" } + return restorePFAnchorForReconcile(p, reason) +} - // Restore: always rebuild anchor rules from scratch to ensure tunnel interface - // rules are up-to-date (VPN interfaces may have appeared/disappeared since the - // anchor file was last written). - mainLog.Load().Info().Msg("DNS intercept watchdog: rebuilding anchor rules with current network state") - var vpnExemptions []vpnDNSExemption - if p.vpnDNS != nil { - vpnExemptions = p.vpnDNS.CurrentExemptions() +// dnsInterceptIgnoredChangeReconcileDue reports whether an ignored macOS +// network delta should run the expensive leading-edge pf/VPN-DNS reconciliation. +// Tunnel changes bypass this decision in the caller. The periodic watchdog and +// coalesced delayed checks remain independent safety nets. +func (p *prog) dnsInterceptIgnoredChangeReconcileDue(now time.Time) bool { + nowMillis := now.UnixMilli() + for { + last := p.pfIgnoredChangeLastReconcile.Load() + if last > 0 && now.Sub(time.UnixMilli(last)) < pfIgnoredChangeReconcileInterval { + return false + } + if p.pfIgnoredChangeLastReconcile.CompareAndSwap(last, nowMillis) { + return true + } } - forwarded := p.currentForwardedSources() - rulesStr := p.buildPFAnchorRulesWith(vpnExemptions, forwarded) - if err := writePFAnchorFile(rulesStr); err != nil { - mainLog.Load().Error().Err(err).Msg("DNS intercept watchdog: failed to write anchor file") - } else if out, err := exec.Command("pfctl", "-a", pfAnchorName, "-f", pfAnchorFile).CombinedOutput(); err != nil { - p.pfBackoffResourceExhaustion(err, out, "load rebuilt anchor") - mainLog.Load().Error().Err(err).Msgf("DNS intercept watchdog: failed to load rebuilt anchor (output: %s)", strings.TrimSpace(string(out))) - } else { - p.recordAppliedForwardedSources(forwarded) - flushPFStates() - mainLog.Load().Info().Msg("DNS intercept watchdog: rebuilt and loaded anchor rules") - } - - // Update tracked tunnel interfaces after rebuild so checkTunnelInterfaceChanges() - // has an accurate baseline for subsequent comparisons. - p.mu.Lock() - p.lastTunnelIfaces = discoverTunnelInterfaces() - p.mu.Unlock() - - // Verify the restoration worked. - p.verifyPFState() - - // Proactively reset upstream transports. When another program replaces the pf - // ruleset with "pfctl -f", it flushes the entire state table — killing all - // existing TCP connections including our DoH connections to upstream DNS servers. - // Without this reset, Go's http.Transport keeps trying dead connections until - // the 5s context deadline, causing a DNS blackout. Re-bootstrapping forces fresh - // TLS handshakes on the next query (~200ms vs ~5s recovery). - p.resetUpstreamTransports() - - p.pfLastRestoreTime.Store(time.Now().UnixMilli()) - mainLog.Load().Info().Msg("DNS intercept watchdog: pf anchor restored successfully") - return true } func (p *prog) pfExecBackoffActive() bool { @@ -1611,18 +1729,22 @@ func (p *prog) pfWatchdog() { return } - restored := p.ensurePFAnchorActive() - if !restored { - // Rules are intact in text form — also probe actual interception. - if !p.pfStabilizing.Load() && !p.pfMonitorRunning.Load() { - if !p.probePFIntercept() { - mainLog.Load().Warn().Msg("DNS intercept watchdog: rules intact but probe FAILED — forcing full reload") - p.forceReloadPFMainRuleset() - restored = true + result := p.ensurePFAnchorActive() + if result == pfAnchorCheckIntact { + // Only an authoritative intact result may trigger the functional probe. + // Skipped/backoff/failed checks must not be treated as healthy text state. + if !p.pfMonitorRunning.Load() && !p.probePFIntercept() { + mainLog.Load().Warn().Msg("DNS intercept watchdog: rules intact but probe FAILED — forcing full reload") + if p.forceReloadPFMainRuleset() { + result = pfAnchorCheckRestored + } else { + result = pfAnchorCheckFailed } } + } - // Check if backoff should be reset. + if result == pfAnchorCheckIntact { + // Check if backoff should be reset only after an authoritative healthy check. if p.pfBackoffMultiplier.Load() > 0 && p.pfLastRestoreTime.Load() > 0 { elapsed := time.Since(time.UnixMilli(p.pfLastRestoreTime.Load())) if elapsed > 60*time.Second { @@ -1631,17 +1753,37 @@ func (p *prog) pfWatchdog() { } } } - if restored { + + switch result { + case pfAnchorCheckRestored: misses := consecutiveMisses.Add(1) if misses >= pfConsecutiveMissThreshold { mainLog.Load().Error().Msgf("DNS intercept watchdog: pf anchor has been missing for %d consecutive checks — something is persistently overwriting pf rules", misses) } else { mainLog.Load().Warn().Msgf("DNS intercept watchdog: pf anchor was missing and restored (consecutive misses: %d)", misses) } - } else { + case pfAnchorCheckIntact: if old := consecutiveMisses.Swap(0); old > 0 { mainLog.Load().Info().Msgf("DNS intercept watchdog: pf anchor stable again after %d consecutive restores", old) } + case pfAnchorCheckSkipped, pfAnchorCheckDeferred, pfAnchorCheckFailed: + // Preserve the previous authoritative state. Do not probe, count a + // repair, or reset misses from an indeterminate/deferred result. + } + + // Tunnel transitions and failed PF/WFP exemption updates are normally + // event-driven, but the final delayed attempt must remain retryable even + // if macOS emits no further event. + tunnelChanged := p.checkTunnelInterfaceChanges() + pendingExemptions := p.vpnDNS != nil && p.vpnDNS.interceptExemptionsPending() + // Stabilization owns pf while a VPN's ruleset is still settling, and a + // refresh rebuilds and reloads the anchor - exactly the mutual overwriting + // that stabilization exists to stop. Nothing is lost by deferring: the + // pending tunnel state and pending exemptions both survive, and the next + // tick retries once stabilization has finished. + if (tunnelChanged || pendingExemptions) && p.vpnDNS != nil && + !p.pfExecBackoffActive() && !p.pfStabilizing.Load() { + p.vpnDNS.Refresh(ctrld.LoggerCtx(context.Background(), p.logger.Load()), true) } // VM/container networks appear and disappear without always @@ -1663,9 +1805,34 @@ func (p *prog) exemptVPNDNSServers(exemptions []vpnDNSExemption) error { if p.dnsInterceptState == nil { return fmt.Errorf("pf state not available") } + if p.pfExecBackoffActive() { + return fmt.Errorf("pf exemption update deferred during exec backoff") + } + // Defence in depth for the callers guarded at their own call sites: this function + // rewrites and reloads the whole anchor, which is the mutual overwriting that + // stabilization suppresses while a VPN's ruleset settles. + // + // This does defer handleRecovery's DHCP-nameserver exemption, which has no retry of + // its own - the trade is deliberate. Those exemptions are ad hoc: they are never + // registered with the vpnDNSManager, so the post-stabilization reconcile rebuilds + // the anchor from CurrentExemptions and drops them regardless. Letting them through + // mid-settle would buy a few seconds of captive-portal access at the cost of the + // collision stabilization exists to prevent. The error text surfaces in recovery's + // warning so the reason is visible rather than silent. + // + // Manager-driven callers lose nothing: appliedExemptions only advances on success, + // so interceptExemptionsPending stays true and the next tick retries. + if p.pfStabilizing.Load() { + return fmt.Errorf("pf exemption update deferred during stabilization") + } + if !p.pfEnsureRunning.CompareAndSwap(false, true) { + return fmt.Errorf("pf reconciliation already running") + } + defer p.pfEnsureRunning.Store(false) forwarded := p.currentForwardedSources() - rulesStr := p.buildPFAnchorRulesWith(exemptions, forwarded) + tunnelIfaces := append([]string(nil), discoverTunnelInterfacesForReconcile()...) + rulesStr := p.buildPFAnchorRulesForState(exemptions, tunnelIfaces, forwarded) if err := writePFAnchorFile(rulesStr); err != nil { return fmt.Errorf("dns intercept: failed to rewrite pf anchor: %w", err) @@ -1673,6 +1840,7 @@ func (p *prog) exemptVPNDNSServers(exemptions []vpnDNSExemption) error { out, err := exec.Command("pfctl", "-a", pfAnchorName, "-f", pfAnchorFile).CombinedOutput() if err != nil { + p.pfBackoffResourceExhaustion(err, out, "load VPN DNS anchor") return fmt.Errorf("dns intercept: failed to reload pf anchor: %w (output: %s)", err, strings.TrimSpace(string(out))) } p.recordAppliedForwardedSources(forwarded) @@ -1683,8 +1851,12 @@ func (p *prog) exemptVPNDNSServers(exemptions []vpnDNSExemption) error { // Ensure the anchor reference still exists in the main ruleset. // Another program may have replaced the ruleset since we last checked. if err := p.ensurePFAnchorReference(); err != nil { - mainLog.Load().Warn().Err(err).Msg("DNS intercept: failed to verify anchor reference during VPN DNS update") + p.pfBackoffResourceExhaustion(err, nil, "restore VPN DNS anchor reference") + p.resetUpstreamTransports() + return fmt.Errorf("dns intercept: failed to restore anchor reference during VPN DNS update: %w", err) } + p.resetUpstreamTransports() + p.commitPFReconcileState(tunnelIfaces) mainLog.Load().Info().Msgf("DNS intercept: updated pf rules — exempted %d VPN DNS + %d OS resolver servers", len(exemptions), len(ctrld.OsResolverNameservers())) @@ -1848,7 +2020,15 @@ func (p *prog) pfInterceptMonitor() { // // The reload is safe for VPN interop because it reassembles from the current running // ruleset (pfctl -sr/-sn), preserving all existing anchors and rules. -func (p *prog) forceReloadPFMainRuleset() { +func (p *prog) forceReloadPFMainRuleset() bool { + if p.dnsInterceptState == nil || p.pfExecBackoffActive() { + return false + } + if !p.pfEnsureRunning.CompareAndSwap(false, true) { + return false + } + defer p.pfEnsureRunning.Store(false) + rdrAnchorRef := fmt.Sprintf("rdr-anchor \"%s\"", pfAnchorName) anchorRef := fmt.Sprintf("anchor \"%s\"", pfAnchorName) @@ -1856,13 +2036,13 @@ func (p *prog) forceReloadPFMainRuleset() { natOut, err := exec.Command("pfctl", "-sn").CombinedOutput() if err != nil { mainLog.Load().Error().Err(err).Msg("DNS intercept: force reload — failed to dump NAT rules") - return + return false } filterOut, err := exec.Command("pfctl", "-sr").CombinedOutput() if err != nil { mainLog.Load().Error().Err(err).Msg("DNS intercept: force reload — failed to dump filter rules") - return + return false } natLines := pfFilterRuleLines(string(natOut)) @@ -1886,14 +2066,10 @@ func (p *prog) forceReloadPFMainRuleset() { pureFilterLines = append([]string{anchorRef}, pureFilterLines...) } - // Clean pf options (remove "set skip on lo0" if present). - cleanedOptions, _ := pfGetCleanedOptions() - - // Reassemble in pf's required order: options → scrub → translation → filtering. + // Reassemble in pf's required order: scrub → translation → filtering. As in + // ensurePFAnchorReference, no options section is emitted; see the KNOWN GAP note + // there. var combined strings.Builder - if cleanedOptions != "" { - combined.WriteString(cleanedOptions) - } for _, line := range scrubLines { combined.WriteString(line + "\n") } @@ -1909,35 +2085,19 @@ func (p *prog) forceReloadPFMainRuleset() { out, err := cmd.CombinedOutput() if err != nil { mainLog.Load().Error().Err(err).Msgf("DNS intercept: force reload — pfctl -f - failed (output: %s)", strings.TrimSpace(string(out))) - return + return false } - - // Also reload the anchor rules to ensure they're fresh. - var vpnExemptions []vpnDNSExemption - if p.vpnDNS != nil { - vpnExemptions = p.vpnDNS.CurrentExemptions() - } - forwarded := p.currentForwardedSources() - rulesStr := p.buildPFAnchorRulesWith(vpnExemptions, forwarded) - if err := writePFAnchorFile(rulesStr); err != nil { - mainLog.Load().Error().Err(err).Msg("DNS intercept: force reload — failed to write anchor file") - } else if out, err := exec.Command("pfctl", "-a", pfAnchorName, "-f", pfAnchorFile).CombinedOutput(); err != nil { - mainLog.Load().Error().Err(err).Msgf("DNS intercept: force reload — failed to load anchor (output: %s)", strings.TrimSpace(string(out))) - } else { - p.recordAppliedForwardedSources(forwarded) - } - - // 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. + // A successful main ruleset reload invalidates PF state even if the + // subsequent anchor rebuild fails, so retire stale DoH transports now. p.resetUpstreamTransports() + if result := p.restorePFAnchorWithTransportReset("forced main ruleset reload", false); result != pfAnchorCheckRestored { + mainLog.Load().Error().Msg("DNS intercept: force reload — anchor rebuild failed") + return false + } + mainLog.Load().Info().Msg("DNS intercept: force reload — pf ruleset and anchor reloaded successfully") + return true } // osHealthcheckSuppressed always returns false on darwin — WFP loopback diff --git a/cmd/cli/dns_intercept_darwin_test.go b/cmd/cli/dns_intercept_darwin_test.go index 3ab810a..1729275 100644 --- a/cmd/cli/dns_intercept_darwin_test.go +++ b/cmd/cli/dns_intercept_darwin_test.go @@ -3,9 +3,16 @@ package cli import ( + "context" "errors" + "fmt" + "os" + "path/filepath" "strings" "testing" + "time" + + "tailscale.com/net/netmon" "github.com/Control-D-Inc/ctrld" ) @@ -215,3 +222,609 @@ func TestIsResourceExhaustion(t *testing.T) { }) } } + +func stubPFAnchorCheckCommand(t *testing.T, outputs map[string]string) { + t.Helper() + original := runPFAnchorCheckCommand + runPFAnchorCheckCommand = func(args ...string) ([]byte, error) { + key := strings.Join(args, " ") + output, ok := outputs[key] + if !ok { + t.Fatalf("unexpected pf anchor check command: pfctl %s", key) + } + return []byte(output), nil + } + t.Cleanup(func() { + runPFAnchorCheckCommand = original + }) +} + +func TestEnsurePFAnchorActiveRecentRestoreWithIntactRulesDoesNotStabilize(t *testing.T) { + stubPFAnchorCheckCommand(t, map[string]string{ + "-sn": `rdr-anchor "com.controld.ctrld"`, + "-sr": `anchor "com.controld.ctrld"`, + "-a com.controld.ctrld -sr": "pass in quick on lo0", + "-a com.controld.ctrld -sn": "rdr on lo0", + }) + + p := &prog{ + dnsInterceptState: &pfState{}, + stopCh: make(chan struct{}), + } + restoredAt := time.Now().Add(-time.Second).UnixMilli() + p.pfLastRestoreTime.Store(restoredAt) + + if result := p.ensurePFAnchorActive(); result != pfAnchorCheckIntact { + t.Fatalf("intact rules result = %v, want intact", result) + } + if p.pfBackoffMultiplier.Load() != 0 { + t.Fatalf("intact rules incremented backoff to %d", p.pfBackoffMultiplier.Load()) + } + if p.pfStabilizing.Load() { + t.Fatal("intact rules must not enter stabilization") + } + if got := p.pfLastRestoreTime.Load(); got != restoredAt { + t.Fatalf("intact check changed restore timestamp: got %d, want %d", got, restoredAt) + } +} + +func TestEnsurePFAnchorActiveCheckFailureIsNotIntact(t *testing.T) { + original := runPFAnchorCheckCommand + runPFAnchorCheckCommand = func(...string) ([]byte, error) { + return nil, errors.New("pfctl unavailable") + } + t.Cleanup(func() { runPFAnchorCheckCommand = original }) + + p := &prog{dnsInterceptState: &pfState{}} + if result := p.ensurePFAnchorActive(); result != pfAnchorCheckFailed { + t.Fatalf("failed PF inspection result = %v, want failed", result) + } +} + +func TestEnsurePFAnchorActiveRecentActualWipeStartsStabilization(t *testing.T) { + stubPFAnchorCheckCommand(t, map[string]string{ + "-sn": "", + }) + + stopCh := make(chan struct{}) + close(stopCh) + p := &prog{ + dnsInterceptState: &pfState{}, + stopCh: stopCh, + } + restoredAt := time.Now().Add(-time.Second).UnixMilli() + p.pfLastRestoreTime.Store(restoredAt) + + if result := p.ensurePFAnchorActive(); result != pfAnchorCheckDeferred { + t.Fatalf("recent repeated wipe result = %v, want deferred", result) + } + if got := p.pfBackoffMultiplier.Load(); got != 1 { + t.Fatalf("recent repeated wipe backoff = %d, want 1", got) + } + if got := p.pfLastRestoreTime.Load(); got != restoredAt { + t.Fatalf("deferred restore changed restore timestamp: got %d, want %d", got, restoredAt) + } + deadline := time.Now().Add(time.Second) + for p.pfStabilizing.Load() && time.Now().Before(deadline) { + time.Sleep(time.Millisecond) + } + if p.pfStabilizing.Load() { + t.Fatal("stabilization goroutine did not observe closed stop channel") + } +} + +func TestDNSInterceptIgnoredChangeReconcileDue(t *testing.T) { + p := &prog{} + start := time.Unix(1_000_000, 0) + + if !p.dnsInterceptIgnoredChangeReconcileDue(start) { + t.Fatal("first ignored change must reconcile immediately") + } + if p.dnsInterceptIgnoredChangeReconcileDue(start.Add(pfIgnoredChangeReconcileInterval - time.Millisecond)) { + t.Fatal("ignored changes inside the interval must be coalesced") + } + if !p.dnsInterceptIgnoredChangeReconcileDue(start.Add(pfIgnoredChangeReconcileInterval)) { + t.Fatal("continuous ignored changes must reconcile again at the interval boundary") + } +} + +func TestIgnoredNetworkChangeCallbackBoundsWorkWithoutBurningStabilizedSlot(t *testing.T) { + outputs := map[string]string{ + "-sn": `rdr-anchor "com.controld.ctrld"`, + "-sr": `anchor "com.controld.ctrld"`, + "-a com.controld.ctrld -sr": "pass in quick on lo0", + "-a com.controld.ctrld -sn": "rdr on lo0", + } + originalCheck := runPFAnchorCheckCommand + pfChecks := 0 + runPFAnchorCheckCommand = func(args ...string) ([]byte, error) { + key := strings.Join(args, " ") + output, ok := outputs[key] + if !ok { + t.Fatalf("unexpected pf anchor check command: pfctl %s", key) + } + if key == "-sn" { + pfChecks++ + } + return []byte(output), nil + } + originalDiscover := discoverTunnelInterfacesForReconcile + discoverTunnelInterfacesForReconcile = func() []string { return nil } + t.Cleanup(func() { + runPFAnchorCheckCommand = originalCheck + discoverTunnelInterfacesForReconcile = originalDiscover + }) + + refreshes := 0 + vpnDNS := newVPNDNSManager(&mainLog, nil) + vpnDNS.discoverVPNDNS = func(context.Context) []ctrld.VPNDNSConfig { + refreshes++ + return nil + } + p := &prog{dnsInterceptState: &pfState{}, vpnDNS: vpnDNS} + t.Cleanup(func() { + p.pfDelayedRecheckMu.Lock() + defer p.pfDelayedRecheckMu.Unlock() + for _, timer := range p.pfDelayedRecheckTimers { + if timer != nil { + timer.Stop() + } + } + }) + + delta := &netmon.ChangeDelta{ + Old: &netmon.State{Interface: map[string]netmon.Interface{}}, + New: &netmon.State{Interface: map[string]netmon.Interface{}}, + } + start := time.Unix(1_000_000, 0) + p.handleDNSInterceptIgnoredNetworkChange(delta, start) + if pfChecks != 1 || refreshes != 1 { + t.Fatalf("first ignored delta work: pf checks=%d refreshes=%d, want 1 each", pfChecks, refreshes) + } + + p.pfStabilizing.Store(true) + p.handleDNSInterceptIgnoredNetworkChange(delta, start.Add(pfIgnoredChangeReconcileInterval)) + if pfChecks != 1 || refreshes != 1 { + t.Fatalf("stabilized delta ran leading reconciliation: pf checks=%d refreshes=%d", pfChecks, refreshes) + } + + p.pfStabilizing.Store(false) + resumeAt := start.Add(pfIgnoredChangeReconcileInterval + time.Millisecond) + p.handleDNSInterceptIgnoredNetworkChange(delta, resumeAt) + if pfChecks != 2 || refreshes != 2 { + t.Fatalf("first post-stabilization delta did not reconcile immediately: pf checks=%d refreshes=%d", pfChecks, refreshes) + } + + for i := 1; i <= 8; i++ { + p.handleDNSInterceptIgnoredNetworkChange(delta, resumeAt.Add(time.Duration(i)*100*time.Millisecond)) + } + if pfChecks != 2 || refreshes != 2 { + t.Fatalf("ignored delta burst was not coalesced: pf checks=%d refreshes=%d", pfChecks, refreshes) + } + + p.handleDNSInterceptIgnoredNetworkChange(delta, resumeAt.Add(pfIgnoredChangeReconcileInterval)) + if pfChecks != 3 || refreshes != 3 { + t.Fatalf("interval boundary did not reconcile: pf checks=%d refreshes=%d, want 3 each", pfChecks, refreshes) + } +} + +func TestRestorePFAnchorFailureIsNotReportedOrTimestamped(t *testing.T) { + originalReference := ensurePFAnchorReferenceForRestore + originalRebuild := rebuildPFAnchorRulesForReconcile + ensurePFAnchorReferenceForRestore = func(*prog) error { return nil } + rebuildPFAnchorRulesForReconcile = func(*prog, []vpnDNSExemption) ([]string, error) { + return nil, errors.New("pf load failed") + } + t.Cleanup(func() { + ensurePFAnchorReferenceForRestore = originalReference + rebuildPFAnchorRulesForReconcile = originalRebuild + }) + + p := &prog{dnsInterceptState: &pfState{}} + if result := p.restorePFAnchor("test"); result != pfAnchorCheckFailed { + t.Fatalf("failed restore result = %v, want failed", result) + } + if got := p.pfLastRestoreTime.Load(); got != 0 { + t.Fatalf("failed restore changed timestamp to %d", got) + } + if len(p.lastTunnelIfaces) != 0 { + t.Fatalf("failed restore committed tunnel state: %v", p.lastTunnelIfaces) + } +} + +func TestPFStabilizationTimeoutReturnsOwnershipToDelayedRecovery(t *testing.T) { + p := &prog{dnsInterceptState: &pfState{}} + p.pfStabilizing.Store(true) + p.pfStabilizationLoopWithMaxWait(t.Context(), time.Hour, 25*time.Millisecond) + + if p.pfStabilizing.Load() { + t.Fatal("stabilization retained ownership after the maximum wait") + } + p.pfDelayedRecheckMu.Lock() + timers := append([]*time.Timer(nil), p.pfDelayedRecheckTimers...) + p.pfDelayedRecheckTimers = nil + p.pfDelayedRecheckMu.Unlock() + if len(timers) != 2 { + t.Fatalf("expected bounded timeout to schedule delayed recovery, got %d timers", len(timers)) + } + for _, timer := range timers { + timer.Stop() + } +} + +func TestStopDNSInterceptWaitsForInFlightPFMutation(t *testing.T) { + binDir := t.TempDir() + pfctlPath := filepath.Join(binDir, "pfctl") + if err := os.WriteFile(pfctlPath, []byte("#!/bin/sh\nexit 0\n"), 0755); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", binDir+":"+os.Getenv("PATH")) + + anchorFile := filepath.Join(t.TempDir(), "anchor") + if err := os.WriteFile(anchorFile, []byte("rules"), 0600); err != nil { + t.Fatal(err) + } + p := &prog{dnsInterceptState: &pfState{anchorName: pfAnchorName, anchorFile: anchorFile}} + p.pfEnsureRunning.Store(true) + + revoked := make(chan struct{}) + originalRevokedHook := pfShutdownStateRevokedForTest + pfShutdownStateRevokedForTest = func() { close(revoked) } + t.Cleanup(func() { pfShutdownStateRevokedForTest = originalRevokedHook }) + + done := make(chan error, 1) + go func() { done <- p.stopDNSIntercept() }() + + select { + case <-revoked: + case <-time.After(time.Second): + t.Fatal("shutdown did not revoke PF lifecycle state before waiting") + } + select { + case err := <-done: + t.Fatalf("shutdown completed before in-flight PF owner released: %v", err) + case <-time.After(25 * time.Millisecond): + } + + p.pfEnsureRunning.Store(false) + if err := <-done; err != nil { + t.Fatalf("stopDNSIntercept() error: %v", err) + } + if _, err := os.Stat(anchorFile); !os.IsNotExist(err) { + t.Fatalf("anchor file remained after serialized shutdown: %v", err) + } +} + +func TestPostStabilizationReconcileRetainsOwnershipAndForcesRebuild(t *testing.T) { + stubPFAnchorCheckCommand(t, map[string]string{ + "-sn": `rdr-anchor "com.controld.ctrld"`, + "-sr": `anchor "com.controld.ctrld"`, + "-a com.controld.ctrld -sr": "pass in quick on lo0", + "-a com.controld.ctrld -sn": "rdr on lo0", + }) + originalRestore := restorePFAnchorForReconcile + calls := 0 + restorePFAnchorForReconcile = func(*prog, string) pfAnchorCheckResult { + calls++ + return pfAnchorCheckRestored + } + t.Cleanup(func() { restorePFAnchorForReconcile = originalRestore }) + + p := &prog{ + dnsInterceptState: &pfState{}, + pendingTunnelIfaces: []string{"utun9"}, + hasPendingTunnelIfaces: true, + } + p.pfStabilizing.Store(true) + if result := p.reconcilePFAnchorAfterStabilization(); result != pfAnchorCheckRestored { + t.Fatalf("post-stabilization result = %v, want restored", result) + } + if calls != 1 { + t.Fatalf("post-stabilization restore calls = %d, want 1", calls) + } + if !p.pfStabilizing.Load() { + t.Fatal("post-stabilization reconcile released loop ownership") + } + if p.pfBackoffMultiplier.Load() != 0 { + t.Fatalf("post-stabilization reconcile changed backoff to %d", p.pfBackoffMultiplier.Load()) + } +} + +func TestPostStabilizationIntactWithoutPendingAvoidsRebuild(t *testing.T) { + stubPFAnchorCheckCommand(t, map[string]string{ + "-sn": `rdr-anchor "com.controld.ctrld"`, + "-sr": `anchor "com.controld.ctrld"`, + "-a com.controld.ctrld -sr": "pass in quick on lo0", + "-a com.controld.ctrld -sn": "rdr on lo0", + }) + originalRestore := restorePFAnchorForReconcile + calls := 0 + restorePFAnchorForReconcile = func(*prog, string) pfAnchorCheckResult { + calls++ + return pfAnchorCheckRestored + } + t.Cleanup(func() { restorePFAnchorForReconcile = originalRestore }) + + p := &prog{dnsInterceptState: &pfState{}} + p.pfStabilizing.Store(true) + if result := p.reconcilePFAnchorAfterStabilization(); result != pfAnchorCheckIntact { + t.Fatalf("post-stabilization result = %v, want intact", result) + } + if calls != 0 { + t.Fatalf("intact post-stabilization anchor rebuilt %d times", calls) + } + if !p.pfStabilizing.Load() { + t.Fatal("intact post-stabilization reconcile released loop ownership") + } +} + +func TestTunnelRemovalFailureRetriesBeforeCommittingBaseline(t *testing.T) { + originalDiscover := discoverTunnelInterfacesForReconcile + originalRestore := restorePFAnchorForReconcile + current := []string{} + discoverTunnelInterfacesForReconcile = func() []string { + return append([]string(nil), current...) + } + calls := 0 + restorePFAnchorForReconcile = func(p *prog, _ string) pfAnchorCheckResult { + calls++ + if calls == 1 { + return pfAnchorCheckFailed + } + p.commitPFReconcileState(current) + return pfAnchorCheckRestored + } + t.Cleanup(func() { + discoverTunnelInterfacesForReconcile = originalDiscover + restorePFAnchorForReconcile = originalRestore + }) + + p := &prog{ + dnsInterceptState: &pfState{}, + lastTunnelIfaces: []string{"utun7"}, + } + if !p.checkTunnelInterfaceChanges() { + t.Fatal("first tunnel removal was not detected") + } + if !stringSlicesEqual(p.lastTunnelIfaces, []string{"utun7"}) { + t.Fatalf("failed removal committed baseline: %v", p.lastTunnelIfaces) + } + if !p.hasPendingTunnelReconcile() { + t.Fatal("failed removal did not retain desired tunnel state for retry") + } + if !p.checkTunnelInterfaceChanges() { + t.Fatal("failed tunnel removal was not retried") + } + if len(p.lastTunnelIfaces) != 0 { + t.Fatalf("successful retry did not commit empty tunnel baseline: %v", p.lastTunnelIfaces) + } + if calls != 2 { + t.Fatalf("restore calls = %d, want 2", calls) + } +} + +func TestPendingTunnelStateRetriesAfterStabilization(t *testing.T) { + originalDiscover := discoverTunnelInterfacesForReconcile + originalRestore := restorePFAnchorForReconcile + current := []string{} + discoverTunnelInterfacesForReconcile = func() []string { return nil } + calls := 0 + restorePFAnchorForReconcile = func(p *prog, _ string) pfAnchorCheckResult { + calls++ + p.commitPFReconcileState(current) + return pfAnchorCheckRestored + } + t.Cleanup(func() { + discoverTunnelInterfacesForReconcile = originalDiscover + restorePFAnchorForReconcile = originalRestore + }) + + p := &prog{ + dnsInterceptState: &pfState{}, + lastTunnelIfaces: []string{"utun7"}, + pendingTunnelIfaces: current, + hasPendingTunnelIfaces: true, + } + if !p.checkTunnelInterfaceChanges() { + t.Fatal("pending tunnel removal was not retried after stabilization") + } + if calls != 1 || len(p.lastTunnelIfaces) != 0 || p.hasPendingTunnelReconcile() { + t.Fatalf("pending retry result: calls=%d baseline=%v pending=%v", calls, p.lastTunnelIfaces, p.hasPendingTunnelReconcile()) + } +} + +func TestTunnelReconcileHonorsPFExecBackoff(t *testing.T) { + originalDiscover := discoverTunnelInterfacesForReconcile + originalRestore := restorePFAnchorForReconcile + current := []string{} + discoverTunnelInterfacesForReconcile = func() []string { return nil } + calls := 0 + restorePFAnchorForReconcile = func(p *prog, _ string) pfAnchorCheckResult { + calls++ + p.commitPFReconcileState(current) + return pfAnchorCheckRestored + } + t.Cleanup(func() { + discoverTunnelInterfacesForReconcile = originalDiscover + restorePFAnchorForReconcile = originalRestore + }) + + p := &prog{dnsInterceptState: &pfState{}, lastTunnelIfaces: []string{"utun7"}} + p.pfExecBackoffUntil.Store(time.Now().Add(time.Minute).UnixMilli()) + if !p.checkTunnelInterfaceChanges() { + t.Fatal("tunnel removal was not detected during PF exec backoff") + } + if calls != 0 || !stringSlicesEqual(p.lastTunnelIfaces, []string{"utun7"}) { + t.Fatalf("PF restore ran during exec backoff: calls=%d baseline=%v", calls, p.lastTunnelIfaces) + } + if p.checkTunnelInterfaceChanges() { + t.Fatal("identical deferred tunnel retry bypassed the ignored-event limiter") + } + p.pfExecBackoffUntil.Store(0) + if !p.checkTunnelInterfaceChanges() || calls != 1 || len(p.lastTunnelIfaces) != 0 { + t.Fatalf("tunnel removal did not retry after backoff: calls=%d baseline=%v", calls, p.lastTunnelIfaces) + } +} + +func TestTunnelRapidReversalClearsUnappliedPendingState(t *testing.T) { + originalDiscover := discoverTunnelInterfacesForReconcile + discoverTunnelInterfacesForReconcile = func() []string { return nil } + t.Cleanup(func() { discoverTunnelInterfacesForReconcile = originalDiscover }) + + p := &prog{ + dnsInterceptState: &pfState{}, + pendingTunnelIfaces: []string{"utun9"}, + hasPendingTunnelIfaces: true, + } + p.pfStabilizing.Store(true) + if !p.checkTunnelInterfaceChanges() { + t.Fatal("rapid tunnel reversal was not observed") + } + if p.hasPendingTunnelReconcile() || len(p.lastTunnelIfaces) != 0 { + t.Fatalf("rapid reversal left unapplied tunnel state: baseline=%v pending=%v", p.lastTunnelIfaces, p.hasPendingTunnelReconcile()) + } +} + +func TestTunnelAdditionIsCoalescedUntilSuccessfulRebuild(t *testing.T) { + originalDiscover := discoverTunnelInterfacesForReconcile + current := []string{"utun9"} + discoverTunnelInterfacesForReconcile = func() []string { + return append([]string(nil), current...) + } + t.Cleanup(func() { discoverTunnelInterfacesForReconcile = originalDiscover }) + + p := &prog{dnsInterceptState: &pfState{}} + p.pfStabilizing.Store(true) + if !p.checkTunnelInterfaceChanges() { + t.Fatal("new tunnel was not detected") + } + if p.checkTunnelInterfaceChanges() { + t.Fatal("identical pending tunnel state was not coalesced") + } + if len(p.lastTunnelIfaces) != 0 { + t.Fatalf("pending tunnel was committed before PF rebuild: %v", p.lastTunnelIfaces) + } + if !p.hasPendingTunnelReconcile() { + t.Fatal("new tunnel was not retained as pending") + } + + p.commitPFReconcileState(current) + if !stringSlicesEqual(p.lastTunnelIfaces, current) { + t.Fatalf("successful rebuild baseline = %v, want %v", p.lastTunnelIfaces, current) + } + if p.hasPendingTunnelReconcile() { + t.Fatal("successful rebuild did not clear pending tunnel state") + } +} + +// TestVPNDNSRefreshDeferredWhileStabilizing covers the ignored network-change path, +// which can trigger a VPN DNS refresh from outside stabilization. +// +// A refresh rebuilds and reloads the pf anchor. Stabilization owns pf while a VPN's +// ruleset is still settling, so refreshing then is the mutual-overwrite collision +// stabilization exists to prevent - and these deltas arrive exactly when a VPN is +// coming up. Deferring is safe: checkTunnelInterfaceChanges keeps the observation +// pending, so the transition is retried afterwards. +// +// The watchdog tick carries the same guard for the same reason; it is not driven here +// because that would mean running its 30s loop. +func TestVPNDNSRefreshDeferredWhileStabilizing(t *testing.T) { + newProg := func(t *testing.T, refreshes *int, tunnels []string) *prog { + t.Helper() + outputs := map[string]string{ + "-sn": `rdr-anchor "com.controld.ctrld"`, + "-sr": `anchor "com.controld.ctrld"`, + "-a com.controld.ctrld -sr": "pass in quick on lo0", + "-a com.controld.ctrld -sn": "rdr on lo0", + } + originalCheck := runPFAnchorCheckCommand + runPFAnchorCheckCommand = func(args ...string) ([]byte, error) { + output, ok := outputs[strings.Join(args, " ")] + if !ok { + return nil, fmt.Errorf("unexpected pf anchor check command") + } + return []byte(output), nil + } + // Discovery reports no tunnels. With a seeded baseline that is a removal, which + // checkTunnelInterfaceChanges reports as a change without touching pf while + // stabilizing - so this fixture never reaches a real pfctl write. + originalDiscover := discoverTunnelInterfacesForReconcile + discoverTunnelInterfacesForReconcile = func() []string { return nil } + t.Cleanup(func() { + runPFAnchorCheckCommand = originalCheck + discoverTunnelInterfacesForReconcile = originalDiscover + }) + + vpnDNS := newVPNDNSManager(&mainLog, nil) + vpnDNS.discoverVPNDNS = func(context.Context) []ctrld.VPNDNSConfig { + *refreshes++ + return nil + } + p := &prog{dnsInterceptState: &pfState{}, vpnDNS: vpnDNS, lastTunnelIfaces: tunnels} + t.Cleanup(func() { + p.pfDelayedRecheckMu.Lock() + defer p.pfDelayedRecheckMu.Unlock() + for _, timer := range p.pfDelayedRecheckTimers { + if timer != nil { + timer.Stop() + } + } + }) + return p + } + delta := func() *netmon.ChangeDelta { + return &netmon.ChangeDelta{ + Old: &netmon.State{Interface: map[string]netmon.Interface{}}, + New: &netmon.State{Interface: map[string]netmon.Interface{}}, + } + } + + t.Run("tunnel change during stabilization does not refresh", func(t *testing.T) { + refreshes := 0 + // Seeded baseline plus empty discovery = a tunnel transition to report, so the + // refresh is eligible on everything except the stabilization guard. + p := newProg(t, &refreshes, []string{"utun9"}) + p.pfStabilizing.Store(true) + + p.handleDNSInterceptIgnoredNetworkChange(delta(), time.Unix(1_000_000, 0)) + + if refreshes != 0 { + t.Errorf("refreshed %d time(s) while stabilizing — that rebuilds the anchor under a settling VPN ruleset", refreshes) + } + }) + + t.Run("refresh still happens outside stabilization", func(t *testing.T) { + refreshes := 0 + p := newProg(t, &refreshes, nil) + + p.handleDNSInterceptIgnoredNetworkChange(delta(), time.Unix(1_000_000, 0)) + + if refreshes == 0 { + t.Error("no refresh outside stabilization — the guard must defer, not disable") + } + }) +} + +// TestExemptVPNDNSServersDeferredWhileStabilizing checks the mutation point itself, +// not just the call sites: any future caller reaching it during stabilization is +// refused before the anchor is rewritten. +// +// It returns before pfEnsureRunning is taken and before any pfctl work, so this drives +// the real function without touching the host's pf state. +func TestExemptVPNDNSServersDeferredWhileStabilizing(t *testing.T) { + p := &prog{dnsInterceptState: &pfState{}} + p.pfStabilizing.Store(true) + + err := p.exemptVPNDNSServers([]vpnDNSExemption{{Server: "192.168.1.1"}}) + if err == nil { + t.Fatal("exemption applied while stabilizing — that rewrites the anchor under a settling VPN ruleset") + } + if !strings.Contains(err.Error(), "stabilization") { + t.Errorf("error does not name the reason: %v", err) + } + // The refusal must happen before the reconcile latch is claimed, or a deferral + // would lock out the reconcile that runs once stabilization ends. + if p.pfEnsureRunning.Load() { + t.Error("pfEnsureRunning was left held by a deferred exemption") + } +} diff --git a/cmd/cli/dns_intercept_ignored_change_windows_test.go b/cmd/cli/dns_intercept_ignored_change_windows_test.go new file mode 100644 index 0000000..8ea5d5e --- /dev/null +++ b/cmd/cli/dns_intercept_ignored_change_windows_test.go @@ -0,0 +1,20 @@ +//go:build windows + +package cli + +import ( + "testing" + "time" +) + +func TestDNSInterceptIgnoredChangeReconcileDueWindowsPreservesImmediateBehavior(t *testing.T) { + p := &prog{} + now := time.Now() + + if !p.dnsInterceptIgnoredChangeReconcileDue(now) { + t.Fatal("first ignored Windows change must reconcile immediately") + } + if !p.dnsInterceptIgnoredChangeReconcileDue(now) { + t.Fatal("Windows ignored changes must not inherit the macOS pf rate limit") + } +} diff --git a/cmd/cli/dns_intercept_others.go b/cmd/cli/dns_intercept_others.go index 9106d5b..8988353 100644 --- a/cmd/cli/dns_intercept_others.go +++ b/cmd/cli/dns_intercept_others.go @@ -4,6 +4,7 @@ package cli import ( "fmt" + "time" ) // startDNSIntercept is not supported on this platform. @@ -23,8 +24,8 @@ func (p *prog) exemptVPNDNSServers(exemptions []vpnDNSExemption) error { } // ensurePFAnchorActive is a no-op on unsupported platforms. -func (p *prog) ensurePFAnchorActive() bool { - return false +func (p *prog) ensurePFAnchorActive() pfAnchorCheckResult { + return pfAnchorCheckSkipped } // checkTunnelInterfaceChanges is a no-op on unsupported platforms. @@ -32,6 +33,10 @@ func (p *prog) checkTunnelInterfaceChanges() bool { return false } +func (p *prog) dnsInterceptIgnoredChangeReconcileDue(time.Time) bool { + return false +} + // scheduleDelayedRechecks is a no-op on unsupported platforms. func (p *prog) scheduleDelayedRechecks() {} diff --git a/cmd/cli/dns_intercept_settle.go b/cmd/cli/dns_intercept_settle.go index 6d6ffaf..5aa9355 100644 --- a/cmd/cli/dns_intercept_settle.go +++ b/cmd/cli/dns_intercept_settle.go @@ -19,21 +19,9 @@ func (p *prog) refreshDNSAfterVPNSettle(reason string) (routes, domainlessServer 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)) - } + mainLog.Load().Info().Msgf("DNS intercept: post-settle VPN DNS route refresh completed — %d routes, %d domainless servers, %d exemptions", + routes, domainlessServers, exemptions) return routes, domainlessServers, exemptions } diff --git a/cmd/cli/dns_intercept_settle_test.go b/cmd/cli/dns_intercept_settle_test.go index b6c80be..6a49dc7 100644 --- a/cmd/cli/dns_intercept_settle_test.go +++ b/cmd/cli/dns_intercept_settle_test.go @@ -43,7 +43,12 @@ func TestRefreshDNSAfterVPNSettleRefreshesOSResolverAndVPNRoutes(t *testing.T) { 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) + if len(exemptionUpdates) != 1 || len(exemptionUpdates[0]) != 1 || exemptionUpdates[0][0].Server != "10.102.26.10" { + t.Fatalf("expected one serialized pf exemption update for the late VPN DNS server, got %+v", exemptionUpdates) + } + + p.refreshDNSAfterVPNSettle("test-repeat") + if len(exemptionUpdates) != 1 { + t.Fatalf("unchanged post-settle VPN DNS state rewrote pf: %+v", exemptionUpdates) } } diff --git a/cmd/cli/dns_intercept_windows.go b/cmd/cli/dns_intercept_windows.go index 3471ef4..fe13e60 100644 --- a/cmd/cli/dns_intercept_windows.go +++ b/cmd/cli/dns_intercept_windows.go @@ -1616,8 +1616,8 @@ func parseIPv4AsUint32(ipStr string) uint32 { } // ensurePFAnchorActive is a no-op on Windows (WFP handles intercept differently). -func (p *prog) ensurePFAnchorActive() bool { - return false +func (p *prog) ensurePFAnchorActive() pfAnchorCheckResult { + return pfAnchorCheckSkipped } // checkTunnelInterfaceChanges is a no-op on Windows (WFP handles intercept differently). @@ -1625,6 +1625,12 @@ func (p *prog) checkTunnelInterfaceChanges() bool { return false } +// Windows preserves the existing immediate reconciliation behavior. NRPT/WFP +// and adapter DNS settling have different lifecycle requirements from macOS pf. +func (p *prog) dnsInterceptIgnoredChangeReconcileDue(time.Time) bool { + return true +} + // pfAnchorRecheckDelay is the delay for deferred pf anchor re-checks. // Defined here as a stub for Windows (referenced from dns_proxy.go). const pfAnchorRecheckDelay = 2 * time.Second diff --git a/cmd/cli/dns_proxy.go b/cmd/cli/dns_proxy.go index cccf039..8ecfdcd 100644 --- a/cmd/cli/dns_proxy.go +++ b/cmd/cli/dns_proxy.go @@ -1754,65 +1754,13 @@ func (p *prog) monitorNetworkChanges(ctx context.Context) error { p.Debug().Msg("Ignoring interface change - no valid interfaces affected") // check if the default IPs are still on an interface that is up ValidateDefaultLocalIPsFromDelta(delta.New) - // Even minor interface changes can trigger macOS pf reloads — verify anchor. - // We check immediately AND schedule delayed re-checks (2s + 4s) to catch - // programs like Windscribe that modify pf rules and DNS settings - // asynchronously after the network change event fires. + // Minor interface changes can still accompany pf/WFP or VPN DNS changes. + // On macOS, bound the immediate full reconciliation so link-local-only + // notification storms do not run pfctl/scutil work for every event. + // Windows keeps the existing immediate behavior. Tunnel changes always + // bypass the macOS limit, and delayed checks provide a trailing refresh. if dnsIntercept && p.dnsInterceptState != nil { - if !p.pfStabilizing.Load() { - p.ensurePFAnchorActive() - } - // Check tunnel interfaces unconditionally — it decides internally - // whether to enter stabilization or rebuild immediately. - p.checkTunnelInterfaceChanges() - // Schedule delayed re-checks to catch async VPN teardown changes. - // These also refresh the OS resolver and VPN DNS routes. - p.scheduleDelayedRechecks() - - // Detect interface appearance/disappearance — hypervisors (Parallels, - // VMware, VirtualBox) reload pf when creating/destroying virtual network - // interfaces, which can corrupt pf's internal translation state. - if delta.Old != nil { - interfaceChanged := false - var changedIface string - for ifaceName := range delta.Old.Interface { - if ifaceName == "lo0" { - continue - } - if _, exists := delta.New.Interface[ifaceName]; !exists { - interfaceChanged = true - changedIface = ifaceName - break - } - } - if !interfaceChanged { - for ifaceName := range delta.New.Interface { - if ifaceName == "lo0" { - continue - } - if _, exists := delta.Old.Interface[ifaceName]; !exists { - interfaceChanged = true - changedIface = ifaceName - break - } - } - } - if interfaceChanged { - p.Info().Str("interface", changedIface). - Msg("DNS intercept: interface appeared/disappeared — starting interception probe monitor") - go p.pfInterceptMonitor() - // A VM/container bridge appearing/disappearing changes the - // effective forwarded-source set; rebuild the anchor if so, since - // the probe monitor alone won't (an intact anchor passes its probe). - p.reconcileForwardedSources() - } - } - } - // Refresh VPN DNS on tunnel interface changes (e.g., Tailscale connect/disconnect) - // even though the physical interface didn't change. Runs after tunnel checks - // so the pf anchor rebuild includes current VPN DNS exemptions. - if dnsIntercept && p.vpnDNS != nil { - p.vpnDNS.Refresh(ctx, true) + p.handleDNSInterceptIgnoredNetworkChange(delta, time.Now()) } return } @@ -1919,6 +1867,79 @@ func (p *prog) monitorNetworkChanges(ctx context.Context) error { return nil } +// handleDNSInterceptIgnoredNetworkChange runs the DNS-intercept work for a +// network delta that did not affect a usable interface. Keeping this path in a +// method lets tests exercise the callback wiring with synthetic deltas. +func (p *prog) handleDNSInterceptIgnoredNetworkChange(delta *netmon.ChangeDelta, now time.Time) { + reconcileNow := false + // Stabilization owns PF repair. Do not consume the next leading-edge slot + // until an ignored delta can actually perform the corresponding PF check. + if !p.pfStabilizing.Load() { + reconcileNow = p.dnsInterceptIgnoredChangeReconcileDue(now) + if reconcileNow { + p.ensurePFAnchorActive() + } + } + + // Check tunnel interfaces unconditionally — it decides internally whether + // to enter stabilization or rebuild immediately. + tunnelChanged := p.checkTunnelInterfaceChanges() + // Schedule delayed re-checks to catch async VPN teardown changes. These also + // refresh the OS resolver and VPN DNS routes. + p.scheduleDelayedRechecks() + + // Detect interface appearance/disappearance — hypervisors (Parallels, + // VMware, VirtualBox) reload pf when creating/destroying virtual network + // interfaces, which can corrupt pf's internal translation state. The rdr + // rules survive in text form (watchdog says "intact") but stop evaluating. + // Spawn an async monitor that probes pf interception with backoff and forces + // a full pf reload if broken. + if delta.Old != nil { + interfaceChanged := false + var changedIface string + for ifaceName := range delta.Old.Interface { + if ifaceName == "lo0" { + continue + } + if _, exists := delta.New.Interface[ifaceName]; !exists { + interfaceChanged = true + changedIface = ifaceName + break + } + } + if !interfaceChanged { + for ifaceName := range delta.New.Interface { + if ifaceName == "lo0" { + continue + } + if _, exists := delta.Old.Interface[ifaceName]; !exists { + interfaceChanged = true + changedIface = ifaceName + break + } + } + } + if interfaceChanged { + mainLog.Load().Info().Str("interface", changedIface). + Msg("DNS intercept: interface appeared/disappeared — starting interception probe monitor") + go p.pfInterceptMonitor() + // A VM/container bridge appearing/disappearing changes the effective + // forwarded-source set; an intact anchor/probe does not detect that. + p.reconcileForwardedSources() + } + } + + // Refresh VPN DNS immediately for real tunnel changes even when the periodic + // ignored-change reconciliation is currently rate-limited - but not while + // stabilization owns pf. A refresh rebuilds the anchor, and these deltas arrive + // exactly when a VPN is bringing its own ruleset up, which is the collision + // stabilization is there to prevent. checkTunnelInterfaceChanges keeps the + // observation pending, so the transition is retried rather than dropped. + if p.vpnDNS != nil && (reconcileNow || tunnelChanged) && !p.pfStabilizing.Load() { + p.vpnDNS.Refresh(ctrld.LoggerCtx(context.Background(), p.logger.Load()), true) + } +} + // interfaceStatesEqual compares two interface states func interfaceStatesEqual(a, b *netmon.Interface) bool { if a == nil || b == nil { diff --git a/cmd/cli/pf_ruleset.go b/cmd/cli/pf_ruleset.go new file mode 100644 index 0000000..73d5df6 --- /dev/null +++ b/cmd/cli/pf_ruleset.go @@ -0,0 +1,79 @@ +package cli + +import ( + "fmt" + "strings" +) + +// pfNoRulesMarker is what pfctl prints for a ruleset that contains nothing. +const pfNoRulesMarker = "(no rules)" + +// pfFilterRuleLines reduces pfctl output to the lines that are actually pf rules. +// +// It exists because every pfctl reader here uses CombinedOutput, and pfctl on macOS +// writes "No ALTQ support in kernel" and "ALTQ related functions disabled" to stderr on +// essentially every show command, so raw output is never a clean rule list. An empty +// ruleset can also report "(no rules)", which is a status line rather than a rule. +// +// Two consequences follow from getting this wrong, and both have bitten this file: +// callers that test the output for emptiness can never see empty, and callers that feed +// the lines back into "pfctl -f -" would splice non-rule text into a ruleset and have +// the reload rejected. +// +// Registry access and platform specifics stay elsewhere; this is pure string handling +// so it can be tested on any host. +func pfFilterRuleLines(output string) []string { + var rules []string + for _, line := range strings.Split(output, "\n") { + line = strings.TrimSpace(line) + if line == "" { + continue + } + // pfctl stderr warnings, merged in by CombinedOutput. + if strings.Contains(line, "ALTQ") { + continue + } + // Status line for an empty ruleset, not a rule. + if line == pfNoRulesMarker { + continue + } + rules = append(rules, line) + } + return rules +} + +// pfRulesetEmpty reports whether pfctl output describes a ruleset with no rules. +// +// Use this rather than testing the raw output for emptiness: the merged stderr warnings +// described above mean a raw test is always false, so the condition it guards - an +// anchor whose contents were flushed - would never be detected. +func pfRulesetEmpty(output string) bool { + return len(pfFilterRuleLines(output)) == 0 +} + +// pfContainsRule checks if any line in the slice contains the given rule string. +// Uses substring matching because pfctl may append extra tokens like " all" to rules +// (e.g., `rdr-anchor "com.controld.ctrld" all`), which would fail exact matching. +func pfContainsRule(lines []string, rule string) bool { + for _, line := range lines { + if strings.Contains(line, rule) { + return true + } + } + return false +} + +// pfAnchorReferencesPresent reports whether ctrld's anchor references appear in the +// running ruleset, given the output of "pfctl -sn" and "pfctl -sr". +// +// Removing the references means reloading the entire main ruleset, and that reload +// carries no options section - so it resets system-wide pf options, including any +// third-party "set skip" directives. Doing that when there is nothing of ours to +// remove is pure collateral damage, which is what a startup rollback would otherwise +// cause after failing before the references were ever added. +func pfAnchorReferencesPresent(natOutput, filterOutput, anchorName string) bool { + rdrAnchorRef := fmt.Sprintf("rdr-anchor %q", anchorName) + anchorRef := fmt.Sprintf("anchor %q", anchorName) + return pfContainsRule(pfFilterRuleLines(natOutput), rdrAnchorRef) || + pfContainsRule(pfFilterRuleLines(filterOutput), anchorRef) +} diff --git a/cmd/cli/pf_ruleset_test.go b/cmd/cli/pf_ruleset_test.go new file mode 100644 index 0000000..d84252d --- /dev/null +++ b/cmd/cli/pf_ruleset_test.go @@ -0,0 +1,157 @@ +package cli + +import "testing" + +// altqNoise is what macOS pfctl writes to stderr on show commands. Because every +// pfctl reader here uses CombinedOutput, it lands in the middle of the data being +// parsed — which is why these helpers exist. +const altqNoise = "No ALTQ support in kernel\nALTQ related functions disabled\n" + +// TestPFRulesetEmpty is the regression guard for a flushed anchor being undetectable. +// +// The anchor-content checks in verifyPFState and ensurePFAnchorActive decide whether pf +// still has ctrld's rules. Testing the raw pfctl output for emptiness can never be true +// on macOS, because the merged ALTQ warnings are always present — so a genuinely flushed +// anchor reads as healthy and neither the startup gate nor the watchdog restore fires. +func TestPFRulesetEmpty(t *testing.T) { + tests := []struct { + name string + output string + want bool + }{ + { + // The case that was broken: nothing but merged stderr. + name: "only ALTQ warnings", + output: altqNoise, + want: true, + }, + { + // As captured on macOS 26.6 from "pfctl -sn -a com.controld.ctrld". + name: "ALTQ warnings plus the empty-ruleset marker", + output: altqNoise + "(no rules)\n", + want: true, + }, + { + name: "empty output", + output: "", + want: true, + }, + { + name: "whitespace only", + output: "\n \n\t\n", + want: true, + }, + { + name: "a real rdr rule behind the warnings", + output: altqNoise + "rdr on lo0 inet proto udp from any to ! 127.0.0.1 port = 53 -> 127.0.0.1 port 5354\n", + want: false, + }, + { + name: "a real filter rule behind the warnings", + output: altqNoise + "pass in quick on lo0 reply-to lo0 inet proto udp from any to 127.0.0.1 port = 5354\n", + want: false, + }, + { + name: "rule with no warnings at all", + output: "anchor \"com.controld.ctrld\" all\n", + want: false, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := pfRulesetEmpty(tc.output); got != tc.want { + t.Errorf("pfRulesetEmpty() = %v, want %v\noutput:\n%s", got, tc.want, tc.output) + } + }) + } +} + +// TestPFFilterRuleLines checks what survives filtering, since these lines are fed back +// into "pfctl -f -" by the ruleset-rebuild paths. Splicing a warning or the +// empty-ruleset marker into a ruleset would have the reload rejected outright. +func TestPFFilterRuleLines(t *testing.T) { + got := pfFilterRuleLines(altqNoise + "(no rules)\nrdr-anchor \"com.controld.ctrld\" all\n\nanchor \"com.controld.ctrld\" all\n") + want := []string{ + `rdr-anchor "com.controld.ctrld" all`, + `anchor "com.controld.ctrld" all`, + } + if len(got) != len(want) { + t.Fatalf("got %d lines %q, want %d %q", len(got), got, len(want), want) + } + for i := range want { + if got[i] != want[i] { + t.Errorf("line %d = %q, want %q", i, got[i], want[i]) + } + } + + if lines := pfFilterRuleLines(altqNoise); lines != nil { + t.Errorf("warnings alone must yield no rule lines, got %q", lines) + } +} + +// TestPFAnchorReferencesPresent guards when the main ruleset may be rewritten. +// +// Removing our anchor references means reloading the whole main ruleset, and that +// reload carries no options section — so it resets system-wide pf options, including +// third-party "set skip" directives. Startup rollback runs after failures that happen +// before the references were ever added, so without this check it would reset another +// application's pf options while removing nothing of ours. +func TestPFAnchorReferencesPresent(t *testing.T) { + const anchor = "com.controld.ctrld" + const otherAppRules = "scrub-anchor \"com.apple/*\" all fragment reassemble\nanchor \"com.vendor.vpn\" all\n" + + tests := []struct { + name string + nat string + filter string + want bool + }{ + { + name: "both references present", + nat: altqNoise + "rdr-anchor \"com.controld.ctrld\" all\n", + filter: altqNoise + "anchor \"com.controld.ctrld\" all\n", + want: true, + }, + { + // pfctl appends tokens like " all", so matching is substring-based. + name: "rdr reference only", + nat: altqNoise + "rdr-anchor \"com.controld.ctrld\" all\n", + filter: altqNoise + otherAppRules, + want: true, + }, + { + name: "filter reference only", + nat: altqNoise, + filter: altqNoise + "anchor \"com.controld.ctrld\"\n", + want: true, + }, + { + // The rollback case: we failed before adding anything, and another + // application owns the ruleset. Rewriting it would be pure collateral. + name: "someone else's ruleset, none of ours", + nat: altqNoise, + filter: altqNoise + otherAppRules, + want: false, + }, + { + name: "empty ruleset", + nat: altqNoise + "(no rules)\n", + filter: altqNoise + "(no rules)\n", + want: false, + }, + { + // A different anchor whose name merely contains ours must not count. + name: "another anchor with a similar name", + nat: altqNoise, + filter: altqNoise + "anchor \"com.vendor.controld-shim\" all\n", + want: false, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := pfAnchorReferencesPresent(tc.nat, tc.filter, anchor); got != tc.want { + t.Errorf("pfAnchorReferencesPresent() = %v, want %v", got, tc.want) + } + }) + } +} diff --git a/cmd/cli/prog.go b/cmd/cli/prog.go index 6ca5a51..06e9909 100644 --- a/cmd/cli/prog.go +++ b/cmd/cli/prog.go @@ -84,6 +84,16 @@ var noopLogf = func(format string, args ...any) {} var useSystemdResolved = false +type pfAnchorCheckResult uint8 + +const ( + pfAnchorCheckSkipped pfAnchorCheckResult = iota + pfAnchorCheckIntact + pfAnchorCheckRestored + pfAnchorCheckDeferred + pfAnchorCheckFailed +) + type prog struct { mu sync.Mutex waitCh chan struct{} @@ -151,11 +161,12 @@ type prog struct { // On Windows: *wfpState, on macOS: *pfState, nil on other platforms. dnsInterceptState any - // lastTunnelIfaces tracks the set of active VPN/tunnel interfaces (utun*, ipsec*, etc.) - // discovered during the last pf anchor rule build. When the set changes (e.g., a VPN - // connects and creates utun420), we rebuild the pf anchor to add interface-specific - // intercept rules for the new interface. Protected by mu. - lastTunnelIfaces []string //lint:ignore U1000 used on darwin + // lastTunnelIfaces tracks the tunnel set included in the last successfully loaded + // pf anchor. Pending tunnel state is kept separately so failed PF work is retried + // instead of being mistaken for an applied update. Protected by mu. + lastTunnelIfaces []string //lint:ignore U1000 used on darwin + pendingTunnelIfaces []string //lint:ignore U1000 used on darwin + hasPendingTunnelIfaces bool //lint:ignore U1000 used on darwin // pfStabilizing is true while we're waiting for a VPN's pf ruleset to settle. // While true, the watchdog and network change callbacks do NOT restore our rules. @@ -178,10 +189,10 @@ type prog struct { // interception with exponential backoff and auto-heals if broken. pfMonitorRunning atomic.Bool //lint:ignore U1000 used on darwin - // pfEnsureRunning ensures only one pf anchor validation/restoration runs at a time. - // Network-change callbacks, delayed rechecks, and the periodic watchdog can all - // converge during macOS interface churn; concurrent pfctl/scutil exec storms can - // exhaust process/file limits and make the outage worse. + // pfEnsureRunning ensures only one pf validation or mutation runs at a time. + // Network callbacks, VPN exemption updates, delayed rechecks, probes, and the + // watchdog can converge during macOS churn; concurrent pfctl/scutil work can + // exhaust process/file limits or interleave anchor snapshots. pfEnsureRunning atomic.Bool //lint:ignore U1000 used on darwin // pfExecBackoffUntil suppresses pf anchor validation after pfctl/scutil execs @@ -193,6 +204,11 @@ type prog struct { pfDelayedRecheckMu sync.Mutex //lint:ignore U1000 used on darwin pfDelayedRecheckTimers []*time.Timer //lint:ignore U1000 used on darwin + // pfIgnoredChangeLastReconcile bounds immediate pf/VPN-DNS work for noisy + // ignored macOS network deltas. Tunnel changes bypass this limit, and the + // existing delayed checks provide a trailing reconciliation after churn. + pfIgnoredChangeLastReconcile atomic.Int64 //lint:ignore U1000 used on darwin + // pfProbeExpected holds the domain name of a pending pf interception probe. pfProbeExpected atomic.Value // string @@ -829,6 +845,39 @@ func (p *prog) deAllocateIP() error { return nil } +// Seams for the intercept-start failure lifecycle. Choosing between the interface-DNS +// fallback and refusing it has side effects - restoring the host's DNS, then +// terminating - which a test has to observe without reconfiguring the host or exiting +// the test binary. The intercept start itself is indirected for the same reason: it is +// the real platform interceptor, which on macOS mutates pf and on Windows installs an +// NRPT rule, so a test of what happens *after* it fails must not be the thing that +// runs it. +var ( + startDNSInterceptFn = (*prog).startDNSIntercept + setDnsForRunningIfaceFn = (*prog).setDnsForRunningIface + resetDNSFn = (*prog).resetDNS + refuseFallbackFatal = func(format string, v ...any) { + mainLog.Load().Fatal().Msgf(format, v...) + } +) + +// interfaceDNSFallbackViable reports whether the interface-DNS fallback can actually +// direct queries to ctrld's listener. +// +// Interface DNS names a resolver by IP and has no port field - true of macOS interface +// settings and of Windows NRPT rules - so pointing the system straight at a listener +// that did not bind :53 sends queries to whatever owns :53 instead, and that resolver's +// upstream is ctrld's address: a loop, not a fallback. +// +// A nil or portless listener is treated as viable: the port is resolved elsewhere and +// defaults to 53, so there is nothing to refuse yet. +// +// master no longer supports the router-local forwarding-resolver path, so there is +// no intermediary that can make a non-53 listener reachable from interface DNS. +func interfaceDNSFallbackViable(lc *ctrld.ListenerConfig) bool { + return lc == nil || lc.Port == 0 || lc.Port == 53 +} + func (p *prog) setDNS() { setDnsOK := false defer func() { @@ -861,7 +910,30 @@ func (p *prog) setDNS() { // modifying interface DNS settings. This eliminates race conditions with VPN // software that also manages DNS. See issue #489. if dnsIntercept { - if err := p.startDNSIntercept(); err != nil { + if err := startDNSInterceptFn(p); err != nil { + // Interface DNS cannot express a port: macOS interface settings and Windows + // NRPT rules both name a resolver by IP alone. So it is only a usable + // fallback when the listener actually bound :53. When something else owns + // :53 - mDNSResponder on macOS, which is the whole reason the :5354 fallback + // exists - pointing the system at 127.0.0.1 hands queries to that other + // resolver, whose own upstream is now ctrld's address. That is a resolution + // loop, not degraded operation: a healthy ctrld listener nothing on the host + // can reach, no working DNS, and no recovery short of stopping the service. + // + // Refuse instead, after putting the host's own DNS back. A visible startup + // failure beats DNS that is broken by design, and it stops a fallback that + // cannot work from quietly undoing the fail-closed verification above. + if lc := cfg.FirstListener(); !interfaceDNSFallbackViable(lc) { + mainLog.Load().Error().Err(err).Msgf("DNS intercept mode failed with the listener on port %d", lc.Port) + // Leave the host resolvable: restore static settings or DHCP rather than + // exiting with an interface still pointed at a ctrld that is not serving. + resetDNSFn(p, false, true) + refuseFallbackFatal("Refusing to fall back to interface DNS: it cannot direct queries to %s:%d, which would leave this host with no working resolver. Free port 53 for ctrld, or resolve the intercept failure, then start again.", lc.IP, lc.Port) + // Unreachable in production - the line above exits - but returning + // explicitly keeps the refusal from depending on that, so nothing can + // fall through to installing the fallback this just rejected. + return + } p.Error().Err(err).Msg("DNS intercept mode failed — falling back to interface DNS settings") // Fall through to traditional setDNS behavior. } else { @@ -913,7 +985,7 @@ func (p *prog) setDNS() { slices.Sort(nameservers) netIfaceName := "" - netIface := p.setDnsForRunningIface(nameservers) + netIface := setDnsForRunningIfaceFn(p, nameservers) if netIface != nil { netIfaceName = netIface.Name } diff --git a/cmd/cli/prog_intercept_fallback_test.go b/cmd/cli/prog_intercept_fallback_test.go new file mode 100644 index 0000000..5a0a9b6 --- /dev/null +++ b/cmd/cli/prog_intercept_fallback_test.go @@ -0,0 +1,180 @@ +package cli + +import ( + "errors" + "fmt" + "net" + "strings" + "testing" + + "github.com/Control-D-Inc/ctrld" +) + +// TestInterfaceDNSFallbackViable covers when the interface-DNS fallback may be used +// after DNS intercept fails to start. +// +// The fallback names a resolver by IP with no port, so it can only reach a listener on +// :53. Taking it with the listener on a redirect-dependent port produced a total DNS +// outage on macOS: the interface points at 127.0.0.1, mDNSResponder answers there, and +// its upstream is ctrld's own address - a resolution loop with a healthy ctrld listener +// nothing can reach. Intercept startup refuses the fallback in that case rather than +// creating it. +func TestInterfaceDNSFallbackViable(t *testing.T) { + tests := []struct { + name string + lc *ctrld.ListenerConfig + want bool + }{ + { + name: "listener on 53 can be reached by interface DNS", + lc: &ctrld.ListenerConfig{IP: "127.0.0.1", Port: 53}, + want: true, + }, + { + // The reported outage: no local resolver, so the :5354 fallback port + // cannot be expressed by interface DNS. + name: "listener on the fallback port cannot", + lc: &ctrld.ListenerConfig{IP: "127.0.0.1", Port: 5354}, + want: false, + }, + { + name: "any other non-53 port cannot", + lc: &ctrld.ListenerConfig{IP: "127.0.0.1", Port: 5300}, + want: false, + }, + + { + // Port is resolved elsewhere and defaults to 53; nothing to refuse yet. + name: "unset port is not refused", + lc: &ctrld.ListenerConfig{IP: "127.0.0.1"}, + want: true, + }, + { + name: "no listener is not refused", + lc: nil, + want: true, + }, + { + // A non-loopback listener on 53 is still reachable by IP. + name: "non-loopback listener on 53", + lc: &ctrld.ListenerConfig{IP: "192.168.1.10", Port: 53}, + want: true, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := interfaceDNSFallbackViable(tc.lc); got != tc.want { + t.Errorf("interfaceDNSFallbackViable() = %v, want %v", got, tc.want) + } + }) + } +} + +// interceptFallbackHarness drives setDNS() through the intercept-start failure path and +// records the side effects that decide whether the host ends up with a working +// resolver. +// +// Every host-touching step is stubbed, including the intercept start itself: this test +// runs untagged on Linux, macOS and Windows runners, where the real startDNSIntercept +// would set up pf or install an NRPT rule on the machine running the tests. Stubbing it +// also makes the precondition deterministic - the failure under test is injected rather +// than depending on the runner denying a privileged operation. +type interceptFallbackHarness struct { + interceptCalls int + installedNameservers []string + installCalls int + resetCalls int + refusals []string +} + +func newInterceptFallbackHarness(t *testing.T, lc *ctrld.ListenerConfig) *interceptFallbackHarness { + t.Helper() + h := &interceptFallbackHarness{} + + origStart, origInstall := startDNSInterceptFn, setDnsForRunningIfaceFn + origReset, origFatal := resetDNSFn, refuseFallbackFatal + origCfg, origMode, origIntercept, origHard := cfg, interceptMode, dnsIntercept, hardIntercept + t.Cleanup(func() { + startDNSInterceptFn, setDnsForRunningIfaceFn = origStart, origInstall + resetDNSFn, refuseFallbackFatal = origReset, origFatal + cfg, interceptMode, dnsIntercept, hardIntercept = origCfg, origMode, origIntercept, origHard + }) + + // Never reach the real interceptor: it would configure pf on macOS and NRPT on + // Windows, on the machine running the tests. + startDNSInterceptFn = func(_ *prog) error { + h.interceptCalls++ + return errors.New("dns intercept: injected start failure") + } + setDnsForRunningIfaceFn = func(_ *prog, nameservers []string) *net.Interface { + h.installCalls++ + h.installedNameservers = nameservers + return nil + } + resetDNSFn = func(_ *prog, _ bool, _ bool) { h.resetCalls++ } + refuseFallbackFatal = func(format string, v ...any) { + h.refusals = append(h.refusals, fmt.Sprintf(format, v...)) + } + + cfg = ctrld.Config{} + cfg.Service.InterceptMode = "dns" + cfg.Listener = map[string]*ctrld.ListenerConfig{"0": lc} + watchdogOff := false + cfg.Service.DnsWatchdogEnabled = &watchdogOff + interceptMode, dnsIntercept, hardIntercept = "dns", false, false + return h +} + +func (h *interceptFallbackHarness) run(t *testing.T) { + t.Helper() + p := &prog{cfg: &cfg} + p.logger.Store(mainLog.Load()) + p.setDNS() +} + +// TestSetDNSRefusesUnreachableFallback is the behaviour test for the reported outage: it +// drives the real setDNS() lifecycle rather than the classification helper alone. +// +// Deleting or bypassing the guard in setDNS makes the first case fail, because interface +// DNS then gets installed pointing at a listener that cannot answer on :53 - which is +// the resolution loop this refuses to create. +func TestSetDNSRefusesUnreachableFallback(t *testing.T) { + t.Run("non-53 listener refuses the fallback and restores DNS", func(t *testing.T) { + h := newInterceptFallbackHarness(t, &ctrld.ListenerConfig{IP: "127.0.0.1", Port: 5354}) + h.run(t) + + if h.interceptCalls != 1 { + t.Fatalf("intercept start called %d time(s) through the seam, want 1 — the real platform interceptor must never run here", h.interceptCalls) + } + if h.installCalls != 0 { + t.Errorf("interface DNS was installed %d time(s) for a listener on :5354 — that is the resolver loop", h.installCalls) + } + if h.resetCalls == 0 { + t.Error("host DNS was not restored before refusing, leaving the interface pointed at a ctrld that is not serving") + } + if len(h.refusals) == 0 { + t.Fatal("refusal was not surfaced: startup must fail loudly rather than silently skip the fallback") + } + if !strings.Contains(h.refusals[0], "5354") { + t.Errorf("refusal does not name the unreachable port: %q", h.refusals[0]) + } + }) + + t.Run("listener on 53 still reaches the interface-DNS fallback", func(t *testing.T) { + h := newInterceptFallbackHarness(t, &ctrld.ListenerConfig{IP: "127.0.0.1", Port: 53}) + h.run(t) + + if h.interceptCalls != 1 { + t.Fatalf("intercept start called %d time(s) through the seam, want 1", h.interceptCalls) + } + if h.installCalls != 1 { + t.Errorf("interface DNS installed %d time(s), want 1: a listener on :53 is reachable, so the fallback must still apply", h.installCalls) + } + if len(h.refusals) != 0 { + t.Errorf("unexpected refusal for a reachable listener: %v", h.refusals) + } + if len(h.installedNameservers) == 0 { + t.Error("fallback installed no nameservers") + } + }) +} diff --git a/cmd/cli/vpn_dns.go b/cmd/cli/vpn_dns.go index 0143fa4..b475328 100644 --- a/cmd/cli/vpn_dns.go +++ b/cmd/cli/vpn_dns.go @@ -47,6 +47,9 @@ type vpnDNSManager struct { // as additional nameservers for queries that match split-DNS rules // (from ctrld config, AD domain, or VPN suffix config). domainlessServers []string + // appliedExemptions advances only after the platform PF/WFP callback succeeds. + // Keeping it separate from discovered configs makes failed rule updates retryable. + appliedExemptions []vpnDNSExemption // 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 @@ -55,9 +58,13 @@ type vpnDNSManager struct { // 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 - // refreshRunning keeps noisy network-change storms from running overlapping - // scutil/networksetup VPN DNS discovery work. - refreshRunning atomic.Bool + // refreshStateMu keeps noisy network-change storms from running overlapping + // full VPN DNS refreshes and retains one trailing refresh when an event arrives + // during discovery so the newest OS state is not lost. + refreshStateMu sync.Mutex + refreshRunning bool + refreshPending bool + discoveryMu sync.Mutex // Called when VPN DNS server list changes, to update intercept exemptions. onServersChanged vpnDNSExemptFunc } @@ -75,15 +82,41 @@ func newVPNDNSManager(logger *atomic.Pointer[ctrld.Logger], exemptFunc vpnDNSExe } // Refresh re-discovers VPN DNS configs from the OS. -// Called on network change events. +// Called on network change events. Overlapping calls are coalesced into one +// trailing refresh so a newer OS snapshot is never silently discarded. func (m *vpnDNSManager) Refresh(ctx context.Context, guardAgainstNoNameservers ...bool) { logger := ctrld.LoggerFromCtx(ctx) guardedRefresh := len(guardAgainstNoNameservers) > 0 && guardAgainstNoNameservers[0] - if !m.refreshRunning.CompareAndSwap(false, true) { - ctrld.Log(ctx, logger.Debug(), "VPN DNS refresh already running, skipping duplicate") + m.refreshStateMu.Lock() + if m.refreshRunning { + m.refreshPending = true + m.refreshStateMu.Unlock() + ctrld.Log(ctx, logger.Debug(), "VPN DNS refresh already running, coalescing trailing refresh") return } - defer m.refreshRunning.Store(false) + m.refreshRunning = true + m.refreshStateMu.Unlock() + + for { + m.refreshOnce(ctx, guardedRefresh) + + m.refreshStateMu.Lock() + if m.refreshPending { + m.refreshPending = false + m.refreshStateMu.Unlock() + guardedRefresh = true + continue + } + m.refreshRunning = false + m.refreshStateMu.Unlock() + return + } +} + +func (m *vpnDNSManager) refreshOnce(ctx context.Context, guardAgainstNoNameservers bool) { + logger := ctrld.LoggerFromCtx(ctx) + m.discoveryMu.Lock() + defer m.discoveryMu.Unlock() ctrld.Log(ctx, logger.Debug(), "Refreshing VPN DNS configurations") discoverVPNDNS := m.discoverVPNDNS @@ -110,9 +143,7 @@ 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 vpnDNSSettlingEnabled && len(configs) == 0 && guardAgainstNoNameservers && m.hasVPNDNSStateLocked() { if !m.retainedAfterEmptyDiscovery { exemptions := m.currentExemptionsLocked() m.retainedAfterEmptyDiscovery = true @@ -122,6 +153,8 @@ func (m *vpnDNSManager) Refresh(ctx context.Context, guardAgainstNoNameservers . if m.onServersChanged != nil { if err := m.onServersChanged(exemptions); err != nil { ctrld.Log(ctx, logger.Error().Err(err), "Failed to re-apply retained VPN DNS exemptions") + } else { + m.appliedExemptions = append([]vpnDNSExemption(nil), exemptions...) } } return @@ -200,41 +233,47 @@ 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 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") + // Update intercept rules only when desired exemptions differ from the last + // successfully applied set. Failed PF/WFP callbacks remain retryable on the + // next refresh even when discovery returns the same VPN DNS state. + m.updateInterceptExemptionsIfChanged(ctx, logger, exemptions, "VPN DNS") } -func (m *vpnDNSManager) updateInterceptExemptionsIfChanged(ctx context.Context, logger *ctrld.Logger, before, after []vpnDNSExemption, reason string) { +func (m *vpnDNSManager) updateInterceptExemptionsIfChanged(ctx context.Context, logger *ctrld.Logger, desired []vpnDNSExemption, reason string) { if m.onServersChanged == nil { return } - if vpnDNSExemptionsEqual(before, after) { + if vpnDNSExemptionsEqual(m.appliedExemptions, desired) { 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 { + if err := m.onServersChanged(desired); err != nil { ctrld.Log(ctx, logger.Error().Err(err), "Failed to update intercept exemptions for VPN DNS servers") + return } + m.appliedExemptions = append([]vpnDNSExemption(nil), desired...) } -// 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. +// RefreshRoutesOnly re-discovers VPN DNS configs and updates ctrld's +// in-memory split-DNS routes. It applies intercept exemptions only when that set +// changes, while holding the shared discovery lane so a concurrent full refresh +// cannot commit a newer snapshot and then be overwritten by this one. func (m *vpnDNSManager) RefreshRoutesOnly() (routes, domainlessServers, exemptions int) { logger := mainLog.Load() + if m.logger != nil && m.logger.Load() != nil { + logger = m.logger.Load() + } + ctx := ctrld.LoggerCtx(context.Background(), logger) - logger.Debug().Msg("Refreshing VPN DNS route state only") + m.discoveryMu.Lock() + defer m.discoveryMu.Unlock() + + ctrld.Log(ctx, logger.Debug(), "Refreshing VPN DNS route state only") discoverVPNDNS := m.discoverVPNDNS if discoverVPNDNS == nil { discoverVPNDNS = ctrld.DiscoverVPNDNS } - configs := discoverVPNDNS(context.Background()) + configs := discoverVPNDNS(ctx) if dri, err := netmon.DefaultRouteInterface(); err == nil && dri != "" { for i := range configs { @@ -275,10 +314,26 @@ func (m *vpnDNSManager) RefreshRoutesOnly() (routes, domainlessServers, exemptio } } m.domainlessServers = domainless + currentExemptions := m.currentExemptionsLocked() - 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()) + ctrld.Log(ctx, logger.Debug(), "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(currentExemptions)) + m.updateInterceptExemptionsIfChanged(ctx, logger, currentExemptions, "route-only VPN DNS") + return len(m.routes), len(m.domainlessServers), len(currentExemptions) +} + +func (m *vpnDNSManager) markInterceptExemptionsApplied(applied []vpnDNSExemption) { + m.mu.Lock() + defer m.mu.Unlock() + if vpnDNSExemptionsEqual(m.currentExemptionsLocked(), applied) { + m.appliedExemptions = append([]vpnDNSExemption(nil), applied...) + } +} + +func (m *vpnDNSManager) interceptExemptionsPending() bool { + m.mu.RLock() + defer m.mu.RUnlock() + return !vpnDNSExemptionsEqual(m.appliedExemptions, m.currentExemptionsLocked()) } func (m *vpnDNSManager) hasVPNDNSStateLocked() bool { diff --git a/cmd/cli/vpn_dns_test.go b/cmd/cli/vpn_dns_test.go index 702b340..eba943c 100644 --- a/cmd/cli/vpn_dns_test.go +++ b/cmd/cli/vpn_dns_test.go @@ -2,9 +2,11 @@ package cli import ( "context" + "errors" "sync" "sync/atomic" "testing" + "time" "github.com/Control-D-Inc/ctrld" ) @@ -16,7 +18,7 @@ func withVPNDNSSettlingEnabled(t *testing.T) { t.Cleanup(func() { vpnDNSSettlingEnabled = old }) } -func TestVPNDNSRefreshSkipsConcurrentDuplicate(t *testing.T) { +func TestVPNDNSRefreshCoalescesConcurrentTrailingRefresh(t *testing.T) { m := newVPNDNSManager(&mainLog, nil) started := make(chan struct{}) release := make(chan struct{}) @@ -25,9 +27,16 @@ func TestVPNDNSRefreshSkipsConcurrentDuplicate(t *testing.T) { var calls atomic.Int32 m.discoverVPNDNS = func(context.Context) []ctrld.VPNDNSConfig { - calls.Add(1) + call := calls.Add(1) once.Do(func() { close(started) }) <-release + if call == 2 { + return []ctrld.VPNDNSConfig{{ + InterfaceName: "utun-latest", + Servers: []string{"10.0.0.2"}, + Domains: []string{"latest.internal"}, + }} + } return nil } @@ -41,8 +50,11 @@ func TestVPNDNSRefreshSkipsConcurrentDuplicate(t *testing.T) { close(release) <-done - if calls.Load() != 1 { - t.Fatalf("expected overlapping refresh to be skipped, got %d discovery calls", calls.Load()) + if calls.Load() != 2 { + t.Fatalf("expected one active and one trailing discovery call, got %d", calls.Load()) + } + if got := m.Routes()["latest.internal"]; len(got) != 1 || got[0] != "10.0.0.2" { + t.Fatalf("trailing refresh did not publish latest OS snapshot: %v", got) } } @@ -76,7 +88,9 @@ func TestVPNDNSRefreshRetainsStateForOneGuardedEmptyDiscovery(t *testing.T) { func TestVPNDNSRefreshClearsOnSecondGuardedEmptyDiscovery(t *testing.T) { withVPNDNSSettlingEnabled(t) var gotExemptions []vpnDNSExemption + updates := 0 m := newVPNDNSManager(&mainLog, func(exemptions []vpnDNSExemption) error { + updates++ gotExemptions = exemptions return nil }) @@ -86,6 +100,7 @@ func TestVPNDNSRefreshClearsOnSecondGuardedEmptyDiscovery(t *testing.T) { Servers: []string{"10.25.37.21"}, }} m.domainlessServers = []string{"10.25.37.21"} + m.appliedExemptions = []vpnDNSExemption{{Server: "10.25.37.21", Interface: "Ethernet 6"}} m.retainedAfterEmptyDiscovery = true m.Refresh(context.Background(), true) @@ -93,8 +108,8 @@ func TestVPNDNSRefreshClearsOnSecondGuardedEmptyDiscovery(t *testing.T) { 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 updates != 1 || len(gotExemptions) != 0 { + t.Fatalf("expected one empty exemption update after clearing stale state, calls=%d exemptions=%v", updates, gotExemptions) } if m.retainedAfterEmptyDiscovery { t.Fatal("expected retained empty-discovery marker to be cleared with stale state") @@ -126,6 +141,56 @@ func TestVPNDNSRefreshSkipsUnchangedInterceptExemptions(t *testing.T) { } } +func TestVPNDNSRefreshRetriesFailedInterceptExemptionUpdate(t *testing.T) { + attempts := 0 + m := newVPNDNSManager(&mainLog, func([]vpnDNSExemption) error { + attempts++ + if attempts == 1 { + return errors.New("pf update failed") + } + return nil + }) + m.discoverVPNDNS = func(context.Context) []ctrld.VPNDNSConfig { + return []ctrld.VPNDNSConfig{{ + InterfaceName: "utun-test", + Servers: []string{"10.102.26.10"}, + Domains: []string{"internal.test"}, + }} + } + + m.Refresh(context.Background(), true) + if !m.interceptExemptionsPending() { + t.Fatal("failed intercept exemption update was not retained for retry") + } + m.Refresh(context.Background(), true) + if m.interceptExemptionsPending() { + t.Fatal("successful intercept exemption retry did not advance applied state") + } + m.Refresh(context.Background(), true) + + if attempts != 2 { + t.Fatalf("intercept exemption update attempts = %d, want failed attempt plus one retry", attempts) + } + if len(m.appliedExemptions) != 1 || m.appliedExemptions[0].Server != "10.102.26.10" { + t.Fatalf("applied exemptions = %+v, want successful retry state", m.appliedExemptions) + } +} + +func TestVPNDNSMarkAppliedExemptionsRejectsStaleSnapshot(t *testing.T) { + m := newVPNDNSManager(&mainLog, nil) + m.configs = []ctrld.VPNDNSConfig{{InterfaceName: "utun-new", Servers: []string{"10.0.0.2"}}} + + m.markInterceptExemptionsApplied([]vpnDNSExemption{{Server: "10.0.0.1", Interface: "utun-old"}}) + if !m.interceptExemptionsPending() { + t.Fatal("stale PF snapshot incorrectly advanced applied exemptions") + } + + m.markInterceptExemptionsApplied([]vpnDNSExemption{{Server: "10.0.0.2", Interface: "utun-new"}}) + if m.interceptExemptionsPending() { + t.Fatal("current PF snapshot did not advance applied exemptions") + } +} + func TestVPNDNSTransportFailureSuppressesFallbackOnlyWhileRetainingState(t *testing.T) { withVPNDNSSettlingEnabled(t) m := newVPNDNSManager(&mainLog, nil) @@ -145,3 +210,89 @@ func TestVPNDNSTransportFailureSuppressesFallbackOnlyWhileRetainingState(t *test t.Fatal("expected reachable DNS response to clear retained empty-discovery state") } } + +func TestVPNDNSFullAndRouteOnlyDiscoveryAreSerialized(t *testing.T) { + var updateMu sync.Mutex + var exemptionUpdates []string + m := newVPNDNSManager(&mainLog, func(exemptions []vpnDNSExemption) error { + updateMu.Lock() + defer updateMu.Unlock() + if len(exemptions) == 0 { + exemptionUpdates = append(exemptionUpdates, "") + } else { + exemptionUpdates = append(exemptionUpdates, exemptions[0].Server) + } + return nil + }) + firstStarted := make(chan struct{}) + releaseFirst := make(chan struct{}) + secondStarted := make(chan struct{}) + var calls atomic.Int32 + + m.discoverVPNDNS = func(context.Context) []ctrld.VPNDNSConfig { + switch calls.Add(1) { + case 1: + close(firstStarted) + <-releaseFirst + return []ctrld.VPNDNSConfig{{ + InterfaceName: "utun-old", + Servers: []string{"10.0.0.1"}, + Domains: []string{"old.internal"}, + }} + case 2: + close(secondStarted) + return []ctrld.VPNDNSConfig{{ + InterfaceName: "utun-new", + Servers: []string{"10.0.0.2"}, + Domains: []string{"new.internal"}, + }} + default: + t.Fatalf("unexpected discovery call %d", calls.Load()) + return nil + } + } + + routesDone := make(chan struct{}) + go func() { + defer close(routesDone) + m.RefreshRoutesOnly() + }() + <-firstStarted + + fullDone := make(chan struct{}) + go func() { + defer close(fullDone) + m.Refresh(context.Background(), false) + }() + + select { + case <-secondStarted: + t.Fatal("full and route-only VPN DNS discovery overlapped") + case <-time.After(50 * time.Millisecond): + } + close(releaseFirst) + + select { + case <-routesDone: + case <-time.After(time.Second): + t.Fatal("route-only refresh did not finish") + } + select { + case <-fullDone: + case <-time.After(time.Second): + t.Fatal("full refresh did not finish") + } + + routes := m.Routes() + if _, ok := routes["old.internal"]; ok { + t.Fatalf("older route-only snapshot overwrote newer full refresh: %v", routes) + } + if got := routes["new.internal"]; len(got) != 1 || got[0] != "10.0.0.2" { + t.Fatalf("final VPN DNS routes = %v, want new.internal -> 10.0.0.2", routes) + } + updateMu.Lock() + defer updateMu.Unlock() + if len(exemptionUpdates) != 2 || exemptionUpdates[0] != "10.0.0.1" || exemptionUpdates[1] != "10.0.0.2" { + t.Fatalf("serialized exemption updates = %v, want old then new", exemptionUpdates) + } +} diff --git a/docs/pf-dns-intercept.md b/docs/pf-dns-intercept.md index 4a8ac94..cb25b67 100644 --- a/docs/pf-dns-intercept.md +++ b/docs/pf-dns-intercept.md @@ -258,11 +258,22 @@ The full pf reload is VPN-safe: it reassembles from `pfctl -sr` + `pfctl -sn` ### What about `set skip on lo0`? Some pf.conf files include `set skip on lo0` which tells pf to skip ALL processing on loopback. **This would break our approach** since both the `rdr on lo0` and `pass in on lo0` rules would be skipped. -**Mitigation:** When injecting anchor references via `ensurePFAnchorReference()`, -we strip `lo0` from any `set skip on` directives before reloading. The watchdog -also checks for `set skip on lo0` and triggers a restore if detected. The -interception probe provides an additional safety net — if `set skip on lo0` gets -re-applied by another program, the probe will fail and trigger a full reload. +**Mitigation:** the interception probe. `probePFIntercept()` sends a real query from +outside the `_ctrld` group and confirms the listener received the redirect, which cannot +succeed while pf is bypassing loopback — so a skip on `lo0` shows up as a probe failure +and triggers a full reload. + +**Not implemented, contrary to earlier versions of this document:** ctrld does *not* +strip `lo0` from `set skip on` directives, and the watchdog does *not* inspect skip +state. Apple's `pfctl` offers no way to read it — `pfctl(8)` accepts `-s` nat, queue, +rules, Anchors, states, Sources, info, References, labels, timeouts, memory, Tables, +osfp, Interfaces, all, with no options or skip modifier — so text-based detection is not +available on macOS. + +Adding an explicit check is tracked as follow-up: `pfctl(8)` documents +`-s Interfaces -v` as additionally listing which interfaces have skip rules activated, +which is the query to build on once its output shape is confirmed on a host that has a +skip configured. ## Cleanup