test(groom): prove name resolution is dead in the sandbox, non-vacuously - #300
Open
mattmillerai wants to merge 4 commits into
Open
mattmillerai wants to merge 4 commits into
mattmillerai wants to merge 4 commits into
Conversation
The egress-isolation proofs in sandbox-tests.sh (7a/7b/7c) all dial IP
literals on purpose, so not one of them ever touches the resolver: they
prove routing is dead and leave name resolution untested. Add that leg.
The obvious one-liner ("DNS lookup fails") is a trap, and measuring it
says so: on a systemd-resolved host /etc/resolv.conf is a symlink into
/run, and the jail mounts /etc but deliberately not /run, so the resolver
has no nameserver configured whatever the netns looks like. A bare
resolution-failure check therefore stays GREEN under --share-net and
would not notice a future change that put the jail back on a shared
network. So assert both halves: resolution fails (on the resolver-
specific exit codes getent 2 / curl 6, which a missing binary's 127
cannot satisfy), AND a hardcoded nameserver address is unroutable (curl
7, CURLE_COULDNT_CONNECT — a reachable netns returns 52/56/28 here,
never 7). Verified against a shared-netns negative control, which the
combined check correctly fails.
Renumbers the --uds fail-loud check 7d -> 7e; nothing references it.
mattmillerai
marked this pull request as ready for review
September 16, 2026 20:15
|
Warning Review paused — included plan limit reachedKeep your review moving with free on-demand reviews.
On-demand reviews are free for the next 23 days.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Promotion and pricing detailsOn-demand reviews are free for the next 23 days. After that, they cost $0.25 per reviewed file. Review limit detailsOr wait 16 minutes for your next included review. Limit details: You’ve used the included review currently available. Your 127 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour. 📝 WalkthroughWalkthroughThe sandbox contract and tests now verify DNS failure and network isolation. Tests distinguish unavailable host tools or DNS from actual sandbox failures and report skipped checks. ChangesSandbox isolation validation
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The sandbox validation retains non-vacuous DNS and network-isolation checks, with no actionable merge risk identified. 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
mattmillerai
added
agent-coded
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 5 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 1 |
| 🟢 Low | 3 |
| ⚪ Nit | 1 |
Panel: 6/6 reviewers contributed findings.
…rl exit code Review found the netns half of §7d rested on an invariant that does not hold: `curl` 7 is CURLE_COULDNT_CONNECT, which equally covers ECONNREFUSED, ENETUNREACH and a firewall REJECT, so a FULLY SHARED netns on a filtered or offline host returns 7 as well — the exact-7 assertion went green on precisely the confinement regression it existed to catch. It was also TCP-only, saying nothing about a routable UDP/53 path. Key (ii) on the netns itself instead, read out of the jail's fresh `--proc /proc` mount: `/proc/net/dev` must list `lo` and nothing else, and `/proc/net/route` must carry no default route. Those facts cannot be faked by host network state, need no egress of any kind, and cover any protocol — no route is no route. The 1.1.1.1:53 probe stays as behavioral confirmation but is relaxed to "did not succeed"; its exact code is no longer load-bearing. Also from review: - the host-side resolvability control now GATES part (i) instead of printing a note on both branches: when the host cannot resolve the name either, part (i) is reported SKIPPED rather than counted as proof (it still runs and must hold). - bound that host-side `getent` with `timeout 5`, so a blackholed resolver cannot stall a suite documented as hermetic in a job with no `timeout-minutes`. - correct the resolver wording in the (ii) comment and the README: with no readable `nameserver` line glibc falls back to 127.0.0.1 per resolv.conf(5), and the jail's `lo` carries all of 127.0.0.0/8 — so a resolver IS configured and routable in there, and lookups fail only because nothing listens on the jail's 127.0.0.1:53. Noted concretely because the jail already runs in-jail loopback listeners, so a future bind to :53 would become the agent's resolver. - soften the `--max-time 10` comment: it is a backstop, not margin over a retrying resolver (glibc timeout:5 attempts:2 burns exactly 10s on one blackholing nameserver). Verified: positive control (real isolated netns) exit 0; negative control (identical block, `--share-net`) exit 1, naming ens4/docker0/tailscale0. The old exact-7 assertion was separately shown false-passable — a fully shared, fully routable netns returns 7 on any closed port. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
mattmillerai
added
cursor-review
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Round 2 — ledger: 5 prior finding(s) across 1 round(s) (0 never answered).
Found 9 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 1 |
| 🟢 Low | 6 |
| ⚪ Nit | 2 |
Panel: 5/6 reviewers contributed findings.
Reviewers that did not contribute: gpt-5.6-sol-max:edge-case (error)
…2 gaps Round 2 of the panel, all nine findings: - MEDIUM: on a host whose OWN netns is empty (a developer running this suite in a `--network=none` container) the interface/route facts are identical whether the jail unshares the netns or shares the host's, so a wrapper that stopped unsharing would still print PASS. Assert netns IDENTITY first — `readlink /proc/self/ns/net` in the jail must differ from the host's, passed in via `--env HOST_NETNS`. That discriminates on every host; the emptiness checks stay as the substantive claim (separate is not the same as empty). - bound the in-jail `getent` with `timeout 5`, matching its host-side twin: on the regression this section exists to catch, glibc would otherwise burn timeout x attempts x nameservers in a job with no `timeout-minutes`, and the run would die at the curl-6 assertion with a clipped 28 before part (ii) printed its precise diagnostic. A clipped lookup exits 124 and reds `= 2` loudly. - both loops must now OBSERVE their table rather than merely find no offending row: `saw_lo` for /proc/net/dev and a header check for /proc/net/route, so an empty or reformatted file cannot pass having read nothing. - also scan `/proc/net/ipv6_route` for a `::/0` default. The v4 FIB cannot see an IPv6 default, which contradicted the "no route for any protocol" claim. Count one only if it exits via a real device: every netns carries kernel-installed `unreachable` v6 defaults (RTF_REJECT, device lo). - drop the 1.1.1.1:53 probe. It could never go red — an HTTP GET to a DNS port cannot return 0 even on a routable netns (a DNS-over-TCP listener reads the request as a length-prefixed message and closes, giving 52/56) — and `rc = 0` was the only failing condition, so it spent 5s advertising an untested fact. This section's own standard: a proof that cannot go red is not a proof. - add `--noproxy "*"` to both in-jail curls: an exported http_proxy/HTTPS_PROXY/ ALL_PROXY would retarget curl at the proxy, exit 5/7, and red a correctly isolated jail. - correct the `--proc /proc` explanation. What makes these files jail-local is that /proc/net is a magic link to /proc/self/net, resolved against the READING task's netns — not the wrapper's `--proc` flag. As written it taught that part (ii) was coupled to that mount. - README: the "First" clause reintroduced the same mechanism error for `127.0.0.53`, which is inside the `127.0.0.0/8` the jail lo carries and so is routable in there. It contradicted the next two sentences and undercut the in-jail-bind hazard; the paragraph now names `127.0.0.53:53` explicitly. - qualify the final banner as `ALL SANDBOX TESTS PASSED (N skipped)` when part (i) is downgraded, so reduced coverage survives to the summary line. The existing prefix is preserved for anything grepping it. Verified: shellcheck clean; §1-§4 + §7a-§7e PASS locally (§5/§6 need node under /usr, pre-existing). Controls re-run after the rekey: real jail exit 0; `--share-net` jail exit 1 on the identity check, `jail netns id net:[...] is the HOST net namespace — the wrapper is not unsharing the netns at all`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Resolve conflict in sandbox-tests.sh: keep main's new section 8 (--preflight-only tests, BE-14756) and this branch's skip-count-aware final PASS message.
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
2 participants
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.
ELI-5
The groom agent runs in a locked box with no way onto the network — the only thing it can reach is a unix socket that proxies to the API. We already had tests proving it can't dial any IP address. But every one of those tests dials a raw IP on purpose, so none of them ever asked the box to look up a name. This adds that test. The interesting part is that the obvious version of it is a lie: it passes even when the box is fully connected to the network, so it would never catch a regression. This adds the version that actually goes red.
What this is
The confinement work (isolated netns, unix-socket broker, in-jail TCP→UDS bridge, all three groom agent steps wired to it) already landed. Reading the tree against the acceptance criteria, everything was in place except one leg of the proof, which is what this PR adds.
Criterion-by-criterion, against the current default branch:
broker.mjstakes a socket path asargv[2]--share-netfor a full isolated netns, broker socket bind-mounted in, TCP→UDS bridge soANTHROPIC_BASE_URLkeeps workingagent-sandbox.sh --uds+--unshare-all, bridged byjail-shim.mjsThe finding: the obvious DNS check is vacuous
My first draft was the one-liner — resolve a name in the jail, assert it fails. I ran it against a shared-netns control before trusting it, and it passed there too.
The reason is that
/etc/resolv.confis a symlink into/run, and the sandbox mounts/etcread-only but deliberately does not mount/run(mounting it would be a confinement regression in its own right). So the resolver has no nameserver configured no matter what the network namespace looks like. A bare "DNS fails" assertion is therefore green whether the jail is isolated or wide open — it could never catch someone putting the sandbox back on a shared network, which is precisely the regression it would exist to catch.So §7d asserts both halves:
Resolution fails, on resolver-specific exit codes rather than a bare non-zero:
getent2 is "key not found" (a missing binary is 127) andcurl6 isCURLE_COULDNT_RESOLVE_HOST(a connect failure is 7, a timeout 28, a missing binary 127). This is the same false-pass class the existing §7a/7b/7c tool-presence guard was written for, closed here by construction instead. Part (i) is only evidence about the sandbox if the name is live off-jail, so a host-side control (bounded bytimeout 5) gates it: when this host cannot resolve the name either, the assertions still run and still have to hold, but part (i) is reportedSKIPrather than counted as proof.The netns is empty — read out of the jail's own
/proc. The wrapper passes--proc /proc, a fresh procfs mount inside the new netns, so/proc/net/devand/proc/net/routedescribe the jail's network: the first must listloand nothing else, the second must carry no default route. This is the half only an isolated netns can produce, and it is what makes the section a real proof.Review's first pass at this keyed part 2 on an exact
curl7 to a hardcoded nameserver address instead, and that was wrong — see the thread on §7d.curl7 isCURLE_COULDNT_CONNECT, which equally covers ECONNREFUSED, ENETUNREACH and a firewall REJECT, so a fully shared netns on a filtered or offline host returns 7 too and the assertion went green on exactly the regression it exists to catch; it was TCP-only besides. The/procfacts cannot be faked by host network state, need no egress of any kind, and cover a UDP/53 path as well as TCP — no route is no route, for any protocol. The 1.1.1.1:53 probe is kept as behavioral confirmation but relaxed to "did not succeed"; its exact code is no longer load-bearing.Verification
Run on a Linux host with unprivileged user namespaces, against the real
agent-sandbox.sh:shellcheck -x .github/groom/*.sh .github/groom/tests/*.sh— clean.python3 -m unittest discover -s .github/groom/tests -p 'test_*.py'— 398 tests, OK.python3 .github/workflow-pins/check_workflow_pins.py— 11 workflows, 0 defaults, all ref checkouts guarded.python3 .github/agents-md-integrity/check_agents_md.py --root .— passed (2 pre-existing warnings: AGENTS.md is 155 lines vs the 150 aspirational target, and no CODEOWNERS; both predate this branch).sandbox-tests.sh§1–§4, §7a–§7e — all PASS, including §7d.Empirical falsification of the new negative assertion. This diff asserts a capability is absent, which is exactly the claim a self-authored test cannot establish on its own, so I went and attempted the capability rather than assuming it:
getentpath,curl's own resolver, and a direct connect to a nameserver address. All three fail; none of them is reachable by any route. The jail's own/proc/net/devlistsloalone and its/proc/net/routeis empty, so there is no route for any protocol to take.--share-net. It correctly goes red:jail netns has non-loopback interface(s): ens4 docker0 tailscale0 — the netns is SHARED with the host, so nothing here proves isolation.curl-7 assertion was separately shown false-passable, which is why part 2 was rekeyed: in a fully shared, fully routable netns, a connect to any closed port returns 7 (ECONNREFUSED). Exit 7 therefore does not imply "no route", and an assertion demanding it cannot distinguish an isolated netns from a shared one.--unshare-allexits 0.Judgment calls
socatfor the in-jail bridge and--net-bridge//run/groom/broker.sockfor the flag and mount path. What shipped usesjail-shim.mjs(node, already present — noapt-get install), the flag--uds, and/run/broker.sock. Those choices are not mine and are not changed here; I mention them only so the plan's wording isn't read as unimplemented.ci-groom.yml. That reference is stale — thesandbox-testsjob lives intest-groom-scripts.yml, path-filtered on.github/groom/**, so the new section is picked up with no workflow edit.Skip-caller-bump: truetrailer. This PR is a test file plus a README, neither of which a consumer ever loads at run time, so the caller bump it will trigger is a no-op SHA churn. But the trailer is documented for comment/docs-only edits and a test file is not literally that, and the guidance is to leave it off when in doubt. Left off deliberately; a reviewer who reads it as docs-only can add it.--max-timeon the in-jail probes is a backstop, not margin. The original comment oversold it; corrected in review. It is not sized to outlast a retrying resolver — glibc's defaults (timeout:5, attempts:2) burn exactly 10s on one blackholing nameserver — but every failure here is immediate in practice, and a clipped lookup exits 28 and fails loud with the code printed rather than passing silently.Residual
CLAUDE_CODE_DISABLE_NONESSENTIAL_TRAFFIC=1— the suppression the criterion names as the expected mitigation — and that the netns has been isolated on the default branch since the phase that shipped it, so any such stall would already be observable in existing groom run history rather than being introduced by this PR. Someone with access to a groom run should still time one dry-run against a recent pre-isolation run and confirm per-turn latency is unchanged; if it is not, the mitigation to investigate is residual non-API traffic from the CLI timing out with no route, and the documented fallback is to stop-ship the isolation and keep the earlier phases as the baseline.sandbox-tests.sh§5 and §6 were not run on my host and are unchanged by this diff. They neednoderesolvable under/usr(the jail'sPATHis/usr/local/bin:/usr/bin:/bin); on my hostnodelives outside/usr, so both fail there. I confirmed this is pre-existing and not caused by this branch by running the unmodified suite from the base commit and getting the identical failure at the identical point. CI runners havenodeunder/usr/local/bin, which is inside the read-only/usrbind, so both sections exercise normally there — but I have not observed them pass on this branch, only on CI's behalf.Part 2 of §7d can itself become vacuous on a host with no egress at all.Resolved in review. This was real and worse than stated — it applied to any filtered host, not just a fully offline one. Part 2 no longer depends on host reachability at all: it reads the jail's interface list and route table out of/proc, which no host network state can fake.Provenance
--share-netjail, exit 1, naming ens4/docker0/tailscale0); the superseded exact-curl-7 form was separately demonstrated false-passable in a shared, routable netns (exit 7 on any closed port), which is the measurement behind the review rekey.agent-sandbox.sh)