[PR #73] [MERGED] feat(groom): isolated netns sandbox — drop --share-net, broker over a bind-mounted unix socket (BE-4421) #83

New issue

Closed

opened 2026-10-26 22:36:02 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:36:02 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/73
Author: @mattmillerai
Created: 7/24/2026
Status: ✅ Merged
Merged: 8/3/2026
Merged by: @mattmillerai

Base: matt/be-4302-groom-sandbox ← Head: matt/be-4421-groom-sandbox-netns

📝 Commits (2)

c5f9300 feat(groom): isolated netns sandbox — drop --share-net, broker over a bind-mounted unix socket (BE-4421)
dc20444 fix(groom): harden UDS broker/shim against review panel findings (BE-4421)

📊 Changes

5 files changed (+281 additions, -47 deletions)

View changed files

📝 .github/groom/README.md (+31 -8)
📝 .github/groom/agent-sandbox.sh (+52 -11)
📝 .github/groom/broker.mjs (+68 -16)
➕ .github/groom/jail-shim.mjs (+44 -0)
📝 .github/groom/tests/sandbox-tests.sh (+86 -12)

📄 Description

STACKED — merging lands on matt/be-4302-groom-sandbox (owned by @mattmillerai, PR #66), NOT main. This is phase 2 on top of the still-open BE-4302 sandbox harness. Do not merge until #66 lands; GitHub auto-retargets this to main when #66 merges. Not "ready to merge" in the unqualified sense.

ELI-5

The groom auto-builder runs an untrusted agent in a locked box. Until now the box still had a working network cable, so a misbehaving agent could in principle phone home, hit the runner's own services, or read cloud metadata. This change cuts the cable entirely: the box gets its own empty network with only a loopback wire. The one thing the agent still needs — talking to Anthropic through the key-broker — comes in through a tiny door (a unix socket) we mount into the box, with a little in-box translator (jail-shim.mjs) so ordinary HTTP tooling can use it. Everything else off-box is now unreachable by construction, and we prove it with tests that show cloud metadata, an arbitrary internet IP, and the host's own loopback all fail to connect from inside.

What & why

Phase 2 of the groom sandbox hardening (design from spike BE-4369: namespace isolation by construction — option b; slirp4netns/nftables/broker-in-jail alternatives were rejected). Removes --share-net so the jail runs in an isolated network namespace with only lo up — no external hosts, no host-loopback services, no cloud metadata (169.254.169.254 / 168.63.129.16) — while keeping the key-broker reachable over a unix-domain socket bind-mounted into the jail.

broker.mjs — now accepts a unix-socket path (absolute) as argv[2] in addition to the legacy numeric TCP port. UDS mode clears a stale socket then server.listen(path); a relative path is refused loud. The request-handling contract (strip/inject key, /v1/* only, /healthz, SSE, crash-proofing) is transport-independent and unchanged.
jail-shim.mjs (new, ~20 lines, node stdlib) — a TCP→UDS forwarder run inside the jail (node jail-shim.mjs <port> /run/broker.sock) so agent tooling speaking HTTP to a 127.0.0.1:<port> base URL reaches the bind-mounted socket (an isolated netns can't dial a host TCP port).
agent-sandbox.sh — drops --share-net from both bwrap invocations (selftest + bwrap_args); adds --uds <host-socket-path> (at most once, absolute, must already be a listening socket) that rw-binds the socket file at the fixed in-jail path /run/broker.sock (connect(2) needs write on the inode; --ro-bind would EROFS). Omit --uds for a fully-offline jail.
tests/sandbox-tests.sh — section 5 reworked onto the UDS+shim path (broker listens on a socket; in-jail shim bridges curl to it; all existing assertions unchanged). New section 7 = the BE-4369 acceptance proofs: host loopback, cloud metadata, and an arbitrary external IP (1.1.1.1) are all unreachable from the jail, plus --uds fail-loud on a nonexistent socket.
README.md — isolated-netns contract row, UDS mode + shim, and the offline consequences (set CLAUDE_CODE_DISABLE_NONESSENTIAL_TRAFFIC=1; in-jail git fetch/npm install cannot work — everything must be in the clone before sandboxing).

Testing

shellcheck -x .github/groom/agent-sandbox.sh .github/groom/tests/sandbox-tests.sh — clean.
The diff contains no remaining --share-net anywhere under .github/groom/.
python3 -m unittest (groom ledger, untouched) — 48 passing (sanity).
The full bash sandbox-tests.sh requires Linux + unprivileged user namespaces (bubblewrap), so — exactly like the phase-1 BE-4302 suite — it runs on the sandbox-tests CI job on ubuntu-latest, not on the macOS dev host. To de-risk the transport-independent JS on the host I ran a smoke test proving the real composition end-to-end: broker on a UDS + jail-shim.mjs bridging TCP→UDS → key injected, caller dummy stripped, /healthz 200, non-/v1 404, SSE frames + [DONE] stream through; plus TCP back-compat, relative-path refusal, and jail-shim arg validation all behave. The netns egress assertions rest on the spike-verified facts (bubblewrap brings up lo in the fresh netns; empty-netns connects fail immediate ENETUNREACH).

Negative-claim falsification (per the coder self-review)

This change's user-facing outcome denies a capability (network egress from the jail) — the trigger for the falsification check. It is not the #581 anti-pattern: cutting egress is the deliberate BE-4369 security goal, and the one capability the agent legitimately needs — reaching Anthropic — is preserved and empirically proven working via the sanctioned UDS+shim path (see the host smoke above: real key forwarded, SSE intact). The "unreachable" assertions target the security boundary (metadata / external IP / host loopback), which is deterministic by netns construction, not a mistaken claim that a needed feature is unavailable.

Out of scope

The groom caller workflow (epic BE-3870) that composes broker + shim + agent in production doesn't exist yet; this PR defines and test-proves the composition pattern the caller will copy.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/github-workflows/pull/73 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 7/24/2026 **Status:** ✅ Merged **Merged:** 8/3/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `matt/be-4302-groom-sandbox` ← **Head:** `matt/be-4421-groom-sandbox-netns` --- ### 📝 Commits (2) - [`c5f9300`](https://github.com/Comfy-Org/github-workflows/commit/c5f93004c6495b5fa184f3ad2f2bae7f539ecde9) feat(groom): isolated netns sandbox — drop --share-net, broker over a bind-mounted unix socket (BE-4421) - [`dc20444`](https://github.com/Comfy-Org/github-workflows/commit/dc204441feb44646c2ebe50a9bebb4630b0c8f7e) fix(groom): harden UDS broker/shim against review panel findings (BE-4421) ### 📊 Changes **5 files changed** (+281 additions, -47 deletions) <details> <summary>View changed files</summary> 📝 `.github/groom/README.md` (+31 -8) 📝 `.github/groom/agent-sandbox.sh` (+52 -11) 📝 `.github/groom/broker.mjs` (+68 -16) ➕ `.github/groom/jail-shim.mjs` (+44 -0) 📝 `.github/groom/tests/sandbox-tests.sh` (+86 -12) </details> ### 📄 Description > **STACKED — merging lands on `matt/be-4302-groom-sandbox` (owned by @mattmillerai, PR #66), NOT `main`.** This is phase 2 on top of the still-open BE-4302 sandbox harness. Do not merge until #66 lands; GitHub auto-retargets this to `main` when #66 merges. Not "ready to merge" in the unqualified sense. ## ELI-5 The groom auto-builder runs an untrusted agent in a locked box. Until now the box still had a working network cable, so a misbehaving agent could in principle phone home, hit the runner's own services, or read cloud metadata. This change cuts the cable entirely: the box gets its own empty network with only a loopback wire. The one thing the agent still needs — talking to Anthropic through the key-broker — comes in through a tiny door (a unix socket) we mount into the box, with a little in-box translator (`jail-shim.mjs`) so ordinary HTTP tooling can use it. Everything else off-box is now unreachable *by construction*, and we prove it with tests that show cloud metadata, an arbitrary internet IP, and the host's own loopback all fail to connect from inside. ## What & why Phase 2 of the groom sandbox hardening (design from spike **BE-4369**: namespace isolation by construction — option b; slirp4netns/nftables/broker-in-jail alternatives were rejected). Removes `--share-net` so the jail runs in an isolated network namespace with only `lo` up — **no external hosts, no host-loopback services, no cloud metadata (169.254.169.254 / 168.63.129.16)** — while keeping the key-broker reachable over a unix-domain socket bind-mounted into the jail. - **`broker.mjs`** — now accepts a unix-socket path (absolute) as `argv[2]` in addition to the legacy numeric TCP port. UDS mode clears a stale socket then `server.listen(path)`; a relative path is refused loud. The request-handling contract (strip/inject key, `/v1/*` only, `/healthz`, SSE, crash-proofing) is transport-independent and unchanged. - **`jail-shim.mjs`** (new, ~20 lines, node stdlib) — a TCP→UDS forwarder run *inside* the jail (`node jail-shim.mjs <port> /run/broker.sock`) so agent tooling speaking HTTP to a `127.0.0.1:<port>` base URL reaches the bind-mounted socket (an isolated netns can't dial a host TCP port). - **`agent-sandbox.sh`** — drops `--share-net` from both bwrap invocations (`selftest` + `bwrap_args`); adds `--uds <host-socket-path>` (at most once, absolute, must already be a listening socket) that **rw**-binds the socket file at the fixed in-jail path `/run/broker.sock` (`connect(2)` needs write on the inode; `--ro-bind` would EROFS). Omit `--uds` for a fully-offline jail. - **`tests/sandbox-tests.sh`** — section 5 reworked onto the UDS+shim path (broker listens on a socket; in-jail shim bridges curl to it; all existing assertions unchanged). New section 7 = the BE-4369 acceptance proofs: host loopback, cloud metadata, and an arbitrary external IP (`1.1.1.1`) are all unreachable from the jail, plus `--uds` fail-loud on a nonexistent socket. - **`README.md`** — isolated-netns contract row, UDS mode + shim, and the offline consequences (set `CLAUDE_CODE_DISABLE_NONESSENTIAL_TRAFFIC=1`; in-jail `git fetch`/`npm install` cannot work — everything must be in the clone before sandboxing). ## Testing - `shellcheck -x .github/groom/agent-sandbox.sh .github/groom/tests/sandbox-tests.sh` — **clean**. - The diff contains **no remaining `--share-net`** anywhere under `.github/groom/`. - `python3 -m unittest` (groom ledger, untouched) — **48 passing** (sanity). - The full `bash sandbox-tests.sh` requires Linux + unprivileged user namespaces (bubblewrap), so — exactly like the phase-1 BE-4302 suite — it runs on the `sandbox-tests` CI job on `ubuntu-latest`, not on the macOS dev host. To de-risk the transport-independent JS on the host I ran a smoke test proving the real composition end-to-end: broker on a UDS + `jail-shim.mjs` bridging TCP→UDS → **key injected, caller dummy stripped, `/healthz` 200, non-`/v1` 404, SSE frames + `[DONE]` stream through**; plus TCP back-compat, relative-path refusal, and `jail-shim` arg validation all behave. The netns egress assertions rest on the spike-verified facts (bubblewrap brings up `lo` in the fresh netns; empty-netns connects fail immediate `ENETUNREACH`). ## Negative-claim falsification (per the coder self-review) This change's user-facing outcome *denies* a capability (network egress from the jail) — the trigger for the falsification check. It is **not** the #581 anti-pattern: cutting egress is the deliberate BE-4369 security goal, and the one capability the agent legitimately needs — reaching Anthropic — is **preserved and empirically proven working** via the sanctioned UDS+shim path (see the host smoke above: real key forwarded, SSE intact). The "unreachable" assertions target the security boundary (metadata / external IP / host loopback), which is deterministic by netns construction, not a mistaken claim that a needed feature is unavailable. ## Out of scope The groom caller workflow (epic BE-3870) that composes broker + shim + agent in production doesn't exist yet; this PR defines and test-proves the composition pattern the caller will copy. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:36:02 +00:00