From 4d026d836c21084ffbfb2db0e12ac3b88e2c66ac Mon Sep 17 00:00:00 2001 From: Dev Scribe Date: Mon, 10 Aug 2026 07:48:17 +0000 Subject: [PATCH] windows: adopt GP-managed NRPT catch-all --- cmd/cli/dns_intercept_darwin.go | 15 +- .../dns_intercept_lifecycle_windows_test.go | 319 +++ cmd/cli/dns_intercept_others.go | 3 + cmd/cli/dns_intercept_windows.go | 1729 +++++++++++++++-- cmd/cli/dns_proxy.go | 8 +- cmd/cli/intercept_probe.go | 68 + cmd/cli/nrpt_external_gp.go | 63 + cmd/cli/nrpt_external_gp_test.go | 129 ++ cmd/cli/nrpt_external_gp_windows_test.go | 20 + cmd/cli/nrpt_handback_windows_test.go | 1309 +++++++++++++ cmd/cli/prog.go | 85 +- docs/dns-intercept-mode.md | 67 +- docs/wfp-dns-intercept.md | 106 +- 13 files changed, 3627 insertions(+), 294 deletions(-) create mode 100644 cmd/cli/dns_intercept_lifecycle_windows_test.go create mode 100644 cmd/cli/intercept_probe.go create mode 100644 cmd/cli/nrpt_external_gp.go create mode 100644 cmd/cli/nrpt_external_gp_test.go create mode 100644 cmd/cli/nrpt_external_gp_windows_test.go create mode 100644 cmd/cli/nrpt_handback_windows_test.go diff --git a/cmd/cli/dns_intercept_darwin.go b/cmd/cli/dns_intercept_darwin.go index 76863b9..6b30941 100644 --- a/cmd/cli/dns_intercept_darwin.go +++ b/cmd/cli/dns_intercept_darwin.go @@ -19,6 +19,10 @@ import ( "github.com/Control-D-Inc/ctrld" ) +// skipInitialDNSReset is Windows-only; macOS keeps the normal adapter cleanup +// before installing its pf redirect. +func (p *prog) skipInitialDNSReset() bool { return false } + const ( // pfWatchdogInterval is how often the periodic pf watchdog checks // that our anchor references are still present in the running ruleset. @@ -1855,14 +1859,9 @@ func (p *prog) probePFIntercept() bool { // Generate unique probe domain probeID := fmt.Sprintf("_pf-probe-%x.%s", time.Now().UnixNano()&0xFFFFFFFF, pfProbeDomain) - // Register probe so DNS handler can detect and signal it - probeCh := make(chan struct{}, 1) - p.pfProbeExpected.Store(probeID) - p.pfProbeCh.Store(&probeCh) - defer func() { - p.pfProbeExpected.Store("") - p.pfProbeCh.Store((*chan struct{})(nil)) - }() + // Register this attempt's own domain: overlapping probes must not cancel each other. + probeCh, deregister := p.registerInterceptProbe(probeID) + defer deregister() // Build a minimal DNS query packet for the probe domain. // We use exec.Command to send from a subprocess with GID=0 (wheel), diff --git a/cmd/cli/dns_intercept_lifecycle_windows_test.go b/cmd/cli/dns_intercept_lifecycle_windows_test.go new file mode 100644 index 0000000..9abad5d --- /dev/null +++ b/cmd/cli/dns_intercept_lifecycle_windows_test.go @@ -0,0 +1,319 @@ +//go:build windows + +package cli + +import ( + "runtime" + "testing" + "time" +) + +// newInterceptTestProg returns a prog with a published intercept state, fake NRPT +// operations already installed, and no WFP engine (engineHandle 0). +// +// The fake is installed here, before anything can inspect registry state, and it is the +// safety boundary - not the empty wfpState. A zero-valued state has owner None, and +// shutdown's None branch sweeps orphaned ctrld rules, so an unfaked stopDNSIntercept would +// reach the production nrptCatchAllRuleExists / removeNRPTCatchAllRule / signalNRPTChange. +// On a host that has ctrld's deterministic key - a developer box, or a CI runner where +// ctrld is installed - that deletes live policy and forces a Group Policy refresh, a +// Dnscache paramchange and a cache flush. A green run on a clean runner proves nothing +// about that. +func newInterceptTestProg(t *testing.T) (*prog, *wfpState, *fakeNRPTOps) { + t.Helper() + f := fakeNRPTOpsForTest(t) + // Prove the fake is in effect before anything can inspect registry state. Asserting + // zero side effects afterwards cannot do that: an uninstalled fake reports zero + // whether it was consulted or bypassed. + requireFakeNRPTOpsInstalled(t, f) + state := &wfpState{stopCh: make(chan struct{}), listenerIP: "127.0.0.1"} + p := &prog{} + p.dnsInterceptState = state + return p, state, f +} + +// assertNoNRPTSideEffects fails when a lifecycle path wrote NRPT policy or signalled the +// DNS Client. Every test in this file exercises a guard that is supposed to stand down, so +// any registry write or signal here means the guard did not hold - and, without the fake, +// would have hit the host's real policy. +func assertNoNRPTSideEffects(t *testing.T, f *fakeNRPTOps) { + t.Helper() + add, remove, signal, _ := f.counts() + if add != 0 || remove != 0 || signal != 0 { + t.Errorf("addRule = %d, removeRule = %d, signal = %d, want 0/0/0: this path must not write NRPT policy", + add, remove, signal) + } + if flush := f.flushCount(); flush != 0 { + t.Errorf("flush calls = %d, want 0: this path must not flush the resolver cache", flush) + } +} + +// TestStopDNSInterceptRevokesBeforeTeardown pins the ordering the shutdown/monitor race +// depends on. Teardown deletes our WFP sublayer, and a missing sublayer is precisely what +// the health monitor treats as "our filters were wiped, rebuild everything". Were the +// state revoked only after teardown, a monitor tick inside that window would rebuild the +// intercept during shutdown. +func TestStopDNSInterceptRevokesBeforeTeardown(t *testing.T) { + p, state, f := newInterceptTestProg(t) + defer assertNoNRPTSideEffects(t, f) + + if p.interceptStateRevoked(state) { + t.Fatal("a freshly published intercept state must not read as retired") + } + if err := p.stopDNSIntercept(); err != nil { + t.Fatalf("stopDNSIntercept() = %v", err) + } + if !p.interceptStateRevoked(state) { + t.Error("state still reads live after shutdown: the monitor and heal flows would keep writing host DNS state") + } + if p.dnsInterceptState != nil { + t.Error("dnsInterceptState survived shutdown") + } + if p.dnsInterceptStopRequested.Load() { + t.Error("stop-requested flag was left set; a later start would see a phantom shutdown") + } +} + +// TestRebuildDNSInterceptRefusedAfterShutdown is the regression test for the reported +// race: SCM stop runs resetDNS -> stopDNSIntercept while the health monitor is mid-tick, +// and the monitor then reaches the rebuild path before the process exits. The rebuild +// must refuse - completing it would re-add the NRPT catch-all and the WFP filters moments +// before ctrld disappears, leaving Windows resolving through a listener that is gone. +// +// That refusal is also what keeps this test safe on a real Windows host: a rebuild that +// did not refuse would run startDNSIntercept and write NRPT policy to the machine +// running the tests. +func TestRebuildDNSInterceptRefusedAfterShutdown(t *testing.T) { + p, state, f := newInterceptTestProg(t) + defer assertNoNRPTSideEffects(t, f) + if err := p.stopDNSIntercept(); err != nil { + t.Fatalf("stopDNSIntercept() = %v", err) + } + + if got := p.rebuildDNSIntercept(state, "WFP sublayer missing during health check"); got != interceptRebuildRetired { + t.Fatalf("rebuildDNSIntercept() = %v, want interceptRebuildRetired - a post-shutdown rebuild resurrects DNS interception", got) + } + if p.dnsInterceptState != nil { + t.Error("rebuild published new intercept state after shutdown") + } +} + +// TestRebuildDNSInterceptRefusedForReplacedState covers the other stale-owner case: an +// earlier rebuild already replaced the state, so a goroutine still holding the old one +// must not tear down its successor. +func TestRebuildDNSInterceptRefusedForReplacedState(t *testing.T) { + p, old, f := newInterceptTestProg(t) + defer assertNoNRPTSideEffects(t, f) + current := &wfpState{stopCh: make(chan struct{}), listenerIP: "127.0.0.1"} + p.dnsInterceptState = current + + if got := p.rebuildDNSIntercept(old, "WFP sublayer missing during health check"); got != interceptRebuildRetired { + t.Fatalf("rebuildDNSIntercept() = %v, want interceptRebuildRetired for a superseded state", got) + } + if p.dnsInterceptState != any(current) { + t.Error("a superseded state's rebuild replaced the live intercept") + } + if p.interceptStateRevoked(current) { + t.Error("the live state was revoked by a superseded rebuild") + } +} + +// TestRepairMissingWFPStandsDownAfterShutdown checks the monitor's entry point. It must +// not even query WFP for a retired state - the sublayer it looks for is what teardown +// just deleted - and it must tell the monitor goroutine to exit. +func TestRepairMissingWFPStandsDownAfterShutdown(t *testing.T) { + p, state, f := newInterceptTestProg(t) + defer assertNoNRPTSideEffects(t, f) + if err := p.stopDNSIntercept(); err != nil { + t.Fatalf("stopDNSIntercept() = %v", err) + } + // Set the handle only after teardown. A fake handle proves the revocation check + // comes first, but must never reach the real WFP calls in cleanupWFPFilters. + state.engineHandle = 1 + + if !p.repairMissingWFP(state) { + t.Error("repairMissingWFP() = false after shutdown; the health monitor would keep running for a dead intercept") + } + if p.dnsInterceptState != nil { + t.Error("repairMissingWFP rebuilt the intercept after shutdown") + } +} + +// TestPendingStopSignalsRevocation covers how a stop avoids waiting: while it is blocked +// on the lifecycle lock it must already read as revoked, so an in-flight NRPT heal +// abandons its probe backoff instead of making the service stop wait it out. A stop that +// waits too long is killed by the Service Control Manager, which cleans up nothing. +func TestPendingStopSignalsRevocation(t *testing.T) { + p, state, f := newInterceptTestProg(t) + defer assertNoNRPTSideEffects(t, f) + + p.dnsInterceptMu.Lock() + stopped := make(chan struct{}) + go func() { + defer close(stopped) + _ = p.stopDNSIntercept() + }() + + // Wait for the stop to announce itself while it is blocked on the lock. + deadline := time.Now().Add(5 * time.Second) + for !p.dnsInterceptStopRequested.Load() { + if time.Now().After(deadline) { + p.dnsInterceptMu.Unlock() + <-stopped + t.Fatal("stop never announced itself before waiting for the lifecycle lock") + } + runtime.Gosched() + } + if !p.interceptStateRevoked(state) { + t.Error("a pending stop does not read as revoked; the heal flows would keep it waiting") + } + p.dnsInterceptMu.Unlock() + <-stopped + + if p.dnsInterceptState != nil { + t.Error("the pending stop did not tear down the intercept once it acquired the lock") + } +} + +// TestInterceptWaitAbandonsPromptlyOnPendingStop is the bound on how long a stop can be +// delayed by a recovery flow: the heal sequence's waits add up to tens of seconds, and +// each one must end as soon as a stop is pending. +func TestInterceptWaitAbandonsPromptlyOnPendingStop(t *testing.T) { + p, state, f := newInterceptTestProg(t) + defer assertNoNRPTSideEffects(t, f) + p.dnsInterceptStopRequested.Store(true) + + start := time.Now() + if p.interceptWait(state, 30*time.Second) { + t.Fatal("interceptWait() = true with a stop pending; the caller would carry on writing host DNS state") + } + if elapsed := time.Since(start); elapsed > 2*time.Second { + t.Errorf("interceptWait took %v to notice a pending stop; shutdown would inherit that delay", elapsed) + } +} + +// TestInterceptWaitRunsToCompletionWhileLive guards the other direction: the cancellable +// wait must still actually wait, or the recovery flows lose their backoff. +func TestInterceptWaitRunsToCompletionWhileLive(t *testing.T) { + p, state, f := newInterceptTestProg(t) + defer assertNoNRPTSideEffects(t, f) + + start := time.Now() + if !p.interceptWait(state, 250*time.Millisecond) { + t.Fatal("interceptWait() = false for a live intercept") + } + if elapsed := time.Since(start); elapsed < 250*time.Millisecond { + t.Errorf("interceptWait returned after %v, want at least 250ms", elapsed) + } +} + +// TestNRPTNeedsCtrldActivation covers the recovery gap that left a machine unfiltered +// until restart: a failed NRPT write clears ownership, and an owner-None tick used to do +// nothing at all, so nothing ever retried the write. +func TestNRPTNeedsCtrldActivation(t *testing.T) { + tests := []struct { + name string + owner nrptRuleOwner + ruleExists bool + want bool + }{ + { + // The reported hole: activation failed, ownership was cleared, and no + // other path re-arms it. In hard mode WFP keeps blocking DNS meanwhile. + name: "no owner retries the failed write", + owner: nrptRuleOwnerNone, + want: true, + }, + { + name: "no owner retries even if a rule is somehow present", + owner: nrptRuleOwnerNone, + ruleExists: true, + want: true, + }, + { + name: "ctrld-owned rule removed externally is re-added", + owner: nrptRuleOwnerCtrld, + want: true, + }, + { + name: "healthy ctrld-owned rule is left alone", + owner: nrptRuleOwnerCtrld, + ruleExists: true, + want: false, + }, + { + // Writing beside external policy would be ambiguous policy, not recovery. + name: "external policy is never overwritten", + owner: nrptRuleOwnerGroupPolicy, + want: false, + }, + { + name: "external policy is never overwritten even with a ctrld rule present", + owner: nrptRuleOwnerGroupPolicy, + ruleExists: true, + want: false, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := nrptNeedsCtrldActivation(tc.owner, tc.ruleExists); got != tc.want { + t.Errorf("nrptNeedsCtrldActivation(%v, %v) = %v, want %v", tc.owner, tc.ruleExists, got, tc.want) + } + }) + } +} + +// TestActivateCtrldNRPTFallbackRefusedAfterShutdown guards the worst leftover. A +// catch-all re-added after shutdown points every DNS query on the machine at a listener +// that no longer exists, so nothing resolves at all. Refusing early also keeps this test +// from writing NRPT policy on the machine running it. +func TestActivateCtrldNRPTFallbackRefusedAfterShutdown(t *testing.T) { + p, state, f := newInterceptTestProg(t) + defer assertNoNRPTSideEffects(t, f) + if err := p.stopDNSIntercept(); err != nil { + t.Fatalf("stopDNSIntercept() = %v", err) + } + + if p.activateCtrldNRPTFallback(state, "ctrld-owned rule missing during health check") { + t.Error("activateCtrldNRPTFallback() = true after shutdown: the catch-all would outlive ctrld") + } + if owner, _ := state.nrptPolicyOwner(); owner != nrptRuleOwnerNone { + t.Errorf("NRPT owner = %v after a refused fallback, want nrptRuleOwnerNone", owner) + } +} + +// TestAllowHandbackAttemptRateLimits covers the throttle on testing an external +// catch-all. Each attempt takes ctrld's rule out of the way for a probe, so a rule that +// never routes would cost a brief DNS outage on every 30s health tick without this - in +// hard mode a window where WFP blocks DNS and nothing redirects it. +func TestHandbackThrottleIsPerRule(t *testing.T) { + state := &wfpState{stopCh: make(chan struct{})} + now := time.Now() + + if !state.handbackAllowed(now, "{GP-RULE}", nrptHandbackRetryInterval) { + t.Fatal("first handback attempt must be allowed") + } + // Checking alone must not spend the budget: a pre-probe can still abort the attempt + // without disturbing NRPT, and that must not cost the rule its next window. + if !state.handbackAllowed(now, "{GP-RULE}", nrptHandbackRetryInterval) { + t.Error("handbackAllowed must not consume the budget by itself") + } + + state.recordHandbackAttempt(now, "{GP-RULE}", nrptHandbackRetryInterval) + if state.handbackAllowed(now.Add(nrptHandbackRetryInterval-time.Second), "{GP-RULE}", nrptHandbackRetryInterval) { + t.Error("re-testing the same rule inside the interval must be suppressed") + } + // Group Policy alternating between two names must not erase either one's memory: + // with a single slot every swap costs another removal of the live rule. + if !state.handbackAllowed(now.Add(time.Second), "{OTHER-RULE}", nrptHandbackRetryInterval) { + t.Error("a different rule name means the administrator changed policy: test it now") + } + state.recordHandbackAttempt(now.Add(time.Second), "{OTHER-RULE}", nrptHandbackRetryInterval) + if state.handbackAllowed(now.Add(2*time.Second), "{GP-RULE}", nrptHandbackRetryInterval) { + t.Error("testing another rule must not clear the first rule's throttle") + } + + if !state.handbackAllowed(now.Add(2*nrptHandbackRetryInterval), "{GP-RULE}", nrptHandbackRetryInterval) { + t.Error("the same rule must be testable again after the interval") + } +} diff --git a/cmd/cli/dns_intercept_others.go b/cmd/cli/dns_intercept_others.go index cf7d107..6729f01 100644 --- a/cmd/cli/dns_intercept_others.go +++ b/cmd/cli/dns_intercept_others.go @@ -18,6 +18,9 @@ func (p *prog) stopDNSIntercept() error { return nil } +// skipInitialDNSReset is Windows-only; other platforms keep the normal reset. +func (p *prog) skipInitialDNSReset() bool { return false } + // exemptVPNDNSServers is a no-op on unsupported platforms. func (p *prog) exemptVPNDNSServers(exemptions []vpnDNSExemption) error { return nil diff --git a/cmd/cli/dns_intercept_windows.go b/cmd/cli/dns_intercept_windows.go index 9f99db4..7776303 100644 --- a/cmd/cli/dns_intercept_windows.go +++ b/cmd/cli/dns_intercept_windows.go @@ -9,6 +9,7 @@ import ( "net" "os/exec" "runtime" + "strings" "sync" "sync/atomic" "time" @@ -235,6 +236,14 @@ type fwpmAction0 struct { filterType windows.GUID // union: filterType or calloutKey } +type nrptRuleOwner uint8 + +const ( + nrptRuleOwnerNone nrptRuleOwner = iota + nrptRuleOwnerCtrld + nrptRuleOwnerGroupPolicy +) + // wfpState holds the state of the WFP DNS interception filters. // It tracks the engine handle and all filter IDs for cleanup on shutdown. // All filter IDs are stored so we can remove them individually without @@ -258,18 +267,20 @@ type wfpState struct { // Static permit filter IDs for RFC1918/CGNAT subnet ranges. // These allow VPN DNS servers on private IPs to work without dynamic exemptions. subnetPermitFilterIDs []uint64 - // nrptActive tracks whether the NRPT catch-all rule was successfully added. - // Used by stopDNSIntercept to know whether cleanup is needed. - nrptActive bool + // nrptOwner distinguishes a ctrld-created rule from a GP rule that ctrld is + // only observing. Shutdown and recovery must never delete the latter. + nrptOwner nrptRuleOwner + // externalGPRuleName is the GP child key currently routing the catch-all to + // listenerIP. It is diagnostic identity, not an ownership claim. + externalGPRuleName string // listenerIP is the actual IP address ctrld is listening on (e.g., "127.0.0.1" // or "127.0.0.2" on AD DC). Used by NRPT rule creation and health monitor to // ensure NRPT points to the correct address. listenerIP string // stopCh is used to shut down the NRPT health monitor goroutine. stopCh chan struct{} - // mu protects loopbackProtectActive, loopbackPermitIDs, and engineHandle - // from concurrent access between nrptProbeAndHeal (goroutine) and - // stopDNSIntercept / cleanupWFPFilters (main goroutine). + // mu protects NRPT ownership, externalGPRuleName, loopbackProtectActive, + // loopbackPermitIDs, and engineHandle from concurrent monitor/recovery/stop use. mu sync.Mutex // loopbackProtectActive is true when DNS mode has activated a minimal WFP // session to permit loopback DNS. This counters third-party WFP block filters @@ -281,6 +292,59 @@ type wfpState struct { // nrptRecoveryLimiter prevents repeated Windows policy/Dnscache signaling // when another agent keeps putting NRPT back into a broken state. nrptRecoveryLimiter nrptRecoveryLimiter + // handbackAttempts records, per external rule name, when ctrld last removed its own + // catch-all to test whether that rule routes on its own. Protected by mu. + // + // It is a map rather than one (rule, time) pair because Group Policy can alternate + // between two rule names: with a single slot each swap erases the memory of the other + // one, and every swap costs another removal of the live rule. + handbackAttempts map[string]time.Time +} + +// handbackAllowed reports whether ctrld may test external rule ruleName again, without +// recording anything. +// +// Each attempt takes ctrld's rule out of the way for a probe, so a rule that never routes +// would otherwise cost a brief DNS outage on every health tick - in hard mode a window +// where WFP blocks DNS with nothing redirecting it. A different rule name means the +// administrator changed policy, which is worth testing immediately; the same name is held +// off for minInterval. +func (s *wfpState) handbackAllowed(now time.Time, ruleName string, minInterval time.Duration) bool { + s.mu.Lock() + defer s.mu.Unlock() + last, ok := s.handbackAttempts[ruleName] + return !ok || now.Sub(last) >= minInterval +} + +// recordHandbackAttempt spends ruleName's budget. Callers record only once an attempt is +// actually about to disturb NRPT, so a cheap abort - a pre-probe that shows nothing is +// routing - does not cost the rule its next 15 minutes. +func (s *wfpState) recordHandbackAttempt(now time.Time, ruleName string, minInterval time.Duration) { + s.mu.Lock() + defer s.mu.Unlock() + if s.handbackAttempts == nil { + s.handbackAttempts = make(map[string]time.Time, 2) + } + // Drop entries whose window has passed, so a churning GP store cannot grow this map. + for rule, at := range s.handbackAttempts { + if now.Sub(at) >= minInterval { + delete(s.handbackAttempts, rule) + } + } + s.handbackAttempts[ruleName] = now +} + +func (s *wfpState) nrptPolicyOwner() (nrptRuleOwner, string) { + s.mu.Lock() + defer s.mu.Unlock() + return s.nrptOwner, s.externalGPRuleName +} + +func (s *wfpState) setNRPTPolicyOwner(owner nrptRuleOwner, externalGPRuleName string) { + s.mu.Lock() + defer s.mu.Unlock() + s.nrptOwner = owner + s.externalGPRuleName = externalGPRuleName } // Lazy-loaded WFP DLL procedures. @@ -328,8 +392,6 @@ const ( // from both locations, but on some machines (including stock Win11) it only // honors the direct path. This is the same path Add-DnsClientNrptRule uses. nrptDirectKey = `SYSTEM\CurrentControlSet\Services\Dnscache\Parameters\DnsPolicyConfig` - // nrptRuleName is the name of our specific rule key under the GP path. - nrptRuleName = `CtrldCatchAll` // nrptDirectRuleName is the key name for the direct service store path. // The DNS Client requires direct-path rules to use GUID-in-braces format. // Using a plain name like "CtrldCatchAll" makes the rule visible in @@ -339,6 +401,107 @@ const ( nrptDirectRuleName = `{B2E9A3C1-7F4D-4A8E-9D6B-5C1E0F3A2B8D}` ) +func (p *prog) nrptListenerIP() string { + listenerIP := "127.0.0.1" + if lc := p.cfg.FirstListener(); lc != nil && lc.IP != "" && lc.IP != "0.0.0.0" && lc.IP != "::" { + listenerIP = lc.IP + } + return listenerIP +} + +// skipInitialDNSReset is the first half of GP-rule adoption. postRun normally +// resets adapter DNS before setDNS starts intercept mode; doing that first would +// violate externally managed policy even if startDNSIntercept adopted the GP rule +// a few milliseconds later. This is only a read-only candidate check. The rule is +// not trusted until a DNS Client probe reaches the listener and the same child is +// re-read by startDNSIntercept. +func (p *prog) skipInitialDNSReset() bool { + mode := p.configuredInterceptMode() + if mode != "dns" && mode != "hard" { + return false + } + if ruleName := findMatchingGPNRPTRule(p.nrptListenerIP()); ruleName != "" { + mainLog.Load().Info().Str("rule", ruleName). + Msg("DNS intercept: matching GP-managed NRPT candidate found - preserving adapter DNS until functional verification") + return true + } + return false +} + +// findMatchingGPNRPTRule returns the first non-ctrld GP child that is exactly a +// catch-all for listenerIP. Multiple namespaces or nameservers are deliberately +// rejected: ctrld must not infer exclusive routing from a broader policy shape. +func findMatchingGPNRPTRule(listenerIP string) string { + parent, err := registry.OpenKey(registry.LOCAL_MACHINE, nrptBaseKey, registry.ENUMERATE_SUB_KEYS) + if err != nil { + return "" + } + names, err := parent.ReadSubKeyNames(-1) + parent.Close() + if err != nil { + return "" + } + for _, name := range names { + if gpNRPTRuleMatches(name, listenerIP) { + return name + } + } + return "" +} + +func gpNRPTRuleMatches(ruleName, listenerIP string) bool { + namespaces, dnsServers, ok := readGPNRPTRule(ruleName) + return ok && isMatchingGPNRPTRule(ruleName, namespaces, dnsServers, listenerIP) +} + +func readGPNRPTRule(ruleName string) ([]string, string, bool) { + if ruleName == "" || strings.EqualFold(ruleName, nrptRuleName) { + return nil, "", false + } + key, err := registry.OpenKey(registry.LOCAL_MACHINE, nrptBaseKey+`\`+ruleName, registry.QUERY_VALUE) + if err != nil { + return nil, "", false + } + defer key.Close() + namespaces, _, err := key.GetStringsValue("Name") + if err != nil { + return nil, "", false + } + dnsServers, _, err := key.GetStringValue("GenericDNSServers") + if err != nil { + // A malformed external catch-all is still authoritative enough to block + // ctrld from creating a second catch-all; it simply cannot be adopted. + dnsServers = "" + } + return namespaces, dnsServers, true +} + +// findConflictingGPCatchAll reports an administrator-owned catch-all that no +// longer targets ctrld. Adding another GP catch-all beside it would create the +// same ambiguous policy class as a competing local-store rule, so callers leave +// policy untouched and wait for the administrator to restore/remove it. +func findConflictingGPCatchAll(listenerIP string) (string, string) { + parent, err := registry.OpenKey(registry.LOCAL_MACHINE, nrptBaseKey, registry.ENUMERATE_SUB_KEYS) + if err != nil { + return "", "" + } + names, err := parent.ReadSubKeyNames(-1) + parent.Close() + if err != nil { + return "", "" + } + for _, name := range names { + namespaces, dnsServers, ok := readGPNRPTRule(name) + if !ok || !isExternalGPCatchAll(name, namespaces) { + continue + } + if !isMatchingGPNRPTRule(name, namespaces, dnsServers, listenerIP) { + return name, dnsServers + } + } + return "", "" +} + // addNRPTCatchAllRule creates an NRPT catch-all rule that directs all DNS queries // to the specified listener IP. // @@ -520,29 +683,55 @@ func nrptCatchAllRuleExists() bool { // // Uses RefreshPolicyEx(bMachine=TRUE, dwOptions=RP_FORCE=1) from userenv.dll. // See: https://learn.microsoft.com/en-us/windows/win32/api/userenv/nf-userenv-refreshpolicyex +// nrptSignalExecTimeout bounds every helper process the NRPT signalling path shells out +// to. These run while a transition holds nrptTransitionMu, and a service stop takes that +// same lock: "gpupdate /force" against a slow or unreachable domain controller can take +// tens of seconds, which is exactly the Service Control Manager timeout the locking exists +// to avoid. A signal that cannot finish in this window has already failed as a nudge. +const nrptSignalExecTimeout = 10 * time.Second + +// runBoundedNRPTExec runs one signalling helper with a hard deadline and reports its +// combined output. +func runBoundedNRPTExec(name string, args ...string) ([]byte, error) { + ctx, cancel := context.WithTimeout(context.Background(), nrptSignalExecTimeout) + defer cancel() + out, err := exec.CommandContext(ctx, name, args...).CombinedOutput() + if ctx.Err() != nil { + mainLog.Load().Warn().Str("command", name).Dur("timeout", nrptSignalExecTimeout). + Msg("DNS intercept: NRPT signalling helper timed out and was killed") + } + return out, err +} + +func runGPUpdate() { + if out, err := runBoundedNRPTExec("gpupdate", "/target:computer", "/force"); err != nil { + mainLog.Load().Debug().Msgf("DNS intercept: gpupdate failed: %v: %s", err, string(out)) + } else { + mainLog.Load().Debug().Msg("DNS intercept: triggered GP refresh via gpupdate") + } +} + func refreshNRPTPolicy() { if err := userenvDLL.Load(); err != nil { mainLog.Load().Debug().Err(err).Msg("DNS intercept: userenv.dll not available, falling back to gpupdate") - if out, err := exec.Command("gpupdate", "/target:computer", "/force").CombinedOutput(); err != nil { - mainLog.Load().Debug().Msgf("DNS intercept: gpupdate failed: %v: %s", err, string(out)) - } else { - mainLog.Load().Debug().Msg("DNS intercept: triggered GP refresh via gpupdate") - } + runGPUpdate() return } if err := procRefreshPolicyEx.Find(); err != nil { mainLog.Load().Debug().Err(err).Msg("DNS intercept: RefreshPolicyEx not found, falling back to gpupdate") - exec.Command("gpupdate", "/target:computer", "/force").Run() + runGPUpdate() return } // RefreshPolicyEx(BOOL bMachine, DWORD dwOptions) - // bMachine=1 (TRUE) = refresh computer policy, dwOptions=1 (RP_FORCE) = force refresh + // bMachine=1 (TRUE) = refresh computer policy, dwOptions=1 (RP_FORCE) = force refresh. + // This one only asks the policy engine to refresh and returns; it does not wait for a + // domain controller, which is why it is preferred over gpupdate. ret, _, _ := procRefreshPolicyEx.Call(1, 1) if ret != 0 { mainLog.Load().Debug().Msg("DNS intercept: triggered machine GP refresh via RefreshPolicyEx") } else { mainLog.Load().Debug().Msg("DNS intercept: RefreshPolicyEx returned FALSE, falling back to gpupdate") - exec.Command("gpupdate", "/target:computer", "/force").Run() + runGPUpdate() } } @@ -564,7 +753,7 @@ func flushDNSCacheOnly() { } } } - if out, err := exec.Command("ipconfig", "/flushdns").CombinedOutput(); err != nil { + if out, err := runBoundedNRPTExec("ipconfig", "/flushdns"); err != nil { mainLog.Load().Debug().Msgf("DNS intercept: ipconfig /flushdns failed: %v: %s", err, string(out)) } else { mainLog.Load().Debug().Msg("DNS intercept: flushed DNS resolver cache via ipconfig /flushdns") @@ -591,13 +780,23 @@ func signalNRPTChange() { // the OS cannot reach those servers on port 53 — queries fail and fall back // to ctrld via the loopback address. func (p *prog) startDNSIntercept() error { - // Resolve the actual listener IP. On AD DC / Windows Server with a local DNS - // server, ctrld may have fallen back to 127.0.0.x:53 instead of 127.0.0.1:53. - // NRPT must point to whichever address ctrld is actually listening on. - listenerIP := "127.0.0.1" - if lc := p.cfg.FirstListener(); lc != nil && lc.IP != "" && lc.IP != "0.0.0.0" && lc.IP != "::" { - listenerIP = lc.IP - } else if lc != nil && (lc.IP == "0.0.0.0" || lc.IP == "::") { + p.dnsInterceptMu.Lock() + defer p.dnsInterceptMu.Unlock() + return p.startDNSInterceptLocked() +} + +// startDNSInterceptLocked is startDNSIntercept with p.dnsInterceptMu already held, so +// the rebuild path can make teardown and re-create one atomic transition. +// +// Nothing it calls may take p.dnsInterceptMu. It runs nrptProbeAndHeal synchronously, +// and that whole family - nrptProbeAndHeal, activateCtrldNRPTFallback, +// tryAdoptMatchingGPNRPT - stays lock-free on purpose and uses interceptStateRevoked +// instead: those flows wait seconds between probes, and holding the lifecycle lock +// across them would make a service stop wait just as long. +func (p *prog) startDNSInterceptLocked() error { + ops := p.nrptOps() + listenerIP := p.nrptListenerIP() + if lc := p.cfg.FirstListener(); lc != nil && (lc.IP == "0.0.0.0" || lc.IP == "::") { mainLog.Load().Warn().Str("configured_ip", lc.IP). Msg("DNS intercept: listener configured with wildcard IP, using 127.0.0.1 for NRPT rules") } @@ -606,35 +805,131 @@ func (p *prog) startDNSIntercept() error { stopCh: make(chan struct{}), listenerIP: listenerIP, } + // The probe and heal flows take state as an argument rather than reading the + // published field, so nothing is published until startup has fully succeeded. + // Publishing a provisional state would expose a half-built wfpState - no engine + // handle yet, filter IDs still being assigned - to exemptVPNDNSServers, which the + // VPN DNS manager can call at any time. - // Step 1: Add NRPT catch-all rule (both dns and hard modes). - // NRPT must succeed before proceeding with WFP in hard mode. mainLog.Load().Info().Msgf("DNS intercept: initializing (mode: %s)", interceptMode) logNRPTParentKeyState("pre-write") - // Empty parent key recovery: if the GP DnsPolicyConfig key exists but is - // empty, DNS Client enters GP mode and hides local rules. Delete empty - // parents first, then send one change signal so DNS Client drops stale state. - if cleanEmptyNRPTParent() { - signalNRPTChange() + // GP adoption is a two-part proof. Registry shape establishes ownership; the + // DNS Client probe establishes that the policy actually routes to this listener. + // Re-reading the same child after the probe prevents adopting a rule replaced + // during a concurrent Group Policy refresh. + externalProbeOK := false + if ruleName := ops.findGPRule(listenerIP); ruleName != "" { + // Adoption goes through the same handback transition the running service uses. + // A rule left behind by an earlier unclean exit points at this very listener, so + // a probe taken with it still installed proves nothing about the GP rule; the + // transition removes ctrld's keys first, and puts them back if the GP rule + // cannot carry DNS on its own. + switch p.nrptHandbackToExternal(state, ruleName, "startup GP-managed catch-all candidate") { + case nrptHandbackVerified: + externalProbeOK = true + mainLog.Load().Info().Str("rule", ruleName).Str("listener", listenerIP). + Msg("DNS intercept: adopted working GP-managed NRPT catch-all; ctrld will not modify NRPT policy") + case nrptHandbackUnverified: + mainLog.Load().Warn().Str("rule", ruleName).Str("listener", listenerIP). + Msg("DNS intercept: GP-managed NRPT catch-all is present but the probe did not reach ctrld; leaving external policy untouched") + case nrptHandbackKeptCtrld: + mainLog.Load().Warn().Str("rule", ruleName).Str("listener", listenerIP). + Msg("DNS intercept: GP-managed NRPT catch-all could not carry DNS alone; keeping the ctrld-owned rule from the previous run") + case nrptHandbackConflict: + // The candidate turned out to target another resolver. Startup then refuses to + // write a competing rule below, which is a hard startup failure by design. + mainLog.Load().Error().Str("rule", ruleName).Str("listener", listenerIP). + Msg("DNS intercept: GP catch-all does not target ctrld; refusing to write a competing NRPT rule") + case nrptHandbackAborted: + // Undecided, not decided: nothing was proved about the candidate and no + // ownership was recorded. Say so, because the fall-through below writes + // ctrld's rule, and an unlogged fall-through here is indistinguishable from + // "no external policy exists". + mainLog.Load().Warn().Str("rule", ruleName).Str("listener", listenerIP). + Msg("DNS intercept: GP-managed NRPT candidate could not be tested at startup; continuing without external ownership") + } } - if err := addNRPTCatchAllRule(listenerIP); err != nil { - return fmt.Errorf("dns intercept: failed to add NRPT catch-all rule: %w", err) + owner, _ := state.nrptPolicyOwner() + if owner == nrptRuleOwnerNone { + // No working external ownership contract exists. Preserve the current ctrld + // path unless another GP catch-all already owns the namespace; writing a + // second catch-all would create an ambiguous policy rather than recovery. + if gpCatchAllConflictBlocksFallback(state, "startup GP catch-all does not target ctrld") { + return fmt.Errorf("dns intercept: conflicting GP NRPT catch-all targets another resolver") + } + // A matching candidate that is still there means the handback above came back + // undecided - typically because the pre-probe ran while the DNS Client was still + // settling at boot. Give it one more pass before writing anything: the DNS Client + // has had the startup work since, and a decision here avoids writing beside an + // administrator rule that no probe has tested. + if ruleName := ops.findGPRule(listenerIP); ruleName != "" { + switch p.nrptHandbackToExternal(state, ruleName, "startup retry of an undecided GP candidate") { + case nrptHandbackVerified: + externalProbeOK = true + case nrptHandbackUnverified, nrptHandbackConflict, nrptHandbackKeptCtrld: + // Ownership is recorded by the transition (or ctrld's own rule was put + // back), so the write below is neither needed nor safe. + } + } } - logNRPTParentKeyState("post-write") - state.nrptActive = true - signalNRPTChange() - mainLog.Load().Info().Msgf("DNS intercept: NRPT catch-all rule active — all DNS queries directed to %s", listenerIP) - // Step 2: In hard mode, also set up WFP filters to block non-local DNS. + owner, _ = state.nrptPolicyOwner() + if owner == nrptRuleOwnerNone { + if ops.ruleExists() { + // A rule from an earlier run already points at this listener. Adopt it rather + // than writing a second time: with a GP child present, addNRPTCatchAllRule + // would also write ctrld's GP-path sibling. + state.setNRPTPolicyOwner(nrptRuleOwnerCtrld, "") + mainLog.Load().Info().Str("listener", listenerIP). + Msg("DNS intercept: adopting the ctrld NRPT catch-all already present from an earlier run") + } else { + if ops.cleanParent() { + ops.signal() + } + if err := ops.addRule(listenerIP); err != nil { + return fmt.Errorf("dns intercept: failed to add NRPT catch-all rule: %w", err) + } + logNRPTParentKeyState("post-write") + state.setNRPTPolicyOwner(nrptRuleOwnerCtrld, "") + ops.signal() + mainLog.Load().Info().Msgf("DNS intercept: NRPT catch-all rule active - all DNS queries directed to %s", listenerIP) + } + } + + // In hard mode, also set up WFP filters to block non-local DNS. if hardIntercept { - if err := p.startWFPFilters(state); err != nil { - // Roll back NRPT since WFP failed. - mainLog.Load().Error().Err(err).Msg("DNS intercept: WFP setup failed, rolling back NRPT") - _ = removeNRPTCatchAllRule() - flushDNSCache() - state.nrptActive = false + if err := ops.startWFP(state); err != nil { + owner, _ := state.nrptPolicyOwner() + if owner == nrptRuleOwnerGroupPolicy && externalProbeOK { + // A GP rule the probe proved is routing keeps DNS flowing through ctrld + // even with no WFP filters, and rewriting adapter DNS would violate that + // external policy - so this is not a fall-back-to-adapter-DNS failure. + // + // It is still a hard-mode enforcement gap: with no block filters, raw DNS + // to a public resolver, DoH clients and apps with their own resolver are + // not filtered at all. Publish the state and start the health monitor so + // repairMissingWFP keeps retrying WFP instead of the process running + // unenforced for its whole life on one error line. Ownership stays with + // Group Policy so the monitor never writes NRPT policy here. + mainLog.Load().Error().Err(err). + Msg("DNS intercept: WFP setup failed while GP-managed NRPT is verified routing - DNS resolves through ctrld but hard-mode enforcement is OFF; retrying WFP in the background") + p.dnsInterceptState = state + go p.nrptHealthMonitor(state) + // The service start is still reported as failed (setDnsOK stays false in + // setDNS): a hard-mode process with no block filters must not read as a + // healthy start, even though name resolution works. + return fmt.Errorf("dns intercept: WFP setup failed: %w: %w", err, errGPNRPTVerified) + } + if owner == nrptRuleOwnerCtrld { + mainLog.Load().Error().Err(err).Msg("DNS intercept: WFP setup failed, rolling back ctrld-owned NRPT") + _ = ops.removeRule() + ops.flush() + } else { + mainLog.Load().Error().Err(err).Msg("DNS intercept: WFP setup failed; leaving GP-managed NRPT untouched") + } + state.setNRPTPolicyOwner(nrptRuleOwnerNone, "") return fmt.Errorf("dns intercept: WFP setup failed: %w", err) } } else { @@ -645,26 +940,80 @@ func (p *prog) startDNSIntercept() error { // only) and use CLEAR_ACTION_RIGHT to override any block from other // sublayers. Adding them at startup eliminates the DNS outage window // that would otherwise occur between VPN connect and reactive activation. - if err := p.activateLoopbackWFPProtect(state); err != nil { + if err := ops.loopback(state); err != nil { // Non-fatal: loopback protect is a defense-in-depth measure. // NRPT still works when no third-party WFP blocks are present. mainLog.Load().Warn().Err(err).Msg("DNS intercept: failed to activate proactive loopback WFP protect — will retry on probe failure") } } - p.dnsInterceptState = state + owner, externalRuleName := state.nrptPolicyOwner() + if owner == nrptRuleOwnerGroupPolicy && !externalProbeOK { + // The first probe ran before loopback WFP protection existed. Verify once + // more synchronously after WFP setup so service readiness does not race an + // async proof; this path is ownership-aware and never mutates GP NRPT. + externalProbeOK = p.nrptProbeAndHeal(state) + } - // Start periodic NRPT health monitor. + // Everything the host needs is in place: publish, then start the goroutines that + // keep it that way. They receive state directly, so this ordering is only about + // when the rest of ctrld may observe intercept mode as active. + p.dnsInterceptState = state go p.nrptHealthMonitor(state) - // Verify NRPT is actually working (async — doesn't block startup). - // This catches the race condition where RefreshPolicyEx returns before - // the DNS Client service has loaded the NRPT rule from registry. - go p.nrptProbeAndHeal() + owner, _ = state.nrptPolicyOwner() + if owner == nrptRuleOwnerCtrld { + // ctrld-owned policy keeps the existing asynchronous activation/heal path. + go p.nrptProbeAndHeal(state) + } + + if owner == nrptRuleOwnerGroupPolicy && !externalProbeOK { + // External policy owns the namespace and neither synchronous proof reached ctrld. + // The recovery state and monitor stay up - the rule may start routing once the DNS + // Client settles, and only external policy may fix it - but this start is not + // ready: the DNS Client is not delivering queries to ctrld, adapter DNS was + // deliberately preserved, and no owned fallback may be written beside an + // administrator's catch-all. Reporting success here would publish readiness while + // nothing is filtering, and in hard mode WFP is simultaneously blocking every + // other resolver, which is an outage rather than degraded health. + mainLog.Load().Error().Str("rule", externalRuleName).Str("listener", listenerIP). + Msg("DNS intercept: GP-managed NRPT owns the namespace but no probe reached ctrld; leaving adapter DNS untouched and reporting a failed start until a probe succeeds") + return fmt.Errorf("dns intercept: %w (rule %q)", errGPNRPTIneffective, externalRuleName) + } return nil } +// removeOrphanedCtrldNRPTRule deletes a ctrld-owned NRPT catch-all that no running +// ctrld is backing. It is safe against external policy: the rule is found by ctrld's +// own deterministic GUID, never by shape. +// +// Without this, one unclean exit can strand a rule that later takes the whole machine +// off DNS. ctrld dies without a stop while it owns policy, so the GUID rule stays in +// the local store pointing at 127.0.0.1. The org then deploys a GP catch-all: from that +// point every start adopts the GP rule and every stop takes the GP branch, so nothing +// ever looks at the local store - including the stop during uninstall. The orphan stays +// invisible, because any rule in the GP store puts the DNS Client in GP mode where the +// local store is ignored entirely. When the admin eventually removes the GP rule - +// most plausibly while cleaning up after uninstalling ctrld - the DNS Client leaves GP +// mode, reads the local store again, and every query on the machine goes to a listener +// that has not existed for months. It is also miserable to diagnose: local-store rules +// do not appear in Get-DnsClientNrptPolicy, so the standard tooling reports no policy +// at all while nothing resolves. +func (p *prog) removeOrphanedCtrldNRPTRule(reason string) { + ops := p.nrptOps() + if !ops.ruleExists() { + return + } + mainLog.Load().Warn().Str("reason", reason). + Msg("DNS intercept: removing orphaned ctrld NRPT catch-all left by an earlier run") + if err := ops.removeRule(); err != nil { + mainLog.Load().Warn().Err(err).Msg("DNS intercept: failed to remove orphaned ctrld NRPT catch-all") + return + } + ops.signal() +} + // startWFPFilters opens the WFP engine and adds all block/permit filters. // Called only in hard intercept mode. func (p *prog) startWFPFilters(state *wfpState) error { @@ -1017,12 +1366,15 @@ func (p *prog) cleanupWFPFilters(state *wfpState) { return } - // Clean up loopback protect filters (DNS mode VPN workaround). + // Hold state.mu across the whole teardown: engineHandle and every filter ID slice + // below is shared with the VPN DNS exemption path and the recovery flows. state.mu.Lock() + defer state.mu.Unlock() + + // Clean up loopback protect filters (DNS mode VPN workaround). loopbackIDs := state.loopbackPermitIDs state.loopbackPermitIDs = nil state.loopbackProtectActive = false - state.mu.Unlock() for _, filterID := range loopbackIDs { r1, _, _ := procFwpmFilterDeleteById0.Call(state.engineHandle, uintptr(filterID)) if r1 != 0 { @@ -1264,6 +1616,27 @@ func (p *prog) addWFPHardPermitLocalhostFilter(engineHandle uintptr, name string // stopDNSIntercept removes all WFP filters and shuts down the DNS interception. func (p *prog) stopDNSIntercept() error { + // Announce the stop before waiting for the lifecycle lock. A rebuild that holds it + // runs a full start, whose NRPT verification can sit in probe backoffs for seconds; + // the flag is what lets those flows abandon their work instead of making the stop + // wait. A stop that waits too long is not a delay but a failure mode: the Service + // Control Manager kills ctrld on timeout, and then nothing is cleaned up at all. + p.dnsInterceptStopRequested.Store(true) + p.dnsInterceptMu.Lock() + defer p.dnsInterceptMu.Unlock() + defer p.dnsInterceptStopRequested.Store(false) + return p.stopDNSInterceptLocked() +} + +// stopDNSInterceptLocked is stopDNSIntercept with p.dnsInterceptMu already held. +// +// It revokes the state before removing anything. Teardown deletes the WFP sublayer, and +// a missing sublayer is exactly what the health monitor reads as "our filters were +// wiped, rebuild everything" - so a monitor tick landing inside the shutdown window +// would otherwise re-add the NRPT catch-all and the WFP filters moments before the +// process exits, leaving Windows resolving through a ctrld that is gone. Revoking first +// means every such flow sees a retired state and stands down. +func (p *prog) stopDNSInterceptLocked() error { if p.dnsInterceptState == nil { mainLog.Load().Debug().Msg("DNS intercept: no state to clean up") return nil @@ -1271,22 +1644,48 @@ func (p *prog) stopDNSIntercept() error { state := p.dnsInterceptState.(*wfpState) + // Revoke first, then remove. Both signals are what the monitor, the delayed + // rechecks and the NRPT heal flows read to decide whether they may still write + // host DNS state. + p.dnsInterceptState = nil // Stop the health monitor goroutine. if state.stopCh != nil { close(state.stopCh) } - // Remove NRPT rule BEFORE WFP cleanup — restore normal DNS resolution - // before removing the block filters that enforce it. - if state.nrptActive { - if err := removeNRPTCatchAllRule(); err != nil { - mainLog.Load().Warn().Err(err).Msg("DNS intercept: failed to remove NRPT catch-all rule") + // Remove only ctrld-owned NRPT state. A GP-owned catch-all is an external + // deployment contract and must survive service stop, restart, and uninstall. + // + // Hold the transition lock across the removal so an in-flight NRPT transition + // finishes first and any later one sees the revoked state under the same lock. The + // stop-requested flag is already set, so an in-flight transition abandons its probes + // rather than making this wait. + p.nrptTransitionMu.Lock() + defer p.nrptTransitionMu.Unlock() + + ops := p.nrptOps() + owner, externalRuleName := state.nrptPolicyOwner() + switch owner { + case nrptRuleOwnerCtrld: + if err := ops.removeRule(); err != nil { + mainLog.Load().Warn().Err(err).Msg("DNS intercept: failed to remove ctrld-owned NRPT catch-all rule") } else { - mainLog.Load().Info().Msg("DNS intercept: removed NRPT catch-all rule") + mainLog.Load().Info().Msg("DNS intercept: removed ctrld-owned NRPT catch-all rule") } - flushDNSCache() - state.nrptActive = false + ops.flush() + case nrptRuleOwnerGroupPolicy: + mainLog.Load().Info().Str("rule", externalRuleName). + Msg("DNS intercept: leaving GP-managed NRPT catch-all untouched during shutdown") + // External policy stays, but a ctrld rule from an earlier unclean exit must + // not: while GP mode hides the local store, this stop is the last chance + // anything will look there. See removeOrphanedCtrldNRPTRule. + p.removeOrphanedCtrldNRPTRule("shutdown with externally owned NRPT policy") + case nrptRuleOwnerNone: + // No ownership was ever established this run - an activation that failed, or a + // start that never got that far. A ctrld rule found here is still ours. + p.removeOrphanedCtrldNRPTRule("shutdown with no NRPT owner") } + state.setNRPTPolicyOwner(nrptRuleOwnerNone, "") // Clean up WFP if the engine was opened (hard mode or loopback protect). if state.engineHandle != 0 { @@ -1295,11 +1694,129 @@ func (p *prog) stopDNSIntercept() error { mainLog.Load().Info().Msg("DNS intercept: WFP shutdown complete") } - p.dnsInterceptState = nil mainLog.Load().Info().Msg("DNS intercept: shutdown complete") return nil } +// interceptRebuildResult reports what rebuildDNSIntercept did. +type interceptRebuildResult int + +const ( + // interceptRebuildRetired means the caller's state was no longer the live one - + // shutdown revoked it, or an earlier rebuild replaced it - so nothing was touched. + interceptRebuildRetired interceptRebuildResult = iota + // interceptRebuildDone means the intercept was torn down and re-created. + interceptRebuildDone + // interceptRebuildFailed means teardown ran but the re-create returned an error. + // dnsInterceptState is nil afterwards, except on the one path that publishes a + // partial intercept on purpose - hard mode with verified GP NRPT but no WFP - which + // leaves a health monitor running to keep retrying. + interceptRebuildFailed +) + +// rebuildDNSIntercept tears the intercept down and creates it again, for callers that +// found our filters wiped from underneath us. +// +// It refuses unless state is still the published intercept state. That check is what +// stops a health monitor tick from resurrecting DNS interception during or after a +// service stop: shutdown revokes the state under this same lock before it removes +// anything, so a monitor arriving late finds its state retired and does nothing. +// Holding the lock across teardown and create also means a stop that arrives +// mid-rebuild waits, then tears down whatever the rebuild published - never a +// half-built intercept. +// +// Whatever the result, the caller's state is dead afterwards: the monitor goroutine +// that owns it must exit. +// rebuildDNSInterceptFn is the rebuild entry point. Indirected so tests can assert which +// conditions ask for a rebuild without running a real teardown and start. Assigned in +// init because the rebuild reaches back here through the health monitor. +var rebuildDNSInterceptFn func(*prog, *wfpState, string) interceptRebuildResult + +func init() { + rebuildDNSInterceptFn = (*prog).rebuildDNSIntercept +} + +func (p *prog) rebuildDNSIntercept(state *wfpState, reason string) interceptRebuildResult { + p.dnsInterceptMu.Lock() + defer p.dnsInterceptMu.Unlock() + + if live, ok := p.dnsInterceptState.(*wfpState); !ok || live != state { + mainLog.Load().Info().Str("reason", reason). + Msg("DNS intercept: not rebuilding - this intercept was already retired by shutdown or an earlier rebuild") + return interceptRebuildRetired + } + + mainLog.Load().Warn().Str("reason", reason).Msg("DNS intercept: rebuilding interception") + _ = p.stopDNSInterceptLocked() + if err := p.startDNSInterceptLocked(); err != nil { + mainLog.Load().Error().Err(err).Str("reason", reason).Msg("DNS intercept: rebuild failed") + return interceptRebuildFailed + } + return interceptRebuildDone +} + +// interceptStateRevoked reports whether state has been retired, meaning nothing may +// write host DNS state on its behalf any more. +// +// stopDNSInterceptLocked closes stopCh before it removes the NRPT rule or the WFP +// filters, and a stop waiting for the lifecycle lock sets dnsInterceptStopRequested +// first. Together they are the cheap shutdown signal for the flows that must not take +// p.dnsInterceptMu: the NRPT probe and heal sequences, which wait seconds between +// probes and also run from inside the locked start path. +func (p *prog) interceptStateRevoked(state *wfpState) bool { + if state == nil || state.stopCh == nil { + return true + } + if p.dnsInterceptStopRequested.Load() { + return true + } + select { + case <-state.stopCh: + return true + default: + return false + } +} + +// interceptRevocationPollInterval is how quickly the recovery flows notice a stop that +// is waiting for the lifecycle lock. A pending stop can only set a flag - it cannot +// close stopCh until it owns the lock - so waits poll instead of selecting on a channel. +const interceptRevocationPollInterval = 100 * time.Millisecond + +// interceptWait waits for d, or until the intercept is retired, whichever comes first. +// It reports whether the caller may keep working. +// +// Every wait in the NRPT recovery flows goes through this. Those flows can be holding +// the lifecycle lock - startDNSInterceptLocked runs one synchronously, and a rebuild +// holds the lock across the whole start - and their backoffs add up to tens of seconds. +// A plain time.Sleep there makes a service stop wait that long for the lock, risking +// the Service Control Manager killing ctrld before it removes the NRPT rule and the WFP +// filters, which is the unclean shutdown the locking exists to prevent. Bounding each +// wait by the shutdown signal keeps a stop's wait to about one poll interval plus +// whatever uncancellable Windows call is in flight. +func (p *prog) interceptWait(state *wfpState, d time.Duration) bool { + deadline := time.Now().Add(d) + for { + if p.interceptStateRevoked(state) { + return false + } + remaining := time.Until(deadline) + if remaining <= 0 { + return true + } + if remaining > interceptRevocationPollInterval { + remaining = interceptRevocationPollInterval + } + timer := time.NewTimer(remaining) + select { + case <-state.stopCh: + timer.Stop() + return false + case <-timer.C: + } + } +} + // exemptVPNDNSServers updates the WFP filters to permit outbound DNS to the given // VPN DNS server IPs. This prevents the block filters from intercepting ctrld's own // forwarded queries to VPN DNS servers (split DNS routing). @@ -1317,6 +1834,12 @@ func (p *prog) exemptVPNDNSServers(exemptions []vpnDNSExemption) error { if !ok || state == nil { return fmt.Errorf("DNS intercept state not available") } + // engineHandle, loopbackProtectActive and vpnPermitFilterIDs are all shared with the + // monitor, the recovery flows and teardown, so this runs under state.mu like every + // other reader and writer of them. + state.mu.Lock() + defer state.mu.Unlock() + // In dns mode (no WFP) or loopback-protect-only mode, VPN DNS exemptions // are not needed — there are no ctrld block filters to exempt from. // Loopback protect only adds hard-permit filters for localhost DNS; @@ -1548,6 +2071,582 @@ const pfAnchorRecheckDelay = 2 * time.Second // pfAnchorRecheckDelayLong is the longer delayed re-check for slower VPN teardowns. const pfAnchorRecheckDelayLong = 4 * time.Second +func gpCatchAllConflictBlocksFallback(state *wfpState, reason string) bool { + ruleName, dnsServers := findConflictingGPCatchAll(state.listenerIP) + if ruleName == "" { + return false + } + mainLog.Load().Error().Str("rule", ruleName).Str("nameservers", dnsServers).Str("reason", reason). + Msg("DNS intercept: GP catch-all targets another resolver; refusing to create a competing fallback rule") + return true +} + +// nrptOps is the seam between NRPT ownership decisions and the Windows side effects +// they cause. Tests substitute it to drive a transition - probe outcomes, registry +// state, concurrency - without touching the host's registry or DNS Client. +type nrptOps struct { + probe func(state *wfpState) bool + ruleExists func() bool + addRule func(listenerIP string) error + removeRule func() error + signal func() + findGPRule func(listenerIP string) string + gpRuleMatches func(ruleName, listenerIP string) bool + gpConflicts func(state *wfpState, reason string) bool + loopback func(state *wfpState) error + wait func(state *wfpState, d time.Duration) bool + flush func() + parentEmpty func(keyPath string) bool + cleanParent func() bool + startWFP func(state *wfpState) error +} + +// nrptOpsForTest overrides the NRPT side effects. Windows tests only. +var nrptOpsForTest *nrptOps + +func (p *prog) nrptOps() nrptOps { + if nrptOpsForTest != nil { + return *nrptOpsForTest + } + return nrptOps{ + probe: p.probeNRPT, + ruleExists: nrptCatchAllRuleExists, + addRule: addNRPTCatchAllRule, + removeRule: removeNRPTCatchAllRule, + signal: signalNRPTChange, + findGPRule: findMatchingGPNRPTRule, + gpRuleMatches: gpNRPTRuleMatches, + gpConflicts: p.gpCatchAllConflictBlocksFallbackOps, + loopback: p.activateLoopbackWFPProtect, + wait: p.interceptWait, + flush: flushDNSCache, + parentEmpty: nrptParentKeyEmpty, + cleanParent: cleanEmptyNRPTParent, + startWFP: p.startWFPFilters, + } +} + +func (p *prog) gpCatchAllConflictBlocksFallbackOps(state *wfpState, reason string) bool { + return gpCatchAllConflictBlocksFallback(state, reason) +} + +// nrptHandbackResult reports how a handback attempt ended. +type nrptHandbackResult int + +const ( + // nrptHandbackVerified: external policy owns NRPT and proved, with ctrld's own keys + // gone, that it routes to this listener. + nrptHandbackVerified nrptHandbackResult = iota + // nrptHandbackUnverified: external policy owns the namespace but is not routing. + // Nothing of ctrld's was removed, so there was nothing to lose by recording it. + nrptHandbackUnverified + // nrptHandbackKeptCtrld: the proof failed with ctrld's rule removed, so the rule was + // restored and ctrld keeps ownership. + nrptHandbackKeptCtrld + // nrptHandbackAborted: shutdown landed, a registry step failed, the attempt was + // throttled, or the candidate child is no longer there. Nothing was decided. + nrptHandbackAborted + // nrptHandbackConflict: an administrator-owned catch-all is present that does not + // target ctrld. External policy owns the namespace and ctrld must not write beside it. + nrptHandbackConflict +) + +// nrptHandbackRetryInterval bounds how often ctrld will take its own rule out of the way +// to re-test the same external catch-all. Each attempt briefly removes the only working +// route, so retrying on every 30s health tick would be its own outage. A rule the +// administrator has changed is retested immediately regardless. +const nrptHandbackRetryInterval = 15 * time.Minute + +// nrptHandbackProbePasses bounds how many probes one handback spends chasing a Group +// Policy store that keeps changing under it. Each pass costs a probe timeout, and the +// budget being spent is not an excuse to guess: see externalAfterRemoval. +const nrptHandbackProbePasses = 2 + +// nrptHandbackToExternal is the only path that records external (Group Policy) +// ownership of NRPT. +// +// The proof has to be produced with ctrld's own keys gone. A probe taken while the ctrld +// fallback is still installed can be answered by that fallback, so a present-but- +// ineffective GP child would otherwise let ctrld delete the last working route and then +// declare external ownership. In hard mode that is a machine-wide DNS outage: WFP keeps +// blocking outbound DNS with nothing redirecting it to ctrld. +// +// The transition is: remove only ctrld's own keys, signal, probe again, re-read the same +// GP child - and if that second probe fails, put ctrld's rule back and keep ctrld +// ownership. Everything runs under nrptTransitionMu so a stop cannot interleave with it. +func (p *prog) nrptHandbackToExternal(state *wfpState, ruleName, reason string) nrptHandbackResult { + ops := p.nrptOps() + + p.nrptTransitionMu.Lock() + defer p.nrptTransitionMu.Unlock() + + if p.interceptStateRevoked(state) { + return nrptHandbackAborted + } + + if !ops.gpRuleMatches(ruleName, state.listenerIP) { + return nrptHandbackAborted + } + + // Nothing of ours in the way: a probe already measures external policy alone, and + // refusing would mean writing a competing catch-all beside an administrator's rule. + if !ops.ruleExists() { + child, class, routes := p.externalAfterRemoval(ops, state, ruleName, reason) + switch { + case class == gpChildSameExact && routes: + state.setNRPTPolicyOwner(nrptRuleOwnerGroupPolicy, child) + state.nrptRecoveryLimiter.recordStableSuccess() + mainLog.Load().Info().Str("rule", child).Str("reason", reason). + Msg("DNS intercept: GP-managed catch-all verified routing to ctrld; external policy owns NRPT") + return nrptHandbackVerified + case class == gpChildSameExact: + state.setNRPTPolicyOwner(nrptRuleOwnerGroupPolicy, child) + mainLog.Load().Warn().Str("rule", child).Str("reason", reason). + Msg("DNS intercept: GP-managed catch-all owns the namespace but is not routing; leaving external policy untouched") + return nrptHandbackUnverified + case class == gpChildConflicting: + // An administrator-owned catch-all now targets another resolver, or is + // malformed. It owns the namespace and ctrld must not write a sibling. + state.setNRPTPolicyOwner(nrptRuleOwnerNone, "") + mainLog.Load().Error().Str("rule", ruleName).Str("reason", reason). + Msg("DNS intercept: GP catch-all changed to one that does not target ctrld; refusing to write a competing rule") + return nrptHandbackConflict + default: + // External policy is gone, or the store is still churning. Decide nothing: + // the caller re-reads and retries. + return nrptHandbackAborted + } + } + + // ctrld's rule is installed. Taking it out to test the external one is disruptive, so + // it is throttled: an external rule that never routes would otherwise cost a brief + // outage on every health tick. The check comes before the probe so a throttled tick + // costs nothing, and the budget is only spent below, once the attempt is real. + now := time.Now() + if !state.handbackAllowed(now, ruleName, nrptHandbackRetryInterval) { + return nrptHandbackAborted + } + + // Pre-probe: this measures whatever routes DNS today, ctrld's own rule included, so + // it can never prove anything about external policy - it only says whether there is a + // working route here to risk. If nothing is routing there is nothing to protect and + // nothing to compare against, so leave it to the heal cycle rather than start + // deleting rules. + if !ops.probe(state) { + return nrptHandbackAborted + } + state.recordHandbackAttempt(now, ruleName, nrptHandbackRetryInterval) + + if err := ops.removeRule(); err != nil { + mainLog.Load().Warn().Err(err).Str("rule", ruleName).Str("reason", reason). + Msg("DNS intercept: GP catch-all found but ctrld's own rule could not be removed for the handback probe") + return nrptHandbackAborted + } + ops.signal() + if !ops.wait(state, nrptHandbackSettleDelay) { + // Shutdown landed. NRPT is clean, which is the right state to leave behind. + return nrptHandbackAborted + } + + // Post-removal probe and classification, always - not only when the probe succeeded. + // Group Policy can refresh during the probe, and what it changed into decides whether + // restoring ctrld's rule is right or would create a sibling that must never be written. + child, class, routes := p.externalAfterRemoval(ops, state, ruleName, reason) + + switch { + case class == gpChildSameExact && routes: + state.setNRPTPolicyOwner(nrptRuleOwnerGroupPolicy, child) + state.nrptRecoveryLimiter.recordStableSuccess() + mainLog.Load().Info().Str("rule", child).Str("listener", state.listenerIP).Str("reason", reason). + Msg("DNS intercept: GP-managed catch-all carried DNS without ctrld's rule; returned NRPT ownership to Group Policy") + return nrptHandbackVerified + + case class == gpChildConflicting: + // The child changed into a catch-all that does not target ctrld. It owns the + // namespace, so ctrld's rule stays off: restoring it here is exactly the + // competing sibling beside administrator policy that is forbidden elsewhere. + state.setNRPTPolicyOwner(nrptRuleOwnerNone, "") + mainLog.Load().Error().Str("rule", ruleName).Str("reason", reason). + Msg("DNS intercept: GP catch-all changed to one that does not target ctrld during the handback probe; leaving NRPT to Group Policy and not restoring the ctrld rule") + return nrptHandbackConflict + + case class == gpChildSameExact && child != ruleName: + // A different administrator catch-all took the namespace while ctrld's keys were + // off, and its own pass says it is not routing yet. ctrld's rule still must not + // come back: addNRPTCatchAllRule writes ctrld's GP catch-all whenever another GP + // rule exists, so restoring would put a sibling beside a catch-all that was not + // even there when this transition started. Record external ownership and let the + // health monitor keep watching the new child. + state.setNRPTPolicyOwner(nrptRuleOwnerGroupPolicy, child) + mainLog.Load().Warn().Str("old_rule", ruleName).Str("rule", child).Str("reason", reason). + Msg("DNS intercept: a different GP catch-all took the namespace during the handback probe and is not routing yet; leaving NRPT to Group Policy without restoring the ctrld rule") + return nrptHandbackUnverified + + } + + // The original child is still exact but cannot carry DNS, or external policy is gone + // altogether: ctrld's route is the one that has to come back. This only restores the + // state the transition started from, so it creates no new sibling. + if err := ops.addRule(state.listenerIP); err != nil { + mainLog.Load().Error().Err(err).Str("rule", ruleName). + Msg("DNS intercept: handback probe failed and the ctrld NRPT rule could not be restored; the health monitor will retry") + state.setNRPTPolicyOwner(nrptRuleOwnerNone, "") + return nrptHandbackAborted + } + ops.signal() + state.setNRPTPolicyOwner(nrptRuleOwnerCtrld, "") + if class == gpChildGone { + mainLog.Load().Warn().Str("rule", ruleName).Str("reason", reason). + Msg("DNS intercept: GP-managed catch-all disappeared during the handback probe; restored the ctrld fallback and kept ctrld ownership") + } else { + mainLog.Load().Warn().Str("rule", ruleName).Str("reason", reason). + Msg("DNS intercept: GP-managed catch-all did not carry DNS without ctrld's rule; restored the ctrld fallback and kept ctrld ownership") + } + return nrptHandbackKeptCtrld +} + +// gpChildClass is what an external NRPT catch-all looks like at the moment ctrld checks, +// which is not necessarily what it looked like when a probe was sent: Group Policy can +// refresh while the probe is in flight. +type gpChildClass int + +const ( + // gpChildSameExact: the same child still names exactly this listener. + gpChildSameExact gpChildClass = iota + // gpChildReplaced: a different child now matches this listener exactly. It has not + // proved anything yet, so it is not adopted on this pass. + gpChildReplaced + // gpChildConflicting: an administrator-owned catch-all is present that does not target + // ctrld - another resolver, or malformed. ctrld must not write a rule beside it. + gpChildConflicting + // gpChildGone: no external catch-all owns the namespace any more. + gpChildGone +) + +// externalAfterRemoval probes external policy with ctrld's keys already gone, then says +// which child the result belongs to, what state that child is in, and whether it routed. +// It writes nothing: the caller decides what the verdict means. +// +// A probe result can only ever be attributed to the child that was on disk for the whole +// probe. When Group Policy swaps the child mid-probe the old result is void, so the +// replacement gets one pass of its own here rather than inheriting a verdict it never +// earned. Two passes is the limit; a store that keeps changing is reported as still +// churning so the caller can retry instead of guessing. +func (p *prog) externalAfterRemoval(ops nrptOps, state *wfpState, candidate, reason string) (string, gpChildClass, bool) { + for pass := 0; pass < nrptHandbackProbePasses; pass++ { + routes := ops.probe(state) + class := p.classifyGPChild(ops, state, candidate, reason) + if class != gpChildReplaced { + return candidate, class, routes + } + if pass == nrptHandbackProbePasses-1 { + break + } + next := ops.findGPRule(state.listenerIP) + if next == "" { + return candidate, gpChildGone, routes + } + mainLog.Load().Warn().Str("old_rule", candidate).Str("rule", next).Str("reason", reason). + Msg("DNS intercept: a different GP catch-all appeared during the probe; testing that one instead") + candidate = next + } + + // The probe budget is spent and the store is still moving. Report the store as it is + // now, with no route proved, rather than reporting churn: "undecided" would let + // startup and owned recovery fall through to writing ctrld's rule, and + // addNRPTCatchAllRule puts that in the GP path beside whatever exact catch-all is + // there - the sibling that must never exist. Failing safe from the current store + // keeps every outcome terminal for those callers unless the namespace is genuinely + // free. + mainLog.Load().Warn().Str("rule", candidate).Str("reason", reason). + Msg("DNS intercept: GP catch-alls kept changing during the handback probe; classifying the store as it stands with no route proved") + if current := ops.findGPRule(state.listenerIP); current != "" { + return current, gpChildSameExact, false + } + if ops.gpConflicts(state, reason) { + return candidate, gpChildConflicting, false + } + return candidate, gpChildGone, false +} + +// classifyGPChild re-reads the GP store and says what state the external catch-all is in. +// Every post-probe decision goes through it, so a child that changed mid-probe can never +// be treated as the child that was measured. +func (p *prog) classifyGPChild(ops nrptOps, state *wfpState, ruleName, reason string) gpChildClass { + if ops.gpRuleMatches(ruleName, state.listenerIP) { + return gpChildSameExact + } + if other := ops.findGPRule(state.listenerIP); other != "" { + return gpChildReplaced + } + if ops.gpConflicts(state, reason) { + return gpChildConflicting + } + return gpChildGone +} + +// nrptHandbackSettleDelay gives the DNS Client a moment to drop ctrld's removed rule +// before the handback probe decides whether external policy routes on its own. +const nrptHandbackSettleDelay = 1 * time.Second + +// deferToExternalCatchAll stops ctrld-owned recovery when an administrator catch-all owns +// the namespace, and reports whether the caller must stop. +// +// It hands back where a verdict is possible. Where one is not - the handback needs a +// working route to compare against, and during a heal cycle there often is none - the +// presence of an exact external catch-all is itself the ownership signal, so ownership is +// recorded without proof and recovery stops anyway. Continuing would mean signalling the +// DNS Client, or deleting and recreating ctrld's rule, while administrator policy owns the +// namespace: the two things #576 forbids. The health monitor keeps testing the rule and +// can hand back properly once it routes. +func (p *prog) deferToExternalCatchAll(ops nrptOps, state *wfpState, reason string) bool { + ruleName := ops.findGPRule(state.listenerIP) + if ruleName == "" || !ops.gpRuleMatches(ruleName, state.listenerIP) { + return false + } + + switch result := p.nrptHandbackToExternal(state, ruleName, reason); { + case result == nrptHandbackKeptCtrld: + // The external rule was proved unable to carry DNS and ctrld's rule is back, so + // owned recovery is exactly what should continue. + return false + case nrptExternalOwns(result): + if result == nrptHandbackUnverified { + p.healBlockedLoopbackDNS(state, reason) + } + return true + default: + state.setNRPTPolicyOwner(nrptRuleOwnerGroupPolicy, ruleName) + mainLog.Load().Warn().Str("rule", ruleName).Str("reason", reason). + Msg("DNS intercept: a GP catch-all owns the namespace but could not be tested; stopping ctrld-owned recovery and leaving external policy untouched") + p.healBlockedLoopbackDNS(state, reason) + return true + } +} + +// nrptExternalOwns reports whether a handback left external policy owning the namespace. +// +// All three outcomes count, not just Verified: an administrator's catch-all owns the +// namespace whether it routes to ctrld (Verified), does not route at all (Unverified), or +// points somewhere else entirely (Conflict). Each is terminal for ctrld-owned recovery, +// because continuing would signal the DNS Client and delete and recreate ctrld's rule +// beside that catch-all - the competing, ambiguous policy #576 exists to avoid. Where the +// external rule is present but ineffective, loopback WFP protect is the only remediation +// left. +func nrptExternalOwns(result nrptHandbackResult) bool { + switch result { + case nrptHandbackVerified, nrptHandbackUnverified, nrptHandbackConflict: + return true + default: + return false + } +} + +// nrptTransition runs one complete NRPT mutation - write or delete, signal, record +// owner - with the transition lock held, and reports whether it ran. +// +// Checking interceptStateRevoked and then mutating is not enough on its own: a stop can +// land in between, revoke the state, remove NRPT and finish, after which the mutation +// would write a catch-all pointing at a listener that no longer exists. The stop takes +// this same lock around its own NRPT removal, so holding it across the whole +// observe-mutate-signal step is what makes the two mutually exclusive. Callers must keep +// probe backoffs outside fn: the lock is for a single transition, not for a heal cycle. +func (p *prog) nrptTransition(state *wfpState, fn func()) bool { + p.nrptTransitionMu.Lock() + defer p.nrptTransitionMu.Unlock() + if p.interceptStateRevoked(state) { + return false + } + fn() + return true +} + +func (p *prog) activateCtrldNRPTFallback(state *wfpState, reason string) bool { + ops := p.nrptOps() + + // Close the observation-to-write race in both directions. Group Policy can refresh + // between the monitor's missing-rule check and this call, and again between this + // check and the write - so the write path re-checks under the transition lock and + // sends us back here when a matching child has appeared. Two passes is enough: the + // second either hands back or writes with the store checked under the lock. + for attempt := 0; attempt < 2; attempt++ { + if ruleName := ops.findGPRule(state.listenerIP); ruleName != "" && + ops.gpRuleMatches(ruleName, state.listenerIP) { + // A matching child owns the namespace, so hand back rather than create a + // sibling rule. The handback runs its own transition, so it cannot be called + // with the lock held. + if p.nrptHandbackToExternal(state, ruleName, reason) == nrptHandbackKeptCtrld { + // The handback restored ctrld's rule, which is what this call wanted. + return true + } + // Every other outcome means external policy owns the namespace, or that + // nothing could be decided. Either way this must not write a rule beside it. + return false + } + wrote, gpAppeared := p.writeCtrldFallback(ops, state, reason) + if !gpAppeared { + return wrote + } + } + return false +} + +// writeCtrldFallback writes ctrld's own catch-all under the transition lock. It reports +// whether it wrote, and whether it stood down because a matching GP child appeared - in +// which case the caller must take the handback path instead. +func (p *prog) writeCtrldFallback(ops nrptOps, state *wfpState, reason string) (wrote, gpAppeared bool) { + p.nrptTransitionMu.Lock() + defer p.nrptTransitionMu.Unlock() + + // Re-check under the transition lock. A retired state must not write NRPT policy: + // shutdown has already removed the ctrld-owned catch-all, and re-adding it + // afterwards leaves the DNS Client routing every query to a listener that no longer + // exists - a machine-wide resolution outage, not a cosmetic leftover. Checking + // before the lock cannot close that gap, because a stop can land between the check + // and the write; the stop takes this same lock around its own NRPT removal. + if p.interceptStateRevoked(state) { + mainLog.Load().Debug().Str("reason", reason). + Msg("DNS intercept: skipping ctrld NRPT fallback - intercept was retired") + return false, false + } + // The same reasoning applies to the GP store, which the caller read before taking + // this lock. gpConflicts alone does not cover it: findConflictingGPCatchAll skips a + // *matching* child by design, so without this re-read a policy refresh inside that + // window would let the write land beside an administrator catch-all that no probe has + // tested. + if ruleName := ops.findGPRule(state.listenerIP); ruleName != "" && + ops.gpRuleMatches(ruleName, state.listenerIP) { + mainLog.Load().Info().Str("rule", ruleName).Str("reason", reason). + Msg("DNS intercept: a matching GP catch-all appeared before the fallback write; handing back instead of writing beside it") + return false, true + } + if ops.gpConflicts(state, reason) { + return false, false + } + // Another health or delayed-recheck path may have written the rule while this one + // waited for the lock; do not duplicate the write and the signalling. + if ops.ruleExists() { + state.setNRPTPolicyOwner(nrptRuleOwnerCtrld, "") + mainLog.Load().Debug().Str("reason", reason). + Msg("DNS intercept: ctrld NRPT rule was already restored by a concurrent transition") + return false, false + } + if err := ops.addRule(state.listenerIP); err != nil { + mainLog.Load().Error().Err(err).Str("reason", reason). + Msg("DNS intercept: failed to activate ctrld-owned NRPT fallback; the health monitor will retry") + state.setNRPTPolicyOwner(nrptRuleOwnerNone, "") + return false, false + } + state.setNRPTPolicyOwner(nrptRuleOwnerCtrld, "") + ops.signal() + mainLog.Load().Warn().Str("reason", reason). + Msg("DNS intercept: GP-managed catch-all unavailable - activated ctrld-owned NRPT fallback") + return true, false +} + +// tryAdoptMatchingGPNRPT hands NRPT ownership back to Group Policy when a matching +// external catch-all exists. It reports whether external policy now owns NRPT. +// +// The proof lives in nrptHandbackToExternal: a probe taken while ctrld's fallback is +// still installed proves nothing about the external rule, so the decision is always +// made with ctrld's keys removed. +func (p *prog) tryAdoptMatchingGPNRPT(state *wfpState) bool { + if p.interceptStateRevoked(state) { + return false + } + ruleName := p.nrptOps().findGPRule(state.listenerIP) + if ruleName == "" { + return false + } + switch p.nrptHandbackToExternal(state, ruleName, "matching GP-managed catch-all detected") { + case nrptHandbackVerified: + return true + case nrptHandbackUnverified: + // External policy owns the namespace but is not routing. ctrld must not write a + // competing rule, so the only remediation left is loopback WFP protect. Report + // external ownership: the caller must not fall through to owned recovery. + p.healBlockedLoopbackDNS(state, "GP-managed catch-all owns the namespace but is not routing") + return true + case nrptHandbackConflict: + // External policy now points somewhere other than ctrld. It owns the namespace, + // so report external ownership: owned recovery must not write beside it. + return true + case nrptHandbackKeptCtrld: + // The external rule could not carry DNS alone. A third-party WFP block dropping + // DNS below NRPT looks exactly like this, so try the one remediation that leaves + // external policy untouched; the next handback attempt can then succeed. + p.healBlockedLoopbackDNS(state, "GP-managed catch-all did not carry DNS on its own") + return false + } + return false +} + +// nrptNeedsCtrldActivation reports whether ctrld should write its own NRPT catch-all, +// given who owns policy and whether a ctrld rule is currently present. +// +// Owner None is the interesting case: it means an earlier write failed. Nothing else +// re-arms it - nrptProbeAndHeal returns early without ctrld ownership, and the health +// monitor used to skip the owner-None tick entirely - so without retrying, the machine +// keeps no NRPT rule for the rest of the process lifetime. In hard mode that is a full +// DNS outage rather than degraded interception: WFP goes on blocking outbound DNS while +// nothing redirects it to ctrld, and only a restart recovers. +func nrptNeedsCtrldActivation(owner nrptRuleOwner, ruleExists bool) bool { + switch owner { + case nrptRuleOwnerNone: + return true + case nrptRuleOwnerCtrld: + return !ruleExists + default: + // nrptRuleOwnerGroupPolicy: external policy owns the namespace, and a competing + // ctrld catch-all beside it would be ambiguous policy, not recovery. + return false + } +} + +// loopbackProtectSettleDelay gives WFP a moment to apply the loopback permits before +// the retry probe goes out. +const loopbackProtectSettleDelay = 500 * time.Millisecond + +// healBlockedLoopbackDNS retries the NRPT probe from behind loopback WFP protection and +// reports whether the probe then succeeded. +// +// A failed probe does not always mean the NRPT rule is wrong. Third-party WFP filters - +// OpenVPN's block-outside-dns is the common one - can drop DNS below NRPT, so the +// policy is correct and the packets never arrive. Loopback protect is the one +// remediation that fixes this while leaving externally owned policy untouched. Without +// this attempt, a GP rule blocked that way is never adopted and never healed: the +// monitor keeps finding a "present but not routing" rule for the life of the process. +// It is also the only remediation ctrld may run while external policy owns NRPT. +// signalNRPTChange is not a content-neutral nudge - it forces machine Group Policy via +// RefreshPolicyEx, sends Dnscache paramchange and flushes the resolver cache - so #576's +// contract for a present-but-ineffective GP rule is a warning plus WFP-only retries, with +// no refresh/paramchange/flush loop. +func (p *prog) healBlockedLoopbackDNS(state *wfpState, reason string) bool { + ops := p.nrptOps() + if p.interceptStateRevoked(state) { + return false + } + if hardIntercept { + // Hard mode owns the whole sublayer; loopback protect deliberately does + // nothing there, so a retry probe would only burn the probe timeout. + return false + } + if err := ops.loopback(state); err != nil { + mainLog.Load().Warn().Err(err).Str("reason", reason). + Msg("DNS intercept: could not activate loopback WFP protect while retrying a failed probe") + return false + } + if !ops.wait(state, loopbackProtectSettleDelay) { + return false + } + if !ops.probe(state) { + return false + } + mainLog.Load().Info().Str("reason", reason). + Msg("DNS intercept: probe recovered behind loopback WFP protect - a third-party WFP block was dropping DNS below NRPT") + return true +} + // scheduleDelayedRechecks schedules delayed OS resolver and VPN DNS refreshes after // network change events. While WFP filters don't get wiped like pf anchors, the OS // resolver and VPN DNS state can still be stale after VPN disconnect (same issue as macOS). @@ -1564,39 +2663,84 @@ func (p *prog) scheduleDelayedRechecks() { p.vpnDNS.Refresh(true) } - // NRPT watchdog: some VPN software clears NRPT policy rules on - // connect/disconnect. Re-add our catch-all rule if it was removed. + // Delayed rechecks must respect NRPT ownership. The old path looked only + // for ctrld's deterministic key, so it recreated that key immediately + // after startup had correctly adopted a GP-managed catch-all. state, ok := p.dnsInterceptState.(*wfpState) - if ok && state.nrptActive && !nrptCatchAllRuleExists() { - mainLog.Load().Warn().Msg("DNS intercept: NRPT catch-all rule was removed externally — re-adding") - if err := addNRPTCatchAllRule(state.listenerIP); err != nil { - mainLog.Load().Error().Err(err).Msg("DNS intercept: failed to re-add NRPT catch-all rule") - state.nrptActive = false - } else { - signalNRPTChange() - mainLog.Load().Info().Msg("DNS intercept: NRPT catch-all rule restored") + if ok && !p.interceptStateRevoked(state) { + owner, _ := state.nrptPolicyOwner() + switch owner { + case nrptRuleOwnerGroupPolicy: + if findMatchingGPNRPTRule(state.listenerIP) == "" { + if p.activateCtrldNRPTFallback(state, "matching GP rule disappeared during delayed network recheck") { + go p.nrptProbeAndHeal(state) + } + } + case nrptRuleOwnerNone, nrptRuleOwnerCtrld: + if nrptNeedsCtrldActivation(owner, nrptCatchAllRuleExists()) { + mainLog.Load().Warn().Msg("DNS intercept: no ctrld NRPT catch-all in place - re-adding") + if p.activateCtrldNRPTFallback(state, "ctrld NRPT rule missing during delayed network recheck") { + go p.nrptProbeAndHeal(state) + } + } } } // WFP watchdog: verify our sublayer still exists. If another program - // or a crash removed it, the block filters are gone too. - if ok && state.engineHandle != 0 && !wfpSublayerExists(state.engineHandle) { + // or a crash removed it, the block filters are gone too. A timer that + // fires during shutdown must not rebuild what teardown removed, so this + // goes through rebuildDNSIntercept's ownership check. + if ok && !p.interceptStateRevoked(state) && state.engineHandle != 0 && !wfpSublayerExists(state.engineHandle) { mainLog.Load().Warn().Msg("DNS intercept: WFP sublayer was removed externally — re-creating all filters") - // Full teardown + re-init. stopDNSIntercept clears state, - // then startDNSIntercept creates everything fresh. - _ = p.stopDNSIntercept() - if err := p.startDNSIntercept(); err != nil { - mainLog.Load().Error().Err(err).Msg("DNS intercept: failed to re-create WFP filters") - } + rebuildDNSInterceptFn(p, state, "WFP sublayer removed externally (delayed network recheck)") } }) } } +// repairMissingWFP re-creates the intercept when hard mode has no WFP enforcement - +// because the sublayer disappeared, or because the engine never opened in the first place. +// It reports whether the caller's monitor goroutine must stop. +func (p *prog) repairMissingWFP(state *wfpState) bool { + // Never interrogate WFP for a retired state: shutdown deletes our sublayer, so a + // tick landing in the shutdown window would read "missing" and try to rebuild what + // stopDNSIntercept just removed. + if p.interceptStateRevoked(state) { + return true + } + + reason := "" + switch { + case state.engineHandle == 0: + // In dns mode a closed engine is the normal state: loopback protect opens one + // only when it is needed. In hard mode it means startWFPFilters never got the + // engine open, so nothing is being blocked at all. That is the state a startup + // WFP failure leaves behind, and this is what retries it - without this entry + // point the process would run unenforced for its whole life. + if !hardIntercept { + return false + } + reason = "hard mode has no WFP engine - enforcement never started" + case wfpSublayerExists(state.engineHandle): + return false + default: + reason = "WFP sublayer missing during health check" + } + + mainLog.Load().Warn().Str("reason", reason).Msg("DNS intercept: WFP health check - re-initializing all filters") + if rebuildDNSInterceptFn(p, state, reason) == interceptRebuildDone { + mainLog.Load().Info().Msg("DNS intercept: WFP filters restored by health monitor") + } + return true +} + // nrptHealthMonitor periodically checks that the NRPT catch-all rule is still // present and re-adds it if removed by VPN software or Group Policy updates. // In hard mode, it also verifies the WFP sublayer exists and re-initializes // all filters if they were removed. +// +// One monitor belongs to one intercept state, and it exits when that state is retired: +// nothing here may act on state after stopDNSIntercept or a rebuild has moved on. func (p *prog) nrptHealthMonitor(state *wfpState) { ticker := time.NewTicker(30 * time.Second) defer ticker.Stop() @@ -1605,12 +2749,67 @@ func (p *prog) nrptHealthMonitor(state *wfpState) { case <-state.stopCh: return case <-ticker.C: - if !state.nrptActive { + // A tick and a shutdown can become ready together and select picks either, + // so re-check before doing any health work for a retired intercept. + if p.interceptStateRevoked(state) { + return + } + owner, externalRuleName := state.nrptPolicyOwner() + switch owner { + case nrptRuleOwnerNone: + // No owner means an earlier NRPT write failed. Nothing else re-arms + // it - nrptProbeAndHeal returns early without ctrld ownership, and the + // missing-rule branch below used to be unreachable from here - so + // without this retry the machine keeps no NRPT rule for the rest of the + // process lifetime. In hard mode that is a full DNS outage: WFP still + // blocks outbound DNS while nothing redirects it to ctrld. Fall through + // to the activation path below. + case nrptRuleOwnerGroupPolicy: + currentRule := p.nrptOps().findGPRule(state.listenerIP) + if currentRule == "" { + if p.activateCtrldNRPTFallback(state, "matching GP rule disappeared during health check") { + go p.nrptProbeAndHeal(state) + } + continue + } + // Confirm through the handback transition so the verdict is always about + // external policy alone, never about a ctrld rule left behind by an + // earlier run that happens to answer the probe. + switch p.nrptHandbackToExternal(state, currentRule, "GP-managed NRPT health check") { + case nrptHandbackVerified: + // Ownership and stable-success are recorded by the transition. + case nrptHandbackKeptCtrld: + // External policy could not carry DNS; ctrld owns the rule again. + // The next tick continues as ctrld-owned. + case nrptHandbackConflict: + // The administrator's catch-all no longer targets ctrld. Nothing to + // remediate: ctrld may not write beside it, and the conflict is + // already logged by the transition. + default: + mainLog.Load().Warn().Str("rule", externalRuleName). + Msg("DNS intercept: GP-managed NRPT rule is present but its probe failed; leaving external policy untouched") + // The only remediation allowed over external policy: a third-party + // WFP block dropping DNS below NRPT looks exactly like an ineffective + // rule. No policy refresh, paramchange or cache flush here - see + // healBlockedLoopbackDNS. + if !p.healBlockedLoopbackDNS(state, "GP-managed NRPT rule present but not routing") { + go p.nrptProbeAndHeal(state) + } + } + if p.repairMissingWFP(state) { + return + } continue + case nrptRuleOwnerCtrld: + // Group Policy may have returned after ctrld activated its fallback. + // Prefer the externally owned rule once it proves the same route without + // ctrld's rule installed. + if p.tryAdoptMatchingGPNRPT(state) { + continue + } } - // Step 1: Check registry key exists. - if !nrptCatchAllRuleExists() { + if nrptNeedsCtrldActivation(owner, nrptCatchAllRuleExists()) { now := time.Now() if ok, wait := state.nrptRecoveryLimiter.allow(now, p.cfg); !ok { if state.nrptRecoveryLimiter.shouldLogSkip(now) { @@ -1619,39 +2818,32 @@ func (p *prog) nrptHealthMonitor(state *wfpState) { } continue } - mainLog.Load().Warn().Msg("DNS intercept: NRPT health check — catch-all rule missing, restoring") - if err := addNRPTCatchAllRule(state.listenerIP); err != nil { - mainLog.Load().Error().Err(err).Msg("DNS intercept: failed to restore NRPT catch-all rule") - state.nrptActive = false - continue + reason := "ctrld-owned rule missing during health check" + if owner == nrptRuleOwnerNone { + reason = "no NRPT owner during health check - retrying a failed activation" + } + if p.activateCtrldNRPTFallback(state, reason) { + state.nrptRecoveryLimiter.recordRecoveryFlow(time.Now(), p.cfg) + go p.nrptProbeAndHeal(state) + } else if owner == nrptRuleOwnerNone && hardIntercept { + // Worth shouting about: hard mode keeps blocking outbound DNS + // whether or not NRPT redirects it, so a machine stuck here has no + // working resolver until activation succeeds. + mainLog.Load().Error(). + Msg("DNS intercept: hard mode is blocking DNS but no NRPT rule could be activated - DNS will not resolve until this recovers") } - signalNRPTChange() - state.nrptRecoveryLimiter.recordRecoveryFlow(time.Now(), p.cfg) - mainLog.Load().Info().Msg("DNS intercept: NRPT catch-all rule restored by health monitor") - // After restoring, verify it's actually working. - go p.nrptProbeAndHeal() continue } - // Step 2: Registry key exists — verify NRPT is actually routing - // queries to ctrld (catches the async GP refresh race). - if !p.probeNRPT() { - mainLog.Load().Warn().Msg("DNS intercept: NRPT health check — rule present but probe failed, running heal cycle") - go p.nrptProbeAndHeal() + if !p.nrptOps().probe(state) { + mainLog.Load().Warn().Msg("DNS intercept: ctrld-owned NRPT rule present but probe failed, running heal cycle") + go p.nrptProbeAndHeal(state) } else { state.nrptRecoveryLimiter.recordStableSuccess() } - // Step 3: In hard mode, also verify WFP sublayer. - if state.engineHandle != 0 && !wfpSublayerExists(state.engineHandle) { - mainLog.Load().Warn().Msg("DNS intercept: WFP health check — sublayer missing, re-initializing all filters") - _ = p.stopDNSIntercept() - if err := p.startDNSIntercept(); err != nil { - mainLog.Load().Error().Err(err).Msg("DNS intercept: failed to re-initialize after WFP sublayer loss") - } else { - mainLog.Load().Info().Msg("DNS intercept: WFP filters restored by health monitor") - } - return // stopDNSIntercept closed our stopCh; startDNSIntercept started a new monitor + if p.repairMissingWFP(state) { + return // our state was retired: shutdown, or a rebuild that started a new monitor } } } @@ -1680,24 +2872,24 @@ var nrptProbeRunning atomic.Bool // the Windows DNS Client service (via Go's net.Resolver / GetAddrInfoW). If ctrld // receives the query on its listener, NRPT is working. // -// Returns true if NRPT is verified working, false if the probe timed out. -func (p *prog) probeNRPT() bool { - if p.dnsInterceptState == nil { +// Returns true if NRPT is verified working, false if the probe timed out or a shutdown +// arrived first. Reporting an abandoned probe as "not working" is safe: every recovery +// action a failed probe can trigger checks for revocation before it writes anything. +// +// state is passed explicitly rather than read from p.dnsInterceptState so that startup +// can probe before it publishes, and so a probe belongs to exactly one intercept. +func (p *prog) probeNRPT(state *wfpState) bool { + if state == nil { return true } // Generate unique probe domain to defeat DNS caching. probeID := fmt.Sprintf("_nrpt-probe-%x.%s", rand.Uint32(), nrptProbeDomain) - // Register probe so DNS handler can detect and signal it. - // Reuse the same mechanism as macOS pf probes (pfProbeExpected/pfProbeCh). - probeCh := make(chan struct{}, 1) - p.pfProbeExpected.Store(probeID) - p.pfProbeCh.Store(&probeCh) - defer func() { - p.pfProbeExpected.Store("") - p.pfProbeCh.Store((*chan struct{})(nil)) - }() + // Register this attempt's own domain so overlapping probes - a health tick, a + // handback and a heal cycle can each have one out - cannot cancel each other. + probeCh, deregister := p.registerInterceptProbe(probeID) + defer deregister() mainLog.Load().Debug().Str("domain", probeID).Msg("DNS intercept: sending NRPT verification probe") @@ -1713,13 +2905,24 @@ func (p *prog) probeNRPT() bool { _, _ = resolver.LookupHost(ctx, probeID) }() - select { - case <-probeCh: - mainLog.Load().Debug().Str("domain", probeID).Msg("DNS intercept: NRPT probe received — interception verified") - return true - case <-ctx.Done(): - mainLog.Load().Debug().Str("domain", probeID).Msg("DNS intercept: NRPT probe timed out — interception not working") - return false + // Poll for a pending stop so a service stop never waits out the probe timeout on + // top of the lifecycle lock. + shutdownPoll := time.NewTicker(interceptRevocationPollInterval) + defer shutdownPoll.Stop() + for { + select { + case <-probeCh: + mainLog.Load().Debug().Str("domain", probeID).Msg("DNS intercept: NRPT probe received - interception verified") + return true + case <-ctx.Done(): + mainLog.Load().Debug().Str("domain", probeID).Msg("DNS intercept: NRPT probe timed out - interception not working") + return false + case <-shutdownPoll.C: + if p.interceptStateRevoked(state) { + mainLog.Load().Debug().Str("domain", probeID).Msg("DNS intercept: abandoning NRPT probe - shutdown in progress") + return false + } + } } } @@ -1730,7 +2933,7 @@ func (p *prog) probeNRPT() bool { // restarting the Dnscache service (which always fails on modern Windows because // Dnscache is a protected shared svchost service). func sendParamChange() { - if out, err := exec.Command("sc", "control", "dnscache", "paramchange").CombinedOutput(); err != nil { + if out, err := runBoundedNRPTExec("sc", "control", "dnscache", "paramchange"); err != nil { mainLog.Load().Debug().Err(err).Str("output", string(out)).Msg("DNS intercept: sc control dnscache paramchange failed") } else { mainLog.Load().Debug().Msg("DNS intercept: sent paramchange to Dnscache service") @@ -1814,25 +3017,83 @@ func logNRPTParentKeyState(context string) { // Dnscache paramchange cannot make local rules visible while GP mode is // selected by an empty GP parent. // 3. Otherwise, signal DNS Client with increasing backoff between probes. -func (p *prog) nrptProbeAndHeal() { - state, _ := p.dnsInterceptState.(*wfpState) - if state != nil { - now := time.Now() - if ok, wait := state.nrptRecoveryLimiter.allow(now, p.cfg); !ok { - if state.nrptRecoveryLimiter.shouldLogSkip(now) { - mainLog.Load().Warn().Dur("remaining", wait). - Msg("DNS intercept: NRPT recovery suppressed after repeated failed recovery flows") - } - return - } +// +// state is passed in rather than read from p.dnsInterceptState: this runs from the +// locked start path before anything is published, and it must keep acting on the +// intercept it was launched for and no other. +// It reports whether the cycle ended with a probe that reached ctrld. Callers that need a +// readiness answer - startup's synchronous verification - use that; the asynchronous +// callers ignore it. +func (p *prog) nrptProbeAndHeal(state *wfpState) bool { + if p.interceptStateRevoked(state) { + return false } - if !nrptProbeRunning.CompareAndSwap(false, true) { mainLog.Load().Debug().Msg("DNS intercept: NRPT probe already running, skipping") - return + return false } defer nrptProbeRunning.Store(false) + ops := p.nrptOps() + owner, externalRuleName := state.nrptPolicyOwner() + if owner == nrptRuleOwnerGroupPolicy { + currentRule := ops.findGPRule(state.listenerIP) + if currentRule == "" { + if !p.activateCtrldNRPTFallback(state, "matching GP rule disappeared before verification") { + return false + } + owner = nrptRuleOwnerCtrld + } else { + switch p.nrptHandbackToExternal(state, currentRule, "GP-managed NRPT verification") { + case nrptHandbackVerified: + mainLog.Load().Info().Str("rule", currentRule). + Msg("DNS intercept: GP-managed NRPT verified working") + return true + case nrptHandbackKeptCtrld: + // External policy could not carry DNS without ctrld's rule, which the + // transition has restored. Continue as the ctrld-owned flow below. + owner = nrptRuleOwnerCtrld + case nrptHandbackConflict: + // External policy owns the namespace and points elsewhere. Owned recovery + // must not write beside it, so this heal cycle ends here. + return false + default: + if ops.findGPRule(state.listenerIP) == "" { + // The GP child went away while we were looking at it. + if !p.activateCtrldNRPTFallback(state, "matching GP rule disappeared during verification") { + return false + } + owner = nrptRuleOwnerCtrld + break + } + // A matching external rule owns the GP store, so ctrld may not rewrite + // policy or signal the DNS Client here. Loopback WFP protect is the one + // ctrld-owned remediation left: a third-party WFP block dropping DNS + // below NRPT presents exactly as an ineffective rule. + if p.healBlockedLoopbackDNS(state, "GP-managed NRPT present but not routing") { + mainLog.Load().Info().Str("rule", currentRule). + Msg("DNS intercept: GP-managed NRPT verified after loopback WFP protection") + return true + } + mainLog.Load().Error().Str("rule", externalRuleName). + Msg("DNS intercept: GP-managed NRPT remains present but ineffective; no NRPT recovery actions were taken") + return false + } + } + } + if owner != nrptRuleOwnerCtrld { + return false + } + + now := time.Now() + if ok, wait := state.nrptRecoveryLimiter.allow(now, p.cfg); !ok { + if state.nrptRecoveryLimiter.shouldLogSkip(now) { + mainLog.Load().Warn().Dur("remaining", wait). + Msg("DNS intercept: NRPT recovery suppressed after repeated failed recovery flows") + } + return false + } + remediated := false defer func() { if remediated && state != nil { @@ -1846,9 +3107,16 @@ func (p *prog) nrptProbeAndHeal() { logNRPTParentKeyState("probe-start") // Attempt 1: immediate probe - if p.probeNRPT() { + if ops.probe(state) { mainLog.Load().Info().Msg("DNS intercept: NRPT verified working") - return + return true + } + // Group Policy can appear while a ctrld-owned heal is running. Once an exact + // administrator catch-all is there, this cycle stops: everything below signals the + // DNS Client or rewrites ctrld's rule, and neither may happen beside external policy. + if p.deferToExternalCatchAll(ops, state, "GP catch-all appeared during ctrld recovery") { + mainLog.Load().Warn().Msg("DNS intercept: matching GP catch-all present during ctrld recovery; stopping NRPT mutations and deferring to Group Policy") + return false } remediated = true @@ -1856,20 +3124,22 @@ func (p *prog) nrptProbeAndHeal() { // signaling. Those retries create SIEM noise but cannot succeed because DNS // Client is still reading the empty GP store instead of the populated local // store. Delete the blocker, send one notification, then re-probe. - if nrptParentKeyEmpty(nrptBaseKey) { + if ops.parentEmpty(nrptBaseKey) { mainLog.Load().Warn().Msg("DNS intercept: NRPT probe failed with empty GP parent — cleaning before retry signaling") - if cleanEmptyNRPTParent() { - signalNRPTChange() - time.Sleep(1 * time.Second) + if ops.cleanParent() { + ops.signal() + if !ops.wait(state, nrptHandbackSettleDelay) { + return false + } logNRPTParentKeyState("empty-gp-after-clean") - if p.probeNRPT() { + if ops.probe(state) { mainLog.Load().Info().Msg("DNS intercept: NRPT verified working after empty GP parent cleanup") - return + return true } } - if nrptParentKeyEmpty(nrptBaseKey) { + if ops.parentEmpty(nrptBaseKey) { mainLog.Load().Warn().Msg("DNS intercept: empty GP NRPT parent still present after cleanup; skipping redundant policy refresh retries") - return + return false } } @@ -1877,50 +3147,134 @@ func (p *prog) nrptProbeAndHeal() { delays := []time.Duration{1 * time.Second, 2 * time.Second, 4 * time.Second} for i, delay := range delays { attempt := i + 2 + // Each round signals Windows and then waits; a stop must not have the whole + // remaining backoff added to the time it waits for the lifecycle lock. + if p.interceptStateRevoked(state) { + mainLog.Load().Debug().Int("attempt", attempt). + Msg("DNS intercept: intercept retired - abandoning NRPT probe retries") + return false + } mainLog.Load().Info().Int("attempt", attempt).Dur("delay", delay). Msg("DNS intercept: NRPT probe failed, retrying with policy refresh + paramchange") logNRPTParentKeyState(fmt.Sprintf("probe-attempt-%d", attempt)) - signalNRPTChange() - time.Sleep(delay) - if p.probeNRPT() { + ops.signal() + if !ops.wait(state, delay) { + return false + } + if ops.probe(state) { mainLog.Load().Info().Int("attempt", attempt). Msg("DNS intercept: NRPT verified working") - return + return true } } + // Re-check external ownership before the destructive two-phase recovery. A GP refresh + // can land during the bounded retry waits above, and the delete half of that recovery + // must not run once it has: it would strand the machine mid-recovery under policy + // ctrld does not own. + if p.deferToExternalCatchAll(ops, state, "GP catch-all appeared during ctrld retries") { + mainLog.Load().Warn().Msg("DNS intercept: matching GP catch-all present after the retries; skipping ctrld two-phase recovery") + return false + } + if gpCatchAllConflictBlocksFallback(state, "GP catch-all changed during ctrld recovery") { + return false + } + + // A stop can land during the retry waits above, and its own NRPT removal has + // already run: deleting and re-adding from here would leave the rule behind. + if p.interceptStateRevoked(state) { + mainLog.Load().Debug().Msg("DNS intercept: intercept retired during probe retries - skipping two-phase NRPT recovery") + return false + } + // Nuclear option: two-phase delete → re-add cycle. // DNS Client may have cached a stale "no rules" state. Delete our rule, // signal DNS Client to forget it, wait, then re-add and signal again. mainLog.Load().Warn().Msg("DNS intercept: all probes failed — attempting two-phase NRPT recovery (delete → signal → re-add)") - listenerIP := "127.0.0.1" - if state != nil { - listenerIP = state.listenerIP + listenerIP := state.listenerIP + + // Phase 1: Remove our rule and the parent key if now empty. Each phase is its own + // transition: serialized against a stop and against other NRPT writers, but the lock + // is released across the wait between them. + if !p.nrptTransition(state, func() { + _ = ops.removeRule() + // If parent key is now empty after removing our rule, delete it too. + ops.cleanParent() + ops.signal() + logNRPTParentKeyState("nuclear-after-delete") + }) { + mainLog.Load().Debug().Msg("DNS intercept: intercept retired - skipping two-phase NRPT recovery") + return false } - // Phase 1: Remove our rule and the parent key if now empty. - _ = removeNRPTCatchAllRule() - // If parent key is now empty after removing our rule, delete it too. - cleanEmptyNRPTParent() - signalNRPTChange() - logNRPTParentKeyState("nuclear-after-delete") + // Wait for DNS Client to process the deletion. Stopping here is the clean outcome: + // phase 1 left no ctrld rule behind. + if !ops.wait(state, nrptHandbackSettleDelay) { + mainLog.Load().Debug().Msg("DNS intercept: intercept retired mid-recovery - leaving NRPT removed") + return false + } - // Wait for DNS Client to process the deletion. - time.Sleep(1 * time.Second) + // Group Policy may refresh while the ctrld-owned rule is absent. Never re-create our + // catch-all beside a newly authoritative GP catch-all. With ctrld's rule already + // gone, this is the one place a probe measures external policy on its own. + if p.deferToExternalCatchAll(ops, state, "GP catch-all appeared during two-phase recovery") { + mainLog.Load().Warn().Msg("DNS intercept: matching GP catch-all appeared during two-phase recovery; skipping ctrld re-add") + return false + } + if ops.gpConflicts(state, "GP catch-all appeared during two-phase recovery") { + return false + } - // Phase 2: Re-add the rule. - if err := addNRPTCatchAllRule(listenerIP); err != nil { - mainLog.Load().Error().Err(err).Msg("DNS intercept: failed to re-add NRPT after nuclear recovery") - return + // Phase 2: Re-add the rule. Phase 1 left NRPT clean, so if a stop arrived during the + // wait the transition refuses and the host stays clean. + readded := false + if !p.nrptTransition(state, func() { + // Re-validate the ownership picture here, inside the lock. The checks above ran + // before this transition queued for nrptTransitionMu, and another health or + // delayed path can complete a whole handback while this one waits: it may have + // given the namespace to a GP child that arrived meanwhile and recorded + // GroupPolicy. Re-adding on top of that would plant the competing catch-all this + // work exists to prevent, and then stamp ctrld ownership over the administrator's. + if owner, ruleName := state.nrptPolicyOwner(); owner == nrptRuleOwnerGroupPolicy { + mainLog.Load().Warn().Str("rule", ruleName). + Msg("DNS intercept: ownership moved to Group Policy while the two-phase re-add waited for the transition lock; leaving NRPT to external policy") + return + } + if ruleName := ops.findGPRule(state.listenerIP); ruleName != "" && + ops.gpRuleMatches(ruleName, state.listenerIP) { + mainLog.Load().Warn().Str("rule", ruleName). + Msg("DNS intercept: a matching GP catch-all appeared while the two-phase re-add waited for the transition lock; skipping the ctrld re-add") + return + } + if ops.gpConflicts(state, "two-phase re-add revalidation") { + return + } + if err := ops.addRule(listenerIP); err != nil { + mainLog.Load().Error().Err(err).Msg("DNS intercept: failed to re-add NRPT after nuclear recovery") + return + } + ops.signal() + state.setNRPTPolicyOwner(nrptRuleOwnerCtrld, "") + logNRPTParentKeyState("nuclear-after-readd") + readded = true + }) || !readded { + mainLog.Load().Debug().Msg("DNS intercept: two-phase recovery did not restore the ctrld NRPT rule") + return false } - signalNRPTChange() - logNRPTParentKeyState("nuclear-after-readd") // Final probe after recovery. - time.Sleep(1 * time.Second) - if p.probeNRPT() { + if !ops.wait(state, nrptHandbackSettleDelay) { + // This cycle is retired. Do not clean up from here: the deterministic ctrld key is + // process-global, and by now it can belong to a successor intercept that a rebuild + // started while this goroutine waited - deleting it would take out the successor's + // route and leave hard mode blocking DNS with nothing redirecting it. Teardown of + // the state this cycle belonged to already owns that cleanup. + mainLog.Load().Debug().Msg("DNS intercept: intercept retired after the two-phase re-add - leaving cleanup to the owning teardown") + return false + } + if ops.probe(state) { mainLog.Load().Info().Msg("DNS intercept: NRPT verified working after two-phase recovery") - return + return true } logNRPTParentKeyState("probe-failed-final") @@ -1932,32 +3286,27 @@ func (p *prog) nrptProbeAndHeal() { // loopback. A high-priority "hard permit" for localhost DNS overrides these // blocks and restores NRPT routing to ctrld's listener. // See: https://gitlab.int.windscribe.com/controld/clients/ctrld/-/issues/526 - loopbackState, ok := p.dnsInterceptState.(*wfpState) - if !ok || loopbackState == nil { - mainLog.Load().Error().Msg("DNS intercept: no state available for loopback WFP protect") - return - } - // Bail out if shutdown is in progress — avoid racing with cleanupWFPFilters. - select { - case <-loopbackState.stopCh: + if p.interceptStateRevoked(state) { mainLog.Load().Info().Msg("DNS intercept: shutdown in progress, skipping loopback WFP protect activation") - return - default: + return false } - if err := p.activateLoopbackWFPProtect(loopbackState); err != nil { + if err := p.activateLoopbackWFPProtect(state); err != nil { mainLog.Load().Error().Err(err).Msg("DNS intercept: failed to activate loopback WFP protect — " + "DNS queries may not be routed through ctrld. A network interface toggle may be needed.") - return + return false } // Retry NRPT probe now that loopback DNS is explicitly permitted through WFP. - time.Sleep(500 * time.Millisecond) - if p.probeNRPT() { + if !ops.wait(state, loopbackProtectSettleDelay) { + return false + } + if ops.probe(state) { mainLog.Load().Info().Msg("DNS intercept: NRPT verified working after loopback WFP protect activation") - return + return true } mainLog.Load().Error().Msg("DNS intercept: NRPT probe still failing after loopback WFP protect — " + "DNS queries may not be routed through ctrld. A network interface toggle may be needed.") + return false } diff --git a/cmd/cli/dns_proxy.go b/cmd/cli/dns_proxy.go index 6a64882..b34013c 100644 --- a/cmd/cli/dns_proxy.go +++ b/cmd/cli/dns_proxy.go @@ -130,13 +130,7 @@ func (p *prog) serveDNS(listenerNum string) error { // signal the prober and respond NXDOMAIN. Used by both macOS pf probes // (_pf-probe-*) and Windows NRPT probes (_nrpt-probe-*) to verify that // DNS interception is actually routing queries to ctrld's listener. - if probeID, ok := p.pfProbeExpected.Load().(string); ok && probeID != "" && domain == probeID { - if chPtr, ok := p.pfProbeCh.Load().(*chan struct{}); ok && chPtr != nil { - select { - case *chPtr <- struct{}{}: - default: - } - } + if p.signalInterceptProbe(domain) { answer := new(dns.Msg) answer.SetRcode(m, dns.RcodeNameError) // NXDOMAIN _ = w.WriteMsg(answer) diff --git a/cmd/cli/intercept_probe.go b/cmd/cli/intercept_probe.go new file mode 100644 index 0000000..36e5707 --- /dev/null +++ b/cmd/cli/intercept_probe.go @@ -0,0 +1,68 @@ +package cli + +// Interception probe registry. +// +// A probe sends a DNS query for a unique synthetic domain through the OS resolver and +// waits for ctrld's own handler to receive it. That is the only way to tell "the rules are +// present" from "the rules are actually redirecting packets", and both the macOS pf path +// and the Windows NRPT path use it. +// +// Each attempt registers its own domain, so overlapping probes cannot cancel each other, +// and deregistration only removes the entry it owns. + +// registerInterceptProbe registers domain and returns the channel it will be signalled on +// plus the function that removes the registration. +// +//lint:ignore U1000 used on darwin (pf probes) and windows (NRPT probes) +func (p *prog) registerInterceptProbe(domain string) (<-chan struct{}, func()) { + ch := make(chan struct{}, 1) + + p.interceptProbeMu.Lock() + current, _ := p.interceptProbes.Load().(map[string]chan struct{}) + next := make(map[string]chan struct{}, len(current)+1) + for k, v := range current { + next[k] = v + } + next[domain] = ch + p.interceptProbes.Store(next) + p.interceptProbeMu.Unlock() + + return ch, func() { + p.interceptProbeMu.Lock() + defer p.interceptProbeMu.Unlock() + current, _ := p.interceptProbes.Load().(map[string]chan struct{}) + // Only drop the entry while it is still this attempt's channel. A later probe + // that reused the domain owns the slot now, and clearing it would make that one + // wait out its timeout for a query it already received. + if existing, ok := current[domain]; !ok || existing != ch { + return + } + next := make(map[string]chan struct{}, len(current)) + for k, v := range current { + if k != domain { + next[k] = v + } + } + p.interceptProbes.Store(next) + } +} + +// signalInterceptProbe reports whether domain is a pending probe, signalling its waiter +// when it is. Called from the DNS handler for every query, so the common case is a nil or +// empty map and no allocation. +func (p *prog) signalInterceptProbe(domain string) bool { + probes, _ := p.interceptProbes.Load().(map[string]chan struct{}) + if len(probes) == 0 { + return false + } + ch, ok := probes[domain] + if !ok { + return false + } + select { + case ch <- struct{}{}: + default: + // Buffered channel already holds a signal: the waiter has what it needs. + } + return true +} diff --git a/cmd/cli/nrpt_external_gp.go b/cmd/cli/nrpt_external_gp.go new file mode 100644 index 0000000..62c57f8 --- /dev/null +++ b/cmd/cli/nrpt_external_gp.go @@ -0,0 +1,63 @@ +package cli + +import ( + "errors" + "net/netip" + "strings" +) + +const nrptRuleName = `CtrldCatchAll` + +// errGPNRPTVerified marks an intercept startup failure that happened while an externally +// managed (Group Policy) NRPT catch-all was proved - by probe, not by registry shape +// alone - to be routing DNS to this listener. It is the difference between "intercept +// failed but DNS still reaches ctrld" and "intercept failed and nothing is filtering", +// which is what decides whether the interface-DNS fallback must run. +// +// Only the Windows path produces it, but setDNS is shared, so the sentinel and its +// predicate live here with the other platform-neutral NRPT helpers. +var errGPNRPTVerified = errors.New("GP-managed NRPT verified routing to ctrld") + +// errGPNRPTIneffective marks a startup that ends with externally managed NRPT owning the +// namespace while no probe has proved it routes to ctrld. DNS is not reaching ctrld, but +// adapter DNS was deliberately preserved and no ctrld rule may be written beside an +// administrator's catch-all - so this is a failed start that must not take the +// interface-DNS fallback either. +var errGPNRPTIneffective = errors.New("GP-managed NRPT owns the namespace but no probe reached ctrld") + +// interceptFailedWithVerifiedExternalDNS reports whether an intercept startup failure +// happened while externally managed DNS policy was verified to be routing to ctrld. +func interceptFailedWithVerifiedExternalDNS(err error) bool { + return errors.Is(err, errGPNRPTVerified) +} + +// interceptFailedUnderExternalDNSPolicy reports whether an intercept startup failure +// happened while externally managed DNS policy owned the namespace, whether or not it was +// proved to route. Either way the interface-DNS fallback must not run: adapter DNS was +// preserved on purpose, and rewriting it would violate the policy ctrld just deferred to. +// Only the verified case is a successful start. +func interceptFailedUnderExternalDNSPolicy(err error) bool { + return errors.Is(err, errGPNRPTVerified) || errors.Is(err, errGPNRPTIneffective) +} + +// isExternalGPCatchAll recognizes only a single catch-all namespace that is not +// ctrld's deterministic GP key. Registry access stays in the Windows file; this +// pure classifier is shared with host-runnable tests. +func isExternalGPCatchAll(ruleName string, namespaces []string) bool { + return ruleName != "" && !strings.EqualFold(ruleName, nrptRuleName) && len(namespaces) == 1 && strings.TrimSpace(namespaces[0]) == "." +} + +func isMatchingGPNRPTRule(ruleName string, namespaces []string, dnsServers, listenerIP string) bool { + if !isExternalGPCatchAll(ruleName, namespaces) { + return false + } + server, err := netip.ParseAddr(strings.TrimSpace(dnsServers)) + if err != nil { + return false + } + listener, err := netip.ParseAddr(strings.TrimSpace(listenerIP)) + if err != nil { + return false + } + return server.Unmap() == listener.Unmap() +} diff --git a/cmd/cli/nrpt_external_gp_test.go b/cmd/cli/nrpt_external_gp_test.go new file mode 100644 index 0000000..277581b --- /dev/null +++ b/cmd/cli/nrpt_external_gp_test.go @@ -0,0 +1,129 @@ +package cli + +import ( + "errors" + "fmt" + "testing" +) + +func TestIsMatchingGPNRPTRule(t *testing.T) { + tests := []struct { + name string + ruleName string + namespaces []string + servers string + listener string + want bool + }{ + { + name: "exact IPv4 catch-all", + ruleName: "{A1B2C3D4}", + namespaces: []string{"."}, + servers: "127.0.0.1", + listener: "127.0.0.1", + want: true, + }, + { + name: "normalized IPv4-mapped listener", + ruleName: "{A1B2C3D4}", + namespaces: []string{"."}, + servers: "::ffff:127.0.0.1", + listener: "127.0.0.1", + want: true, + }, + { + name: "ctrld GP key is not external", + ruleName: "ctrldcatchall", + namespaces: []string{"."}, + servers: "127.0.0.1", + listener: "127.0.0.1", + }, + { + name: "partial namespace", + ruleName: "{A1B2C3D4}", + namespaces: []string{"corp.example"}, + servers: "127.0.0.1", + listener: "127.0.0.1", + }, + { + name: "multiple namespaces", + ruleName: "{A1B2C3D4}", + namespaces: []string{".", "corp.example"}, + servers: "127.0.0.1", + listener: "127.0.0.1", + }, + { + name: "wrong listener", + ruleName: "{A1B2C3D4}", + namespaces: []string{"."}, + servers: "127.0.0.2", + listener: "127.0.0.1", + }, + { + name: "multiple nameservers", + ruleName: "{A1B2C3D4}", + namespaces: []string{"."}, + servers: "127.0.0.1;127.0.0.2", + listener: "127.0.0.1", + }, + { + name: "malformed nameserver", + ruleName: "{A1B2C3D4}", + namespaces: []string{"."}, + servers: "localhost", + listener: "127.0.0.1", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := isMatchingGPNRPTRule(tt.ruleName, tt.namespaces, tt.servers, tt.listener); got != tt.want { + t.Fatalf("isMatchingGPNRPTRule() = %t, want %t", got, tt.want) + } + }) + } +} + +func TestIsExternalGPCatchAll(t *testing.T) { + tests := []struct { + name string + ruleName string + namespaces []string + want bool + }{ + {name: "external catch-all", ruleName: "{GP-RULE}", namespaces: []string{"."}, want: true}, + {name: "ctrld key", ruleName: nrptRuleName, namespaces: []string{"."}}, + {name: "partial namespace", ruleName: "{GP-RULE}", namespaces: []string{"corp.example"}}, + {name: "multiple namespaces", ruleName: "{GP-RULE}", namespaces: []string{".", "corp.example"}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := isExternalGPCatchAll(tt.ruleName, tt.namespaces); got != tt.want { + t.Fatalf("isExternalGPCatchAll() = %t, want %t", got, tt.want) + } + }) + } +} + +// TestInterceptFailedWithVerifiedExternalDNS covers the distinction the interface-DNS +// fallback turns on. "A GP rule exists" is not enough: if it is not actually routing and +// intercept failed too, skipping the fallback leaves the machine with no NRPT, no WFP and +// no adapter DNS - that is, unfiltered. Only a probe-verified route earns the skip. +func TestInterceptFailedWithVerifiedExternalDNS(t *testing.T) { + wfpErr := errors.New("FwpmEngineOpen0 failed: HRESULT 0x5") + + verified := fmt.Errorf("dns intercept: WFP setup failed: %w: %w", wfpErr, errGPNRPTVerified) + if !interceptFailedWithVerifiedExternalDNS(verified) { + t.Error("a failure carrying errGPNRPTVerified must skip the interface-DNS fallback") + } + if !errors.Is(verified, wfpErr) { + t.Error("the underlying cause must stay inspectable for logs and callers") + } + + if interceptFailedWithVerifiedExternalDNS(fmt.Errorf("dns intercept: WFP setup failed: %w", wfpErr)) { + t.Error("an unverified failure must take the interface-DNS fallback rather than leave the machine unfiltered") + } + if interceptFailedWithVerifiedExternalDNS(nil) { + t.Error("no error must not read as a verified external route") + } +} diff --git a/cmd/cli/nrpt_external_gp_windows_test.go b/cmd/cli/nrpt_external_gp_windows_test.go new file mode 100644 index 0000000..3d402d7 --- /dev/null +++ b/cmd/cli/nrpt_external_gp_windows_test.go @@ -0,0 +1,20 @@ +//go:build windows + +package cli + +import "testing" + +func TestWFPStateNRPTPolicyOwner(t *testing.T) { + state := &wfpState{} + state.setNRPTPolicyOwner(nrptRuleOwnerGroupPolicy, "{GP-RULE}") + owner, ruleName := state.nrptPolicyOwner() + if owner != nrptRuleOwnerGroupPolicy || ruleName != "{GP-RULE}" { + t.Fatalf("owner = %v, rule = %q", owner, ruleName) + } + + state.setNRPTPolicyOwner(nrptRuleOwnerCtrld, "") + owner, ruleName = state.nrptPolicyOwner() + if owner != nrptRuleOwnerCtrld || ruleName != "" { + t.Fatalf("owner = %v, rule = %q", owner, ruleName) + } +} diff --git a/cmd/cli/nrpt_handback_windows_test.go b/cmd/cli/nrpt_handback_windows_test.go new file mode 100644 index 0000000..153376a --- /dev/null +++ b/cmd/cli/nrpt_handback_windows_test.go @@ -0,0 +1,1309 @@ +//go:build windows + +package cli + +import ( + "errors" + "strings" + "sync" + "testing" + "time" + + "github.com/Control-D-Inc/ctrld" +) + +// fakeNRPTOps records the Windows side effects a transition would cause and serves +// scripted probe results, so the ownership decisions can be driven without touching the +// registry or the DNS Client. +type fakeNRPTOps struct { + mu sync.Mutex + + // probeResults is consumed in order; the last value repeats once exhausted. + probeResults []bool + probeCalls int + flushCalls int + + gpRule string // rule name findGPRule reports, "" for none + gpConflict bool // whether an external catch-all targets another resolver + parentEmpty bool // whether the GP parent key reads as present but empty + cleanCalls int + gpConflictCalls int + ctrldRule bool // whether ctrld's own catch-all exists + existsCalls int + addCalls int + removeCalls int + signalCalls int + + addErr error + removeErr error + + // beforeAdd runs inside addRule, while the transition lock is held. + beforeAdd func() + // onProbe runs after the nth probe (1-based) is answered, so a test can change the + // GP store mid-probe the way a Group Policy refresh would. + onProbe func(call int) + // onGPConflicts runs after each gpConflicts check, which is the last ops call before + // the two-phase re-add queues for the transition lock. + onGPConflicts func(call int) + // waitHook stands in for a cancellable wait: returning false models the intercept + // being retired mid-wait. + waitHook func() bool +} + +func (f *fakeNRPTOps) ops() *nrptOps { + return &nrptOps{ + probe: func(*wfpState) bool { + f.mu.Lock() + f.probeCalls++ + call := f.probeCalls + result := false + if len(f.probeResults) > 0 { + result = f.probeResults[0] + if len(f.probeResults) > 1 { + f.probeResults = f.probeResults[1:] + } + } + onProbe := f.onProbe + f.mu.Unlock() + if onProbe != nil { + onProbe(call) + } + return result + }, + ruleExists: func() bool { + f.mu.Lock() + defer f.mu.Unlock() + f.existsCalls++ + return f.ctrldRule + }, + addRule: func(string) error { + f.mu.Lock() + beforeAdd, err := f.beforeAdd, f.addErr + f.addCalls++ + if err == nil { + f.ctrldRule = true + } + f.mu.Unlock() + if beforeAdd != nil { + beforeAdd() + } + return err + }, + removeRule: func() error { + f.mu.Lock() + defer f.mu.Unlock() + f.removeCalls++ + if f.removeErr != nil { + return f.removeErr + } + f.ctrldRule = false + return nil + }, + signal: func() { + f.mu.Lock() + defer f.mu.Unlock() + f.signalCalls++ + }, + findGPRule: func(string) string { + f.mu.Lock() + defer f.mu.Unlock() + return f.gpRule + }, + gpRuleMatches: func(ruleName, _ string) bool { + f.mu.Lock() + defer f.mu.Unlock() + return ruleName != "" && ruleName == f.gpRule + }, + gpConflicts: func(*wfpState, string) bool { + f.mu.Lock() + f.gpConflictCalls++ + call := f.gpConflictCalls + conflict, hook := f.gpConflict, f.onGPConflicts + f.mu.Unlock() + if hook != nil { + hook(call) + } + return conflict + }, + loopback: func(*wfpState) error { return nil }, + // No real waiting in tests; report "still live" unless a test says otherwise. + wait: func(*wfpState, time.Duration) bool { + f.mu.Lock() + hook := f.waitHook + f.mu.Unlock() + if hook != nil { + return hook() + } + return true + }, + startWFP: func(*wfpState) error { return nil }, + flush: func() { + f.mu.Lock() + defer f.mu.Unlock() + f.flushCalls++ + }, + parentEmpty: func(string) bool { + f.mu.Lock() + defer f.mu.Unlock() + return f.parentEmpty + }, + cleanParent: func() bool { + f.mu.Lock() + defer f.mu.Unlock() + f.cleanCalls++ + wasEmpty := f.parentEmpty + f.parentEmpty = false + return wasEmpty + }, + } +} + +func (f *fakeNRPTOps) counts() (add, remove, signal, probe int) { + f.mu.Lock() + defer f.mu.Unlock() + return f.addCalls, f.removeCalls, f.signalCalls, f.probeCalls +} + +func (f *fakeNRPTOps) ruleExistsCount() int { + f.mu.Lock() + defer f.mu.Unlock() + return f.existsCalls +} + +func (f *fakeNRPTOps) flushCount() int { + f.mu.Lock() + defer f.mu.Unlock() + return f.flushCalls +} + +// setGPRule replaces what the fake GP store reports, as a policy refresh would. +func (f *fakeNRPTOps) setGPRule(ruleName string, conflicting bool) { + f.mu.Lock() + defer f.mu.Unlock() + f.gpRule = ruleName + f.gpConflict = conflicting +} + +// hasCtrldRule reports whether ctrld's catch-all is currently installed. +func (f *fakeNRPTOps) hasCtrldRule() bool { + f.mu.Lock() + defer f.mu.Unlock() + return f.ctrldRule +} + +// installedFakeNRPTOps is the fake currently standing in for the production NRPT +// operations, so fixtures can hand it to tests and can tell whether one is installed at +// all. +var installedFakeNRPTOps *fakeNRPTOps + +// installFakeNRPTOps swaps in the fake for the duration of the test. +func installFakeNRPTOps(t *testing.T, f *fakeNRPTOps) { + t.Helper() + nrptOpsForTest = f.ops() + installedFakeNRPTOps = f + t.Cleanup(func() { + nrptOpsForTest = nil + installedFakeNRPTOps = nil + }) +} + +// fakeNRPTSeed is the starting state a test wants the fixture's fake to have. It is a +// separate type so the fake - which carries a mutex - is never copied. +type fakeNRPTSeed struct { + probeResults []bool + gpRule string + gpConflict bool + parentEmpty bool + ctrldRule bool + addErr error + removeErr error +} + +// configure re-arms the fake the fixture installed with a test's own starting state, so +// the fixture keeps ownership of installation - and therefore of the production-call guard. +func (f *fakeNRPTOps) configure(seed fakeNRPTSeed) { + f.mu.Lock() + defer f.mu.Unlock() + f.probeResults = seed.probeResults + f.gpRule = seed.gpRule + f.gpConflict = seed.gpConflict + f.parentEmpty = seed.parentEmpty + f.ctrldRule = seed.ctrldRule + f.addErr = seed.addErr + f.removeErr = seed.removeErr +} + +// requireFakeNRPTOpsInstalled proves the fake is the table production code will actually +// consult, and fails the test immediately if it is not. +// +// Identity is not enough on its own, and neither is asserting side-effect counters after +// the fact: a fixture that never installed the fake, or a seam that stopped consulting the +// override, leaves those counters at zero and every "no side effects" assertion passes +// while the real registry and signalling functions run. So this calls through +// prog.nrptOps() and requires the call to land on the fake. The probe is only made once an +// override is known to exist, so it can never reach the host itself. +func requireFakeNRPTOpsInstalled(t *testing.T, f *fakeNRPTOps) { + t.Helper() + if err := checkFakeNRPTOpsInstalled(f); err != nil { + t.Fatal(err) + } +} + +// checkFakeNRPTOpsInstalled is the precondition itself, as a predicate so it can be tested +// like any other logic - see TestFakeNRPTOpsPreconditionDetectsBypass. It returns nil only +// when f is the table prog.nrptOps() hands out. +func checkFakeNRPTOpsInstalled(f *fakeNRPTOps) error { + if nrptOpsForTest == nil { + return errors.New("fake NRPT operations are not installed: production registry and signalling functions would run") + } + if installedFakeNRPTOps != f { + return errors.New("the installed fake is not the one this fixture returned") + } + before := f.ruleExistsCount() + _ = (&prog{}).nrptOps().ruleExists() + if f.ruleExistsCount() == before { + return errors.New("prog.nrptOps() did not route to the installed fake: the seam is not in effect") + } + return nil +} + +// fakeNRPTOpsForTest returns the installed fake, installing a default one if the test has +// not already done so. +// +// Every fixture goes through this before publishing state, so no test can reach the +// production registry and signalling functions. That matters because those are not +// read-only: on a host where ctrld's deterministic key exists, one unfaked call can delete +// live NRPT policy and force a Group Policy refresh, a Dnscache paramchange and a cache +// flush. A clean runner is not a safety boundary. +func fakeNRPTOpsForTest(t *testing.T) *fakeNRPTOps { + t.Helper() + if installedFakeNRPTOps != nil { + return installedFakeNRPTOps + } + f := &fakeNRPTOps{} + installFakeNRPTOps(t, f) + return f +} + +func newHandbackTestProg(t *testing.T) (*prog, *wfpState, *fakeNRPTOps) { + t.Helper() + // Never let a test fall through to the production NRPT operations, and prove it here + // rather than trusting that installation happened. + f := fakeNRPTOpsForTest(t) + requireFakeNRPTOpsInstalled(t, f) + state := &wfpState{stopCh: make(chan struct{}), listenerIP: "127.0.0.1"} + p := &prog{} + p.dnsInterceptState = state + return p, state, f +} + +// TestHandbackRestoresFallbackWhenGPCannotRoute is the behaviour test for the handback +// contract. The first probe runs while ctrld's fallback is still installed, so that +// fallback can satisfy it; only a probe taken after ctrld's keys are gone says anything +// about the GP rule. With results [true, false] - the GP child looks fine until ctrld +// steps aside - the transition must put ctrld's rule back and keep ctrld ownership. +// +// Getting this wrong deletes the last working route and declares external ownership: in +// hard mode a machine-wide DNS outage, because WFP keeps blocking outbound DNS with +// nothing redirecting it to ctrld. +func TestHandbackRestoresFallbackWhenGPCannotRoute(t *testing.T) { + f := &fakeNRPTOps{probeResults: []bool{true, false}, gpRule: "{GP-RULE}", ctrldRule: true} + installFakeNRPTOps(t, f) + + p, state, _ := newHandbackTestProg(t) + state.setNRPTPolicyOwner(nrptRuleOwnerCtrld, "") + + if got := p.tryAdoptMatchingGPNRPT(state); got { + t.Error("tryAdoptMatchingGPNRPT() = true for a GP rule that cannot route without ctrld's rule") + } + if owner, _ := state.nrptPolicyOwner(); owner != nrptRuleOwnerCtrld { + t.Errorf("NRPT owner = %v, want nrptRuleOwnerCtrld: ownership must not move to a rule that failed the post-removal probe", owner) + } + add, remove, _, _ := f.counts() + if remove != 1 { + t.Errorf("removeRule calls = %d, want 1: the handback probe must run with ctrld's keys gone", remove) + } + if add != 1 { + t.Errorf("addRule calls = %d, want 1: the ctrld fallback must be restored after the probe fails", add) + } + if !f.hasCtrldRule() { + t.Error("ctrld's NRPT rule is missing after a failed handback; the host has no working route") + } +} + +// TestHandbackAcceptsGPWhenItRoutesWithoutCtrld is the other half of the contract: when +// the post-removal probe passes, external policy really is carrying DNS, so ctrld hands +// ownership over and leaves its keys off the machine. +func TestHandbackAcceptsGPWhenItRoutesWithoutCtrld(t *testing.T) { + f := &fakeNRPTOps{probeResults: []bool{true, true}, gpRule: "{GP-RULE}", ctrldRule: true} + installFakeNRPTOps(t, f) + + p, state, _ := newHandbackTestProg(t) + state.setNRPTPolicyOwner(nrptRuleOwnerCtrld, "") + + if !p.tryAdoptMatchingGPNRPT(state) { + t.Fatal("tryAdoptMatchingGPNRPT() = false for a GP rule that routes on its own") + } + owner, ruleName := state.nrptPolicyOwner() + if owner != nrptRuleOwnerGroupPolicy || ruleName != "{GP-RULE}" { + t.Errorf("owner = %v, rule = %q, want GroupPolicy/{GP-RULE}", owner, ruleName) + } + add, remove, _, _ := f.counts() + if remove != 1 || add != 0 { + t.Errorf("removeRule = %d, addRule = %d, want 1/0: ctrld's rule must be removed and not restored", remove, add) + } + if f.hasCtrldRule() { + t.Error("ctrld's rule is still installed beside the adopted GP catch-all") + } +} + +// TestIneffectiveGPRuleTriggersNoSignalling holds ctrld to #576's contract for a +// present-but-ineffective external rule: warn and retry WFP-only, with no policy +// refresh, no Dnscache paramchange and no cache flush. signalNRPTChange is all three at +// once, so on a permanently dead GP rule a signalling loop would force machine Group +// Policy on a schedule. +func TestIneffectiveGPRuleTriggersNoSignalling(t *testing.T) { + f := &fakeNRPTOps{probeResults: []bool{false}, gpRule: "{GP-RULE}"} + installFakeNRPTOps(t, f) + + p, state, _ := newHandbackTestProg(t) + state.setNRPTPolicyOwner(nrptRuleOwnerGroupPolicy, "{GP-RULE}") + + // The verification path a health tick and the heal cycle both take. + if got := p.nrptHandbackToExternal(state, "{GP-RULE}", "test"); got != nrptHandbackUnverified { + t.Fatalf("nrptHandbackToExternal() = %v, want nrptHandbackUnverified", got) + } + if p.healBlockedLoopbackDNS(state, "test") { + t.Error("healBlockedLoopbackDNS() = true although the probe never succeeds") + } + + add, remove, signal, _ := f.counts() + if signal != 0 { + t.Errorf("signal calls = %d, want 0: ctrld must not force GP refresh, paramchange or a cache flush while external policy owns NRPT", signal) + } + if add != 0 || remove != 0 { + t.Errorf("addRule = %d, removeRule = %d, want 0/0: external policy must be left untouched", add, remove) + } + if owner, _ := state.nrptPolicyOwner(); owner != nrptRuleOwnerGroupPolicy { + t.Errorf("owner = %v, want nrptRuleOwnerGroupPolicy: ctrld must not seize a namespace an administrator owns", owner) + } +} + +// TestConcurrentFallbackActivationWritesOnce drives two health paths - the monitor and a +// delayed recheck - into activation at the same moment. Without a transition lock they +// both observe "rule missing" and both write and signal. +func TestConcurrentFallbackActivationWritesOnce(t *testing.T) { + f := &fakeNRPTOps{probeResults: []bool{true}} + installFakeNRPTOps(t, f) + + p, state, _ := newHandbackTestProg(t) + state.setNRPTPolicyOwner(nrptRuleOwnerCtrld, "") + + // Hold both callers at the barrier until each is inside activateCtrldNRPTFallback. + start := make(chan struct{}) + var wg sync.WaitGroup + results := make([]bool, 2) + for i := range results { + wg.Add(1) + go func(i int) { + defer wg.Done() + <-start + results[i] = p.activateCtrldNRPTFallback(state, "concurrent test") + }(i) + } + close(start) + wg.Wait() + + add, _, signal, _ := f.counts() + if add != 1 { + t.Errorf("addRule calls = %d, want 1: concurrent activations must not both write the catch-all", add) + } + if signal != 1 { + t.Errorf("signal calls = %d, want 1: the DNS Client must be signalled once per transition", signal) + } + if results[0] == results[1] { + t.Errorf("both callers reported %v; exactly one transition should report having written the rule", results[0]) + } + if owner, _ := state.nrptPolicyOwner(); owner != nrptRuleOwnerCtrld { + t.Errorf("owner = %v, want nrptRuleOwnerCtrld", owner) + } +} + +// waitFor polls cond until it holds, failing the test if it never does. +func waitFor(t *testing.T, what string, cond func() bool) { + t.Helper() + deadline := time.Now().Add(5 * time.Second) + for !cond() { + if time.Now().After(deadline) { + t.Fatalf("timed out waiting for %s", what) + } + time.Sleep(time.Millisecond) + } +} + +// TestActivationLosingRaceWithStopWritesNothing is the interleaving a revocation check +// alone cannot catch: the activation passes its check, and the stop then revokes the +// state, removes NRPT and finishes before the write lands. Windows would be left routing +// every query to a listener that is gone. +// +// The test asserts the transition lock is load-bearing, in three steps. The barrier sits +// inside addRule, so the stop starts while a transition is mid-write; the stop is then +// shown to be unable to progress past its own NRPT cleanup while that transition holds +// the lock; and once the transition releases, the stop must actually have removed the +// rule before a later activation attempt is refused. Dropping the lock on either side - +// activation or stop cleanup - makes the second step fail, because the stop's removeRule +// then runs while the transition is still in flight. +func TestActivationLosingRaceWithStopWritesNothing(t *testing.T) { + f := &fakeNRPTOps{probeResults: []bool{true}} + installFakeNRPTOps(t, f) + + p, state, _ := newHandbackTestProg(t) + state.setNRPTPolicyOwner(nrptRuleOwnerCtrld, "") + + stopDone := make(chan struct{}) + f.beforeAdd = func() { + // Runs with the transition lock held, from the test goroutine. + go func() { + defer close(stopDone) + _ = p.stopDNSIntercept() + }() + + // The stop closes stopCh before it reaches the transition lock, so once that is + // closed the stop can only be waiting on this transition. Waiting on stopCh + // rather than interceptStateRevoked matters: the latter also reports the + // stop-requested flag, which is set before the stop has done anything. + waitFor(t, "the stop to revoke the intercept state", func() bool { + select { + case <-state.stopCh: + return true + default: + return false + } + }) + + // Give it a window in which it would visibly proceed if the lock were not held, + // then prove it did not: no NRPT removal, and the stop has not finished. + time.Sleep(100 * time.Millisecond) + if _, remove, _, _ := f.counts(); remove != 0 { + t.Errorf("stop removed NRPT (removeRule calls = %d) while a transition held the lock", remove) + } + select { + case <-stopDone: + t.Error("stop completed while an NRPT transition was still in flight") + default: + } + } + + if !p.activateCtrldNRPTFallback(state, "activation racing a stop") { + t.Fatal("the in-flight activation should have completed its transition") + } + <-stopDone + + // The stop ran after the transition released the lock, so its cleanup must have + // removed the rule this transition wrote. + if f.hasCtrldRule() { + t.Error("ctrld's NRPT rule survived the stop: the machine is left pointing at a listener that is gone") + } + if _, remove, _, _ := f.counts(); remove != 1 { + t.Errorf("removeRule calls = %d, want 1: the stop must clean up the rule written by the racing transition", remove) + } + if p.dnsInterceptState != nil { + t.Error("the stop did not complete after the in-flight transition released the lock") + } + + // A second attempt models the other health path arriving after the stop finished. + addBefore, _, signalBefore, _ := f.counts() + if p.activateCtrldNRPTFallback(state, "activation after the stop completed") { + t.Error("activateCtrldNRPTFallback() = true after shutdown completed") + } + addAfter, _, signalAfter, _ := f.counts() + if addAfter != addBefore || signalAfter != signalBefore { + t.Errorf("addRule %d->%d, signal %d->%d: a post-shutdown transition wrote NRPT policy for a dead listener", + addBefore, addAfter, signalBefore, signalAfter) + } + if f.hasCtrldRule() { + t.Error("a post-shutdown transition re-created ctrld's NRPT rule") + } +} + +// TestOwnedRecoveryDefersToIneffectiveExternalPolicy is the caller-level case for the +// ownership boundary: state says ctrld-owned, ctrld's key has disappeared, and an exact +// GP child is present but not routing. +// +// The handback records Group Policy ownership and reports Unverified. That is terminal: +// an administrator's catch-all owns the namespace whether or not it currently routes, so +// the owned-recovery flow must not go on to signal the DNS Client or delete and recreate +// ctrld's rule beside it. Only loopback WFP protect is allowed, and it touches no policy. +func TestOwnedRecoveryDefersToIneffectiveExternalPolicy(t *testing.T) { + f := &fakeNRPTOps{probeResults: []bool{false}, gpRule: "{GP-RULE}", ctrldRule: false} + installFakeNRPTOps(t, f) + + p, state, _ := newHandbackTestProg(t) + state.setNRPTPolicyOwner(nrptRuleOwnerCtrld, "") + + p.nrptProbeAndHeal(state) + + add, remove, signal, _ := f.counts() + if add != 0 || remove != 0 || signal != 0 { + t.Errorf("addRule = %d, removeRule = %d, signal = %d, want 0/0/0: owned recovery must stop once external policy owns the namespace", + add, remove, signal) + } + if f.hasCtrldRule() { + t.Error("owned recovery recreated ctrld's catch-all beside the administrator's rule") + } + owner, ruleName := state.nrptPolicyOwner() + if owner != nrptRuleOwnerGroupPolicy || ruleName != "{GP-RULE}" { + t.Errorf("owner = %v, rule = %q, want GroupPolicy/{GP-RULE}", owner, ruleName) + } +} + +// TestHandbackAbortedWhenRuleCannotBeRemoved keeps a failed registry step from being +// read as a verdict about external policy. +func TestHandbackAbortedWhenRuleCannotBeRemoved(t *testing.T) { + p, state, f := newHandbackTestProg(t) + f.configure(fakeNRPTSeed{ + probeResults: []bool{true, true}, + gpRule: "{GP-RULE}", + ctrldRule: true, + removeErr: errors.New("access denied"), + }) + state.setNRPTPolicyOwner(nrptRuleOwnerCtrld, "") + + if got := p.nrptHandbackToExternal(state, "{GP-RULE}", "test"); got != nrptHandbackAborted { + t.Fatalf("nrptHandbackToExternal() = %v, want nrptHandbackAborted", got) + } + if owner, _ := state.nrptPolicyOwner(); owner != nrptRuleOwnerCtrld { + t.Errorf("owner = %v, want nrptRuleOwnerCtrld: a failed removal is not proof about external policy", owner) + } +} + +// TestHandbackRestoresFallbackWhenGPChildDisappearsMidProbe covers a Group Policy refresh +// landing during the post-removal probe: the child ctrld was testing is simply gone. There +// is no external policy left to hand ownership to, so ctrld's rule has to come back. +func TestHandbackRestoresFallbackWhenGPChildDisappearsMidProbe(t *testing.T) { + f := &fakeNRPTOps{probeResults: []bool{true, false}, gpRule: "{GP-RULE}", ctrldRule: true} + installFakeNRPTOps(t, f) + f.onProbe = func(call int) { + if call == 2 { + f.setGPRule("", false) // the administrator removed the catch-all + } + } + + p, state, _ := newHandbackTestProg(t) + state.setNRPTPolicyOwner(nrptRuleOwnerCtrld, "") + + if got := p.nrptHandbackToExternal(state, "{GP-RULE}", "test"); got != nrptHandbackKeptCtrld { + t.Fatalf("nrptHandbackToExternal() = %v, want nrptHandbackKeptCtrld", got) + } + if owner, _ := state.nrptPolicyOwner(); owner != nrptRuleOwnerCtrld { + t.Errorf("owner = %v, want nrptRuleOwnerCtrld", owner) + } + if !f.hasCtrldRule() { + t.Error("ctrld's rule was not restored after the GP child disappeared; the host has no route") + } + if add, remove, _, _ := f.counts(); add != 1 || remove != 1 { + t.Errorf("addRule = %d, removeRule = %d, want 1/1", add, remove) + } +} + +// TestHandbackWritesNoSiblingWhenGPChildTurnsConflicting is the case the same-child +// re-read exists for. ctrld removes its keys to test an exact catch-all, and during the +// probe Group Policy replaces it with one that targets another resolver (or a malformed +// one). Restoring ctrld's rule would create exactly the competing sibling beside +// administrator policy that this must never write, so the rule stays off and ownership +// goes to nobody. +func TestHandbackWritesNoSiblingWhenGPChildTurnsConflicting(t *testing.T) { + f := &fakeNRPTOps{probeResults: []bool{true, false}, gpRule: "{GP-RULE}", ctrldRule: true} + installFakeNRPTOps(t, f) + f.onProbe = func(call int) { + if call == 2 { + // The child now names another resolver: not a match for this listener, and + // reported as a conflict by the GP classifier. + f.setGPRule("", true) + } + } + + p, state, _ := newHandbackTestProg(t) + state.setNRPTPolicyOwner(nrptRuleOwnerCtrld, "") + + if got := p.nrptHandbackToExternal(state, "{GP-RULE}", "test"); got != nrptHandbackConflict { + t.Fatalf("nrptHandbackToExternal() = %v, want nrptHandbackConflict", got) + } + if add, _, _, _ := f.counts(); add != 0 { + t.Errorf("addRule calls = %d, want 0: restoring here writes a competing rule beside administrator policy", add) + } + if f.hasCtrldRule() { + t.Error("ctrld's rule is installed beside a GP catch-all that targets another resolver") + } + if owner, _ := state.nrptPolicyOwner(); owner != nrptRuleOwnerNone { + t.Errorf("owner = %v, want nrptRuleOwnerNone: ctrld owns nothing once external policy points elsewhere", owner) + } +} + +// TestHandbackWithoutCtrldRuleClassifiesAfterFailedProbe covers the other short-circuit: +// with no ctrld rule to remove, a failed probe must still be classified before anything is +// recorded. A child that changed into a conflicting catch-all is not an ineffective ctrld +// candidate, and must not be recorded as one. +func TestHandbackWithoutCtrldRuleClassifiesAfterFailedProbe(t *testing.T) { + f := &fakeNRPTOps{probeResults: []bool{false}, gpRule: "{GP-RULE}"} + installFakeNRPTOps(t, f) + f.onProbe = func(call int) { + if call == 1 { + f.setGPRule("", true) + } + } + + p, state, _ := newHandbackTestProg(t) + state.setNRPTPolicyOwner(nrptRuleOwnerCtrld, "") + + if got := p.nrptHandbackToExternal(state, "{GP-RULE}", "test"); got != nrptHandbackConflict { + t.Fatalf("nrptHandbackToExternal() = %v, want nrptHandbackConflict", got) + } + if add, remove, signal, _ := f.counts(); add != 0 || remove != 0 || signal != 0 { + t.Errorf("addRule = %d, removeRule = %d, signal = %d, want 0/0/0", add, remove, signal) + } + owner, ruleName := state.nrptPolicyOwner() + if owner != nrptRuleOwnerNone || ruleName != "" { + t.Errorf("owner = %v, rule = %q, want nrptRuleOwnerNone/\"\": a conflicting child must not be recorded as ctrld's external owner", owner, ruleName) + } +} + +// TestHandbackGivesReplacementChildItsOwnPass keeps an unproved child from inheriting a +// verdict that was measured against a different one. With no ctrld rule installed the +// replacement is simply probed on its own pass; failing that pass records it as external +// policy that is not routing, and writes nothing either way. +func TestHandbackGivesReplacementChildItsOwnPass(t *testing.T) { + f := &fakeNRPTOps{probeResults: []bool{false, false}, gpRule: "{GP-RULE}"} + installFakeNRPTOps(t, f) + f.onProbe = func(call int) { + if call == 1 { + f.setGPRule("{NEW-GP-RULE}", false) + } + } + + p, state, _ := newHandbackTestProg(t) + + if got := p.nrptHandbackToExternal(state, "{GP-RULE}", "test"); got != nrptHandbackUnverified { + t.Fatalf("nrptHandbackToExternal() = %v, want nrptHandbackUnverified", got) + } + if add, remove, signal, _ := f.counts(); add != 0 || remove != 0 || signal != 0 { + t.Errorf("addRule = %d, removeRule = %d, signal = %d, want 0/0/0", add, remove, signal) + } + owner, ruleName := state.nrptPolicyOwner() + if owner != nrptRuleOwnerGroupPolicy || ruleName != "{NEW-GP-RULE}" { + t.Errorf("owner = %v, rule = %q, want GroupPolicy/{NEW-GP-RULE}: the verdict must name the child it was measured against", owner, ruleName) + } +} + +// TestHandbackWritesNoSiblingWhenReplacementChildTakesOver is the case where ctrld's rule +// was removed for the probe and a *different* exact catch-all took the namespace while it +// was off. +// +// Restoring ctrld's rule here is not a return to the status quo: the replacement was not +// there when the transition started, and addNRPTCatchAllRule writes ctrld's own GP +// catch-all whenever another GP rule exists, so the restore would plant a sibling beside +// the administrator's new rule. The replacement gets its own probe instead, and a failed +// one leaves NRPT to Group Policy with nothing of ctrld's written. +func TestHandbackWritesNoSiblingWhenReplacementChildTakesOver(t *testing.T) { + // [pre-probe true, post-removal probe false, replacement's own pass false] + f := &fakeNRPTOps{probeResults: []bool{true, false, false}, gpRule: "{GP-RULE}", ctrldRule: true} + installFakeNRPTOps(t, f) + f.onProbe = func(call int) { + if call == 2 { + f.setGPRule("{NEW-GP-RULE}", false) // a policy refresh swaps the child + } + } + + p, state, _ := newHandbackTestProg(t) + state.setNRPTPolicyOwner(nrptRuleOwnerCtrld, "") + + if got := p.nrptHandbackToExternal(state, "{GP-RULE}", "test"); got != nrptHandbackUnverified { + t.Fatalf("nrptHandbackToExternal() = %v, want nrptHandbackUnverified", got) + } + if add, remove, _, _ := f.counts(); add != 0 || remove != 1 { + t.Errorf("addRule = %d, removeRule = %d, want 0/1: ctrld's rule must not be restored beside a replacement catch-all", add, remove) + } + if f.hasCtrldRule() { + t.Error("ctrld's rule is installed beside an administrator catch-all that took the namespace") + } + owner, ruleName := state.nrptPolicyOwner() + if owner != nrptRuleOwnerGroupPolicy || ruleName != "{NEW-GP-RULE}" { + t.Errorf("owner = %v, rule = %q, want GroupPolicy/{NEW-GP-RULE}", owner, ruleName) + } +} + +// TestHandbackAdoptsReplacementChildThatRoutes is the same swap where the replacement does +// carry DNS on its own: it is adopted, still with no ctrld write. +func TestHandbackAdoptsReplacementChildThatRoutes(t *testing.T) { + f := &fakeNRPTOps{probeResults: []bool{true, false, true}, gpRule: "{GP-RULE}", ctrldRule: true} + installFakeNRPTOps(t, f) + f.onProbe = func(call int) { + if call == 2 { + f.setGPRule("{NEW-GP-RULE}", false) + } + } + + p, state, _ := newHandbackTestProg(t) + state.setNRPTPolicyOwner(nrptRuleOwnerCtrld, "") + + if got := p.nrptHandbackToExternal(state, "{GP-RULE}", "test"); got != nrptHandbackVerified { + t.Fatalf("nrptHandbackToExternal() = %v, want nrptHandbackVerified", got) + } + if add, _, _, _ := f.counts(); add != 0 { + t.Errorf("addRule calls = %d, want 0", add) + } + owner, ruleName := state.nrptPolicyOwner() + if owner != nrptRuleOwnerGroupPolicy || ruleName != "{NEW-GP-RULE}" { + t.Errorf("owner = %v, rule = %q, want GroupPolicy/{NEW-GP-RULE}", owner, ruleName) + } +} + +// TestHandbackChurnFailsSafeToCurrentExternalOwner bounds the extra pass without letting +// exhaustion become a licence to write. +// +// When the probe budget runs out with the store still changing, the transition classifies +// the store as it stands and proves no route. Reporting "undecided" instead would be +// unsafe: that disposition is not terminal external ownership, so startup and owned +// recovery fall through to writing ctrld's rule, and addNRPTCatchAllRule puts it in the GP +// path beside whichever exact catch-all is there now. +func TestHandbackChurnFailsSafeToCurrentExternalOwner(t *testing.T) { + f := &fakeNRPTOps{probeResults: []bool{true, false, false}, gpRule: "{GP-RULE}", ctrldRule: true} + installFakeNRPTOps(t, f) + f.onProbe = func(call int) { + switch call { + case 2: + f.setGPRule("{GP-RULE-2}", false) + case 3: + f.setGPRule("{GP-RULE-3}", false) + } + } + + p, state, _ := newHandbackTestProg(t) + state.setNRPTPolicyOwner(nrptRuleOwnerCtrld, "") + + got := p.nrptHandbackToExternal(state, "{GP-RULE}", "test") + if got != nrptHandbackUnverified { + t.Fatalf("nrptHandbackToExternal() = %v, want nrptHandbackUnverified", got) + } + if !nrptExternalOwns(got) { + t.Error("the churn disposition is not terminal external ownership; callers would fall through and write a sibling") + } + if add, _, _, _ := f.counts(); add != 0 { + t.Errorf("addRule calls = %d, want 0: no sibling write while the GP store is churning", add) + } + owner, ruleName := state.nrptPolicyOwner() + if owner != nrptRuleOwnerGroupPolicy || ruleName != "{GP-RULE-3}" { + t.Errorf("owner = %v, rule = %q, want GroupPolicy/{GP-RULE-3}: exhaustion must record the store as it stands", owner, ruleName) + } +} + +// TestHandbackChurnFailsSafeToConflict covers exhaustion where the store has settled on a +// catch-all that does not target ctrld: still terminal, still no write. +func TestHandbackChurnFailsSafeToConflict(t *testing.T) { + f := &fakeNRPTOps{probeResults: []bool{true, false, false}, gpRule: "{GP-RULE}", ctrldRule: true} + installFakeNRPTOps(t, f) + f.onProbe = func(call int) { + switch call { + case 2: + f.setGPRule("{GP-RULE-2}", false) + case 3: + f.setGPRule("", true) // now an administrator catch-all for another resolver + } + } + + p, state, _ := newHandbackTestProg(t) + state.setNRPTPolicyOwner(nrptRuleOwnerCtrld, "") + + if got := p.nrptHandbackToExternal(state, "{GP-RULE}", "test"); got != nrptHandbackConflict { + t.Fatalf("nrptHandbackToExternal() = %v, want nrptHandbackConflict", got) + } + if add, _, _, _ := f.counts(); add != 0 { + t.Errorf("addRule calls = %d, want 0", add) + } + if owner, _ := state.nrptPolicyOwner(); owner != nrptRuleOwnerNone { + t.Errorf("owner = %v, want nrptRuleOwnerNone", owner) + } +} + +// TestHandbackChurnRestoresFallbackWhenNamespaceFreed is the third exhaustion outcome: the +// store ended up with no external catch-all at all, so the namespace is free and ctrld's +// rule is the one that belongs there. +func TestHandbackChurnRestoresFallbackWhenNamespaceFreed(t *testing.T) { + f := &fakeNRPTOps{probeResults: []bool{true, false, false}, gpRule: "{GP-RULE}", ctrldRule: true} + installFakeNRPTOps(t, f) + f.onProbe = func(call int) { + switch call { + case 2: + f.setGPRule("{GP-RULE-2}", false) + case 3: + f.setGPRule("", false) // the administrator removed the policy entirely + } + } + + p, state, _ := newHandbackTestProg(t) + state.setNRPTPolicyOwner(nrptRuleOwnerCtrld, "") + + if got := p.nrptHandbackToExternal(state, "{GP-RULE}", "test"); got != nrptHandbackKeptCtrld { + t.Fatalf("nrptHandbackToExternal() = %v, want nrptHandbackKeptCtrld", got) + } + if !f.hasCtrldRule() { + t.Error("ctrld's rule was not restored although no external catch-all remains") + } + if owner, _ := state.nrptPolicyOwner(); owner != nrptRuleOwnerCtrld { + t.Errorf("owner = %v, want nrptRuleOwnerCtrld", owner) + } +} + +// TestActivationWritesNoSiblingWhileGPStoreChurns is the caller-level closure for the churn +// path. activateCtrldNRPTFallback is the function both startup fallback and owned recovery +// reach when ctrld's rule is missing, and it is where a non-terminal churn disposition +// would turn into addNRPTCatchAllRule writing a GP sibling beside the current catch-all. +func TestActivationWritesNoSiblingWhileGPStoreChurns(t *testing.T) { + f := &fakeNRPTOps{probeResults: []bool{false, false}, gpRule: "{GP-RULE}"} + installFakeNRPTOps(t, f) + f.onProbe = func(call int) { + switch call { + case 1: + f.setGPRule("{GP-RULE-2}", false) + case 2: + f.setGPRule("{GP-RULE-3}", false) + } + } + + p, state, _ := newHandbackTestProg(t) + // Startup and the owner-None retry both arrive here with no ownership recorded. + state.setNRPTPolicyOwner(nrptRuleOwnerNone, "") + + if p.activateCtrldNRPTFallback(state, "churn test") { + t.Error("activateCtrldNRPTFallback() = true while an external catch-all owns the namespace") + } + add, remove, signal, _ := f.counts() + if add != 0 || signal != 0 { + t.Errorf("addRule = %d, signal = %d, want 0/0: ctrld must not write beside the administrator's catch-all", add, signal) + } + if remove != 0 { + t.Errorf("removeRule calls = %d, want 0: there was no ctrld rule to remove", remove) + } + if owner, _ := state.nrptPolicyOwner(); owner != nrptRuleOwnerGroupPolicy { + t.Errorf("owner = %v, want nrptRuleOwnerGroupPolicy: the current external catch-all owns the namespace", owner) + } +} + +// TestStopLeavesGPManagedPolicyAlone is the central promise of this work: an +// administrator's catch-all is a deployment contract that must survive service stop, +// restart and uninstall. A regression that removed NRPT for every owner would delete that +// rule and take the fleet's DNS policy with it. +func TestStopLeavesGPManagedPolicyAlone(t *testing.T) { + f := &fakeNRPTOps{gpRule: "{GP-RULE}"} + installFakeNRPTOps(t, f) + + p, state, _ := newHandbackTestProg(t) + state.setNRPTPolicyOwner(nrptRuleOwnerGroupPolicy, "{GP-RULE}") + + if err := p.stopDNSIntercept(); err != nil { + t.Fatalf("stopDNSIntercept() = %v", err) + } + + add, remove, signal, _ := f.counts() + if remove != 0 || signal != 0 || add != 0 { + t.Errorf("removeRule = %d, signal = %d, addRule = %d, want 0/0/0: shutdown must not touch externally owned NRPT policy", + remove, signal, add) + } + if f.flushCount() != 0 { + t.Errorf("flush calls = %d, want 0: no cache flush is owed for a rule ctrld does not own", f.flushCount()) + } + if p.dnsInterceptState != nil { + t.Error("dnsInterceptState survived shutdown") + } +} + +// TestStopRemovesOrphanWhileLeavingGPPolicy is the same shutdown with a ctrld rule left +// behind by an earlier unclean exit. External policy still stays, but the orphan must go: +// while GP mode hides the local store this stop is the last chance anything looks there. +func TestStopRemovesOrphanWhileLeavingGPPolicy(t *testing.T) { + f := &fakeNRPTOps{gpRule: "{GP-RULE}", ctrldRule: true} + installFakeNRPTOps(t, f) + + p, state, _ := newHandbackTestProg(t) + state.setNRPTPolicyOwner(nrptRuleOwnerGroupPolicy, "{GP-RULE}") + + if err := p.stopDNSIntercept(); err != nil { + t.Fatalf("stopDNSIntercept() = %v", err) + } + + add, remove, signal, _ := f.counts() + if remove != 1 || signal != 1 { + t.Errorf("removeRule = %d, signal = %d, want 1/1: the orphaned ctrld rule must be removed on the way out", remove, signal) + } + if add != 0 { + t.Errorf("addRule calls = %d, want 0", add) + } + if f.hasCtrldRule() { + t.Error("the orphaned ctrld rule survived shutdown; a later GP removal would activate it") + } + if f.flushCount() != 0 { + t.Errorf("flush calls = %d, want 0: the ctrld-owned branch is what owes a flush, not the GP branch", f.flushCount()) + } +} + +// TestRemoveOrphanedCtrldNRPTRule covers the sweep directly, including that it is a no-op +// when there is nothing of ctrld's on disk - it runs on shutdown paths where the common +// case is a clean machine. +func TestRemoveOrphanedCtrldNRPTRule(t *testing.T) { + t.Run("removes and signals when a ctrld rule exists", func(t *testing.T) { + f := &fakeNRPTOps{ctrldRule: true} + installFakeNRPTOps(t, f) + + p, _, _ := newHandbackTestProg(t) + p.removeOrphanedCtrldNRPTRule("test") + + add, remove, signal, _ := f.counts() + if remove != 1 || signal != 1 || add != 0 { + t.Errorf("removeRule = %d, signal = %d, addRule = %d, want 1/1/0", remove, signal, add) + } + if f.hasCtrldRule() { + t.Error("the ctrld rule is still installed") + } + }) + + t.Run("does nothing when no ctrld rule exists", func(t *testing.T) { + f := &fakeNRPTOps{} + installFakeNRPTOps(t, f) + + p, _, _ := newHandbackTestProg(t) + p.removeOrphanedCtrldNRPTRule("test") + + add, remove, signal, _ := f.counts() + if remove != 0 || signal != 0 || add != 0 { + t.Errorf("removeRule = %d, signal = %d, addRule = %d, want 0/0/0: nothing to sweep must cost no registry writes and no signalling", + remove, signal, add) + } + }) +} + +// TestHandbackThrottleBlocksTheSecondAttempt exercises the throttle through the +// transition, not just its accessor. Each attempt removes the live catch-all for a probe, +// so a second attempt inside the window must cost nothing at all: no probe, no removal. +func TestHandbackThrottleBlocksTheSecondAttempt(t *testing.T) { + f := &fakeNRPTOps{probeResults: []bool{true, false}, gpRule: "{GP-RULE}", ctrldRule: true} + installFakeNRPTOps(t, f) + + p, state, _ := newHandbackTestProg(t) + state.setNRPTPolicyOwner(nrptRuleOwnerCtrld, "") + + if got := p.nrptHandbackToExternal(state, "{GP-RULE}", "first"); got != nrptHandbackKeptCtrld { + t.Fatalf("first nrptHandbackToExternal() = %v, want nrptHandbackKeptCtrld", got) + } + _, removeAfterFirst, _, probesAfterFirst := f.counts() + + if got := p.nrptHandbackToExternal(state, "{GP-RULE}", "second"); got != nrptHandbackAborted { + t.Fatalf("second nrptHandbackToExternal() = %v, want nrptHandbackAborted", got) + } + _, remove, _, probes := f.counts() + if probes != probesAfterFirst { + t.Errorf("probe calls went from %d to %d: a throttled attempt must not probe", probesAfterFirst, probes) + } + if remove != removeAfterFirst { + t.Errorf("removeRule calls went from %d to %d: a throttled attempt must not remove the live rule", removeAfterFirst, remove) + } + if owner, _ := state.nrptPolicyOwner(); owner != nrptRuleOwnerCtrld { + t.Errorf("owner = %v, want nrptRuleOwnerCtrld", owner) + } +} + +// TestHealLadderRunsRetriesThenTwoPhaseRecovery drives the ctrld-owned heal cycle end to +// end: the immediate probe, the signal-and-backoff retries, then the two-phase delete and +// re-add, with the last probe finally succeeding. Each nrptTransition phase is asserted +// through its side effects, since these are its only call sites. +func TestHealLadderRunsRetriesThenTwoPhaseRecovery(t *testing.T) { + // Every probe fails until the one after the re-add. + f := &fakeNRPTOps{ + probeResults: []bool{false, false, false, false, true}, + ctrldRule: true, + } + installFakeNRPTOps(t, f) + + p, state, _ := newHandbackTestProg(t) + state.setNRPTPolicyOwner(nrptRuleOwnerCtrld, "") + + p.nrptProbeAndHeal(state) + + add, remove, signal, probes := f.counts() + if remove != 1 { + t.Errorf("removeRule calls = %d, want 1: phase one of the two-phase recovery deletes ctrld's rule once", remove) + } + if add != 1 { + t.Errorf("addRule calls = %d, want 1: phase two re-adds it once", add) + } + if !f.hasCtrldRule() { + t.Error("ctrld's rule is missing after the two-phase recovery") + } + // Three backoff rounds signal, then the delete phase and the re-add phase. + if signal < 5 { + t.Errorf("signal calls = %d, want at least 5 (three retries plus both recovery phases)", signal) + } + if probes < 5 { + t.Errorf("probe calls = %d, want at least 5", probes) + } + if owner, _ := state.nrptPolicyOwner(); owner != nrptRuleOwnerCtrld { + t.Errorf("owner = %v, want nrptRuleOwnerCtrld", owner) + } +} + +// TestHealLadderStopsWhenGPAppearsBeforeTwoPhase covers a Group Policy catch-all landing +// during the backoff retries. The ladder must hand back and stop before the destructive +// delete-and-re-add phase, and must not write ctrld's rule beside the new policy. +func TestHealLadderStopsWhenGPAppearsBeforeTwoPhase(t *testing.T) { + f := &fakeNRPTOps{probeResults: []bool{false}, ctrldRule: true} + installFakeNRPTOps(t, f) + // The administrator's rule appears while the fourth probe is in flight, which is the + // last one before the two-phase recovery. + f.onProbe = func(call int) { + if call == 4 { + f.setGPRule("{GP-RULE}", false) + } + } + + p, state, _ := newHandbackTestProg(t) + state.setNRPTPolicyOwner(nrptRuleOwnerCtrld, "") + + p.nrptProbeAndHeal(state) + + add, remove, _, _ := f.counts() + if remove != 0 { + t.Errorf("removeRule calls = %d, want 0: the ladder must stop before the delete half of the two-phase recovery", remove) + } + if add != 0 { + t.Errorf("addRule calls = %d, want 0: the ladder must not recreate ctrld's rule beside a new GP catch-all", add) + } + if !f.hasCtrldRule() { + t.Error("ctrld's rule was deleted after an administrator catch-all appeared") + } + owner, ruleName := state.nrptPolicyOwner() + if owner != nrptRuleOwnerGroupPolicy || ruleName != "{GP-RULE}" { + t.Errorf("owner = %v, rule = %q, want GroupPolicy/{GP-RULE}: the ladder must defer rather than continue", owner, ruleName) + } +} + +// TestHealLadderCleansEmptyGPParentBeforeRetrying pins the empty-GP-parent shortcut: an +// empty parent key puts the DNS Client in GP mode where ctrld's local rule is invisible, so +// signalling retries cannot succeed until it is deleted. +func TestHealLadderCleansEmptyGPParentBeforeRetrying(t *testing.T) { + f := &fakeNRPTOps{probeResults: []bool{false, true}, ctrldRule: true, parentEmpty: true} + installFakeNRPTOps(t, f) + + p, state, _ := newHandbackTestProg(t) + state.setNRPTPolicyOwner(nrptRuleOwnerCtrld, "") + + p.nrptProbeAndHeal(state) + + if f.cleanCalls != 1 { + t.Errorf("cleanParent calls = %d, want 1: the empty GP parent must be removed before burning retries", f.cleanCalls) + } + if add, remove, _, _ := f.counts(); add != 0 || remove != 0 { + t.Errorf("addRule = %d, removeRule = %d, want 0/0: the probe passed after the cleanup, so no recovery was needed", add, remove) + } +} + +// TestRepairMissingWFPRetriesHardModeWithNoEngine covers the entry point a startup WFP +// failure depends on. In hard mode a closed engine means nothing is being blocked, so the +// health monitor must keep retrying; in dns mode a closed engine is the normal state and +// must not trigger anything. +func TestRepairMissingWFPRetriesHardModeWithNoEngine(t *testing.T) { + originalRebuild, originalHard := rebuildDNSInterceptFn, hardIntercept + t.Cleanup(func() { rebuildDNSInterceptFn, hardIntercept = originalRebuild, originalHard }) + + var reasons []string + rebuildDNSInterceptFn = func(_ *prog, _ *wfpState, reason string) interceptRebuildResult { + reasons = append(reasons, reason) + return interceptRebuildDone + } + + t.Run("hard mode retries when the engine never opened", func(t *testing.T) { + reasons = nil + hardIntercept = true + p, state, _ := newHandbackTestProg(t) + + if !p.repairMissingWFP(state) { + t.Error("repairMissingWFP() = false; the monitor must hand over to the rebuilt intercept") + } + if len(reasons) != 1 { + t.Fatalf("rebuild requests = %d, want 1: hard mode with no WFP engine must be retried", len(reasons)) + } + if !strings.Contains(reasons[0], "no WFP engine") { + t.Errorf("rebuild reason = %q, want it to name the missing engine", reasons[0]) + } + }) + + t.Run("dns mode leaves a closed engine alone", func(t *testing.T) { + reasons = nil + hardIntercept = false + p, state, _ := newHandbackTestProg(t) + + if p.repairMissingWFP(state) { + t.Error("repairMissingWFP() = true in dns mode; a closed engine is normal there") + } + if len(reasons) != 0 { + t.Errorf("rebuild requests = %d, want 0", len(reasons)) + } + }) +} + +// TestPhaseTwoRevalidatesOwnershipAfterWaitingForTheLock is the ordering a pre-lock check +// cannot cover. Phase one removes ctrld's rule and releases the transition lock; the +// pre-re-add checks see no GP rule; another health path then takes the lock, hands the +// namespace to a GP catch-all that has just arrived, and records GroupPolicy. Phase two +// acquires the lock afterwards. +// +// Without a re-read inside the locked closure, phase two re-adds ctrld's rule beside the +// administrator's catch-all and stamps ctrld ownership over theirs - exactly the competing +// policy this work exists to prevent. +func TestPhaseTwoRevalidatesOwnershipAfterWaitingForTheLock(t *testing.T) { + p, state, f := newHandbackTestProg(t) + // Every probe fails, so the ladder runs to the two-phase recovery. + f.configure(fakeNRPTSeed{probeResults: []bool{false}, ctrldRule: true}) + state.setNRPTPolicyOwner(nrptRuleOwnerCtrld, "") + + // The last ops call before phase two queues for the lock. Hold the lock from another + // goroutine and complete the competing transition while phase two waits for it. + f.onGPConflicts = func(call int) { + if call != 1 { + return + } + locked := make(chan struct{}) + go func() { + p.nrptTransitionMu.Lock() + close(locked) + f.setGPRule("{GP-RULE}", false) + state.setNRPTPolicyOwner(nrptRuleOwnerGroupPolicy, "{GP-RULE}") + // Hold it long enough that phase two must queue behind this transition. + time.Sleep(100 * time.Millisecond) + p.nrptTransitionMu.Unlock() + }() + <-locked + } + + p.nrptProbeAndHeal(state) + + if add, _, _, _ := f.counts(); add != 0 { + t.Errorf("addRule calls = %d, want 0: phase two re-added ctrld's rule beside a GP catch-all that took ownership while it waited for the lock", add) + } + if f.hasCtrldRule() { + t.Error("ctrld's rule is installed beside the administrator's catch-all") + } + owner, ruleName := state.nrptPolicyOwner() + if owner != nrptRuleOwnerGroupPolicy || ruleName != "{GP-RULE}" { + t.Errorf("owner = %v, rule = %q, want GroupPolicy/{GP-RULE}: phase two overwrote a newer ownership transition", owner, ruleName) + } +} + +// TestRetiredHealDoesNotDeleteSuccessorRule covers the other side of a rebuild. The heal +// cycle re-adds ctrld's rule, then its final settle wait observes that its own state was +// retired - while a successor intercept is already running with the same deterministic key. +// +// The key is process-global, so any cleanup from here would delete the successor's route +// and leave hard mode blocking DNS with nothing redirecting it. Teardown of the retired +// state owns that cleanup instead. +func TestRetiredHealDoesNotDeleteSuccessorRule(t *testing.T) { + p, state, f := newHandbackTestProg(t) + f.configure(fakeNRPTSeed{probeResults: []bool{false}, ctrldRule: true}) + state.setNRPTPolicyOwner(nrptRuleOwnerCtrld, "") + + successor := &wfpState{stopCh: make(chan struct{}), listenerIP: "127.0.0.1"} + // Retire this cycle's state during the final wait - the one after the re-add, which is + // the only wait where both a removal and an add have happened - and publish a successor + // that owns the same deterministic key. + f.waitHook = func() bool { + if add, remove, _, _ := f.counts(); add == 0 || remove == 0 { + return true + } + close(state.stopCh) // a rebuild retired the old state + p.dnsInterceptState = successor + successor.setNRPTPolicyOwner(nrptRuleOwnerCtrld, "") + return false + } + + p.nrptProbeAndHeal(state) + + if !f.hasCtrldRule() { + t.Error("the successor's NRPT rule was deleted by a retired heal cycle") + } + if owner, _ := successor.nrptPolicyOwner(); owner != nrptRuleOwnerCtrld { + t.Errorf("successor owner = %v, want nrptRuleOwnerCtrld", owner) + } +} + +// TestStartupReportsFailureWhenExternalPolicyNeverRoutes is the readiness direction of the +// GP-managed contract, driven through the real startup control flow. +// +// An exact GP child is present and no probe ever reaches ctrld. Startup must keep the +// recovery state and the monitor - the rule may start routing later, and only external +// policy can fix it - must leave adapter DNS alone, and must not write an owned fallback. +// What it must not do is return success: setDNS records readiness from a nil error, so +// reporting ready here publishes "service healthy" while the DNS Client is not delivering +// queries to ctrld, and in hard mode WFP is blocking every other resolver at the same time. +func TestStartupReportsFailureWhenExternalPolicyNeverRoutes(t *testing.T) { + originalCfg, originalMode, originalIntercept, originalHard := cfg, interceptMode, dnsIntercept, hardIntercept + t.Cleanup(func() { + cfg, interceptMode, dnsIntercept, hardIntercept = originalCfg, originalMode, originalIntercept, originalHard + }) + cfg = ctrld.Config{} + cfg.Listener = map[string]*ctrld.ListenerConfig{"0": {IP: "127.0.0.1", Port: 53}} + interceptMode, dnsIntercept, hardIntercept = "dns", true, false + + p, _, f := newHandbackTestProg(t) + p.cfg = &cfg + p.dnsInterceptState = nil // startup publishes its own state + // An exact GP child that never answers a probe. + f.configure(fakeNRPTSeed{probeResults: []bool{false}, gpRule: "{GP-RULE}"}) + + err := p.startDNSInterceptLocked() + if err == nil { + t.Fatal("startDNSInterceptLocked() = nil: startup reported success while no probe reached ctrld, so setDNS would mark the service ready") + } + if !interceptFailedUnderExternalDNSPolicy(err) { + t.Errorf("err = %v; the failure must tell setDNS to leave adapter DNS untouched", err) + } + if interceptFailedWithVerifiedExternalDNS(err) { + t.Errorf("err = %v; this route was never verified, so it must not read as the verified case", err) + } + if add, remove, signal, _ := f.counts(); add != 0 || remove != 0 || signal != 0 { + t.Errorf("addRule = %d, removeRule = %d, signal = %d, want 0/0/0: no owned fallback may be written beside an administrator catch-all", add, remove, signal) + } + + // Recovery must stay live so the rule can be re-tested. + state, ok := p.dnsInterceptState.(*wfpState) + if !ok || state == nil { + t.Fatal("intercept state was not published; nothing would keep re-testing the external rule") + } + if owner, ruleName := state.nrptPolicyOwner(); owner != nrptRuleOwnerGroupPolicy || ruleName != "{GP-RULE}" { + t.Errorf("owner = %v, rule = %q, want GroupPolicy/{GP-RULE}", owner, ruleName) + } + // Stop the monitor startup launched. + _ = p.stopDNSIntercept() +} + +// TestFakeNRPTOpsPreconditionDetectsBypass guards the guard. The fixtures' promise is that +// no test can reach the production registry and signalling functions, and that promise is +// only as good as this precondition: if it stops detecting a bypass, every "no side +// effects" assertion in the Windows suite silently becomes vacuous while the real host +// policy is at risk. +func TestFakeNRPTOpsPreconditionDetectsBypass(t *testing.T) { + f := &fakeNRPTOps{} + + // The exact bypass to catch: a fixture that returns a fake it never installed. + if err := checkFakeNRPTOpsInstalled(f); err == nil { + t.Error("an uninstalled fake passed the precondition; a test could fall through to production NRPT operations") + } + + installFakeNRPTOps(t, f) + if err := checkFakeNRPTOpsInstalled(f); err != nil { + t.Errorf("an installed fake failed the precondition: %v", err) + } + + // A different fake than the installed one must not pass either: side-effect counters + // would be read from an object nothing consults. + if err := checkFakeNRPTOpsInstalled(&fakeNRPTOps{}); err == nil { + t.Error("a fake that is not the installed one passed the precondition") + } +} diff --git a/cmd/cli/prog.go b/cmd/cli/prog.go index 04b7ead..6f13349 100644 --- a/cmd/cli/prog.go +++ b/cmd/cli/prog.go @@ -172,6 +172,22 @@ type prog struct { // On Windows: *wfpState, on macOS: *pfState, nil on other platforms. dnsInterceptState any + // dnsInterceptMu serializes DNS intercept lifecycle transitions - start, stop and + // the health monitor's rebuild - and guards every write to dnsInterceptState, so a + // service stop can never interleave with a monitor-driven rebuild. + dnsInterceptMu sync.Mutex //lint:ignore U1000 used on windows + + // dnsInterceptStopRequested is set while a stop waits for dnsInterceptMu. The + // health and recovery flows read it as a shutdown signal and abandon their work, + // rather than making the stop wait out their probe backoffs. + dnsInterceptStopRequested atomic.Bool //lint:ignore U1000 used on windows + + // nrptTransitionMu makes one NRPT ownership transition - observe, mutate, signal, + // record owner - atomic against shutdown and against another transition. It is + // deliberately finer-grained than dnsInterceptMu: it is taken for the duration of a + // single transition, never across the recovery flows' probe backoffs. + nrptTransitionMu sync.Mutex //lint:ignore U1000 used on windows + // 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. @@ -220,15 +236,22 @@ type prog struct { // 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. - // When non-empty, the DNS handler checks incoming queries against this value - // and signals pfProbeCh if matched. The probe verifies that pf's rdr rules - // are actually translating packets (not just present in rule text). - pfProbeExpected atomic.Value // string - - // pfProbeCh is signaled when the DNS handler receives the expected probe query. - // The channel is created by probePFIntercept() and closed when the probe arrives. - pfProbeCh atomic.Value // *chan struct{} + // interceptProbes maps the domain of each pending interception probe to the channel + // that probe waits on. A probe verifies that interception is actually translating or + // redirecting packets, not merely present in rule text: the DNS handler looks up + // incoming queries here and signals the matching waiter. + // + // It holds one entry per in-flight probe rather than a single slot, because probes do + // overlap - the health monitor, a handback and a heal cycle can each have one out at + // the same time - and a single slot means the last registration wins and the loser + // waits out its timeout for a query that was answered. A false failure then triggers + // recovery work that was not needed. + // + // Registrations are rare and lookups happen on every query, so the map is stored as + // an immutable snapshot behind an atomic: readers never take a lock, writers copy + // under interceptProbeMu. + interceptProbes atomic.Value // map[string]chan struct{} + interceptProbeMu sync.Mutex //lint:ignore U1000 written only by registerInterceptProbe, used on darwin/windows // VPN DNS manager for split DNS routing when intercept mode is active. vpnDNS *vpnDNSManager @@ -424,7 +447,12 @@ func (p *prog) postRun() { p.runningOnDomainController = isDC mainLog.Load().Debug().Msgf("running on domain controller: %t, role: %d", p.runningOnDomainController, roleInt) } - p.resetDNS(false, false) + // A Windows organization can install a GP-owned NRPT catch-all before + // starting ctrld. Detect that policy before resetDNS touches adapter DNS; + // startDNSIntercept will then prove the rule functionally before adopting it. + if !p.skipInitialDNSReset() { + p.resetDNS(false, false) + } ns := ctrld.InitializeOsResolver(false) mainLog.Load().Debug().Msgf("initialized OS resolver with nameservers: %v", ns) p.setDNS() @@ -907,7 +935,7 @@ func (p *prog) setDNS() { mainLog.Load().Fatal().Msgf("invalid --intercept-mode value %q: must be 'off', 'dns', or 'hard'", interceptMode) } if interceptMode == "" || interceptMode == "off" { - interceptMode = cfg.Service.InterceptMode + interceptMode = p.configuredInterceptMode() if interceptMode != "" && interceptMode != "off" { mainLog.Load().Info().Msgf("Intercept mode enabled via config (intercept_mode = %q)", interceptMode) } @@ -927,6 +955,30 @@ func (p *prog) setDNS() { // software that also manages DNS. See issue #489. if dnsIntercept { if err := startDNSInterceptFn(p); err != nil { + // This check comes first: it is the one failure where DNS already works + // without ctrld touching anything else, so neither the refusal below nor the + // fallback applies. + // + // An externally managed rule was proved - by probe, not by registry shape - + // to be routing DNS to this listener. Falling through would rewrite adapter + // DNS after explicitly preserving it, and DNS still works, so stop here. + // + // Only a verified route earns this. A rule that merely exists does not: if it + // is not actually routing and intercept failed too, the machine would be left + // with no NRPT, no WFP and no adapter fallback - that is, unfiltered - so + // every other failure takes the paths below. + if interceptFailedUnderExternalDNSPolicy(err) { + if interceptFailedWithVerifiedExternalDNS(err) { + mainLog.Load().Error().Err(err).Msg("DNS intercept mode failed but externally managed DNS policy is verified routing to ctrld — not falling back to interface DNS settings") + } else { + // Owned by external policy but not proved to route: DNS is not + // reaching ctrld. Adapter DNS still stays as the organization set it, + // and setDnsOK stays false, so this start reports as failed until a + // probe succeeds. + mainLog.Load().Error().Err(err).Msg("DNS intercept mode failed and externally managed DNS policy is not routing to ctrld — leaving interface DNS settings untouched; the service is not ready") + } + return + } // 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 @@ -1037,6 +1089,17 @@ func (p *prog) setDNS() { } } +// configuredInterceptMode resolves the service's effective intercept mode without +// mutating package state. Platform startup preflights use the same precedence as +// setDNS so they do not make adapter-DNS decisions from a different mode value. +func (p *prog) configuredInterceptMode() string { + im := interceptMode + if im == "" || im == "off" { + im = p.cfg.Service.InterceptMode + } + return im +} + func (p *prog) setDnsForRunningIface(nameservers []string) (runningIface *net.Interface) { if p.runningIface == "" { return diff --git a/docs/dns-intercept-mode.md b/docs/dns-intercept-mode.md index c089ea1..0b92dea 100644 --- a/docs/dns-intercept-mode.md +++ b/docs/dns-intercept-mode.md @@ -67,28 +67,27 @@ Separating them into modes means most users get `dns` mode (safe, can never brea #### Startup Sequence (dns mode) -1. Creates NRPT catch-all registry rule (`.` → `127.0.0.1`) under `HKLM\...\DnsPolicyConfig\CtrldCatchAll` -2. Triggers Group Policy refresh via `RefreshPolicyEx` (userenv.dll) so DNS Client loads NRPT immediately -3. Flushes DNS cache to clear stale entries -4. **Activates loopback WFP protect** — adds 4 permit filters (IPv4/IPv6 × UDP/TCP) for DNS to localhost with `FWPM_FILTER_FLAG_CLEAR_ACTION_RIGHT`. These prevent third-party WFP block filters from blocking the NRPT → `127.0.0.1` path (see [Loopback WFP Protect](#loopback-wfp-protect) below). Non-fatal if this fails. -5. Starts NRPT health monitor (30s periodic check) -6. Launches async NRPT probe-and-heal to verify NRPT is actually routing queries +1. Checks for a non-ctrld GP child whose only namespace is `.` and whose only nameserver is ctrld's actual listener IP. +2. When that candidate exists, preserves adapter DNS, sends a DNS Client probe before any NRPT mutation, and re-reads the same GP child. A matching before/after rule plus a received probe enters **GP-managed mode**; ctrld does not write NRPT, call `RefreshPolicyEx`/`paramchange`, or flush DNS for policy activation. +3. Without a still-matching GP candidate, creates the normal ctrld-owned catch-all, signals DNS Client, and flushes stale cache entries. +4. **Activates loopback WFP protect** — adds 4 permit filters (IPv4/IPv6 × UDP/TCP) for DNS to localhost with `FWPM_FILTER_FLAG_CLEAR_ACTION_RIGHT`. These prevent third-party WFP block filters from blocking the NRPT → listener path (see [Loopback WFP Protect](#loopback-wfp-protect) below). Non-fatal if this fails. +5. Starts the 30-second ownership-aware NRPT health monitor. +6. Re-verifies an initially ineffective GP candidate synchronously after WFP setup; ctrld-owned NRPT uses the asynchronous probe-and-heal sequence. #### Startup Sequence (hard mode) -1. Creates NRPT catch-all rule + GP refresh + DNS flush (same as dns mode) -2. Opens WFP engine with `RPC_C_AUTHN_DEFAULT` (0xFFFFFFFF) -3. Cleans up any stale sublayer from a previous unclean shutdown -4. Creates sublayer with maximum weight (0xFFFF) -5. Adds **permit** filters (weight 10) for DNS to localhost (`127.0.0.1`/`::1` port 53) -6. Adds **permit** filters (weight 10) for DNS to RFC1918 + CGNAT subnets (10/8, 172.16/12, 192.168/16, 100.64/10) -7. Adds **block** filters (weight 1) for all other outbound DNS (port 53 UDP+TCP) -8. Starts NRPT health monitor (also verifies WFP sublayer in hard mode) -9. Launches async NRPT probe-and-heal +1. Establishes NRPT routing using the same GP-managed adoption or ctrld-owned fallback sequence as `dns` mode. +2. Opens WFP engine with `RPC_C_AUTHN_DEFAULT` (0xFFFFFFFF). +3. Cleans up any stale sublayer from a previous unclean shutdown. +4. Creates sublayer with maximum weight (0xFFFF). +5. Adds **permit** filters (weight 10) for DNS to localhost (`127.0.0.1`/`::1` port 53). +6. Adds **permit** filters (weight 10) for DNS to RFC1918 + CGNAT subnets (10/8, 172.16/12, 192.168/16, 100.64/10). +7. Adds **block** filters (weight 1) for all other outbound DNS (port 53 UDP+TCP). +8. Starts the NRPT/WFP health monitor. -**Atomic guarantee:** NRPT must succeed before WFP starts. If NRPT fails, WFP is not attempted. If WFP fails, NRPT is rolled back. This prevents DNS blackholes where WFP blocks everything but nothing routes to ctrld. +**Atomic guarantee:** NRPT routing must exist before WFP starts. If WFP setup fails, ctrld rolls back only a rule it owns. A GP-managed child is never deleted, rewritten, or replaced with interface DNS merely because ctrld's WFP setup failed. -On shutdown: stops health monitor, removes NRPT rule, flushes DNS, then (hard mode only) removes all WFP filters and closes engine. +On shutdown, ctrld stops its monitor and WFP session. It removes and signals only ctrld-owned NRPT state; a GP-managed catch-all remains untouched. #### NRPT Details @@ -103,7 +102,27 @@ The **Name Resolution Policy Table** is a Windows feature (originally for Direct **Registry path**: `HKLM\SOFTWARE\Policies\Microsoft\Windows NT\DNSClient\DnsPolicyConfig\CtrldCatchAll` -**Group Policy refresh**: The DNS Client service only reads NRPT from registry during Group Policy processing cycles (default: every 90 minutes). ctrld calls `RefreshPolicyEx(bMachine=TRUE, dwOptions=RP_FORCE)` from `userenv.dll` to trigger an immediate refresh. Falls back to `gpupdate /target:computer /force` if the DLL call fails. +**Group Policy refresh**: The DNS Client service only reads NRPT from registry during Group Policy processing cycles (default: every 90 minutes). ctrld calls `RefreshPolicyEx(bMachine=TRUE, dwOptions=RP_FORCE)` when activating or repairing rules it owns. While Group Policy remains the owner, ctrld does not run NRPT activation/heal signaling; the one transition that removes a ctrld fallback is signaled after the external rule has been proven. + +#### GP-managed NRPT ownership + +Enterprise deployments may install a computer-scoped GP child before starting ctrld with: + +- exactly one namespace: `.`; +- exactly one `GenericDNSServers` value; and +- that nameserver equal to ctrld's actual loopback listener (`127.0.0.1` or the alternate loopback selected on an AD DNS server). + +At service startup ctrld reads that candidate before the normal adapter reset, probes through Windows DNS Client while its listener is already bound, and re-reads the same child. When the rule remains present and the probe arrives, ctrld records **Group Policy** as the NRPT owner. Adapter DNS stays on the organization's resolvers, and ctrld does not create, delete, refresh, or flush NRPT policy. + +The health monitor keeps using functional probes: + +- matching GP rule + successful probe: observe only; +- matching GP rule + failed probe: retry loopback WFP protection, then report the external policy as ineffective without running NRPT heal signals; +- matching GP rule disappears: create the normal ctrld-owned fallback and verify it, unless another GP catch-all targets a different resolver; +- GP catch-all targets another resolver: report the conflict and do not create a second ambiguous catch-all; +- matching GP rule returns: prove it with a probe, remove only ctrld's deterministic fallback keys, and return ownership to Group Policy. + +Deploy the GPO **before** starting or restarting ctrld if adapter DNS must remain completely untouched. Remove or unlink the GP rule before intentionally removing the ctrld service. A GP catch-all that remains pointed at loopback while no listener is running causes DNS failure by design; ctrld cannot safely delete an administrator-owned policy during uninstall. #### WFP Filter Architecture @@ -145,17 +164,19 @@ See: [Issue #526](https://gitlab.int.windscribe.com/controld/clients/ctrld/-/iss ctrld verifies NRPT is actually working by sending a probe DNS query (`_nrpt-probe-.nrpt-probe.ctrld.test`) through Go's `net.Resolver` (which calls `GetAddrInfoW` → DNS Client → NRPT path). If ctrld receives the probe on its listener, NRPT is active. -**Startup probe (async, non-blocking):** After NRPT setup, an async goroutine probes with escalating remediation: (1) immediate probe, (2) GP refresh + retry, (3) DNS Client service restart + retry, (4) final retry. Only one probe sequence runs at a time. +**Startup probes:** A matching GP candidate is probed synchronously before any NRPT mutation and re-read afterward. ctrld-owned rules keep the asynchronous activation/heal sequence: immediate probe, bounded policy signaling retries, then two-phase delete/re-add recovery. Only one probe sequence runs at a time. -**DNS Client restart (nuclear option):** If GP refresh alone isn't enough, ctrld restarts the `Dnscache` service to force full NRPT re-initialization. This briefly interrupts all DNS (~100ms) but only fires when NRPT is already not working. +**Ownership boundary:** When the active owner is Group Policy, a failed probe never enters ctrld's NRPT refresh/delete/re-add sequence. ctrld may repair its narrowly scoped loopback WFP permits, but leaves the external registry child and DNS Client policy signaling to the administrator. #### NRPT Health Monitor A dedicated background goroutine (`nrptHealthMonitor`) runs every 30 seconds and now performs active probing: -1. **Registry check:** If the NRPT catch-all rule is missing from the registry, restore it + GP refresh + probe-and-heal -2. **Active probe:** If the rule exists, send a probe query to verify it's actually routing — catches cases where the registry key is present but DNS Client hasn't loaded it -3. **(hard mode)** Verify WFP sublayer exists; full restart on loss +1. **Ownership check:** Distinguish a matching external GP child from ctrld's deterministic local/GP keys. +2. **Active probe:** Verify Windows DNS Client still routes to the listener. +3. **Transition:** If the external child disappears, activate ctrld's normal fallback. If it returns while the fallback is active, prove it before removing only ctrld's keys. +4. **Owned recovery:** Restore/heal only when ctrld owns the NRPT rule. +5. **(hard mode)** Verify the WFP sublayer exists and fully restart intercept state on loss. This is periodic (not just network-event-driven) because VPN software can clear NRPT at any time. Additionally, `scheduleDelayedRechecks()` (called on network change events) performs immediate NRPT verification at 2s and 4s after changes. diff --git a/docs/wfp-dns-intercept.md b/docs/wfp-dns-intercept.md index 527d20d..60b5428 100644 --- a/docs/wfp-dns-intercept.md +++ b/docs/wfp-dns-intercept.md @@ -7,9 +7,7 @@ On Windows, DNS intercept mode uses a two-layer architecture: - **`dns` mode (default)**: NRPT only — graceful DNS routing via the Windows DNS Client service - **`hard` mode**: NRPT + WFP — full enforcement with kernel-level block filters -This dual-mode design ensures that `dns` mode can never break DNS (at worst, a VPN -overwrites NRPT and queries bypass ctrld temporarily), while `hard` mode provides -the same enforcement guarantees as macOS pf. +`dns` mode avoids ctrld's outbound block filters and therefore degrades more gracefully when owned NRPT is removed. Resolution still depends on the active NRPT target being reachable; an administrator-owned GP catch-all intentionally remains fail-closed if its loopback listener is stopped. ## Architecture: dns vs hard Mode @@ -24,9 +22,8 @@ the same enforcement guarantees as macOS pf. │ localhost, CLEAR_ACTION_RIGHT) prevent third-party VPN WFP │ │ blocks (e.g., OpenVPN block-outside-dns) from breaking NRPT. │ │ │ -│ If VPN clears NRPT: health monitor re-adds within 30s │ -│ Worst case: queries go to VPN DNS until NRPT restored │ -│ DNS never breaks — graceful degradation │ +│ Owned rule missing → restore; GP missing → owned fallback │ +│ GP rule + dead listener remains intentionally fail-closed │ └─────────────────────────────────────────────────────────────────┘ ┌─────────────────────────────────────────────────────────────────┐ @@ -37,9 +34,8 @@ the same enforcement guarantees as macOS pf. │ Bypass attempt (raw 8.8.8.8:53) → WFP BLOCK filter │ │ VPN DNS on private IP → WFP subnet PERMIT filter → allowed │ │ │ -│ NRPT must be active before WFP starts (atomic guarantee) │ -│ If NRPT fails → WFP not started (avoids DNS blackhole) │ -│ If WFP fails → NRPT rolled back (all-or-nothing) │ +│ NRPT route is established before WFP starts │ +│ WFP failure rolls back only ctrld-owned NRPT; GP is untouched │ └─────────────────────────────────────────────────────────────────┘ ``` @@ -79,7 +75,7 @@ unreliable. If we write to the GP path, DNS Client enters GP mode but the rules never activate — resulting in `Get-DnsClientNrptPolicy` returning empty even though `Get-DnsClientNrptRule` shows the rule in registry. -ctrld uses an adaptive strategy (matching [Tailscale's approach](https://github.com/tailscale/tailscale/blob/main/net/dns/nrpt_windows.go)): +ctrld uses an adaptive strategy (matching [Tailscale's approach](https://github.com/tailscale/tailscale/blob/main/net/dns/nrpt_windows.go)) when it owns NRPT: 1. **Always write to the local path** using a deterministic GUID key name (`{B2E9A3C1-7F4D-4A8E-9D6B-5C1E0F3A2B8D}`). This is the baseline that works @@ -91,6 +87,23 @@ ctrld uses an adaptive strategy (matching [Tailscale's approach](https://github. the empty GP parent key. This ensures DNS Client stays in "local mode" where the local-path rule activates immediately via `paramchange`. +### Adopting an Organization-Owned GP Catch-All + +Before applying that ctrld-owned strategy, service startup looks for a non-ctrld GP child with exactly `Name=["."]` and one `GenericDNSServers` value equal to the actual listener IP. The registry match is only an ownership candidate: ctrld sends its unique DNS Client probe before any NRPT write and re-reads the same child afterward. + +Intercept state stays unpublished while that happens. Startup publishes nothing until it has fully succeeded, which is why the probe and heal flows take the state as an argument instead of reading the published field — publishing early would expose a half-built `wfpState`, with no engine handle and filter IDs still being assigned, to callers such as the VPN DNS exemption path. The one deliberate exception is hard mode when WFP setup fails while GP-managed NRPT is verified routing: that state is published so the health monitor can keep retrying WFP, and the service start is still reported as failed. + +When the rule remains unchanged and the probe arrives, Group Policy owns NRPT: + +- the startup adapter reset is skipped; +- no ctrld NRPT key is created; +- `RefreshPolicyEx`, Dnscache `paramchange`, and cache flush are not used for that policy; +- shutdown/uninstall leave the GP child untouched. + +A matching GP child that remains present but fails its probe is reported as ineffective and is not rewritten. If the child disappears, ctrld activates its normal owned fallback. A GP catch-all that instead changes to another resolver is reported as a conflict; ctrld does not create a second ambiguous catch-all. If the matching rule later returns, ctrld probes first, removes only its deterministic fallback keys, signals the ownership transition once, and resumes observing Group Policy. + +**Deployment ordering:** apply the GPO before starting ctrld to guarantee adapter DNS is never reset. Remove/unlink it before intentionally stopping or uninstalling ctrld. A GP catch-all still targeting loopback with no listener running is a deliberate fail-closed state and will break DNS. + ### Reproducing the Empty GP Parent Case This is a production code reference, so the temporary repro script is not kept in @@ -209,28 +222,28 @@ by the VPN's own WFP rules. **Startup (hard mode):** ``` -1. Add NRPT catch-all rule + GP refresh + DNS flush +1. Adopt a proven matching GP catch-all, or install ctrld-owned NRPT 2. FwpmEngineOpen0() with RPC_C_AUTHN_DEFAULT (0xFFFFFFFF) 3. Delete stale sublayer (crash recovery) 4. FwpmSubLayerAdd0() — weight 0xFFFF 5. Add 4 localhost permit filters 6. Add 4 block filters 7. Add RFC1918 + CGNAT subnet permits -8. Start NRPT health monitor goroutine +8. Start ownership-aware NRPT/WFP health monitor ``` **Startup (dns mode):** ``` -1. Add NRPT catch-all rule + GP refresh + DNS flush +1. Adopt a proven matching GP catch-all, or install ctrld-owned NRPT 2. Activate loopback WFP protect (4 hard-permit filters for localhost DNS) -3. Start NRPT health monitor goroutine +3. Start ownership-aware NRPT health monitor ``` **Shutdown:** ``` 1. Stop NRPT health monitor -2. Remove NRPT catch-all rule + DNS flush -3. (hard mode only) Clean up all WFP filters, sublayer, close engine +2. Remove + signal only ctrld-owned NRPT; leave GP-managed policy untouched +3. Clean up ctrld WFP filters, sublayer, and engine session ``` **Crash Recovery:** @@ -259,42 +272,24 @@ Windows DNS Client path to verify NRPT is actually working: 4. ctrld's DNS handler recognizes the probe prefix and signals success 5. If the probe times out (2s), NRPT isn't loaded yet → retry with remediation -### Startup Probe (Async) +### Startup Probes -After NRPT setup, an async goroutine runs the probe-and-heal sequence without -blocking startup: +For a matching GP candidate, startup blocks for one 2-second probe before any NRPT mutation and then re-reads the same child. A received query plus the unchanged rule proves both routing and external ownership. If that first probe fails, ctrld installs its normal WFP protection and performs one more ownership-safe probe before advertising startup readiness. + +ctrld-owned NRPT retains the asynchronous sequence: ``` -Probe attempt 1 (2s timeout) +Immediate probe ├─ Success → "NRPT verified working", done - └─ Timeout → GP refresh + DNS flush, sleep 1s - Probe attempt 2 (2s timeout) - ├─ Success → done - └─ Timeout → Restart DNS Client service (nuclear), sleep 2s - Re-add NRPT + GP refresh + DNS flush - Probe attempt 3 (2s timeout) - ├─ Success → done - └─ Timeout → GP refresh + DNS flush, sleep 4s - Probe attempt 4 (2s timeout) - ├─ Success → done - └─ Timeout → log error, continue + └─ Timeout + ├─ Empty GP parent → clean once, signal once, re-probe + └─ Otherwise → bounded 1s/2s/4s signal + probe retries + └─ Still failing → two-phase remove/signal/re-add/final probe ``` -### DNS Client Restart (Nuclear Option) +### GP-Managed Probe Failure -If GP refresh alone isn't enough, ctrld restarts the Windows DNS Client service -(`Dnscache`). This forces the DNS Client to fully re-initialize, including -re-reading all NRPT rules from the registry. This is the equivalent of macOS -`forceReloadPFMainRuleset()`. - -**Trade-offs:** -- Briefly interrupts ALL DNS resolution (few hundred ms during restart) -- Clears the system DNS cache (all apps need to re-resolve) -- VPN NRPT rules survive (they're in registry, re-read on restart) -- Enterprise security tools may log the service restart event - -This only fires as attempt #3 after two GP refresh attempts fail — at that point -DNS isn't working through ctrld anyway, so a brief DNS blip is acceptable. +A matching GP child still owns Windows' effective NRPT store even when its probe fails. Writing a local rule cannot override that precedence, and rewriting the GP child would violate administrator ownership. ctrld therefore retries only its loopback WFP permit protection, reports the ineffective external policy, and leaves NRPT registry values and policy signals untouched. ### Health Monitor Integration @@ -302,19 +297,20 @@ The 30s periodic health monitor now does actual probing, not just registry check ``` Every 30s: - ├─ Registry check: nrptCatchAllRuleExists()? - │ ├─ Missing → re-add + GP refresh + flush + probe-and-heal - │ └─ Present → probe to verify it's actually routing - │ ├─ Probe success → OK - │ └─ Probe failure → probe-and-heal cycle + ├─ GP-managed owner + │ ├─ Matching child + probe success → observe only + │ ├─ Matching child + probe failure → WFP-only retry; no NRPT mutation + │ └─ Matching child gone → activate ctrld-owned fallback + verify │ - └─ (hard mode only) Check: wfpSublayerExists()? - ├─ Missing → full restart (stopDNSIntercept + startDNSIntercept) - └─ Present → OK + ├─ ctrld-owned owner + │ ├─ Working matching GP child returns → remove only ctrld keys; adopt GP + │ ├─ ctrld key missing → restore + signal + verify + │ └─ ctrld key present → probe; run owned heal sequence on failure + │ + └─ (hard mode) Check WFP sublayer; full intercept restart if missing ``` -**Singleton guard:** Only one probe-and-heal sequence runs at a time (atomic bool). -The startup probe and health monitor cannot overlap. +**Singleton guard:** Only one asynchronous probe-and-heal sequence runs at a time (atomic bool). Startup's GP-candidate probe completes before the health monitor starts; direct periodic probes finish before they schedule a heal sequence. **Why periodic, not just network-event?** VPN software or Group Policy updates can clear NRPT at any time, not just during network changes. A 30s periodic check ensures