diff --git a/cmd/cli/api_reload_test.go b/cmd/cli/api_reload_test.go new file mode 100644 index 0000000..a674ff2 --- /dev/null +++ b/cmd/cli/api_reload_test.go @@ -0,0 +1,240 @@ +package cli + +import ( + "context" + "net/netip" + "sync" + "testing" + "time" + + "github.com/Control-D-Inc/ctrld" + "github.com/Control-D-Inc/ctrld/internal/controld" +) + +// progForRefresh builds a prog that can run a configuration refresh: Firewall +// Mode on, and a buffered reload channel so a refresh that decides ctrld must +// reload does not block on a listener that does not exist in a test. +func progForRefresh() *prog { + p := progWithAllowList() + p.rc = &controld.ResolverConfig{} + p.apiReloadCh = make(chan *ctrld.Config, 1) + return p +} + +// refresh runs one configuration refresh through the handler apiConfigReload +// uses, which is the point: a test that called applyAllowedDestinations directly +// would still pass if the refresh path stopped calling it. +func refresh(t *testing.T, p *prog, forced bool, rc *controld.ResolverConfig) { + t.Helper() + p.applyFetchedResolverConfig(context.Background(), discardLogger(), rc, forced, time.Now().Unix()) +} + +// TestRefreshAppliesAllowedDestinations drives the real refresh handler for both +// the scheduled and the forced path, in the case where nothing else about the +// configuration changed - no custom config, unchanged exclusions - so the +// refresh takes its early return. That is where an allowed-destination update is +// easiest to lose, because the refresh has no other work to do. +func TestRefreshAppliesAllowedDestinations(t *testing.T) { + direct := netip.MustParseAddr("203.0.113.10") + inRange := netip.MustParseAddr("198.51.100.7") + + for _, forced := range []bool{false, true} { + name := "scheduled refresh" + if forced { + name = "forced refresh" + } + t.Run(name, func(t *testing.T) { + p := progForRefresh() + + refresh(t, p, forced, &controld.ResolverConfig{ + DestinationIPs: []string{"203.0.113.10", "198.51.100.0/24"}, + }) + if !p.allowList.Contains(direct) || !p.allowList.Contains(inRange) { + t.Fatalf("refresh did not apply the organization list: %s=%v %s=%v", + direct, p.allowList.Contains(direct), inRange, p.allowList.Contains(inRange)) + } + select { + case cfg := <-p.apiReloadCh: + t.Fatalf("unchanged configuration signaled a reload (%v)", cfg) + default: + } + + // A later refresh withdraws one entry and keeps the other. + refresh(t, p, forced, &controld.ResolverConfig{ + DestinationIPs: []string{"203.0.113.10"}, + }) + if p.allowList.Contains(inRange) { + t.Fatalf("%s still allowed after the refresh that withdrew it", inRange) + } + if !p.allowList.Contains(direct) { + t.Fatalf("%s should still be allowed", direct) + } + + // And one clears the list entirely. + refresh(t, p, forced, &controld.ResolverConfig{}) + if p.allowList.Contains(direct) { + t.Fatalf("%s still allowed after the refresh that cleared the list", direct) + } + }) + } +} + +// TestRefreshAppliesAllowedDestinationsWhenReloading covers the other branch of +// the same handler: a refresh that also changes the exclusion list, and so +// signals a ctrld reload, must still apply the destinations - and they must +// survive the allowlist flush that the reload performs. +func TestRefreshAppliesAllowedDestinationsWhenReloading(t *testing.T) { + p := progForRefresh() + p.rc = &controld.ResolverConfig{Exclude: []string{"example.com"}} + direct := netip.MustParseAddr("203.0.113.10") + + refresh(t, p, false, &controld.ResolverConfig{ + Exclude: []string{"example.com", "example.net"}, + DestinationIPs: []string{"203.0.113.10"}, + }) + + select { + case <-p.apiReloadCh: + default: + t.Fatal("exclusion list change did not signal a reload") + } + if !p.allowList.Contains(direct) { + t.Fatalf("%s not allowed after a refresh that reloaded ctrld", direct) + } +} + +// TestRefreshKeepsAllowedDestinationsPendingUntilMirrored pins that the refresh +// path reports honestly: while platform enforcement is rejecting the change, the +// refresh leaves it pending and retries it, instead of recording it as applied. +func TestRefreshKeepsAllowedDestinationsPendingUntilMirrored(t *testing.T) { + p := progForRefresh() + var calls []mirrorCall + failing := true + stubMirror(t, &calls, &failing) + + rc := &controld.ResolverConfig{DestinationIPs: []string{"203.0.113.10"}} + refresh(t, p, false, rc) + if got := p.pendingDestinations(p.allowList); got != 1 { + t.Fatalf("pendingDestinations = %d after a rejected mirror, want 1", got) + } + + refresh(t, p, false, rc) + if len(calls) != 2 { + t.Fatalf("refresh did not retry the rejected change: calls = %v", calls) + } + + failing = false + refresh(t, p, false, rc) + if got := p.pendingDestinations(p.allowList); got != 0 { + t.Fatalf("pendingDestinations = %d after the mirror succeeded, want 0", got) + } +} + +// stubResolverConfigFetch replaces the refresh loop's API call for the duration +// of a test, and points cdUID at a device so apiConfigReload does not return +// immediately. Each fetch returns the config the test currently wants and +// reports on fetched, which is how a test knows a refresh cycle has run. +func stubResolverConfigFetch(t *testing.T, config func() *controld.ResolverConfig, fetched chan<- struct{}) { + t.Helper() + origFetch, origUID := fetchResolverConfigFn, cdUID + t.Cleanup(func() { fetchResolverConfigFn, cdUID = origFetch, origUID }) + + cdUID = "test-uid" + fetchResolverConfigFn = func(context.Context, *controld.ResolverConfigRequest, bool) (*controld.ResolverConfig, error) { + rc := config() + select { + case fetched <- struct{}{}: + default: + } + return rc, nil + } +} + +// startRefreshLoop runs apiConfigReload in the background and joins it before the +// test finishes. The loop must not outlive the test: it calls through the same +// package-level stubs the next test replaces, so a leaked one would both race +// those globals and act on another test's prog. +func startRefreshLoop(t *testing.T, p *prog) { + t.Helper() + done := make(chan struct{}) + go func() { + defer close(done) + p.apiConfigReload() + }() + t.Cleanup(func() { + close(p.stopCh) + select { + case <-done: + case <-time.After(5 * time.Second): + t.Error("apiConfigReload did not stop") + } + }) +} + +// waitForCondition polls until cond holds, failing the test if it never does. The +// refresh loop runs in its own goroutine, so its effects land asynchronously. +func waitForCondition(t *testing.T, what string, cond func() bool) { + t.Helper() + deadline := time.Now().Add(5 * time.Second) + for time.Now().Before(deadline) { + if cond() { + return + } + time.Sleep(5 * time.Millisecond) + } + t.Fatalf("timed out waiting for %s", what) +} + +// TestApiConfigReloadAppliesAllowedDestinations drives apiConfigReload itself - +// the loop that owns the refresh ticker and the forced-reload channel - rather +// than the handler it calls, so the wiring between them is covered too: removing +// the handler call from the loop must fail a test, not just removing the work +// inside the handler. +func TestApiConfigReloadAppliesAllowedDestinations(t *testing.T) { + direct := netip.MustParseAddr("203.0.113.10") + + for _, forced := range []bool{true, false} { + name := "forced reload" + if !forced { + name = "refresh ticker" + } + t.Run(name, func(t *testing.T) { + p := progForRefresh() + p.cfg = &ctrld.Config{} + refetch := 1 // seconds; only the ticker path waits for it + p.cfg.Service.RefetchTime = &refetch + p.stopCh = make(chan struct{}) + p.apiForceReloadCh = make(chan struct{}) + + var mu sync.Mutex + destinations := []string{"203.0.113.10"} + fetched := make(chan struct{}, 1) + stubResolverConfigFetch(t, func() *controld.ResolverConfig { + mu.Lock() + defer mu.Unlock() + return &controld.ResolverConfig{DestinationIPs: append([]string(nil), destinations...)} + }, fetched) + + startRefreshLoop(t, p) + + if forced { + p.apiForceReloadCh <- struct{}{} + } + waitForCondition(t, "the destination to be applied", func() bool { + return p.allowList.Contains(direct) + }) + + // The organization withdraws it; the next cycle must take it away. + mu.Lock() + destinations = nil + mu.Unlock() + + if forced { + p.apiForceReloadCh <- struct{}{} + } + waitForCondition(t, "the withdrawn destination to stop being allowed", func() bool { + return !p.allowList.Contains(direct) + }) + }) + } +} diff --git a/cmd/cli/control_server.go b/cmd/cli/control_server.go index b716686..0e8195a 100644 --- a/cmd/cli/control_server.go +++ b/cmd/cli/control_server.go @@ -247,6 +247,16 @@ func (p *prog) registerControlServerHandler() { } else { cdDeactivationPin.Store(defaultDeactivationPin) } + // Every resolver-config response carries the organization's allowed + // destinations, including this one, so apply them rather than discarding + // a fresher list until the next scheduled refresh converges. + // + // Only the destinations: p.rc is deliberately left alone. The scheduled + // refresh decides whether to reload ctrld by comparing the response + // against p.rc, so storing this one here would let an exclude-list change + // be compared away and never reloaded. The destination set needs no + // reload - it is enforced directly and applying it is idempotent. + p.applyAllowedDestinations(p.firewallAllowList(), rc.DestinationIPs) } else { p.Warn().Err(err).Msg("Could not re-fetch deactivation pin code") } diff --git a/cmd/cli/firewall.go b/cmd/cli/firewall.go index 4b4cfea..8dcb0e9 100644 --- a/cmd/cli/firewall.go +++ b/cmd/cli/firewall.go @@ -16,7 +16,30 @@ import ( // firewallModeEnabled reports whether firewall mode is active for this prog instance. func (p *prog) firewallModeEnabled() bool { - return p.allowList != nil + return p.firewallAllowList() != nil +} + +// firewallAllowList returns the allowlist this run enforces, or nil when Firewall +// Mode is off. +// +// syncFirewallMode replaces the field from the reload goroutine while refreshes +// read it, so a read that is followed by a dereference has to work from a +// snapshot rather than from the field: otherwise a reload landing in between +// turns the pointer to nil under the caller. Acting on a superseded allowlist is +// harmless - reconcileDestinations re-checks the firewall generation under +// destinationsMu and does nothing for a generation that has ended. +func (p *prog) firewallAllowList() *firewall.AllowList { + p.mu.Lock() + defer p.mu.Unlock() + return p.allowList +} + +// setFirewallAllowList publishes this run's allowlist. Only syncFirewallMode +// calls it; every other goroutine reads through firewallAllowList. +func (p *prog) setFirewallAllowList(al *firewall.AllowList) { + p.mu.Lock() + defer p.mu.Unlock() + p.allowList = al } // initFirewallAllowList populates the permanent allowlist entries and starts the @@ -30,9 +53,7 @@ func (p *prog) firewallModeEnabled() bool { // - ctrld listener IPs // - DoH/DoT/DoQ upstream resolver IPs // - ControlD API endpoint IPs -func (p *prog) initFirewallAllowList(ctx context.Context) { - al := p.allowList - +func (p *prog) initFirewallAllowList(ctx context.Context, al *firewall.AllowList) { // Loopback. al.AddPermanentPrefix(netip.MustParsePrefix("127.0.0.0/8")) al.AddPermanent(netip.MustParseAddr("::1")) @@ -74,15 +95,23 @@ func (p *prog) initFirewallAllowList(ctx context.Context) { // Reloads create a new run-scoped context, so background firewall workers must // be restarted each run even when the allowlist object is reused. func (p *prog) syncFirewallMode(ctx context.Context) { + // This is the only writer of p.allowList, so its own reads need no lock; every + // other goroutine reads the published pointer through firewallAllowList. + al := p.allowList + if p.cfg.Service.FirewallMode != "on" { - if p.allowList != nil || p.platformFirewallState != nil { + if al != nil || p.platformFirewallState != nil { p.Info().Msg("Firewall mode disabled: removing platform enforcement and clearing allowlist") } - if p.allowList != nil { - p.allowList.SetOnChange(nil) - p.allowList.SetOnBatchChange(nil) - p.allowList = nil + if al != nil { + al.SetOnChange(nil) + al.SetOnBatchChange(nil) + p.setFirewallAllowList(nil) } + // Platform enforcement is torn down below, so nothing may be mirrored any + // more: end this generation, which retires the maintenance worker, and + // forget what was applied so a later re-enable reinstalls it. + p.retireFirewallDestinations() if p.platformFirewallState != nil { p.shutdownPlatformFirewall() p.platformFirewallState = nil @@ -90,22 +119,33 @@ func (p *prog) syncFirewallMode(ctx context.Context) { return } - if p.allowList == nil { - p.allowList = firewall.New() - p.initFirewallAllowList(ctx) + if al == nil { + al = firewall.New() + p.initFirewallAllowList(ctx, al) + p.setFirewallAllowList(al) if service.Interactive() { p.Warn().Msg("Firewall mode has no effect in interactive mode; run ctrld as a service for enforcement") } else { p.Info().Msg("Firewall mode enabled: only DNS-resolved IPs will be allowed") } } else { - p.addUpstreamIPsToPermanent(p.allowList) + p.addUpstreamIPsToPermanent(al) } - // The run context is canceled on each reload. Restart the reaper/stats + // Open this run's firewall generation before any work is scheduled against it, + // so the previous run's maintenance worker stops acting on enforcement this + // run is now responsible for. + gen := p.startFirewallGeneration() + + // Apply the organization's allowed destinations from the resolver config this + // run started with. A reload that turns Firewall Mode on builds a fresh + // allowlist, so the set has to be re-applied rather than assumed present. + p.syncAllowedDestinations() + + // The run context is canceled on each reload. Restart the reaper/maintenance // workers for this run so reused allowlists keep expiring entries. - p.allowList.StartReaper(ctx) - go p.logFirewallStats(ctx) + al.StartReaper(ctx) + go p.firewallMaintenance(ctx, al, gen) // On reload, postRun() is not called, so initialize platform enforcement here // if intercept state already exists. Initial startup still defers to postRun() @@ -150,6 +190,371 @@ func (p *prog) addUpstreamIPsToPermanent(al *firewall.AllowList) { } } +// syncAllowedDestinations applies the organization's Allowed Destination IP list +// from the resolver config currently held by prog. +// +// Called whenever that config could have changed: on every start and reload (via +// syncFirewallMode) and after every API refresh, forced or scheduled (via +// apiConfigReload). Reading the list from p.rc rather than taking it as an +// argument keeps those callers from having to know whether Firewall Mode is on. +func (p *prog) syncAllowedDestinations() { + p.mu.Lock() + rc := p.rc + al := p.allowList + p.mu.Unlock() + + var entries []string + if rc != nil { + entries = rc.DestinationIPs + } + p.applyAllowedDestinations(al, entries) +} + +// applyAllowedDestinations records entries as the desired Firewall Mode exception +// set and mirrors it into platform enforcement. Entries the API sent that are not +// a valid address or CIDR are dropped individually, so one bad entry never voids +// the rest of an organization's list. +// +// A no-op when Firewall Mode is off: with no allowlist there is nothing to +// except from, and the set is re-applied from p.rc if the mode is turned on. +// +// The allowlist is an argument rather than a field read because a reload can +// replace p.allowList - including with nil - between the nil check and the +// SetExceptions call below, which would dereference nil. Callers snapshot it +// once under mu (see firewallAllowList) and pass what they snapshotted. +func (p *prog) applyAllowedDestinations(al *firewall.AllowList, entries []string) { + if al == nil { + return + } + + prefixes, rejected, wide := parseAllowedDestinations(entries) + p.warnRejectedAllowedDestinations(rejected) + p.warnWideAllowedDestinations(wide) + + al.SetExceptions(prefixes) + p.reconcileDestinations(al, p.firewallGen.Load()) +} + +// reconcileAllowedDestinations brings platform enforcement in line with the +// desired allowed-destination set, installing what is missing and removing what +// the organization has withdrawn. +// +// The applied snapshot advances ONLY after the platform accepted the change. A +// failed pfctl call or WFP filter operation therefore leaves the previous +// snapshot recorded, so the same delta is recomputed - and retried - by the next +// refresh and by the periodic reconcile, instead of being silently dropped while +// the logs claim the new set is in force. Retrying the whole delta is safe +// because both mirrors are idempotent: installing an entry that is already there +// and removing one that is already gone are no-ops. +// +// Callers must not hold destinationsMu; the mirror can block on pfctl. +func (p *prog) reconcileAllowedDestinations() { + p.reconcileDestinations(p.firewallAllowList(), p.firewallGen.Load()) +} + +// reconcileDestinations is reconcileAllowedDestinations for one firewall +// generation. Background workers pass the allowlist and generation they were +// started with, and the generation is re-checked under destinationsMu: teardown +// bumps it while holding the same lock, so a worker from a previous run can never +// mirror anything into enforcement that is being (or has been) removed. +func (p *prog) reconcileDestinations(al *firewall.AllowList, gen uint64) { + if al == nil { + return + } + desired := al.Exceptions() + + p.destinationsMu.Lock() + defer p.destinationsMu.Unlock() + + if p.firewallGen.Load() != gen { + return + } + + // A resync owes the platform the whole set, not a delta: it means enforcement + // started with state ctrld does not know (a persist pf table from a previous + // run) or with none at all. Until the replace succeeds nothing about the + // applied set can be assumed, so the flag stays set and it is retried. + if p.destinationsNeedResync { + if err := firewallReplaceExceptionsFn(p, desired); err != nil { + p.Warn().Err(err).Int("total", len(desired)). + Msg("Firewall: could not install organization allowed destinations, will retry") + return + } + p.destinationsNeedResync = false + p.appliedDestinations = desired + p.logDestinationChange(len(desired), 0, desired, nil) + return + } + + added := prefixesNotIn(desired, p.appliedDestinations) + removed := prefixesNotIn(p.appliedDestinations, desired) + if len(added) == 0 && len(removed) == 0 { + return + } + + if err := firewallMirrorExceptionsFn(p, added, removed); err != nil { + p.Warn().Err(err). + Int("pending_add", len(added)). + Int("pending_remove", len(removed)). + Msg("Firewall: could not apply all organization allowed destinations, will retry") + return + } + + p.appliedDestinations = desired + p.logDestinationChange(len(added), len(removed), added, removed) +} + +// logDestinationChange reports an applied change: counts at Info, addresses at +// Debug. The list is an organization's network topology, and Info-level logs are +// persisted and uploaded with support bundles, so the counts are all that goes +// into the routine record. +func (p *prog) logDestinationChange(nAdded, nRemoved int, added, removed []netip.Prefix) { + p.Info(). + Int("added", nAdded). + Int("removed", nRemoved). + Int("total", len(p.appliedDestinations)). + Msg("Firewall: applied organization allowed destination IPs") + p.Debug(). + Strs("added", prefixStrings(added)). + Strs("removed", prefixStrings(removed)). + Msg("Firewall: organization allowed destination changes") +} + +// pendingDestinations reports how many allowed-destination changes platform +// enforcement has not accepted yet. Non-zero means a mirror attempt failed and +// the reconcile is still retrying, which is the difference between "the set is in +// force" and "the set is what we want" - the stats line must not conflate them. +func (p *prog) pendingDestinations(al *firewall.AllowList) int { + if al == nil { + return 0 + } + desired := al.Exceptions() + + p.destinationsMu.Lock() + defer p.destinationsMu.Unlock() + + if p.destinationsNeedResync { + // Nothing about the applied set is known, so everything is outstanding - + // and an empty desired set still owes the platform a flush of whatever it + // is holding, which is one pending operation, not zero. + return max(len(desired), 1) + } + return len(prefixesNotIn(desired, p.appliedDestinations)) + len(prefixesNotIn(p.appliedDestinations, desired)) +} + +// startFirewallGeneration opens a new firewall generation and returns it, +// retiring the workers of the previous one. Called for every run (start or +// reload) that has Firewall Mode on; the applied snapshot is left alone because +// platform enforcement survives a reload. +func (p *prog) startFirewallGeneration() uint64 { + p.destinationsMu.Lock() + defer p.destinationsMu.Unlock() + return p.firewallGen.Add(1) +} + +// markDestinationsForResync records that platform enforcement holds unknown +// state, so the next reconcile replaces its whole allowed-destination set rather +// than applying a delta against a snapshot that no longer describes anything. +// Called when enforcement starts: a fresh WFP session holds nothing, and a pf +// persist table may still hold what a previous run put there. +func (p *prog) markDestinationsForResync() { + p.destinationsMu.Lock() + defer p.destinationsMu.Unlock() + p.appliedDestinations = nil + p.destinationsNeedResync = true +} + +// retireFirewallDestinations ends the current firewall generation and forgets the +// applied set, for teardown: enforcement is about to be removed, so there is +// nothing left to reconcile against and no resync to owe. +// +// Bumping the generation under destinationsMu is what makes teardown safe against +// the maintenance worker: either the worker is mid-reconcile and this blocks +// until it finishes, or it reaches its own reconcile afterwards, sees a +// generation it does not own, and does nothing. +func (p *prog) retireFirewallDestinations() { + p.destinationsMu.Lock() + defer p.destinationsMu.Unlock() + p.firewallGen.Add(1) + p.appliedDestinations = nil + p.destinationsNeedResync = false +} + +// firewallMirrorExceptionsFn mirrors an allowed-destination delta into platform +// enforcement, and firewallReplaceExceptionsFn makes enforcement hold exactly the +// given set regardless of what it held before. Indirected so the +// failure-and-retry paths are testable without pf or WFP. +var ( + firewallMirrorExceptionsFn = (*prog).firewallApplyExceptionsPlatform + firewallReplaceExceptionsFn = (*prog).firewallReplaceExceptionsPlatform +) + +// prefixesNotIn returns the members of a that are absent from b. +func prefixesNotIn(a, b []netip.Prefix) []netip.Prefix { + if len(a) == 0 { + return nil + } + inB := make(map[netip.Prefix]struct{}, len(b)) + for _, prefix := range b { + inB[prefix] = struct{}{} + } + var out []netip.Prefix + for _, prefix := range a { + if _, ok := inB[prefix]; ok { + continue + } + out = append(out, prefix) + } + return out +} + +// parseAllowedDestinations converts the API's Allowed Destination IP entries into +// prefixes, returning the usable ones and the raw entries that were rejected. +// +// The API reports a single host as a bare address ("1.2.3.4", "2606:1a40::1") and +// anything wider in CIDR form, so both spellings are accepted; a bare address +// becomes a single-host prefix. IPv4-in-IPv6 forms are unmapped to match how the +// allowlist stores addresses, otherwise a "::ffff:1.2.3.4" entry would never +// match the IPv4 address it denotes. +// The third result is the accepted prefixes that are wide enough to be worth +// reporting; see wideAllowedDestination. +func parseAllowedDestinations(entries []string) (accepted []netip.Prefix, rejected []string, wide []netip.Prefix) { + if len(entries) == 0 { + return nil, nil, nil + } + accepted = make([]netip.Prefix, 0, len(entries)) + for _, entry := range entries { + entry = strings.TrimSpace(entry) + if entry == "" { + continue + } + if prefix, err := netip.ParsePrefix(entry); err == nil { + prefix = unmapPrefix(prefix) + accepted = append(accepted, prefix) + if wideAllowedDestination(prefix) { + wide = append(wide, prefix) + } + continue + } + if addr, err := netip.ParseAddr(entry); err == nil { + addr = addr.Unmap() + accepted = append(accepted, netip.PrefixFrom(addr, addr.BitLen())) + continue + } + rejected = append(rejected, entry) + } + return accepted, rejected, wide +} + +// An accepted prefix with fewer mask bits than these is reported. The floors are +// set below anything an organization plausibly means: /8 is the widest classical +// IPv4 network and the size of RFC1918's 10.0.0.0/8, and /32 is a whole IPv6 RIR +// allocation. A bare address always parses to a single-host prefix, so only a +// CIDR entry can reach either floor. +const ( + minSaneAllowedDestinationV4Bits = 8 + minSaneAllowedDestinationV6Bits = 32 +) + +// wideAllowedDestination reports whether an accepted prefix covers enough of the +// address space to deserve a line in the log. +// +// netip.ParsePrefix takes "1.2.3.4/0", and normalizeExceptions masks it to +// 0.0.0.0/0; "::/0" and - through unmapPrefix - "::ffff:0:0/96" do the same for +// IPv6. One such entry lets every destination of that family bypass Firewall +// Mode while the mode still reports on, and logDestinationChange records only +// counts at Info, so without this the bypass is invisible outside Debug logs. +// +// This is not a defence against a hostile API, which can already turn the mode +// off through custom_config. It is a defence against a wide prefix arriving by +// accident - a dashboard bug, or an admin who typed the wrong mask - and nobody +// noticing. +func wideAllowedDestination(prefix netip.Prefix) bool { + if prefix.Addr().Is4() { + return prefix.Bits() < minSaneAllowedDestinationV4Bits + } + return prefix.Bits() < minSaneAllowedDestinationV6Bits +} + +// unmapPrefix rewrites an IPv4-in-IPv6 prefix to its IPv4 form, adjusting the +// mask by the 96-bit IPv4-mapped prefix length. Prefixes of other families are +// returned unchanged. +func unmapPrefix(prefix netip.Prefix) netip.Prefix { + addr := prefix.Addr() + if !addr.Is4In6() { + return prefix + } + bits := prefix.Bits() - 96 + if bits < 0 { + // A mask wider than the mapped range does not denote an IPv4 network; + // leave it as the IPv6 prefix it literally is. + return prefix + } + return netip.PrefixFrom(addr.Unmap(), bits) +} + +// warnRejectedAllowedDestinations reports unusable entries, but only when the set +// of rejections changes. The list is re-parsed on every refresh (hourly by +// default), so warning unconditionally would repeat the same lines for the life +// of the process while still saying nothing new. +// The rejected values themselves go to Debug, never to Warn. A rejected entry is +// still an organization's topology - "10.0.0.0/33" names a real network - and +// Warn logs are persisted and travel in support bundles exactly like Info ones, +// so they follow the same rule as logDestinationChange: counts in the routine +// record, addresses only when someone turned Debug on to look. +func (p *prog) warnRejectedAllowedDestinations(rejected []string) { + key := strings.Join(rejected, ",") + + p.mu.Lock() + unchanged := p.rejectedDestinationsKey == key + p.rejectedDestinationsKey = key + p.mu.Unlock() + + if unchanged || len(rejected) == 0 { + return + } + p.Warn().Int("rejected", len(rejected)). + Msg("Firewall: ignoring organization allowed destinations that are not a valid IP address or CIDR") + p.Debug().Strs("values", rejected). + Msg("Firewall: rejected organization allowed destinations") +} + +// warnWideAllowedDestinations reports accepted entries wide enough to blanket an +// address family, with the mask width but not the address, and only when the set +// of them changes - the list is re-parsed on every refresh, so an unconditional +// warning would repeat the same lines for the life of the process. +func (p *prog) warnWideAllowedDestinations(wide []netip.Prefix) { + key := strings.Join(prefixStrings(wide), ",") + + p.mu.Lock() + unchanged := p.wideDestinationsKey == key + p.wideDestinationsKey = key + p.mu.Unlock() + + if unchanged || len(wide) == 0 { + return + } + for _, prefix := range wide { + family := "ipv6" + if prefix.Addr().Is4() { + family = "ipv4" + } + p.Warn().Str("family", family).Int("bits", prefix.Bits()). + Msg("Firewall: organization allowed destination covers a very wide range; traffic to it bypasses Firewall Mode") + } + p.Debug().Strs("values", prefixStrings(wide)). + Msg("Firewall: wide organization allowed destinations") +} + +// prefixStrings renders prefixes for logging. +func prefixStrings(prefixes []netip.Prefix) []string { + out := make([]string, 0, len(prefixes)) + for _, prefix := range prefixes { + out = append(out, prefix.String()) + } + return out +} + // extractHostFromEndpoint extracts the hostname or IP from a DoH/DoT/DoQ endpoint URL. // Handles formats like: // - "https://dns.controld.com/abcdef" @@ -275,13 +680,26 @@ func (p *prog) firewallOnNetworkChange() { p.allowList.Flush() } -// logFirewallStats logs allowlist metrics immediately, then every 5 minutes -// while firewall mode is active. -func (p *prog) logFirewallStats(ctx context.Context) { - if p.allowList == nil { +// firewallMaintenance logs allowlist metrics immediately, then every 5 minutes +// while firewall mode is active, and retries any allowed-destination change that +// platform enforcement rejected. +// +// The retry has to be time-based, not only refresh-driven: configuration +// refreshes are hourly by default, so a transient pfctl or WFP failure would +// otherwise leave an approved destination blocked - or worse, a withdrawn one +// permitted - for up to an hour. +// It works on the allowlist and generation it was started with, not on +// p.allowList: a reload replaces that field from another goroutine, and this +// worker outlives the run whose context it was given by however long it takes to +// observe cancellation. Once its generation is over - a reload, or Firewall Mode +// being turned off - the worker retires rather than reconciling enforcement it no +// longer owns; reconcileDestinations re-checks the generation under +// destinationsMu, so even a worker that is already inside it cannot act late. +func (p *prog) firewallMaintenance(ctx context.Context, al *firewall.AllowList, gen uint64) { + if al == nil { return } - p.logFirewallStatsOnce() + p.logFirewallStatsOnce(al) ticker := time.NewTicker(5 * time.Minute) defer ticker.Stop() @@ -291,19 +709,27 @@ func (p *prog) logFirewallStats(ctx context.Context) { case <-ctx.Done(): return case <-ticker.C: - p.logFirewallStatsOnce() + // A tick can win the select against an already-canceled context, and + // a generation can end without the context being canceled at all. + if ctx.Err() != nil || p.firewallGen.Load() != gen { + return + } + p.reconcileDestinations(al, gen) + p.logFirewallStatsOnce(al) } } } -func (p *prog) logFirewallStatsOnce() { - if p.allowList == nil { +func (p *prog) logFirewallStatsOnce(al *firewall.AllowList) { + if al == nil { return } - stats := p.allowList.Stats() + stats := al.Stats() p.Info(). Int("allowed_ips", stats.AllowedIPs). Int("permanent_ips", stats.PermanentIPs). + Int("allowed_destinations", stats.ExceptionPrefixes). + Int("allowed_destinations_pending", p.pendingDestinations(al)). Int("tracked_domains", stats.TrackedDomains). Int64("total_hits", stats.TotalHits). Int64("total_misses", stats.TotalMisses). diff --git a/cmd/cli/firewall_darwin.go b/cmd/cli/firewall_darwin.go index 7206208..80bdca9 100644 --- a/cmd/cli/firewall_darwin.go +++ b/cmd/cli/firewall_darwin.go @@ -3,6 +3,8 @@ package cli import ( + "context" + "errors" "fmt" "net" "net/netip" @@ -60,6 +62,13 @@ const ( // pfFirewallTable is the pf table name for dynamically-allowed IPs. pfFirewallTable = "ctrld_allowed" + // pfFirewallExceptionTable is the pf table name for the organization's + // Allowed Destination IP list. Kept separate from pfFirewallTable so the + // flushes that discard DNS-resolved IPs (config reload, network change) + // leave administratively allowed destinations in place, and so a destination + // removed upstream can be deleted without touching resolved entries. + pfFirewallExceptionTable = "ctrld_allowed_dst" + // pfFirewallBatchInterval is the accumulation window for batching pf table updates. // Short enough for responsiveness, long enough to avoid per-DNS-response pfctl calls. pfFirewallBatchInterval = 200 * time.Millisecond @@ -162,6 +171,13 @@ func (p *prog) firewallFlushPlatform() { // p.allowList is nil. func (p *prog) shutdownPlatformFirewall() { p.pfFirewallFlushTable() + // The organization's allowed destinations live in their own persist table, + // which the dynamic flush does not touch. Clear it too so turning Firewall + // Mode off leaves no table content behind for a later run to inherit. + if out, err := pfExceptionTableCommand("flush", nil); err != nil { + p.Debug().Err(err).Str("output", strings.TrimSpace(string(out))). + Msgf("Firewall: failed to flush pf table %s during shutdown (may not exist)", pfFirewallExceptionTable) + } if p.dnsInterceptState == nil { return @@ -243,6 +259,14 @@ func (p *prog) initPFFirewall() { // as the in-memory allowlist. p.pfFirewallPopulateTable() + // Likewise for the organization's allowed destinations, which are applied as + // soon as the allowlist exists - before pf enforcement comes up. The table is + // a persist table that may still hold what a previous run put in it, so mark + // the set for a full replace rather than a delta; the reconcile retries until + // pf has exactly the current set. + p.markDestinationsForResync() + p.reconcileAllowedDestinations() + // Seed the forwarded-source snapshot with what the anchor was just built with, // so the first reconcile only fires on a real subsequent change. sources := p.forwardedSources() @@ -373,6 +397,163 @@ func (p *prog) pfFirewallPopulateTable() { } } +// firewallApplyExceptionsPlatform mirrors a change to the organization's Allowed +// Destination IP list into the pf exception table, reporting whether pf took it. +// +// An error - including "pf enforcement is not up yet", because the anchor that +// declares the table has not been loaded and pfctl would fail - leaves the +// caller's applied snapshot unadvanced, so the same delta is retried later. +func (p *prog) firewallApplyExceptionsPlatform(added, removed []netip.Prefix) error { + if state, ok := p.platformFirewallState.(*pfFirewallState); !ok || state == nil { + return errors.New("pf firewall enforcement is not initialized") + } + return errors.Join( + p.pfFirewallExceptionTableOp("add", prefixStrings(added)), + p.pfFirewallExceptionTableOp("delete", prefixStrings(removed)), + ) +} + +const ( + // pfExceptionTableOpTimeout bounds one pfctl call against the exception table. + // reconcileDestinations holds destinationsMu across the mirror, and both the + // configuration refresh loop and Firewall Mode teardown contend on that lock, + // so a pfctl that never returns would stall refresh detection of custom_config + // and pin changes along with the teardown itself. Generous enough that a busy + // pf never trips it, short enough that a wedged one is not indefinite. + pfExceptionTableOpTimeout = 30 * time.Second + + // pfExceptionTableOpChunk caps the addresses handed to one pfctl invocation. + // The organization's list is API-supplied and unbounded, and every entry + // becomes an argv element, so a long enough list would exceed ARG_MAX and fail + // as a whole rather than being applied. + pfExceptionTableOpChunk = 500 +) + +// pfFirewallExceptionTableOp runs one pfctl table operation ("add", "delete" or +// "replace") against the exception table. pf table entries are addressed exactly, +// so deleting a network never disturbs a resolved host address inside it. +// +// Long lists are split across invocations. Only the first chunk carries the +// caller's operation: a chunked "replace" would otherwise leave the table holding +// the last chunk alone, each call having discarded what the previous one +// installed, so the chunks after it add to what the replace established. +func (p *prog) pfFirewallExceptionTableOp(op string, entries []string) error { + if len(entries) == 0 { + return nil + } + for _, chunk := range pfExceptionTableChunks(op, entries) { + if err := p.pfFirewallExceptionTableCall(chunk.op, chunk.entries); err != nil { + return err + } + } + return nil +} + +// pfExceptionTableChunk is one pfctl invocation's share of a table operation. +type pfExceptionTableChunk struct { + op string + entries []string +} + +// pfExceptionTableChunks splits a table operation into invocation-sized pieces. +// Only the first piece carries the requested operation; the rest add, so that a +// split "replace" installs the whole set instead of each piece discarding what +// the previous one installed. "add" and "delete" are per-entry operations, so +// splitting them changes nothing. +func pfExceptionTableChunks(op string, entries []string) []pfExceptionTableChunk { + var chunks []pfExceptionTableChunk + for start := 0; start < len(entries); start += pfExceptionTableOpChunk { + end := min(start+pfExceptionTableOpChunk, len(entries)) + chunkOp := op + // Only a replace changes after the first chunk. "add" and "delete" are + // per-entry, and rewriting a later delete chunk as an add would put back + // exactly the destinations the organization withdrew. + if op == "replace" && start > 0 { + chunkOp = "add" + } + chunks = append(chunks, pfExceptionTableChunk{op: chunkOp, entries: entries[start:end]}) + } + return chunks +} + +// pfFirewallExceptionTableCall runs a single pfctl invocation for one chunk. +func (p *prog) pfFirewallExceptionTableCall(op string, entries []string) error { + out, err := pfExceptionTableCommand(op, entries) + if err != nil { + // A delete against a table that does not exist has already achieved what it + // asked for: with no table there is nothing permitting the entry. Treating + // it as a failure would keep the withdrawal pending forever, since no later + // retry can make an absent table deletable. This matches the WFP mirror, + // which tolerates FWP_E_FILTER_NOT_FOUND on delete for the same reason. + if op == "delete" && pfTableMissing(out) { + p.Debug().Int("entries", len(entries)). + Msgf("Firewall: pf table %s does not exist; the allowed destinations it would have held are already not permitted", pfFirewallExceptionTable) + return nil + } + return fmt.Errorf("pfctl -t %s -T %s (%d entries): %w (output: %s)", + pfFirewallExceptionTable, op, len(entries), err, strings.TrimSpace(string(out))) + } + p.Debug().Strs("entries", entries). + Msgf("Firewall: %s %d allowed destinations in pf table %s", pfTableOpPastTense(op), len(entries), pfFirewallExceptionTable) + return nil +} + +// pfExceptionTableCommand runs one pfctl exception-table call under a timeout. +func pfExceptionTableCommand(op string, entries []string) ([]byte, error) { + ctx, cancel := context.WithTimeout(context.Background(), pfExceptionTableOpTimeout) + defer cancel() + args := append([]string{"-a", pfAnchorName, "-t", pfFirewallExceptionTable, "-T", op}, entries...) + return exec.CommandContext(ctx, "pfctl", args...).CombinedOutput() +} + +// pfTableMissing reports whether pfctl failed because the table is not loaded. +func pfTableMissing(out []byte) bool { + return strings.Contains(strings.ToLower(string(out)), "table does not exist") +} + +// pfTableOpPastTense renders a pfctl table operation for log messages. +func pfTableOpPastTense(op string) string { + switch op { + case "delete": + return "removed" + case "replace": + return "installed" + default: + return "added" + } +} + +// firewallReplaceExceptionsPlatform makes the pf exception table hold exactly +// desired, whatever it held before. +// +// This is what runs when pf enforcement starts, and it must succeed before the +// applied snapshot is established: the table is a persist table that outlives the +// process, so a destination the organization withdrew while ctrld was stopped is +// still in it. Reporting failure is the point - a discarded error here would +// leave that entry bypassing Firewall Mode for the life of the process, with +// nothing pending to say so. +func (p *prog) firewallReplaceExceptionsPlatform(desired []netip.Prefix) error { + if state, ok := p.platformFirewallState.(*pfFirewallState); !ok || state == nil { + return errors.New("pf firewall enforcement is not initialized") + } + entries := prefixStrings(desired) + if len(entries) == 0 { + // pfctl -T replace needs at least one address; emptying is a flush. + out, err := pfExceptionTableCommand("flush", nil) + if err != nil { + if pfTableMissing(out) { + p.Debug().Msgf("Firewall: pf table %s does not exist; nothing is permitted through it", pfFirewallExceptionTable) + return nil + } + return fmt.Errorf("pfctl -t %s -T flush: %w (output: %s)", + pfFirewallExceptionTable, err, strings.TrimSpace(string(out))) + } + p.Debug().Msgf("Firewall: emptied pf table %s", pfFirewallExceptionTable) + return nil + } + return p.pfFirewallExceptionTableOp("replace", entries) +} + // buildPFFirewallRules generates the pf rules for firewall mode enforcement. // These rules are appended to the anchor by buildPFAnchorRules() when firewall // mode is active. @@ -394,14 +575,22 @@ func buildPFFirewallRules() string { rules.WriteString("# Only IPs resolved by ctrld are allowed for outbound connections.\n") rules.WriteString("# Table is dynamically populated from DNS responses.\n\n") - // Declare the table. pfctl -T add/delete operates on this table dynamically. - fmt.Fprintf(&rules, "table <%s> persist\n\n", pfFirewallTable) + // Declare the tables. pfctl -T add/delete operates on these dynamically. + fmt.Fprintf(&rules, "table <%s> persist\n", pfFirewallTable) + fmt.Fprintf(&rules, "table <%s> persist\n\n", pfFirewallExceptionTable) // Pass traffic to allowed IPs (both IPv4 and IPv6). rules.WriteString("# Allow outbound to DNS-resolved IPs.\n") fmt.Fprintf(&rules, "pass out quick inet proto { tcp, udp } from any to <%s>\n", pfFirewallTable) fmt.Fprintf(&rules, "pass out quick inet6 proto { tcp, udp } from any to <%s>\n\n", pfFirewallTable) + // Pass traffic to the organization's allowed destinations. These are reachable + // by literal IP, with no DNS lookup for ctrld to observe, which is the whole + // point of the list; the table is populated from the API's effective set. + rules.WriteString("# Allow outbound to organization allowed destination IPs.\n") + fmt.Fprintf(&rules, "pass out quick inet proto { tcp, udp } from any to <%s>\n", pfFirewallExceptionTable) + fmt.Fprintf(&rules, "pass out quick inet6 proto { tcp, udp } from any to <%s>\n\n", pfFirewallExceptionTable) + // Allow ICMP/ICMPv6 - needed for path MTU discovery, ping, etc. rules.WriteString("# Allow ICMP (path MTU discovery, ping, etc.)\n") rules.WriteString("pass out quick inet proto icmp\n") @@ -901,15 +1090,6 @@ func forwardedSubnetsNotIn(a, b []forwardedSource) []netip.Prefix { return out } -// prefixStrings renders prefixes for logging. -func prefixStrings(prefixes []netip.Prefix) []string { - out := make([]string, 0, len(prefixes)) - for _, pfx := range prefixes { - out = append(out, pfx.String()) - } - return out -} - // forwardedSourceDescriptions renders the effective trust set for logging, naming each // subnet's origin so an admin can tell an auto-detected VM network (and the interface // its rules are scoped to) from an entry they configured. diff --git a/cmd/cli/firewall_darwin_test.go b/cmd/cli/firewall_darwin_test.go index d31289b..cc75459 100644 --- a/cmd/cli/firewall_darwin_test.go +++ b/cmd/cli/firewall_darwin_test.go @@ -569,3 +569,85 @@ func TestPFBuildAnchorRules_ForwardedSourcesGating(t *testing.T) { t.Errorf("forwarded-source redirect (%d) must come before the blanket block (%d)", fwdIdx, blockIdx) } } + +// TestBuildPFFirewallRulesDeclaresExceptionTable pins the pf side of the +// organization's Allowed Destination IP list. +// +// Every cmd/cli test of the allowed-destination paths stubs the platform mirror, +// so nothing else reaches this generator: the table the mirror populates could +// stop being declared, or lose its pass rules, and the mirror would keep +// reporting success while every approved destination stayed blocked. Both +// families are asserted - a list is not usable if only one of them passes. +func TestBuildPFFirewallRulesDeclaresExceptionTable(t *testing.T) { + rules := buildPFFirewallRules() + + wants := []string{ + // Declared persist, like the dynamic table: pfctl -T add/delete/replace + // against an undeclared table fails, and persist is what keeps the table + // alive while it holds no addresses. + "table <" + pfFirewallExceptionTable + "> persist", + "pass out quick inet proto { tcp, udp } from any to <" + pfFirewallExceptionTable + ">", + "pass out quick inet6 proto { tcp, udp } from any to <" + pfFirewallExceptionTable + ">", + } + for _, want := range wants { + if !strings.Contains(rules, want) { + t.Errorf("missing rule:\n %s\nin:\n%s", want, rules) + } + } + + // The exception table is separate from the dynamic one on purpose: the flushes + // that discard DNS-resolved IPs must leave administratively allowed + // destinations in place. + if pfFirewallExceptionTable == pfFirewallTable { + t.Fatal("the exception table and the dynamic table are the same table; a flush would drop the organization's list") + } +} + +// TestPFExceptionTableChunks covers the argv-length split. The organization's +// list is API-supplied and unbounded, and every entry becomes an argv element, so +// a long enough list would blow past ARG_MAX and fail as a whole. +func TestPFExceptionTableChunks(t *testing.T) { + entries := make([]string, pfExceptionTableOpChunk*2+1) + for i := range entries { + entries[i] = "203.0.113.10/32" + } + + if got := pfExceptionTableChunks("replace", nil); len(got) != 0 { + t.Errorf("chunks for an empty list = %d, want 0", len(got)) + } + + short := pfExceptionTableChunks("replace", entries[:2]) + if len(short) != 1 || short[0].op != "replace" || len(short[0].entries) != 2 { + t.Fatalf("a list that fits was split: %+v", short) + } + + // A split replace must replace once and add the rest. Splitting it into three + // replaces would leave pf holding only the final chunk, with the organization's + // other destinations silently dropped while the mirror reported success. + split := pfExceptionTableChunks("replace", entries) + if len(split) != 3 { + t.Fatalf("chunks = %d, want 3 for %d entries at %d per call", len(split), len(entries), pfExceptionTableOpChunk) + } + if split[0].op != "replace" { + t.Errorf("first chunk op = %q, want replace", split[0].op) + } + for _, chunk := range split[1:] { + if chunk.op != "add" { + t.Errorf("chunk after the first has op %q, want add: a second replace discards the first", chunk.op) + } + } + var total int + for _, chunk := range split { + total += len(chunk.entries) + } + if total != len(entries) { + t.Errorf("chunked entries = %d, want %d: the split dropped entries", total, len(entries)) + } + + // delete is per-entry, so every chunk keeps the operation. + for _, chunk := range pfExceptionTableChunks("delete", entries) { + if chunk.op != "delete" { + t.Errorf("delete chunk op = %q, want delete", chunk.op) + } + } +} diff --git a/cmd/cli/firewall_others.go b/cmd/cli/firewall_others.go index 112a74f..20d078d 100644 --- a/cmd/cli/firewall_others.go +++ b/cmd/cli/firewall_others.go @@ -2,6 +2,8 @@ package cli +import "net/netip" + // initPlatformFirewall is a no-op on unsupported platforms (Linux, etc.). // Firewall mode on Linux would require iptables/nftables or eBPF — future work. func (p *prog) initPlatformFirewall() { @@ -13,3 +15,17 @@ func (p *prog) firewallFlushPlatform() {} // shutdownPlatformFirewall is a no-op on unsupported platforms. func (p *prog) shutdownPlatformFirewall() {} + +// firewallApplyExceptionsPlatform succeeds trivially on unsupported platforms. +// Nothing enforces the allowlist here, so the organization's allowed destinations +// need no platform rules and there is nothing that can fail; the in-memory set is +// still maintained for stats and for the Contains() path used by embedders. +func (p *prog) firewallApplyExceptionsPlatform(added, removed []netip.Prefix) error { + return nil +} + +// firewallReplaceExceptionsPlatform succeeds trivially on unsupported platforms, +// for the same reason: there is no platform state to replace. +func (p *prog) firewallReplaceExceptionsPlatform(desired []netip.Prefix) error { + return nil +} diff --git a/cmd/cli/firewall_test.go b/cmd/cli/firewall_test.go index 29dba0d..f11c87d 100644 --- a/cmd/cli/firewall_test.go +++ b/cmd/cli/firewall_test.go @@ -1,6 +1,21 @@ package cli -import "testing" +import ( + "context" + "errors" + "fmt" + "net/netip" + "strings" + "testing" + + "go.uber.org/zap" + "go.uber.org/zap/zapcore" + "go.uber.org/zap/zaptest/observer" + + "github.com/Control-D-Inc/ctrld" + "github.com/Control-D-Inc/ctrld/internal/controld" + "github.com/Control-D-Inc/ctrld/internal/firewall" +) func TestExtractHostFromEndpoint(t *testing.T) { tests := []struct { @@ -24,3 +39,583 @@ func TestExtractHostFromEndpoint(t *testing.T) { }) } } + +func TestParseAllowedDestinations(t *testing.T) { + tests := []struct { + name string + entries []string + want []string + wantRejected []string + wantWide []string + }{ + { + name: "bare IPv4 becomes a host prefix", + entries: []string{"203.0.113.10"}, + want: []string{"203.0.113.10/32"}, + }, + { + name: "bare IPv6 becomes a host prefix", + entries: []string{"2606:1a40::1"}, + want: []string{"2606:1a40::1/128"}, + }, + { + name: "CIDRs of both families", + entries: []string{"198.51.100.0/24", "2001:db8::/48"}, + want: []string{"198.51.100.0/24", "2001:db8::/48"}, + }, + { + name: "IPv4-in-IPv6 is unmapped to its IPv4 form", + entries: []string{"::ffff:203.0.113.10", "::ffff:198.51.100.0/120"}, + want: []string{"203.0.113.10/32", "198.51.100.0/24"}, + }, + { + name: "surrounding whitespace is tolerated", + entries: []string{" 203.0.113.10 ", "\t198.51.100.0/24"}, + want: []string{"203.0.113.10/32", "198.51.100.0/24"}, + }, + { + name: "empty entries are skipped without being reported", + entries: []string{"", " ", "203.0.113.10"}, + want: []string{"203.0.113.10/32"}, + }, + { + name: "one bad entry does not void the rest", + entries: []string{"203.0.113.10", "not-an-ip", "198.51.100.0/33", "example.com"}, + want: []string{"203.0.113.10/32"}, + wantRejected: []string{"not-an-ip", "198.51.100.0/33", "example.com"}, + }, + { + name: "no entries", + entries: nil, + }, + { + // A full-range prefix is accepted - the organization is entitled to one - + // but it lets every destination of that family bypass Firewall Mode, so it + // has to be reported rather than disappearing into a count. + name: "full-range prefixes are reported as wide", + // Masking happens in normalizeExceptions, so the parsed form is still + // the entry as sent; the mask is what makes it a full range. + entries: []string{"1.2.3.4/0", "::/0", "203.0.113.10"}, + want: []string{"1.2.3.4/0", "::/0", "203.0.113.10/32"}, + wantWide: []string{"1.2.3.4/0", "::/0"}, + }, + { + // unmapPrefix turns this into 0.0.0.0/0, which the raw entry does not look + // like at all. + name: "an IPv4-mapped full range is reported after unmapping", + entries: []string{"::ffff:0:0/96"}, + want: []string{"0.0.0.0/0"}, + wantWide: []string{"0.0.0.0/0"}, + }, + { + // /8 and /32 are the floors themselves: a whole classical IPv4 network + // and a whole IPv6 RIR allocation are wide, but both are things an + // organization can legitimately mean, so neither is reported. + name: "prefixes at the floor are not reported as wide", + entries: []string{"198.51.100.0/24", "10.0.0.0/8", "2001:db8::/48", "2001:db8::/32"}, + want: []string{"198.51.100.0/24", "10.0.0.0/8", "2001:db8::/48", "2001:db8::/32"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + prefixes, rejected, wide := parseAllowedDestinations(tt.entries) + if got := strings.Join(prefixStrings(prefixes), ","); got != strings.Join(tt.want, ",") { + t.Errorf("prefixes = %q, want %q", got, strings.Join(tt.want, ",")) + } + if got := strings.Join(rejected, ","); got != strings.Join(tt.wantRejected, ",") { + t.Errorf("rejected = %q, want %q", got, strings.Join(tt.wantRejected, ",")) + } + if got := strings.Join(prefixStrings(wide), ","); got != strings.Join(tt.wantWide, ",") { + t.Errorf("wide = %q, want %q", got, strings.Join(tt.wantWide, ",")) + } + }) + } +} + +// progWithAllowList builds a prog with Firewall Mode's allowlist in place and a +// logger attached, so the allowed-destination paths can be exercised without any +// platform enforcement. +func progWithAllowList() *prog { + p := &prog{allowList: firewall.New()} + p.logger.Store(discardLogger()) + return p +} + +// discardLogger returns a logger that writes nowhere. +// +// Firewall Mode's paths start background workers that log as soon as they run, +// concurrently with the test goroutine. The package-wide test logger writes into +// a shared strings.Builder (see TestMain) that is neither safe for concurrent +// writes nor for a write racing another test's read of it, so tests that spawn +// those workers must not share it. +func discardLogger() *ctrld.Logger { + return &ctrld.Logger{Logger: zap.NewNop()} +} + +// mirrorCall records one attempt to change platform enforcement: a delta, or a +// full replace (replace is true, and added carries the whole desired set). +type mirrorCall struct { + added []string + removed []string + replace bool +} + +// stubMirror replaces both platform mirrors for the duration of a test, +// recording every change they are handed and failing while *failing is true. The +// recorded calls are what proves a rejected change is retried rather than +// forgotten. +func stubMirror(t *testing.T, calls *[]mirrorCall, failing *bool) { + t.Helper() + origMirror, origReplace := firewallMirrorExceptionsFn, firewallReplaceExceptionsFn + t.Cleanup(func() { + firewallMirrorExceptionsFn, firewallReplaceExceptionsFn = origMirror, origReplace + }) + firewallMirrorExceptionsFn = func(_ *prog, added, removed []netip.Prefix) error { + *calls = append(*calls, mirrorCall{added: prefixStrings(added), removed: prefixStrings(removed)}) + if *failing { + return errors.New("platform enforcement rejected the change") + } + return nil + } + firewallReplaceExceptionsFn = func(_ *prog, desired []netip.Prefix) error { + *calls = append(*calls, mirrorCall{added: prefixStrings(desired), replace: true}) + if *failing { + return errors.New("platform enforcement rejected the replacement") + } + return nil + } +} + +func (c mirrorCall) String() string { + kind := "delta" + if c.replace { + kind = "replace" + } + return kind + " added=" + strings.Join(c.added, ",") + " removed=" + strings.Join(c.removed, ",") +} + +// TestAllowedDestinationsRetriedAfterMirrorFailure is the regression guard for +// committing a change in memory that platform enforcement refused: an addition +// that pf/WFP rejected must be retried by the next refresh, even though that +// refresh carries an identical list from the API and so produces no new delta. +// Without a separate applied snapshot, the destination would stay blocked with +// the logs claiming it was applied. +func TestAllowedDestinationsRetriedAfterMirrorFailure(t *testing.T) { + p := progWithAllowList() + var calls []mirrorCall + failing := true + stubMirror(t, &calls, &failing) + + p.rc = &controld.ResolverConfig{DestinationIPs: []string{"203.0.113.10"}} + p.syncAllowedDestinations() + if len(calls) != 1 || strings.Join(calls[0].added, ",") != "203.0.113.10/32" { + t.Fatalf("first refresh: calls = %v", calls) + } + + // An identical refresh must retry the rejected addition. + p.syncAllowedDestinations() + if len(calls) != 2 { + t.Fatalf("identical refresh after a failure did not retry: calls = %v", calls) + } + if strings.Join(calls[1].added, ",") != "203.0.113.10/32" { + t.Fatalf("retry carried the wrong delta: %v", calls[1]) + } + if got := p.pendingDestinations(p.allowList); got != 1 { + t.Fatalf("pendingDestinations = %d, want 1 while the mirror is failing", got) + } + + // Once the platform accepts it, the change stops being retried. + failing = false + p.syncAllowedDestinations() + if len(calls) != 3 { + t.Fatalf("recovery refresh did not reach the mirror: calls = %v", calls) + } + p.syncAllowedDestinations() + if len(calls) != 3 { + t.Fatalf("an applied set was mirrored again: calls = %v", calls) + } + if got := p.pendingDestinations(p.allowList); got != 0 { + t.Fatalf("pendingDestinations = %d, want 0 after a successful mirror", got) + } +} + +// TestAllowedDestinationRemovalRetriedAfterMirrorFailure is the same guarantee +// for the direction that matters more: a withdrawn destination whose removal the +// platform rejected must keep being retried, or it stays permitted for good. +func TestAllowedDestinationRemovalRetriedAfterMirrorFailure(t *testing.T) { + p := progWithAllowList() + var calls []mirrorCall + failing := false + stubMirror(t, &calls, &failing) + + p.rc = &controld.ResolverConfig{DestinationIPs: []string{"203.0.113.10", "198.51.100.0/24"}} + p.syncAllowedDestinations() + if len(calls) != 1 { + t.Fatalf("initial apply: calls = %v", calls) + } + + // The organization withdraws one entry and the removal is rejected. + failing = true + p.rc = &controld.ResolverConfig{DestinationIPs: []string{"203.0.113.10"}} + p.syncAllowedDestinations() + if len(calls) != 2 || strings.Join(calls[1].removed, ",") != "198.51.100.0/24" { + t.Fatalf("withdrawal: calls = %v", calls) + } + + // The next refresh sends the same (already reduced) list; the removal must + // still be retried rather than treated as done. + p.syncAllowedDestinations() + if len(calls) != 3 || strings.Join(calls[2].removed, ",") != "198.51.100.0/24" { + t.Fatalf("identical refresh after a failed removal: calls = %v", calls) + } + + failing = false + p.syncAllowedDestinations() + p.syncAllowedDestinations() + if len(calls) != 4 { + t.Fatalf("removal kept being retried after it succeeded: calls = %v", calls) + } + if got := p.pendingDestinations(p.allowList); got != 0 { + t.Fatalf("pendingDestinations = %d, want 0", got) + } +} + +// TestResyncRetriedUntilPlatformAcceptsIt covers enforcement starting over state +// ctrld cannot describe - most importantly a macOS persist pf table that outlived +// the previous run. The whole set is replaced rather than added, and a replace +// the platform rejected must be retried: otherwise a destination the organization +// withdrew while ctrld was stopped stays in that table forever, with nothing +// pending to reveal it. +func TestResyncRetriedUntilPlatformAcceptsIt(t *testing.T) { + p := progWithAllowList() + var calls []mirrorCall + failing := true + stubMirror(t, &calls, &failing) + + p.rc = &controld.ResolverConfig{DestinationIPs: []string{"203.0.113.10"}} + p.markDestinationsForResync() + p.syncAllowedDestinations() + + if len(calls) != 1 || !calls[0].replace { + t.Fatalf("a resync must replace the whole set, not apply a delta: calls = %v", calls) + } + if got := p.pendingDestinations(p.allowList); got == 0 { + t.Fatal("a rejected replace was reported as nothing pending") + } + + // Retried, still as a replace: until it succeeds nothing about what the + // platform holds is known, so a delta would leave stale entries behind. + p.reconcileAllowedDestinations() + if len(calls) != 2 || !calls[1].replace { + t.Fatalf("rejected replace was not retried: calls = %v", calls) + } + + failing = false + p.reconcileAllowedDestinations() + if len(calls) != 3 || !calls[2].replace { + t.Fatalf("recovery did not replace the set: calls = %v", calls) + } + if got := p.pendingDestinations(p.allowList); got != 0 { + t.Fatalf("pendingDestinations = %d after a successful replace, want 0", got) + } + + // Once the platform is known-good, later changes go back to deltas. + p.rc = &controld.ResolverConfig{DestinationIPs: []string{"203.0.113.10", "198.51.100.0/24"}} + p.syncAllowedDestinations() + if len(calls) != 4 || calls[3].replace { + t.Fatalf("a later change should be a delta: calls = %v", calls) + } +} + +// TestResyncWithEmptyListRetriedUntilAccepted is the same guarantee for an +// organization with no entries at all: the platform still owes ctrld a flush of +// whatever it inherited, so an empty desired set is not "nothing to do". +func TestResyncWithEmptyListRetriedUntilAccepted(t *testing.T) { + p := progWithAllowList() + var calls []mirrorCall + failing := true + stubMirror(t, &calls, &failing) + + p.rc = &controld.ResolverConfig{} + p.markDestinationsForResync() + p.syncAllowedDestinations() + + if len(calls) != 1 || !calls[0].replace || len(calls[0].added) != 0 { + t.Fatalf("empty list did not ask the platform to empty itself: calls = %v", calls) + } + if got := p.pendingDestinations(p.allowList); got == 0 { + t.Fatal("a rejected flush of an empty set was reported as nothing pending") + } + + p.reconcileAllowedDestinations() + if len(calls) != 2 { + t.Fatalf("rejected flush was not retried: calls = %v", calls) + } + + failing = false + p.reconcileAllowedDestinations() + if got := p.pendingDestinations(p.allowList); got != 0 { + t.Fatalf("pendingDestinations = %d after the flush succeeded, want 0", got) + } +} + +// TestRetiredGenerationDoesNotTouchEnforcement pins the teardown boundary: a +// maintenance worker holds the allowlist and generation of the run that started +// it, and once that generation is over - Firewall Mode turned off, or a reload - +// it must not mirror anything, or it would reinstall permits into enforcement +// that is being removed or now belongs to another run. +func TestRetiredGenerationDoesNotTouchEnforcement(t *testing.T) { + p := progWithAllowList() + var calls []mirrorCall + failing := false + stubMirror(t, &calls, &failing) + + gen := p.startFirewallGeneration() + p.rc = &controld.ResolverConfig{DestinationIPs: []string{"203.0.113.10"}} + p.syncAllowedDestinations() + if len(calls) != 1 { + t.Fatalf("initial apply: calls = %v", calls) + } + + // Firewall Mode goes off: the generation ends and the applied set is dropped, + // which is exactly the state that used to make a stale worker reinstall. + stale := p.allowList + p.retireFirewallDestinations() + p.allowList = nil + + p.reconcileDestinations(stale, gen) + if len(calls) != 1 { + t.Fatalf("a retired generation reached platform enforcement: calls = %v", calls) + } + + // A new generation may act again. + p.allowList = stale + newGen := p.startFirewallGeneration() + p.reconcileDestinations(stale, newGen) + if len(calls) != 2 { + t.Fatalf("the current generation was blocked: calls = %v", calls) + } +} + +// TestSyncAllowedDestinationsFollowsResolverConfig covers what a configuration +// refresh has to deliver: a destination added to the organization's list becomes +// reachable without ctrld having resolved it, and one removed from the list stops +// being reachable on the next refresh. +func TestSyncAllowedDestinationsFollowsResolverConfig(t *testing.T) { + p := progWithAllowList() + direct := netip.MustParseAddr("203.0.113.10") + inRange := netip.MustParseAddr("198.51.100.7") + + // Refresh before the organization has any entries. + p.rc = &controld.ResolverConfig{} + p.syncAllowedDestinations() + if p.allowList.Contains(direct) { + t.Fatalf("%s allowed with an empty organization list", direct) + } + + // The organization adds an address and a CIDR. + p.rc = &controld.ResolverConfig{DestinationIPs: []string{"203.0.113.10", "198.51.100.0/24"}} + p.syncAllowedDestinations() + if !p.allowList.Contains(direct) || !p.allowList.Contains(inRange) { + t.Fatalf("allowed destinations not reachable: %s=%v %s=%v", + direct, p.allowList.Contains(direct), inRange, p.allowList.Contains(inRange)) + } + + // The organization removes the CIDR; the remaining entry is untouched. + p.rc = &controld.ResolverConfig{DestinationIPs: []string{"203.0.113.10"}} + p.syncAllowedDestinations() + if p.allowList.Contains(inRange) { + t.Fatalf("%s still allowed after its entry was removed", inRange) + } + if !p.allowList.Contains(direct) { + t.Fatalf("%s should still be allowed", direct) + } + + // The organization clears the list entirely. + p.rc = &controld.ResolverConfig{} + p.syncAllowedDestinations() + if p.allowList.Contains(direct) { + t.Fatalf("%s still allowed after the list was cleared", direct) + } +} + +// TestApplyAllowedDestinationsFirewallModeOff pins that devices with Firewall +// Mode disabled are unaffected: there is no allowlist to apply the list to, and +// the refresh path must not panic on the nil one. +func TestApplyAllowedDestinationsFirewallModeOff(t *testing.T) { + p := &prog{rc: &controld.ResolverConfig{DestinationIPs: []string{"203.0.113.10"}}} + p.logger.Store(discardLogger()) + p.syncAllowedDestinations() + if p.allowList != nil { + t.Fatal("applying allowed destinations created an allowlist while firewall mode is off") + } +} + +// TestSyncFirewallModeAppliesAllowedDestinations covers the startup and reload +// path: turning Firewall Mode on builds a fresh allowlist, which must be seeded +// with the organization's list from the resolver config the run started with - +// otherwise the destinations stay blocked until the next hourly refresh. +func TestSyncFirewallModeAppliesAllowedDestinations(t *testing.T) { + p := &prog{cfg: &ctrld.Config{}} + p.cfg.Service.FirewallMode = "on" + p.logger.Store(discardLogger()) + p.rc = &controld.ResolverConfig{DestinationIPs: []string{"203.0.113.10"}} + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + p.syncFirewallMode(ctx) + if p.allowList == nil { + t.Fatal("firewall mode on did not create an allowlist") + } + if !p.allowList.Contains(netip.MustParseAddr("203.0.113.10")) { + t.Fatal("allowed destination not applied when firewall mode came up") + } + + // Turning the mode off drops the set with the allowlist, and forgets what + // enforcement was holding so a later re-enable reinstalls everything. + p.cfg.Service.FirewallMode = "off" + p.syncFirewallMode(ctx) + if p.allowList != nil { + t.Fatal("firewall mode off did not clear the allowlist") + } + if got := p.pendingDestinations(p.allowList); got != 0 { + t.Fatalf("pendingDestinations = %d with firewall mode off, want 0", got) + } + + p.cfg.Service.FirewallMode = "on" + p.syncFirewallMode(ctx) + if !p.allowList.Contains(netip.MustParseAddr("203.0.113.10")) { + t.Fatal("allowed destination not re-applied after firewall mode was turned back on") + } +} + +// TestAllowedDestinationsSurviveConcurrentReload drives the interleaving the race +// detector caught: apiConfigReload applies the organization's destinations on the +// refresh goroutine while a config reload replaces - or clears - the allowlist on +// another. +// +// The apply path used to read p.allowList twice, once to check it for nil and +// again to call SetExceptions on it, with a parse in between. A reload that +// turned Firewall Mode off inside that gap left the second read nil, and ctrld +// panicked on a refresh that had nothing wrong with it. +func TestAllowedDestinationsSurviveConcurrentReload(t *testing.T) { + origMirror, origReplace := firewallMirrorExceptionsFn, firewallReplaceExceptionsFn + t.Cleanup(func() { + firewallMirrorExceptionsFn, firewallReplaceExceptionsFn = origMirror, origReplace + }) + firewallMirrorExceptionsFn = func(*prog, []netip.Prefix, []netip.Prefix) error { return nil } + firewallReplaceExceptionsFn = func(*prog, []netip.Prefix) error { return nil } + + p := &prog{cfg: &ctrld.Config{}} + p.logger.Store(discardLogger()) + p.rc = &controld.ResolverConfig{DestinationIPs: []string{"203.0.113.10", "198.51.100.0/24"}} + p.cfg.Service.FirewallMode = "on" + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + p.syncFirewallMode(ctx) + + stop := make(chan struct{}) + done := make(chan struct{}) + go func() { + defer close(done) + for { + select { + case <-stop: + return + default: + } + p.syncAllowedDestinations() + } + }() + + // Firewall Mode off then on is what clears p.allowList and installs a fresh + // one, which is the whole of the reload path this refresh can collide with. + for range 20 { + p.cfg.Service.FirewallMode = "off" + p.syncFirewallMode(ctx) + p.cfg.Service.FirewallMode = "on" + p.syncFirewallMode(ctx) + } + close(stop) + <-done + + // The churn must not have cost the set: the run that is live at the end owes + // the platform exactly what the API last sent. + p.syncAllowedDestinations() + p.destinationsMu.Lock() + applied := prefixStrings(p.appliedDestinations) + p.destinationsMu.Unlock() + if got := strings.Join(applied, ","); got != "198.51.100.0/24,203.0.113.10/32" { + t.Errorf("applied destinations = %q, want the full set after the reload churn", got) + } +} + +// TestAllowedDestinationLogsKeepAddressesOutOfWarnings holds the whole +// allowed-destination path to one policy: Warn and Info carry counts, addresses +// appear only at Debug. +// +// Warn logs are persisted and travel in support bundles exactly like Info ones, +// and both a rejected entry and an accepted one are an organization's network +// topology - "10.0.0.0/33" names a real network as surely as the entry next to +// it. logDestinationChange already followed this rule; the rejection and +// wide-prefix warnings are the paths that can leak around it. +func TestAllowedDestinationLogsKeepAddressesOutOfWarnings(t *testing.T) { + var calls []mirrorCall + failing := false + stubMirror(t, &calls, &failing) + + core, logs := observer.New(zapcore.DebugLevel) + p := progWithAllowList() + p.logger.Store(&ctrld.Logger{Logger: zap.New(core)}) + + const ( + bad = "10.0.0.0/33" // near-miss topology: a real network, a bad mask + wide = "0.0.0.0/0" // accepted, and blankets the whole family + ordinal = "203.0.113.10/32" // an ordinary accepted entry + ) + p.rc = &controld.ResolverConfig{DestinationIPs: []string{bad, wide, "203.0.113.10"}} + p.syncAllowedDestinations() + + var warned, wideWarned bool + for _, entry := range logs.FilterLevelExact(zapcore.WarnLevel).All() { + line := entry.Message + fmt.Sprint(entry.ContextMap()) + for _, addr := range []string{bad, wide, ordinal, "203.0.113.10"} { + if strings.Contains(line, addr) { + t.Errorf("a Warn line carries the address %q, which support bundles then carry too:\n %s", addr, line) + } + } + if strings.Contains(entry.Message, "not a valid IP address or CIDR") { + warned = true + if got := entry.ContextMap()["rejected"]; got != int64(1) { + t.Errorf("rejected count = %v, want 1", got) + } + } + if strings.Contains(entry.Message, "very wide range") { + wideWarned = true + if got := entry.ContextMap()["bits"]; got != int64(0) { + t.Errorf("wide prefix bits = %v, want 0 for a full range", got) + } + } + } + if !warned { + t.Error("an unusable entry was dropped without any warning") + } + if !wideWarned { + t.Error("a full-range destination was accepted without any warning; the bypass would be invisible") + } + + // The values are still recoverable by whoever turns Debug on to look. + var debugged string + for _, entry := range logs.FilterLevelExact(zapcore.DebugLevel).All() { + debugged += entry.Message + fmt.Sprint(entry.ContextMap()) + } + if !strings.Contains(debugged, bad) { + t.Errorf("the rejected entry %q appears in no Debug line, so nothing can diagnose it", bad) + } + if !strings.Contains(debugged, wide) { + t.Errorf("the wide entry %q appears in no Debug line", wide) + } +} diff --git a/cmd/cli/firewall_windows.go b/cmd/cli/firewall_windows.go index a4c6580..fb64ec4 100644 --- a/cmd/cli/firewall_windows.go +++ b/cmd/cli/firewall_windows.go @@ -3,6 +3,7 @@ package cli import ( + "errors" "fmt" "net/netip" "runtime" @@ -35,6 +36,21 @@ type wfpFirewallState struct { // across dynamic allowlist flushes. permanentFilterMap map[string]uint64 + // exceptionFilterMap tracks permit filters for the organization's Allowed + // Destination IP list, keyed by prefix string. Kept apart from filterMap so + // the flushes that discard DNS-resolved IPs leave them installed, and apart + // from permanentFilterMap because entries are removed when the organization + // removes them. Guarded by mu. + // + // Known gap, shared with filterMap and permanentFilterMap: rebuildDNSIntercept + // recreates the WFP engine without re-initializing this state, so after a + // health-monitor repair every ID here refers to a filter in a session that is + // gone. Mirroring then fails for good and allowed_destinations_pending stays + // non-zero until the service restarts. Predates the allowed-destination work + // and wants its own fix - re-initializing platform firewall state as part of + // the rebuild - rather than a patch here. + exceptionFilterMap map[string]uint64 + // blockFilterIDv4 and blockFilterIDv6 are the base block-all filters. blockFilterIDv4 uint64 blockFilterIDv6 uint64 @@ -72,6 +88,15 @@ func (p *prog) shutdownPlatformFirewall() { func (p *prog) initPlatformFirewall() { if fwState, ok := p.platformFirewallState.(*wfpFirewallState); ok && fwState != nil { fwState.populatePermanentFilters(p) + // Both callers gate on platformFirewallState being nil, so nothing reaches + // this today. Should something re-initialize enforcement over existing + // state, the filter IDs this state holds describe whatever engine session + // installed them, which is not necessarily the live one - so ask for a full + // replace instead of a delta against a snapshot that may describe filters + // that no longer exist. markDestinationsForResync is idempotent and cheap, + // and an unnecessary replace is a no-op the mirrors already tolerate. + p.markDestinationsForResync() + p.reconcileAllowedDestinations() fwState.populateFilters(p) return } @@ -90,6 +115,7 @@ func (p *prog) initPlatformFirewall() { fwState := &wfpFirewallState{ filterMap: make(map[string]uint64), permanentFilterMap: make(map[string]uint64), + exceptionFilterMap: make(map[string]uint64), engineHandle: state.engineHandle, } p.platformFirewallState = fwState @@ -117,6 +143,13 @@ func (p *prog) initPlatformFirewall() { // listener/loopback traffic, LAN ranges, and other permanent exceptions. fwState.populatePermanentFilters(p) + // The organization's allowed destinations are applied as soon as the allowlist + // exists, which is before WFP enforcement comes up. This session holds no + // filters of its own yet, so mark the set for a full install and let the + // reconcile put it in - and retry it if WFP refuses. + p.markDestinationsForResync() + p.reconcileAllowedDestinations() + // Register batch callback. p.allowList.SetOnBatchChange(func(added []netip.Addr, removed []netip.Addr) { fwState.mu.Lock() @@ -260,6 +293,12 @@ func (s *wfpFirewallState) shutdown(p *prog) { } s.permanentFilterMap = make(map[string]uint64) + exceptionFilters := make(map[string]uint64, len(s.exceptionFilterMap)) + for key, filterID := range s.exceptionFilterMap { + exceptionFilters[key] = filterID + } + s.exceptionFilterMap = make(map[string]uint64) + blockIDs := []uint64{s.blockFilterIDv4, s.blockFilterIDv6} s.blockFilterIDv4 = 0 s.blockFilterIDv6 = 0 @@ -270,6 +309,11 @@ func (s *wfpFirewallState) shutdown(p *prog) { p.Debug().Msgf("Firewall: failed to remove permanent WFP filter for %s during shutdown (HRESULT 0x%x, may already be gone)", key, r1) } } + for key, filterID := range exceptionFilters { + if r1, _, _ := procFwpmFilterDeleteById0.Call(s.engineHandle, uintptr(filterID)); r1 != 0 { + p.Debug().Msgf("Firewall: failed to remove allowed destination WFP filter for %s during shutdown (HRESULT 0x%x, may already be gone)", key, r1) + } + } for _, filterID := range blockIDs { if filterID == 0 { continue @@ -321,6 +365,109 @@ func (s *wfpFirewallState) populatePermanentFilters(p *prog) { } } +// fwpErrFilterNotFound is FWP_E_FILTER_NOT_FOUND: the filter is already gone, so +// a delete that reports it has achieved what it was asked to do. +const fwpErrFilterNotFound = 0x80320003 + +// firewallApplyExceptionsPlatform mirrors a change to the organization's Allowed +// Destination IP list into WFP permit filters, reporting whether WFP took it. +// +// An error - including "WFP enforcement is not up yet", because there is no +// engine handle to install filters through - leaves the caller's applied snapshot +// unadvanced, so the same delta is retried later. +func (p *prog) firewallApplyExceptionsPlatform(added, removed []netip.Prefix) error { + fwState, ok := p.platformFirewallState.(*wfpFirewallState) + if !ok || fwState == nil { + return errors.New("WFP firewall enforcement is not initialized") + } + return fwState.syncExceptionFilters(p, added, removed) +} + +// firewallReplaceExceptionsPlatform makes WFP hold permit filters for exactly +// desired, whatever it held before, and reports whether it took. +// +// This is what runs when WFP enforcement starts. A fresh session holds nothing, +// but the session can also be re-initialized over state this process installed +// earlier, so anything not in desired is removed rather than assumed absent. +func (p *prog) firewallReplaceExceptionsPlatform(desired []netip.Prefix) error { + fwState, ok := p.platformFirewallState.(*wfpFirewallState) + if !ok || fwState == nil { + return errors.New("WFP firewall enforcement is not initialized") + } + return fwState.syncExceptionFilters(p, desired, fwState.exceptionsNotIn(desired)) +} + +// exceptionsNotIn returns the prefixes WFP currently permits that are absent from +// keep, i.e. the filters a full resync has to remove. +func (s *wfpFirewallState) exceptionsNotIn(keep []netip.Prefix) []netip.Prefix { + kept := make(map[string]struct{}, len(keep)) + for _, prefix := range keep { + kept[prefix.String()] = struct{}{} + } + + s.mu.Lock() + defer s.mu.Unlock() + + var out []netip.Prefix + for key := range s.exceptionFilterMap { + if _, ok := kept[key]; ok { + continue + } + prefix, err := netip.ParsePrefix(key) + if err != nil { + continue + } + out = append(out, prefix) + } + return out +} + +// syncExceptionFilters installs permit filters for added prefixes and removes the +// filters of removed ones. Permits use the same weight as dynamically allowed +// IPs, so they override the base block-all filter while still losing to the +// higher-weighted DNS permits. +// +// A filter whose deletion failed keeps its ID in the map: dropping it would leak +// a permit that no longer belongs to any allowed destination and that nothing +// could ever remove, while the returned error keeps the withdrawal pending so the +// next reconcile retries the same deletion. +func (s *wfpFirewallState) syncExceptionFilters(p *prog, added, removed []netip.Prefix) error { + s.mu.Lock() + defer s.mu.Unlock() + + var errs []error + + for _, prefix := range removed { + key := prefix.String() + filterID, ok := s.exceptionFilterMap[key] + if !ok { + continue + } + if r1, _, _ := procFwpmFilterDeleteById0.Call(s.engineHandle, uintptr(filterID)); r1 != 0 && r1 != fwpErrFilterNotFound { + errs = append(errs, fmt.Errorf("delete WFP permit filter for allowed destination %s: HRESULT 0x%x", key, r1)) + continue + } + delete(s.exceptionFilterMap, key) + p.Debug().Msgf("Firewall: removed WFP permit filter for allowed destination %s", key) + } + + for _, prefix := range added { + key := prefix.String() + if _, exists := s.exceptionFilterMap[key]; exists { + continue + } + filterID, err := p.addWFPFirewallPermitPrefix(s, prefix) + if err != nil { + errs = append(errs, fmt.Errorf("add WFP permit filter for allowed destination %s: %w", key, err)) + continue + } + s.exceptionFilterMap[key] = filterID + p.Debug().Msgf("Firewall: added WFP permit filter for allowed destination %s (ID: %d)", key, filterID) + } + + return errors.Join(errs...) +} + // populateFilters installs permit filters for IPs already present in the allowlist // before WFP callbacks were registered. func (s *wfpFirewallState) populateFilters(p *prog) { diff --git a/cmd/cli/prog.go b/cmd/cli/prog.go index 0b4d224..c9cb5c8 100644 --- a/cmd/cli/prog.go +++ b/cmd/cli/prog.go @@ -245,6 +245,39 @@ type prog struct { // VPN DNS manager for split DNS routing when intercept mode is active. vpnDNS *vpnDNSManager + // rejectedDestinationsKey is the signature of the organization allowed + // destination entries that were last reported as unusable, so re-parsing an + // unchanged list on every refresh does not repeat the warning. Protected by mu. + rejectedDestinationsKey string + + // wideDestinationsKey is the same signature for the accepted entries that were + // last reported as covering a very wide range. Protected by mu. + wideDestinationsKey string + + // appliedDestinations is the organization allowed destination set that + // platform enforcement (pf/WFP) has actually accepted, which is not always the + // set the API last sent: a failed pfctl call or WFP filter operation leaves + // this behind the desired set, and reconcileAllowedDestinations retries the + // difference until they agree. Protected by destinationsMu, which is separate + // from mu because the mirror it guards runs subprocesses. + appliedDestinations []netip.Prefix + destinationsMu sync.Mutex + + // destinationsNeedResync marks that platform enforcement holds state ctrld + // cannot describe - a pf persist table inherited from a previous run, or a + // WFP session that holds nothing yet - so the next reconcile must replace its + // whole set instead of applying a delta. Cleared only once that replace has + // succeeded. Protected by destinationsMu. + destinationsNeedResync bool + + // firewallGen identifies the current run's firewall enforcement. It advances + // on every Firewall Mode start, reload and teardown, so a maintenance worker + // left over from an earlier run can tell that the enforcement it was given is + // no longer the enforcement in place, and stop touching it. Advanced under + // destinationsMu, which is also held across the platform mirror, so teardown + // and a worker's reconcile cannot interleave. + firewallGen atomic.Uint64 + // allowList tracks IPs resolved by ctrld for firewall mode enforcement. // When firewall_mode is "on", only IPs in this list (plus permanent entries) // are allowed for outbound connections. nil when firewall mode is off. @@ -460,6 +493,12 @@ func (p *prog) postRun() { } } +// fetchResolverConfigFn fetches the resolver config for a configuration refresh. +// Indirected so the refresh loop itself - its ticker and its forced-reload path - +// can be driven in tests without an API server, rather than only the handler it +// calls. +var fetchResolverConfigFn = controld.FetchResolverConfig + // apiConfigReload calls API to check for latest config update then reload ctrld if necessary. func (p *prog) apiConfigReload() { if cdUID == "" { @@ -490,7 +529,7 @@ func (p *prog) apiConfigReload() { Version: appVersion, Metadata: ctrld.SystemMetadata(loggerCtx), } - resolverConfig, err := controld.FetchResolverConfig(loggerCtx, req, cdDev) + resolverConfig, err := fetchResolverConfigFn(loggerCtx, req, cdDev) selfUninstallCheck(err, p, logger) if err != nil { logger.Warn().Err(err).Msg("Could not fetch resolver config") @@ -502,66 +541,7 @@ func (p *prog) apiConfigReload() { _ = selfUpgradeCheck(resolverConfig.Ctrld.VersionTarget, curVer, logger) } - if resolverConfig.DeactivationPin != nil { - newDeactivationPin := *resolverConfig.DeactivationPin - curDeactivationPin := cdDeactivationPin.Load() - switch { - case curDeactivationPin != defaultDeactivationPin: - logger.Debug().Msg("Saving deactivation pin") - case curDeactivationPin != newDeactivationPin: - logger.Debug().Msg("Update deactivation pin") - } - cdDeactivationPin.Store(newDeactivationPin) - } else { - cdDeactivationPin.Store(defaultDeactivationPin) - } - - p.mu.Lock() - rc := p.rc - p.rc = resolverConfig - p.mu.Unlock() - noCustomConfig := resolverConfig.Ctrld.CustomConfig == "" - noExcludeListChanged := true - if rc != nil { - slices.Sort(rc.Exclude) - slices.Sort(resolverConfig.Exclude) - noExcludeListChanged = slices.Equal(rc.Exclude, resolverConfig.Exclude) - } - if noCustomConfig && noExcludeListChanged { - return - } - - if noCustomConfig && !noExcludeListChanged { - logger.Debug().Msg("Exclude list changes detected, reloading...") - p.firewallOnConfigReload() - p.apiReloadCh <- nil - return - } - - if resolverConfig.Ctrld.CustomLastUpdate > lastUpdated || forced { - lastUpdated = time.Now().Unix() - cfg := &ctrld.Config{} - var cfgErr error - if cfgErr = validateCdRemoteConfig(resolverConfig, cfg); cfgErr == nil { - setListenerDefaultValue(cfg) - setNetworkDefaultValue(cfg) - cfgErr = validateConfig(cfg) - } - if cfgErr != nil { - logger.Warn().Err(err).Msg("Skipping invalid custom config") - if _, err := controld.UpdateCustomLastFailed(loggerCtx, cdUID, appVersion, cdDev, true); err != nil { - logger.Error().Err(err).Msg("Could not mark custom last update failed") - } - return - } - logger.Debug().Msg("Custom config changes detected, reloading...") - // Firewall mode: flush allowlist so DNS queries against the new - // config repopulate it with IPs allowed under the updated policy. - p.firewallOnConfigReload() - p.apiReloadCh <- cfg - } else { - logger.Debug().Msg("Custom config does not change") - } + lastUpdated = p.applyFetchedResolverConfig(loggerCtx, logger, resolverConfig, forced, lastUpdated) } for { select { @@ -575,6 +555,94 @@ func (p *prog) apiConfigReload() { } } +// applyFetchedResolverConfig applies a freshly fetched resolver config, and is +// the whole of what a configuration refresh does with one: the deactivation pin, +// the organization's allowed destinations, and the decision whether the change +// warrants reloading ctrld. Returns the lastUpdated watermark to carry into the +// next refresh. +// +// Split out of apiConfigReload's fetch loop so both refresh paths - the scheduled +// tick and a forced reload - can be exercised without an API server, including +// the early return taken when neither the custom config nor the exclusion list +// changed. That case is the one where a destination change would be easiest to +// drop, because nothing else about the refresh has any effect. +func (p *prog) applyFetchedResolverConfig( + loggerCtx context.Context, + logger *ctrld.Logger, + resolverConfig *controld.ResolverConfig, + forced bool, + lastUpdated int64, +) int64 { + if resolverConfig.DeactivationPin != nil { + newDeactivationPin := *resolverConfig.DeactivationPin + curDeactivationPin := cdDeactivationPin.Load() + switch { + case curDeactivationPin != defaultDeactivationPin: + logger.Debug().Msg("Saving deactivation pin") + case curDeactivationPin != newDeactivationPin: + logger.Debug().Msg("Update deactivation pin") + } + cdDeactivationPin.Store(newDeactivationPin) + } else { + cdDeactivationPin.Store(defaultDeactivationPin) + } + + p.mu.Lock() + rc := p.rc + p.rc = resolverConfig + p.mu.Unlock() + + // Apply the organization's Allowed Destination IP list before the early + // returns below: adds and removals must take effect on every refresh, + // scheduled or forced, whether or not anything else changed. It needs no + // ctrld reload - the set is enforced directly. + p.applyAllowedDestinations(p.firewallAllowList(), resolverConfig.DestinationIPs) + + noCustomConfig := resolverConfig.Ctrld.CustomConfig == "" + noExcludeListChanged := true + if rc != nil { + slices.Sort(rc.Exclude) + slices.Sort(resolverConfig.Exclude) + noExcludeListChanged = slices.Equal(rc.Exclude, resolverConfig.Exclude) + } + if noCustomConfig && noExcludeListChanged { + return lastUpdated + } + + if noCustomConfig && !noExcludeListChanged { + logger.Debug().Msg("Exclude list changes detected, reloading...") + p.firewallOnConfigReload() + p.apiReloadCh <- nil + return lastUpdated + } + + if resolverConfig.Ctrld.CustomLastUpdate > lastUpdated || forced { + lastUpdated = time.Now().Unix() + cfg := &ctrld.Config{} + var cfgErr error + if cfgErr = validateCdRemoteConfig(resolverConfig, cfg); cfgErr == nil { + setListenerDefaultValue(cfg) + setNetworkDefaultValue(cfg) + cfgErr = validateConfig(cfg) + } + if cfgErr != nil { + logger.Warn().Err(cfgErr).Msg("Skipping invalid custom config") + if _, err := controld.UpdateCustomLastFailed(loggerCtx, cdUID, appVersion, cdDev, true); err != nil { + logger.Error().Err(err).Msg("Could not mark custom last update failed") + } + return lastUpdated + } + logger.Debug().Msg("Custom config changes detected, reloading...") + // Firewall mode: flush allowlist so DNS queries against the new + // config repopulate it with IPs allowed under the updated policy. + p.firewallOnConfigReload() + p.apiReloadCh <- cfg + } else { + logger.Debug().Msg("Custom config does not change") + } + return lastUpdated +} + func (p *prog) setupUpstream(cfg *ctrld.Config) { localUpstreams := make([]string, 0, len(cfg.Upstream)) ptrNameservers := make([]string, 0, len(cfg.Upstream)) diff --git a/docs/firewall-mode.md b/docs/firewall-mode.md index 71ec9d6..3423154 100644 --- a/docs/firewall-mode.md +++ b/docs/firewall-mode.md @@ -17,6 +17,10 @@ IPs, direct-IP fallbacks, or alternative DNS resolvers to bypass DNS-based filte 3. **Permanent entries are always allowed**: Loopback, RFC1918 private ranges, link-local, CGNAT, multicast, ctrld's own listener, and upstream resolver IPs are always allowed. +4. **The organization's allowed destinations are always allowed**: managed endpoints receive + an explicit list of destinations from the API that stay reachable without a DNS lookup - + see [Organization Allowed Destination IPs](#organization-allowed-destination-ips). + ## Configuration ### TOML Config @@ -36,7 +40,9 @@ ctrld start --firewall-mode on --intercept-mode hard ### Remote API Firewall mode can be toggled remotely via the ControlD API's `custom_config` field, -which is polled by `apiConfigReload()`. +which is polled by `apiConfigReload()`. The same response carries the organization's +`destination_ips` list - see +[Organization Allowed Destination IPs](#organization-allowed-destination-ips). ## Platform-Specific Enforcement @@ -47,6 +53,7 @@ with a `` table: - Default: block all outbound traffic - Pass: traffic to IPs in the `` table +- Pass: traffic to the organization's allowed destinations in the `` table - Pass: traffic to loopback and link-local - Pass: existing DNS intercept rules @@ -61,6 +68,7 @@ sublayer with dynamic permit filters: - Base: block all outbound traffic (low-weight filter) - Dynamic: permit filters for each IP in the allowlist - Static: permits for loopback, RFC1918, ctrld listener +- Organization: permit filters for each entry in the Allowed Destination IP list Permit filters are added/removed dynamically as the allowlist changes. @@ -116,6 +124,54 @@ These IPs are always allowed regardless of DNS resolution: | ctrld listener IPs | Self - DNS proxy must be reachable | | Upstream resolver IPs | DoH/DoT/DoQ endpoints | +## Organization Allowed Destination IPs + +Firewall Mode only permits what ctrld resolved, so a service addressed by literal IP - with +no DNS lookup to observe - is unreachable. An organization can publish a list of destinations +that stay reachable anyway, without having to turn Firewall Mode off. + +The API sends the *effective* list for the endpoint's organization in the `destination_ips` +field of every resolver-config response: the organization's own entries plus any inherited +from a parent organization that applies its settings to sub-organizations. Entries are IPv4 +or IPv6 addresses (bare, e.g. `203.0.113.10`) or CIDRs (e.g. `198.51.100.0/24`, +`2001:db8::/48`). Nothing is configured locally - the list is not a TOML setting. + +How it is applied: + +- **As a set, not as additions.** Every refresh - the scheduled one and a forced + `apiConfigReload` - replaces the previous set. An entry added upstream takes effect on the + next refresh; an entry removed upstream stops bypassing Firewall Mode on the next refresh, + unless it is independently allowed by a DNS resolution or a permanent entry. +- **Without a reload.** Applying the list does not restart listeners or reload the config, + and it is unaffected by the allowlist flushes that follow a profile change or a network + change - unlike DNS-resolved IPs, these entries carry no TTL and are never reaped. +- **Per platform.** macOS puts them in a second pf table, ``, passed by + its own rules; Windows installs a WFP permit filter per entry, at the same weight as the + dynamic permits. Both are kept apart from the DNS-resolved entries so a flush of those + leaves the organization's list installed. On Linux and other unsupported platforms the set + is tracked in memory and reported in stats, but nothing enforces it (see above). +- **Retried until enforcement agrees.** ctrld tracks the set the API asked for separately + from the set pf/WFP has accepted. A failed `pfctl` call or WFP filter operation does not + advance the applied set, so the same change is retried by the next refresh and by a + reconcile every 5 minutes - a rejected addition does not leave an approved destination + blocked, and a rejected removal does not leave a withdrawn one permitted. Until the two + agree the difference is reported as `allowed_destinations_pending` in the stats line, and + the "applied" log line is not written. +- **Enforcement startup replaces, it does not add.** When pf/WFP enforcement comes up, ctrld + knows nothing about what it holds - the macOS table is a `persist` table that outlives the + process, so it can still contain what a previous run put there, including entries the + organization has since withdrawn. The first reconcile therefore replaces the table's whole + contents (an empty list means emptying it), and that replace is retried on the same + schedule until it succeeds; only then does ctrld consider any part of the set applied. +- **Bad entries are dropped individually.** An entry that is not a valid address or CIDR is + logged once and skipped; the rest of the list still applies. +- **Addresses are logged at debug level.** The list is organization network topology, so + Info-level logging - which is persisted and travels in support bundles - carries only + counts. + +Devices with Firewall Mode off are unaffected: there is nothing to make an exception to, so +the list is ignored until the mode is turned on. + ## Live Profile Updates When a ControlD profile changes (domain goes from allowed → blocked or vice versa): @@ -328,9 +384,14 @@ itself, not only the dashboard. Allowlist stats are logged every 5 minutes: ``` -Firewall allowlist stats allowed_ips=142 permanent_ips=18 tracked_domains=89 total_hits=4521 total_misses=23 +Firewall allowlist stats allowed_ips=142 permanent_ips=18 allowed_destinations=3 allowed_destinations_pending=0 tracked_domains=89 total_hits=4521 total_misses=23 ``` +`allowed_destinations` is the number of prefixes in the organization's Allowed Destination +IP list. `allowed_destinations_pending` is how many of its changes platform enforcement has +not accepted yet - normally 0; a non-zero value that persists across reconciles means +`pfctl`/WFP keeps rejecting the change, and the warning that named the error is in the log. + ## Troubleshooting @@ -340,7 +401,17 @@ Firewall allowlist stats allowed_ips=142 permanent_ips=18 tracked_domains=89 tot - Check allowlist stats for hit/miss ratio ### Certain apps don't work -- The app may be using hardcoded IPs (this is the intended behavior - those IPs aren't DNS-resolved) +- The app may be using hardcoded IPs (this is the intended behavior - those IPs aren't DNS-resolved). + For an approved service, add its addresses to the organization's Allowed Destination IP list, + then either wait for the next scheduled refresh (`refetch_time`, hourly by default) or force + one by resolving `.verify.controld.com` through ctrld, which is the only trigger that + makes ctrld re-fetch its resolver config on demand. The applied set is logged as `Firewall: + applied organization allowed destination IPs` and counted as `allowed_destinations` in the + stats line. If that line does not appear, check for `could not apply all organization allowed + destinations` (a delta that enforcement rejected) or `could not install organization allowed + destinations, will retry` (the full install done when enforcement starts, or after a resync), + along with the `allowed_destinations_pending` count - enforcement is refusing the change and + the reconcile is retrying it - Check if the app uses a custom DNS resolver that bypasses ctrld - RFC1918 traffic is always allowed, so LAN-only apps should work diff --git a/internal/controld/config.go b/internal/controld/config.go index 712e2db..895828c 100644 --- a/internal/controld/config.go +++ b/internal/controld/config.go @@ -44,7 +44,14 @@ type ResolverConfig struct { CustomLastUpdate int64 `json:"custom_last_update"` VersionTarget string `json:"version_target"` } `json:"ctrld"` - Exclude []string `json:"exclude"` + Exclude []string `json:"exclude"` + // DestinationIPs is the organization's effective Allowed Destination IP list: + // the entries configured for this endpoint's organization plus any inherited + // from a parent organization. Each entry is an IPv4/IPv6 address or a CIDR + // (the API reports single-host entries as bare addresses, not /32 or /128). + // Under Firewall Mode these destinations stay reachable without a prior DNS + // lookup; see cmd/cli/firewall.go. + DestinationIPs []string `json:"destination_ips"` UID string `json:"uid"` DeactivationPin *int64 `json:"deactivation_pin,omitempty"` } diff --git a/internal/controld/config_test.go b/internal/controld/config_test.go index 5973de0..2b3d6d5 100644 --- a/internal/controld/config_test.go +++ b/internal/controld/config_test.go @@ -7,6 +7,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func Test_parseUID(t *testing.T) { @@ -96,3 +97,40 @@ func TestAPIErrorRecordsHTTPStatus(t *testing.T) { } }) } + +// TestUtilityResponseDecodesDestinationIPs pins the API field that carries the +// organization's effective Allowed Destination IP list. The list is enforced as a +// set of Firewall Mode exceptions, so a silent decode change - a renamed field, a +// nesting change - would leave endpoints blocking destinations the organization +// approved, with nothing in the logs to say why. +func TestUtilityResponseDecodesDestinationIPs(t *testing.T) { + tests := []struct { + name string + body string + want []string + }{ + { + name: "addresses and CIDRs of both families", + body: `{"body":{"resolver":{"doh":"https://dns.controld.dev/abc","destination_ips":["203.0.113.10","198.51.100.0/24","2606:1a40::1","2001:db8::/48"]}},"success":true}`, + want: []string{"203.0.113.10", "198.51.100.0/24", "2606:1a40::1", "2001:db8::/48"}, + }, + { + name: "empty list - the API always sends the field", + body: `{"body":{"resolver":{"doh":"https://dns.controld.dev/abc","destination_ips":[]}},"success":true}`, + want: []string{}, + }, + { + name: "field absent", + body: `{"body":{"resolver":{"doh":"https://dns.controld.dev/abc"}},"success":true}`, + want: nil, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + ur := &utilityResponse{} + require.NoError(t, json.Unmarshal([]byte(tc.body), ur)) + assert.Equal(t, tc.want, ur.Body.Resolver.DestinationIPs) + }) + } +} diff --git a/internal/firewall/allowlist.go b/internal/firewall/allowlist.go index c205daf..e6b9427 100644 --- a/internal/firewall/allowlist.go +++ b/internal/firewall/allowlist.go @@ -5,7 +5,10 @@ // // The AllowList is the core data structure: a concurrent map of allowed IPs // populated by DNS responses, with TTL-based expiry and domain-level invalidation -// for live profile updates. +// for live profile updates. Alongside it are two sets that no DNS response feeds: +// permanent entries (loopback, RFC1918, upstreams — added once, never removed) and +// exceptions, the organization's Allowed Destination IP list, which is replaced as +// a whole set on every configuration refresh (see exceptions.go). package firewall import ( @@ -40,6 +43,19 @@ type AllowList struct { // upstream resolver IPs. permanent sync.Map // netip.Addr → struct{} + // exceptions holds the administratively allowed destinations — the + // organization's Allowed Destination IP list, delivered by the API on every + // configuration refresh. Unlike permanent entries they are replaced as a whole + // set (see SetExceptions), so an entry removed upstream stops being allowed. + // Held as an immutable index behind an atomic pointer because Contains() + // reads it on the hot path. + exceptions atomic.Pointer[exceptionIndex] + + // exceptionsMu serializes SetExceptions so two concurrent refreshes cannot + // interleave their compare and store steps. It is separate from mu so + // replacing the set never blocks DNS-driven allowlist updates. + exceptionsMu sync.Mutex + // onChange is called (if non-nil) whenever the allowlist changes. // The callback receives the IP and whether it was added (true) or removed (false). // Platform-specific enforcement (pf/WFP) registers a callback here. @@ -80,6 +96,9 @@ type Stats struct { AllowedIPs int `json:"allowed_ips"` // PermanentIPs is the number of permanently allowed IPs. PermanentIPs int `json:"permanent_ips"` + // ExceptionPrefixes is the number of administratively allowed destination + // prefixes (the organization's Allowed Destination IP list). + ExceptionPrefixes int `json:"exception_prefixes"` // TrackedDomains is the number of domains with IP associations. TrackedDomains int `json:"tracked_domains"` // TotalAdds is the cumulative number of Add() calls. @@ -190,6 +209,13 @@ func (a *AllowList) Contains(ip netip.Addr) bool { return true } + // Administratively allowed destinations are reachable without ctrld having + // resolved them, so they are checked before the DNS-driven map. + if a.containsException(ip) { + a.totalHits.Add(1) + return true + } + // Check dynamic allowlist. if existing, ok := a.ips.Load(ip); ok { e := existing.(*entry) @@ -417,6 +443,7 @@ func (a *AllowList) Stats() Stats { s.TrackedDomains++ return true }) + s.ExceptionPrefixes = len(a.exceptionsSnapshot()) s.TotalAdds = a.totalAdds.Load() s.TotalRemoves = a.totalRemoves.Load() s.TotalHits = a.totalHits.Load() diff --git a/internal/firewall/exceptions.go b/internal/firewall/exceptions.go new file mode 100644 index 0000000..5de5c74 --- /dev/null +++ b/internal/firewall/exceptions.go @@ -0,0 +1,210 @@ +package firewall + +import ( + "net/netip" + "sort" +) + +// Exception handling: the organization's Allowed Destination IP list. +// +// Firewall Mode only permits what ctrld itself resolved, which makes a service +// addressed by literal IP — with no DNS lookup to observe — unreachable. An +// organization can therefore publish a list of destinations that stay reachable +// regardless; the API delivers the effective list (own entries plus any inherited +// from a parent organization) with every configuration refresh. +// +// The list is applied as a *set*, not as individual additions: each refresh +// replaces the previous snapshot, so an entry removed upstream stops bypassing +// Firewall Mode as soon as the refresh lands. Entries never expire in between — +// unlike DNS-resolved IPs they carry no TTL. +// +// This type holds the *desired* set only. Mirroring it into pf/WFP can fail, so +// what platform enforcement has actually accepted is tracked by the caller (see +// prog.reconcileAllowedDestinations), which retries until the two agree. Storing +// "applied" here would make a failed pfctl call look like a success to every +// later refresh. +// +// The same split matters to anything embedding this package: Contains() answers +// from the set the last SetExceptions call installed, which is what ctrld wants +// enforced, not what the kernel is enforcing. On macOS and Windows the platform +// state is the gate and the difference is tracked and retried, so a mirror that +// failed cannot let traffic through. An embedder that gates on Contains() alone +// has no such gate, and inherits the desired set the moment it is set. + +// SetExceptions replaces the allowed-destination set with prefixes. Entries are +// masked and de-duplicated first, so equivalent spellings of the same network +// (e.g. "10.1.2.3/24" and "10.1.2.0/24") do not rebuild the index. +// +// Deliberately reports nothing about whether the set changed. "Nothing changed" +// is not a licence to skip the platform reconcile: enforcement can be behind the +// desired set from an earlier failed mirror, and that retry is driven by +// comparing against what the platform accepted, not against the previous desired +// set. A caller that skipped on "unchanged" would strand exactly the case the +// retry exists for. +func (a *AllowList) SetExceptions(prefixes []netip.Prefix) { + next := normalizeExceptions(prefixes) + + // Serialized so two concurrent refreshes cannot interleave their compare and + // store steps and leave the older set installed. + a.exceptionsMu.Lock() + defer a.exceptionsMu.Unlock() + + if samePrefixes(a.exceptionsSnapshot(), next) { + return + } + a.exceptions.Store(newExceptionIndex(next)) +} + +// Exceptions returns the current allowed-destination set, ordered and masked. +// The result must not be modified — it is the live snapshot shared with the +// Contains() hot path. +func (a *AllowList) Exceptions() []netip.Prefix { + return a.exceptionsSnapshot() +} + +// exceptionsSnapshot returns the stored set, or nil when none was ever applied. +func (a *AllowList) exceptionsSnapshot() []netip.Prefix { + if idx := a.exceptions.Load(); idx != nil { + return idx.prefixes + } + return nil +} + +// containsException reports whether ip falls inside an allowed destination. +// +// Binary search over sorted, merged address ranges: Contains() is the +// per-connection (and, for embedders, per-packet) hot path, and an organization +// list can hold thousands of prefixes, so a linear scan would put its length on +// that path. An empty set — the overwhelmingly common case — costs one nil check. +func (a *AllowList) containsException(ip netip.Addr) bool { + idx := a.exceptions.Load() + if idx == nil { + return false + } + ranges := idx.v6 + if ip.Is4() { + ranges = idx.v4 + } + if len(ranges) == 0 { + return false + } + // Find the last range whose start is <= ip; it is the only one that can + // contain ip, because ranges are sorted and non-overlapping. + i := sort.Search(len(ranges), func(i int) bool { return ranges[i].lo.Compare(ip) > 0 }) + if i == 0 { + return false + } + return ranges[i-1].hi.Compare(ip) >= 0 +} + +// exceptionIndex is an immutable lookup structure over one allowed-destination +// set: the normalized prefixes as applied, plus per-family sorted address ranges +// for lookups. Published as a whole behind an atomic pointer, so a refresh never +// exposes a half-rebuilt index to a concurrent Contains(). +type exceptionIndex struct { + prefixes []netip.Prefix + v4 []addrRange + v6 []addrRange +} + +// addrRange is an inclusive address range, the range form of one prefix (or of +// several that were merged because they overlap or abut). +type addrRange struct { + lo, hi netip.Addr +} + +// newExceptionIndex builds the lookup index for a normalized prefix set. +func newExceptionIndex(prefixes []netip.Prefix) *exceptionIndex { + idx := &exceptionIndex{prefixes: prefixes} + for _, prefix := range prefixes { + r := addrRange{lo: prefix.Addr(), hi: lastAddr(prefix)} + if prefix.Addr().Is4() { + idx.v4 = append(idx.v4, r) + } else { + idx.v6 = append(idx.v6, r) + } + } + idx.v4 = sortAndMerge(idx.v4) + idx.v6 = sortAndMerge(idx.v6) + return idx +} + +// lastAddr returns the highest address in a masked prefix. +func lastAddr(prefix netip.Prefix) netip.Addr { + if prefix.Addr().Is4() { + b := prefix.Addr().As4() + for i := prefix.Bits(); i < 32; i++ { + b[i/8] |= 1 << (7 - i%8) + } + return netip.AddrFrom4(b) + } + b := prefix.Addr().As16() + for i := prefix.Bits(); i < 128; i++ { + b[i/8] |= 1 << (7 - i%8) + } + return netip.AddrFrom16(b) +} + +// sortAndMerge orders ranges by start address and coalesces the ones that +// overlap or abut, so the search invariant (sorted, non-overlapping) holds even +// when an organization lists a network and an address inside it. +func sortAndMerge(ranges []addrRange) []addrRange { + if len(ranges) < 2 { + return ranges + } + sort.Slice(ranges, func(i, j int) bool { return ranges[i].lo.Compare(ranges[j].lo) < 0 }) + + out := ranges[:1] + for _, r := range ranges[1:] { + last := &out[len(out)-1] + // Abutting counts as overlapping: last.hi.Next() == r.lo means the two + // ranges are contiguous with no gap to preserve. + if r.lo.Compare(last.hi) <= 0 || r.lo == last.hi.Next() { + if r.hi.Compare(last.hi) > 0 { + last.hi = r.hi + } + continue + } + out = append(out, r) + } + return out +} + +// normalizeExceptions masks, de-duplicates and orders prefixes so that two sets +// with the same meaning compare equal, and so logged deltas are stable. +func normalizeExceptions(prefixes []netip.Prefix) []netip.Prefix { + if len(prefixes) == 0 { + return nil + } + seen := make(map[netip.Prefix]struct{}, len(prefixes)) + out := make([]netip.Prefix, 0, len(prefixes)) + for _, prefix := range prefixes { + if !prefix.IsValid() { + continue + } + masked := prefix.Masked() + if _, dup := seen[masked]; dup { + continue + } + seen[masked] = struct{}{} + out = append(out, masked) + } + if len(out) == 0 { + return nil + } + sort.Slice(out, func(i, j int) bool { return out[i].String() < out[j].String() }) + return out +} + +// samePrefixes reports whether two normalized sets are identical. +func samePrefixes(a, b []netip.Prefix) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +} diff --git a/internal/firewall/exceptions_test.go b/internal/firewall/exceptions_test.go new file mode 100644 index 0000000..f279a07 --- /dev/null +++ b/internal/firewall/exceptions_test.go @@ -0,0 +1,214 @@ +package firewall + +import ( + "fmt" + "net/netip" + "testing" + "time" +) + +func prefixes(t *testing.T, ss ...string) []netip.Prefix { + t.Helper() + out := make([]netip.Prefix, 0, len(ss)) + for _, s := range ss { + out = append(out, netip.MustParsePrefix(s)) + } + return out +} + +func TestSetExceptionsNormalizesAndReplaces(t *testing.T) { + al := New() + + al.SetExceptions(prefixes(t, "203.0.113.10/32", "198.51.100.0/24")) + first := al.Exceptions() + if got := prefixStrings(first); len(got) != 2 { + t.Fatalf("Exceptions() = %v, want 2 entries", got) + } + + // Same set, different order and an unmasked spelling of the same network. The + // index must not be rebuilt: the list is re-applied on every configuration + // refresh, and a rebuild would republish it to the Contains() hot path hourly + // for a set that did not change. + al.SetExceptions(prefixes(t, "198.51.100.77/24", "203.0.113.10/32")) + same := al.Exceptions() + if len(same) != len(first) || &same[0] != &first[0] { + t.Fatalf("an equivalent set replaced the index: %v -> %v", prefixStrings(first), prefixStrings(same)) + } + + al.SetExceptions(prefixes(t, "203.0.113.10/32", "2001:db8::/48")) + changed := al.Exceptions() + if got := prefixStrings(changed); len(got) != 2 { + t.Fatalf("Exceptions() = %v, want 2 entries", got) + } + if &changed[0] == &first[0] { + t.Fatal("a changed set left the previous index installed") + } + + // An empty list clears the set: an organization can withdraw everything. + al.SetExceptions(nil) + if got := al.Exceptions(); len(got) != 0 { + t.Fatalf("Exceptions() after clear = %v, want empty", got) + } + al.SetExceptions(nil) + if got := al.Exceptions(); len(got) != 0 { + t.Fatalf("Exceptions() after clearing twice = %v, want empty", got) + } +} + +func TestExceptionsAllowWithoutDNS(t *testing.T) { + al := New() + inRange := netip.MustParseAddr("198.51.100.7") + host := netip.MustParseAddr("203.0.113.10") + other := netip.MustParseAddr("203.0.113.11") + v6 := netip.MustParseAddr("2001:db8::1") + + for _, ip := range []netip.Addr{inRange, host, other, v6} { + if al.Contains(ip) { + t.Fatalf("%s allowed before any exception was applied", ip) + } + } + + al.SetExceptions(prefixes(t, "198.51.100.0/24", "203.0.113.10/32", "2001:db8::/48")) + + // Allowed with no prior DNS resolution — the point of the list. + for _, ip := range []netip.Addr{inRange, host, v6} { + if !al.Contains(ip) { + t.Fatalf("%s not allowed by the exception set", ip) + } + } + // An unresolved public destination outside the set stays blocked. + if al.Contains(other) { + t.Fatalf("%s allowed although it is outside the exception set", other) + } + + // Removing an entry stops it bypassing, and leaves the rest allowed. + al.SetExceptions(prefixes(t, "203.0.113.10/32")) + if al.Contains(inRange) { + t.Fatalf("%s still allowed after its prefix was removed", inRange) + } + if !al.Contains(host) { + t.Fatalf("%s should still be allowed", host) + } +} + +// TestExceptionsBoundaries pins the range arithmetic the binary-search index +// rests on: the first and last address of a prefix are inside it, the addresses +// on either side are not, and a family never matches the other family's ranges. +func TestExceptionsBoundaries(t *testing.T) { + al := New() + al.SetExceptions(prefixes(t, "198.51.100.0/24", "2001:db8:1::/48")) + + in := []string{ + "198.51.100.0", "198.51.100.255", + "2001:db8:1::", "2001:db8:1:ffff:ffff:ffff:ffff:ffff", + } + out := []string{ + "198.51.99.255", "198.51.101.0", + "2001:db8:0:ffff:ffff:ffff:ffff:ffff", "2001:db8:2::", + } + for _, s := range in { + if !al.Contains(netip.MustParseAddr(s)) { + t.Errorf("%s should be inside the exception set", s) + } + } + for _, s := range out { + if al.Contains(netip.MustParseAddr(s)) { + t.Errorf("%s should be outside the exception set", s) + } + } +} + +// TestExceptionsOverlappingEntries covers an organization listing a network and +// an address within it, plus two adjacent networks - both of which the index +// merges, and neither of which may change what is allowed. +func TestExceptionsOverlappingEntries(t *testing.T) { + al := New() + al.SetExceptions(prefixes(t, + "198.51.100.0/24", "198.51.100.7/32", // contained + "203.0.113.0/25", "203.0.113.128/25", // adjacent halves + )) + + for _, s := range []string{"198.51.100.7", "198.51.100.200", "203.0.113.1", "203.0.113.200"} { + if !al.Contains(netip.MustParseAddr(s)) { + t.Errorf("%s should be allowed", s) + } + } + for _, s := range []string{"198.51.101.7", "203.0.114.1"} { + if al.Contains(netip.MustParseAddr(s)) { + t.Errorf("%s should not be allowed", s) + } + } +} + +func TestExceptionsIndependentOfDNSAllowlist(t *testing.T) { + al := New() + al.SetExceptions(prefixes(t, "203.0.113.0/24")) + + resolved := netip.MustParseAddr("192.0.2.5") + al.Add(resolved, "example.com", time.Minute) + + // Flush discards DNS-resolved IPs; exceptions are administrative and survive. + al.Flush() + if al.Contains(resolved) { + t.Fatalf("%s survived a flush", resolved) + } + if !al.Contains(netip.MustParseAddr("203.0.113.9")) { + t.Fatal("exception did not survive a flush of the DNS-resolved allowlist") + } + + // A removed exception is not resurrected by an unrelated DNS resolution. + al.SetExceptions(nil) + if al.Contains(netip.MustParseAddr("203.0.113.9")) { + t.Fatal("cleared exception still allowed") + } +} + +func TestExceptionsInStats(t *testing.T) { + al := New() + if got := al.Stats().ExceptionPrefixes; got != 0 { + t.Fatalf("ExceptionPrefixes = %d, want 0", got) + } + al.SetExceptions(prefixes(t, "203.0.113.0/24", "2001:db8::/48")) + if got := al.Stats().ExceptionPrefixes; got != 2 { + t.Fatalf("ExceptionPrefixes = %d, want 2", got) + } +} + +func prefixStrings(prefixes []netip.Prefix) []string { + out := make([]string, 0, len(prefixes)) + for _, prefix := range prefixes { + out = append(out, prefix.String()) + } + return out +} + +// benchExceptions builds a set of n distinct /24s plus one /32 the benchmark +// looks up, so lookups traverse the whole index rather than hitting an early +// entry. +func benchExceptions(n int) []netip.Prefix { + out := make([]netip.Prefix, 0, n) + for i := 0; i < n; i++ { + out = append(out, netip.MustParsePrefix(fmt.Sprintf("10.%d.%d.0/24", i/256, i%256))) + } + return out +} + +// BenchmarkContainsWithExceptions guards the hot path: Contains() is called per +// connection (per packet for embedders), so the cost of a large organization +// list must not scale with its length. +func BenchmarkContainsWithExceptions(b *testing.B) { + for _, n := range []int{0, 100, 2000} { + b.Run(fmt.Sprintf("prefixes=%d", n), func(b *testing.B) { + al := New() + if n > 0 { + al.SetExceptions(benchExceptions(n)) + } + ip := netip.MustParseAddr("203.0.113.10") // never in the set: worst case + b.ReportAllocs() + b.ResetTimer() + for i := 0; i < b.N; i++ { + al.Contains(ip) + } + }) + } +}