fix(network): enforce the allowlist at DNS resolution instead of pinning IPs#603
Open
mensfeld wants to merge 11 commits into
Open
fix(network): enforce the allowlist at DNS resolution instead of pinning IPs#603mensfeld wants to merge 11 commits into
mensfeld wants to merge 11 commits into
Conversation
…ing IPs Allowlist mode resolved `allowed_domains` on the host, pinned the resulting addresses into nft, and relied on the container later resolving those same names to the same addresses. It does not. For any domain behind a rotating pool of frontend addresses — every Google and AWS API endpoint — the container resolves independently and gets a different member of the pool, hits the default reject, and the agent dies mid-task with "Unable to connect", healing seconds later when a retry happens to land on an address that was pinned. COI is now the container's resolver. A prerouting DNAT redirects all port-53 traffic to it regardless of what /etc/resolv.conf says, and an answer's addresses are installed into the container's nft set BEFORE the answer is returned. The container cannot be told about an address the firewall does not already trust, so the race has nowhere to live. Also fixed, all in the same area: - Wildcards resolved the wrong domain. `*.googleapis.com` was handled by stripping the "*." and resolving the BASE domain, whose addresses have zero overlap with the regional endpoints actually dialled (googleapis.com -> 142.250.130.x, us-central1-aiplatform.googleapis.com -> 172.217.112-119.4). Matching now happens on the name being queried. - A partial-label wildcard like `*-aiplatform.googleapis.com` was not recognised, was passed to DNS verbatim, failed to resolve, and was then dropped from the allowlist WITHOUT stopping — a firewall that quietly did not cover what the config asked for. It is now rejected at startup. - The refresh had a fail-closed window that blocked exactly the addresses it was adding: ReplaceAllowlist appended new accept rules behind the still-present default reject, then deleted stale handles one `sudo nft` exec at a time. Rules now reference nft named sets and never change; updates are set-element operations the kernel applies atomically. - DNS-learned addresses carry a TTL+grace timeout, so an address that rotates out of DNS does not strand an in-flight connection. Public resolvers (8.8.8.8, 1.1.1.1) are dropped from the default allowed_domains: they were an escape hatch for resolving names COI never saw, and interception makes them pointless. Covered by Go unit tests, Go integration tests against the real nft binary and a real resolver, and a tests/network e2e suite that asserts the core invariant on a live container: every address the container is handed is already in the firewall.
…and orphan cleanup DeleteCOIFilterRulesForIP is the shared by-IP teardown path — kill, orphan cleanup and setup's stale-rule purge all go through it — but it only removed forward rules. Allowlist mode also creates two nft sets and a DNAT redirect, so both leaked: sets accumulated on the host across sessions, and a stale intercept would black-hole DNS for whatever container was assigned that IP next. It now removes rules, then sets, then the intercept. Rules must go first (the kernel refuses to drop a set a rule still references) and the intercept last. Containers that never had sets (open/restricted mode) are unaffected: removing something that was never created reports "No such file or directory", which teardown treats as success. NftManager.RemoveRules now delegates here so there is one implementation rather than two that can drift.
IncusExecQuietContext discarded stderr, so any failure surfaced as a bare "exit status 1" with the cause thrown away. `coi kill` could only report "Failed to delete <container>: exit status 1", which tells neither a user nor a CI log what actually went wrong. Quiet should mean "do not write to the terminal", not "hide the reason it failed". Stderr is now captured and attached to the returned error; callers that want silence still get it, callers that hit an error now get something they can act on.
… live
Replaces the in-process DNS proxy with a design that has nothing left to die.
The proxy was wrong for a reason no amount of polish would fix: it was a
goroutine in the coi process, but the container outlives that process.
`coi shell --background` creates the tmux session inside the container and
returns immediately, and even an interactive session ends on a tmux detach. The
proxy's socket died with it while the DNAT rule survived, so every DNS query was
redirected to a listener that no longer existed and the container was left with
no name resolution at all. Making it survive would have meant COI owning a
supervised, long-lived host daemon — startup, reaping, reboot recovery, and a new
way to be broken.
None of that is necessary. The original bug was never "we resolve on the host";
it was that the host and the container resolved INDEPENDENTLY and got different
members of a rotating address pool. Remove the independence and the bug is gone:
- COI resolves the allowlisted hostnames on the host.
- It installs those addresses in the container's nft set.
- It writes THE SAME addresses into the container's /etc/hosts.
- It blocks all DNS egress.
The hosts file is now the container's only route from a name to an address, and
it holds exactly what the firewall holds. The container cannot reach an address
the firewall was not given, because there is nowhere else for an address to come
from. Nothing has to keep running for that to hold: it survives coi exiting, a
tmux detach, or a kill -9. Stale-but-consistent still connects; it was
fresh-but-divergent that broke.
Blocking DNS takes two chains, and missing either leaves the hole open: forward
(for an off-box resolver like 8.8.8.8, rejected BEFORE the set rules so a
resolver inside an allowlisted CIDR is not reachable on port 53) and input (for
the bridge's own dnsmasq, whose traffic is addressed to the host and therefore
never traverses the forward chain at all).
Root inside the container can overwrite /etc/hosts. That costs it its own name
resolution and gains it nothing — the firewall is still the boundary, so the
failure is closed and self-inflicted.
Wildcards are rejected rather than quietly mishandled. There is no answer to write
for a subdomain nobody has asked for yet, so "*.googleapis.com" cannot be honoured
under a resolve-up-front design. The old code pretended otherwise: it stripped the
"*." and resolved the BASE domain, whose addresses have zero overlap with the
endpoints actually dialled. Users now get an error naming what to write instead.
Also fixed here:
- the monitor built its allowed-CIDR list from raw allowed_domains, so a
wildcard made every legitimate connection look like an "Unauthorized
connection attempt" at ThreatLevelHigh — pausing the container mid-task under
auto_pause_on_high. It now compiles through the same policy as the firewall.
- AllowDynamicIPs released its lock before the nft exec, so a concurrent caller
could see an address as installed and act on it while the exec was still in
flight. The lock is now held across the exec, and a test with 8 concurrent
callers fails against the old code (7 of 8 raced).
- the bookkeeping map is pruned instead of growing without bound.
…te the assertions DNS-blocking made obsolete
Two separate things, both surfaced by CI.
1. The new e2e tests were reading an always-empty stream. `coi container exec`
routes the command's stdout to ITS stderr (IncusExecContext sets
cmd.Stdout = os.Stderr), so reading result.stdout returned "" for everything —
`cat /etc/hosts` came back empty, `ip route` came back empty, and every
assertion built on them was vacuous rather than passing. The helper now merges
both streams and says why, so nobody reintroduces it.
2. Two pre-existing tests asserted behaviour the new design deliberately removes:
- test_allowlist_dns_resolution asserted that a container in allowlist mode can
query 8.8.8.8. It cannot, by design: DNS is blocked so the container has no
way to learn an address the firewall was not given. Having 8.8.8.8 in
allowed_domains makes the ADDRESS reachable, not port 53. It now asserts the
query is refused.
- test_allowlist_blocks_non_allowed_domains accepted only "connection refused"
or a timeout as evidence of blocking. A blocked domain now fails earlier and
more cleanly: with no /etc/hosts entry and no DNS, curl cannot resolve it at
all (exit 6) and never reaches a connection attempt. Resolution failure is
now accepted alongside the other two.
…dout on a tuple An earlier bulk edit missed this block (ruff had reformatted it), so test_dns_is_blocked_entirely raised AttributeError instead of asserting. The rest of the lane went green: 80 passed, this was the only failure.
…lure
`coi kill` raced the very cleanup its own stop triggers. Stopping a container ends
the coi session that owns it, and that session deletes its own ephemeral
container — so kill checked the instance existed, stopped it, cleaned up its
firewall rules, and by the time it issued the delete the instance was gone. Incus
answered "Instance not found", which kill reported as
Warning: Failed to delete coi-...: exit status 1
No containers were killed
and exited non-zero, about a container it had in fact just killed.
An instance that is already absent now counts as killed, since that is precisely
the outcome kill exists to produce. Genuine failures — a busy storage volume, an
unreachable daemon — are still reported; the helper is deliberately narrow and a
unit test pins both directions.
This is pre-existing, but this branch exposed it reliably: allowlist teardown does
more work between the stop and the delete (input-chain rules and nft sets, not
just forward rules), which widened the window the race needed. It was invisible
until the previous commit stopped IncusExecQuiet from discarding stderr — until
then all anyone saw was "exit status 1".
These asserted 'exit 0 or 1' and printed only stderr, so a failure said 'exit 2' and nothing else — no way to tell which of ~34 checks broke. CI is currently reporting exactly that, and the report itself is the only thing that names the culprit. Both streams are now included.
The allowlist design moved from an in-process DNS proxy to a static /etc/hosts + DNS-block model (commit 2f7d9ae), but several doc comments still described the proxy that no longer exists — a maintainer following them would hunt for a dnsintercept.go / DNSProxy that was deleted. Corrected to describe what actually ships: - setupAllowlist: resolve-on-host, write /etc/hosts, block DNS; no "see dnsintercept.go", no proxy/redirect. - gateway IP is required for the DHCP accept rule, not a proxy listener. - dynMu / AllowDynamicIPs guard against the setup vs. refresher race, not concurrent proxy queries. - ReplaceAllowlist and dynAllower reference the resolver/refresher path instead of DNSProxy; noted why ReplaceAllowlist stays on the interface. - teardown removes the "input-chain DNS block", not a "DNS intercept". Comments and two log/error strings only; no behaviour change. go build, go vet, and go test ./internal/network/... stay green.
…sher renews them
Dynamic nft set elements were sized at ttl + 10min grace, but the ONLY thing
that re-adds them is the background refresher. Two realistic configs let them
expire while /etc/hosts still points at the address — the container then resolves
an allowlisted name and the connection is rejected, i.e. the exact "Unable to
connect" failure this branch exists to remove:
- refresh_interval_minutes = 0 (in the old design this pinned a permanent
allowlist): startRefresher returns early, nothing re-adds the elements, and
every DNS-learned address drops out of the firewall 10 minutes after setup —
permanently.
- GetMinTTL() == 0 (every name resolved via the net.LookupIP fallback records
TTL 0): the refresh interval falls back to the 30-min config cap while
elements live only 10 min, a ~20-min hole each cycle. A single TTL-0 domain
alongside longer-TTL ones hits the same gap.
elementTimeout now takes the refresh interval and guarantees the element always
outlives it: lifetime = ttl + refreshInterval + grace, and the 12h cap can never
fall below one refresh-plus-grace. When refresh is disabled the element is
installed permanently (nft timeout omitted; dynSeen records the zero time so it
is neither pruned nor renewed), restoring the old "refresh off = pinned" behaviour.
The interval is computed by Manager.dynamicElementLifetimeInterval, which mirrors
startRefresher exactly, so the timeout is sized to the cadence that will actually
renew it. AllowDynamicIPs / the dynAllower interface / the stub gain the interval
argument.
New nftset_test.go pins the invariant (element > interval; permanent when
disabled; the TTL-0-at-30min regression). Real-kernel integration tests updated
to pass an enabled interval and still pass.
Also addresses review cleanups in the same area:
- schema: refresh_interval_minutes description no longer references the removed
"DNS proxy"; documents the =0 pin-at-startup behaviour.
- container: IsNotFoundErr matches the exact "instance not found" phrase instead
of a loose "not found" AND "instance", so an unrelated failure that mentions an
instance is no longer miscounted as a successful kill; drops the redundant
(subsumed) first clause. Removes a duplicated IncusExecQuietContext doc line.
- network/hosts: rewrite the /etc/hosts block strip with awk that removes only a
well-formed BEGIN..END block. A sed range deletes to EOF when the END marker is
missing, which would take the user's own entries with it; the awk re-emits an
unterminated block untouched. Replaces regexpEscapeForSed with shellSingleQuote.
- network/resolver: drop the vestigial `effectiveDomain := domain` left from the
deleted wildcard-rewrite branch.
…t mode Allowlist mode hedged both ways on the gateway: setupAllowlist aborted when it could not be detected, yet ApplyAllowlist still wrapped the DHCP accept rule in `if f.gatewayIP != ""`, as if it were optional. One layer said mandatory, the other said optional. Commit to mandatory, which is the correct posture for a default-reject mode: DHCP lease renewal has to be explicitly allowed, or the lease eventually lapses and the container loses the IP every rule is keyed on. ApplyAllowlist now returns an error up front when the gateway is empty and installs the DHCP rule unconditionally; the `if != ""` guard is gone. Both real callers already pass a validated gateway, so this only removes a dead branch and makes the requirement explicit at the point that consumes it. Restricted mode is unchanged: it is default-allow, so a missing gateway rule is harmless there and warn-and-continue stays correct. Unit test pins that ApplyAllowlist rejects an empty gateway (guard runs before any nft call, so no sudo needed).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
Allowlist mode resolved
allowed_domainson the host, pinned the resulting addresses into nft, and relied on the container later resolving those same names to the same addresses.It doesn't. For any domain behind a rotating pool of frontend addresses — which is every Google and AWS API endpoint — the container resolves independently and gets a different member of the same pool. That address was never pinned, so it hits the default
reject, and the agent dies mid-task withUnable to connect. It then heals on its own a few seconds later, when a retry happens to land on an address that was pinned.This is why Claude-via-Vertex and Claude-via-Bedrock users hit it constantly while people on the direct API essentially never did:
api.anthropic.comresolves to a single stable address, so there's nothing to rotate.The fix
Remove the independence between the two resolvers instead of trying to keep them in sync. There is now a single source of truth for name→address, and it is the same on both sides:
/etc/hosts.With no route to a nameserver, the
/etc/hostsblock COI wrote is the container's only way to turn a name into an address — and it holds exactly what the firewall holds. The container cannot resolve a name to an address the firewall was not given, because there is nowhere else for an address to come from. Stale-but-consistent still connects; it was fresh-but-divergent that broke.Nothing has to keep running for that to hold. The hosts file and the nft set are both already in place and already agree, so the guarantee survives
coiexiting, the user detaching from tmux, or the process beingkill -9'd. (An earlier iteration of this branch used an in-process DNS proxy; it was removed precisely because the container outlives thecoiprocess —coi shell --backgroundreturns immediately — so the proxy's socket died while its redirect rule survived, black-holing DNS. The static-hosts design has nothing left to die.)Blocking DNS takes two nft chains, and missing either leaves the hole open:
8.8.8.8) is routed through the host, so the forward chain sees it. The reject sits before the allowlist set rules, so a resolver that happens to sit inside an allowlisted CIDR is still not reachable on port 53.Root inside the container can overwrite
/etc/hosts, but that costs it its own name resolution and gains it nothing: the firewall is still the boundary, so an address it invents is an address the firewall rejects. The failure is closed and self-inflicted.On coexisting with manual
/etc/hostseditsCOI does not own the whole file — it manages only the region between two marker comments:
Each write deletes that marked block and re-appends a fresh one; everything outside the markers is preserved untouched —
localhost, the container's own hostname, and any entries a user or the agent added themselves. The file is rewritten in place (not swapped) so the inode, ownership, and permissions survive for resolvers holding it open. So a hand-added entry outside the block survives every refresh; an edit inside the block is overwritten on the next one (as the marker says).The important caveat is that in allowlist mode a hosts entry only resolves a name — it does not grant access.
/etc/hostsanswers "what address is this name?"; the firewall independently decides "may I connect to that address?". So hand-adding1.2.3.4 myhostmakes the name resolve but the connection is still rejected unless1.2.3.4is in the allowlist. This is the same property that makes in-container root overwriting the file harmless. The supported way to add a destination is thereforeallowed_domainsin config, which is the single action that updates both sides — the firewall set and the managed hosts block — at once; editing/etc/hostsby hand resolves but never connects (and, since the container is ephemeral, does not persist anyway).Also fixed, same area
Wildcards are now rejected at startup instead of silently mishandled.
*.googleapis.comwas previously handled by stripping the*.and resolving the base domain, whose addresses have zero overlap with the regional endpoints actually dialled (googleapis.com→142.250.130.x;us-central1-aiplatform.googleapis.com→172.217.112–119.4). The result was a firewall permitting addresses nothing would ever connect to. A resolve-up-front design has no answer to write for a subdomain nobody has queried yet, so wildcards can't be honoured — they now fail loudly with an error naming what to write instead (exact hostnames, or the provider's published CIDR ranges). A partial-label form like*-aiplatform.googleapis.com— which previously wasn't even recognised as a wildcard, was passed to DNS verbatim, failed, and was dropped without stopping setup — is caught by the same check.The monitor's allow-list is now compiled through the same policy as the firewall. It previously fed
allowed_domainsto the resolver raw, so a wildcard was resolved as its base domain and the monitor held addresses the container never dials. Every legitimate connection then looked like an "Unauthorized connection attempt" atThreatLevelHigh— which, underauto_pause_on_high, pauses the container mid-task.The refresh had a fail-closed window that blocked exactly the addresses it was adding. The old
ReplaceAllowlistappended new accept rules behind the still-present default reject, then deleted stale handles onesudo nftexec at a time. Rules now reference nft named sets and never change; updates are set-element operations the kernel applies atomically. This also collapses ~2-rules-per-address into 4 rules total.Rotated-out addresses no longer strand live connections. DNS-learned set elements carry a TTL + 10-min grace timeout instead of being evicted on the next refresh, so an address that drops out of DNS keeps working long enough for an in-flight request or a reconnect.
A concurrency race in
AllowDynamicIPsis closed. It previously recorded addresses as installed and released its lock before thenftexec, so a concurrent caller could act on an address the kernel set didn't contain yet. The lock is now held across the exec; a test with 8 concurrent callers fails against the old code.8.8.8.8/1.1.1.1dropped from the defaultallowed_domains— they were an escape hatch for resolving names COI never saw; blocking DNS makes them pointless.coi killno longer reports failure for a container it just killed. Stopping an ephemeral container ends the session that owns it, which deletes the container;killthen raced its own cleanup and got "Instance not found" from the delete, exiting non-zero. An already-absent instance now counts as killed. (Pre-existing, but this branch widened the stop→delete window — allowlist teardown does more work — and made it reliably reproducible.)IncusExecQuietContextnow surfaces stderr on failure instead of discarding it, socoi killand CI logs report why a quiet command failed rather than a bareexit status 1.Verification
/etc/hostsblock rendering, refresh/TTL scheduling.nftbinary (nftset_integration_test.go) — set + rule lifecycle, DNS blocked in both chains (forward and input),AllowDynamicIPsidempotency, the concurrency invariant (concurrent callers never treat an address as installed before thenftexec returns), andDeleteCOIFilterRulesForIPremoving rules + input block + sets cleanly.tests/network/test_allowlist_dns_enforcement.py(new, runs in the existingnetworkCI lane) — asserts the core invariant on a live container against a rotating frontend (oauth2.googleapis.com): every address the container resolves via/etc/hostsis already in the firewall; the managed/etc/hostsblock carries every rotating address without clobbering the rest of the file; DNS is refused (dig @8.8.8.8fails); hijacking/etc/resolv.confto8.8.8.8gains nothing (allowlisted names still resolve from/etc/hosts, an unlisted name does not); and a wildcard entry fails the launch loudly instead of starting a half-covered container.Locally green:
go build,go vet,golangci-lint(0 issues), fullgo test ./...,ruff check/ruff format,check-ci-coverage.py.Reviewer notes
networklane's e2e tests are the real-world check on the design.nsswitch.confconsultingfiles(/etc/hosts) beforedns, and on applications resolving viagetaddrinfo. That covers curl, git, Node (dns.lookup/undici), Go, and python-requests — the overwhelming majority of real traffic. Anything that talks port 53 directly (dig, c-aresdns.resolve, a custom resolver) gets blocked rather than falling back to hosts — fail-closed, but the app breaks instead of degrading. Worth keeping in mind if a future base image ships an unusual nsswitch ordering.inputhook. On a host with a restrictive INPUT policy the interaction is different from the forward path; it still fails closed. A startup probe that resolves one name from inside the container and aborts with a clear message would make a misconfiguration legible — suggested as a follow-up rather than added untested here.