feat(groom): bwrap sandbox harness + key-broker with deterministic CI proofs (BE-4302)

.github/groom/README.md

@@ -172,3 +172,75 @@ python3 .github/groom/ledger.py --repo owner/name --check "<signature>"
 ```bash
 python3 -m unittest discover -s .github/groom/tests -p 'test_*.py' -v
 ```
+
+## The agent sandbox — `agent-sandbox.sh` + `broker.mjs` (BE-4302)
+
+The auto-builder (phase 3) runs an untrusted agent that writes code. These two
+trusted assets confine that agent so a prompt-injected or misbehaving run cannot
+read the runner's secrets, touch anything outside its clone, or exfiltrate the
+API key — while still letting it edit its worktree and reach Anthropic.
+
+- **[`agent-sandbox.sh`](agent-sandbox.sh)** — a [bubblewrap](https://github.com/containers/bubblewrap)
+  (`bwrap`) wrapper that runs an arbitrary command inside an unprivileged jail:
+
+  ```bash
+  agent-sandbox.sh --clone <path> --clone-mode ro|rw-git-ro --out-dir <path> \
+      [--ro-file <path> ...] [--env KEY=VALUE ...] -- <command...>
+  ```
+
+- **[`broker.mjs`](broker.mjs)** — a ~50-line node-stdlib reverse proxy
+  (`node broker.mjs <port>`) that holds the real key on the host and forwards the
+  jail's requests to it.
+
+### The sandbox contract (what the agent can and cannot see)
+
+| Surface | Inside the jail |
+|---|---|
+| `/usr`, `/etc` | read-only |
+| `/tmp`, `/home/agent` (`HOME`) | fresh tmpfs — host `/tmp` is **shadowed**, not shared |
+| the clone (`--clone`) | bound **at its real path**; `ro` = read-only, `rw-git-ro` = worktree writable but `.git` read-only |
+| explicit `--ro-file`s | read-only, at their real paths |
+| the out-dir (`--out-dir`) | the **only** writable host location (created on the host first) |
+| host `$HOME` / `$RUNNER_TEMP` / `$GITHUB_WORKSPACE` / other repos | **invisible** |
+| host process table | **invisible** (own pid namespace) |
+| environment | **cleared** — only `HOME`, `PATH`, `TERM`, and each `--env KEY=VALUE`; nothing inherited from the host |
+| network | shared (so the agent can reach the broker on loopback) |
+
+The `rw-git-ro` worktree write is exactly how the builder's patch is captured: the
+agent edits tracked files, the wrapper's caller reads them back on the host
+afterward, but the agent can never rewrite git history or `.git/config`.
+
+**The real API key never enters the jail.** The broker reads
+`ANTHROPIC_API_KEY` from *its own* (host) environment, **deletes** any inbound
+`x-api-key` / `authorization` header, injects the real key, and forwards only
+`/v1/*` paths to `api.anthropic.com` — streaming the response through unbuffered
+so SSE works. `GET /healthz` answers locally; anything not under `/v1/` is `404`.
+It listens on `127.0.0.1` only, refuses to start with an empty key, and logs one
+line per request — method + path + status, never headers or body.
+
+### The loud-preflight guarantee
+
+Before it runs anything, `agent-sandbox.sh` **proves the sandbox works or exits
+non-zero** — it will **never** fall back to running the command unsandboxed. The
+preflight installs `bubblewrap` if missing, installs an unconfined AppArmor
+profile for `bwrap` when the runner sets
+`kernel.apparmor_restrict_unprivileged_userns=1` (mirroring the runner image's own
+podman workaround), and self-tests a real `bwrap` invocation. If that still fails
+it drops the userns restriction and retests; if it *still* fails it emits
+`::error::bwrap sandbox unavailable …` and exits non-zero. A broken sandbox stops
+the run — it never silently degrades to no sandbox.
+
+### Tests — deterministic, no API spend
+
+[`tests/sandbox-tests.sh`](tests/sandbox-tests.sh) (run by the `sandbox-tests` job
+in [`test-groom-scripts.yml`](../workflows/test-groom-scripts.yml)) asserts every
+row of the contract above with `bash -c` as the sandboxed command — env scrub, FS
+confinement + tmpfs shadowing, both clone modes, pid isolation — and points the
+broker at a local fake upstream ([`tests/fake-upstream.mjs`](tests/fake-upstream.mjs))
+to prove key injection/stripping, the `/healthz` + non-`/v1` behavior, and SSE
+pass-through. No `claude`, no API key, no spend.
+
+```bash
+shellcheck -x .github/groom/agent-sandbox.sh .github/groom/tests/sandbox-tests.sh
+bash .github/groom/tests/sandbox-tests.sh   # Linux + unprivileged userns only
+```

.github/groom/agent-sandbox.sh

@@ -0,0 +1,199 @@
+#!/usr/bin/env bash
+#
+# agent-sandbox.sh — run an arbitrary command inside a bubblewrap (bwrap) jail.
+#
+# This is the confinement harness for the groom auto-builder's agent step
+# (BE-4302, phase 1). It gives an untrusted agent a network-connected shell that
+# can ONLY see: a read-only /usr + /etc, ephemeral /tmp + $HOME, the target clone
+# (read-only, or read-write worktree with a read-only .git), an explicit set of
+# read-only files, and one writable out-dir. Everything else on the host — other
+# repos, the runner's secrets, $HOME, $RUNNER_TEMP, $GITHUB_WORKSPACE, the host
+# process table — is invisible. The real API key never enters the jail; the agent
+# reaches Anthropic only through the broker (broker.mjs) on host loopback.
+#
+# Usage:
+#   agent-sandbox.sh --clone <path> --clone-mode ro|rw-git-ro --out-dir <path> \
+#       [--ro-file <path> ...] [--env KEY=VALUE ...] -- <command...>
+#
+# The preflight FAILS LOUD: if a working bwrap sandbox cannot be established on
+# this runner image, the script exits non-zero and the command is NEVER run. It
+# never falls back to running the command unsandboxed.
+
+set -euo pipefail
+
+die() {
+	echo "agent-sandbox: $*" >&2
+	exit 2
+}
+
+# The base mounts used by BOTH the preflight self-test and the real invocation,
+# minus the caller-supplied clone/ro-file/out-dir/env. `true` runs as the probe.
+selftest() {
+	bwrap \
+		--unshare-all --share-net \
+		--ro-bind /usr /usr \
+		--symlink usr/bin /bin \
+		--symlink usr/lib /lib \
+		--symlink usr/lib64 /lib64 \
+		--symlink usr/sbin /sbin \
+		--proc /proc \
+		--dev /dev \
+		--tmpfs /tmp \
+		true 2>/dev/null
+}
+
+# Establish a working unprivileged-userns bwrap sandbox or exit non-zero. Mirrors
+# the runner image's own podman AppArmor workaround
+# (actions/runner-images: images/ubuntu/scripts/build/install-container-tools.sh):
+# Ubuntu 23.10+ ships kernel.apparmor_restrict_unprivileged_userns=1, which blocks
+# the unprivileged user namespaces bwrap needs unless an unconfined AppArmor
+# profile is installed for /usr/bin/bwrap.
+preflight() {
+	# Fast path: already usable, do nothing (keeps repeated invocations quiet).
+	if command -v bwrap >/dev/null 2>&1 && selftest; then
+		return 0
+	fi
+
+	if ! command -v bwrap >/dev/null 2>&1; then
+		sudo apt-get install -y bubblewrap
+	fi
+
+	local restrict=/proc/sys/kernel/apparmor_restrict_unprivileged_userns
+	if [[ -r "$restrict" && "$(cat "$restrict")" == "1" ]]; then
+		sudo tee /etc/apparmor.d/bwrap >/dev/null <<'PROFILE'
+abi <abi/4.0>,
+include <tunables/global>
+profile bwrap /usr/bin/bwrap flags=(unconfined) {
+  userns,
+  include if exists <local/bwrap>
+}
+PROFILE
+		sudo apparmor_parser -r -W /etc/apparmor.d/bwrap || true
+	fi
+
+	if selftest; then
+		return 0
+	fi
+
+	# Last resort: drop the unprivileged-userns restriction outright and retest.
+	sudo sysctl -w kernel.apparmor_restrict_unprivileged_userns=0 || true
+	if selftest; then
+		return 0
+	fi
+
+	echo "::error::bwrap sandbox unavailable on this runner image — refusing to run the agent unsandboxed"
+	exit 1
+}
+
+main() {
+	local clone="" clone_mode="" out_dir=""
+	local ro_files=() envs=() cmd=()
+
+	while [[ $# -gt 0 ]]; do
+		case "$1" in
+			--clone) [[ $# -ge 2 ]] || die "--clone needs a value"; clone="$2"; shift 2 ;;
+			--clone-mode) [[ $# -ge 2 ]] || die "--clone-mode needs a value"; clone_mode="$2"; shift 2 ;;
+			--out-dir) [[ $# -ge 2 ]] || die "--out-dir needs a value"; out_dir="$2"; shift 2 ;;
+			--ro-file) [[ $# -ge 2 ]] || die "--ro-file needs a value"; ro_files+=("$2"); shift 2 ;;
+			--env) [[ $# -ge 2 ]] || die "--env needs a value"; envs+=("$2"); shift 2 ;;
+			--) shift; cmd=("$@"); break ;;
+			*) die "unknown argument: $1" ;;
+		esac
+	done
+
+	[[ -n "$clone" ]] || die "--clone is required"
+	[[ -n "$out_dir" ]] || die "--out-dir is required"
+	[[ ${#cmd[@]} -gt 0 ]] || die "a -- <command...> is required"
+	# bwrap binds each of these at its REAL path; a relative value would resolve
+	# against an unexpected CWD instead of failing loud, so require absolute paths.
+	[[ "$clone" = /* ]] || die "--clone must be an absolute path (got '$clone')"
+	[[ "$out_dir" = /* ]] || die "--out-dir must be an absolute path (got '$out_dir')"
+	case "$clone_mode" in
+		ro | rw-git-ro) ;;
+		*) die "--clone-mode must be 'ro' or 'rw-git-ro' (got '${clone_mode:-}')" ;;
+	esac
+	[[ -d "$clone" ]] || die "clone path is not a directory: $clone"
+
+	# out-dir must exist on the host before it can be bound rw into the jail; create
+	# it here so we can canonicalize it for the overlap check below.
+	mkdir -p "$out_dir"
+
+	# The writable out-dir is bound LAST, and bwrap's last-bind-wins ordering means
+	# an out-dir that overlaps the clone would shadow the read-only clone/.git
+	# mounts and silently make protected content (including .git under rw-git-ro)
+	# writable — defeating the read-only contract. Canonicalize both and require
+	# them to be disjoint (neither equal nor an ancestor of the other). Fail here,
+	# before the (slow) preflight, so a bad invocation is rejected fast.
+	local clone_real out_real
+	clone_real="$(realpath "$clone")" || die "cannot resolve --clone path: $clone"
+	out_real="$(realpath "$out_dir")" || die "cannot resolve --out-dir path: $out_dir"
+	if [[ "$out_real" == "$clone_real" || "$out_real" == "$clone_real"/* || "$clone_real" == "$out_real"/* ]]; then
+		die "--out-dir must not overlap --clone (out-dir '$out_real' vs clone '$clone_real'): a writable bind over the clone would defeat its read-only mounts"
+	fi
+
+	preflight
+
+	local bwrap_args=(
+		--unshare-all --share-net --die-with-parent --new-session --clearenv
+		--ro-bind /usr /usr
+		--symlink usr/bin /bin
+		--symlink usr/lib /lib
+		--symlink usr/lib64 /lib64
+		--symlink usr/sbin /sbin
+		--ro-bind /etc /etc
+		--proc /proc
+		--dev /dev
+		--tmpfs /tmp
+		--tmpfs /home/agent
+		--setenv HOME /home/agent
+		--setenv PATH /usr/local/bin:/usr/bin:/bin
+		--setenv TERM dumb
+	)
+
+	local e key val
+	if [[ ${#envs[@]} -gt 0 ]]; then
+		for e in "${envs[@]}"; do
+			[[ "$e" == *=* ]] || die "--env expects KEY=VALUE (got '$e')"
+			key="${e%%=*}"
+			val="${e#*=}"
+			bwrap_args+=(--setenv "$key" "$val")
+		done
+	fi
+
+	# The clone is bound AT ITS REAL PATH so tool output paths match the host.
+	case "$clone_mode" in
+		ro)
+			bwrap_args+=(--ro-bind "$clone" "$clone")
+			;;
+		rw-git-ro)
+			# Read-write worktree, but .git stays read-only: the agent may edit
+			# tracked files (the patch we capture) yet can never rewrite history
+			# or git config. Ordering matters — the rw clone bind first, then the
+			# ro .git overlay on top.
+			#
+			# This assumes .git is a real directory. In a git-worktree checkout it
+			# is instead a file holding a `gitdir:` pointer to metadata elsewhere on
+			# the host — which would NOT be mounted into the jail, silently breaking
+			# git rather than protecting it. Fail loud instead (callers pass plain
+			# clones; worktree checkouts are unsupported here).
+			[[ -d "$clone/.git" ]] || die "--clone-mode rw-git-ro needs a plain .git directory (got a gitdir pointer file — git worktree checkouts are unsupported): $clone/.git"
+			bwrap_args+=(--bind "$clone" "$clone" --ro-bind "$clone/.git" "$clone/.git")
+			;;
+	esac
+
+	local f
+	if [[ ${#ro_files[@]} -gt 0 ]]; then
+		for f in "${ro_files[@]}"; do
+			[[ "$f" = /* ]] || die "--ro-file must be an absolute path (got '$f')"
+			bwrap_args+=(--ro-bind "$f" "$f")
+		done
+	fi
+
+	bwrap_args+=(--bind "$out_dir" "$out_dir" --chdir "$clone")
+
+	# stdout/stderr pass through to the host shell; the caller redirects stdout
+	# on the HOST side to capture any exec JSON out of the agent's reach.
+	exec bwrap "${bwrap_args[@]}" -- "${cmd[@]}"
+}
+
+main "$@"

.github/groom/broker.mjs

@@ -0,0 +1,113 @@
+// broker.mjs — a zero-dependency reverse proxy that holds the real Anthropic key
+// so a sandboxed agent never sees it (BE-4302, phase 1).
+//
+//   node broker.mjs <port>
+//
+// The agent inside the bwrap jail (agent-sandbox.sh) talks to this broker on host
+// loopback with NO key of its own; the broker strips any inbound credential,
+// injects the real ANTHROPIC_API_KEY (read from its own env, never the jail's),
+// and forwards to api.anthropic.com. The key lives only in the host process; the
+// jail can spend against it but can never read it.
+//
+// Contract: listens on 127.0.0.1 only; forwards only /v1/* paths; deletes inbound
+// x-api-key / authorization before adding the real one; streams responses through
+// unbuffered so SSE works; logs method + path + status ONLY (never headers/body).
+//
+// BROKER_UPSTREAM_HOST / BROKER_UPSTREAM_PORT override the upstream target for
+// tests only; production leaves them unset and pins api.anthropic.com:443.
+
+import http from 'node:http';
+import https from 'node:https';
+
+const port = Number(process.argv[2]);
+if (!Number.isInteger(port) || port <= 0 || port > 65535) {
+  console.error('broker: usage: node broker.mjs <port>');
+  process.exit(1);
+}
+
+const KEY = process.env.ANTHROPIC_API_KEY;
+if (!KEY) {
+  console.error('broker: ANTHROPIC_API_KEY is empty — refusing to start');
+  process.exit(1);
+}
+
+const UPSTREAM_HOST = process.env.BROKER_UPSTREAM_HOST || 'api.anthropic.com';
+const UPSTREAM_PORT = Number(process.env.BROKER_UPSTREAM_PORT || 443);
+
+const server = http.createServer((req, res) => {
+  // Local health check — never touches upstream, never spends.
+  if (req.method === 'GET' && req.url === '/healthz') {
+    console.log(`${req.method} ${req.url} 200`);
+    res.writeHead(200, { 'content-type': 'text/plain' });
+    res.end('ok\n');
+    return;
+  }
+
+  // Only the Messages/Anthropic API surface is proxied; everything else is denied.
+  // Reject dot-segments too: a raw target like `/v1/../v2/foo` passes a bare
+  // prefix check yet normalizes to a non-/v1 route upstream, so fail it closed.
+  const reqPath = req.url.split('?')[0];
+  if (!req.url.startsWith('/v1/') || reqPath.includes('..')) {
+    console.log(`${req.method} ${req.url} 404`);
+    res.writeHead(404, { 'content-type': 'text/plain' });
+    res.end('not found\n');
+    return;
+  }
+
+  const headers = { ...req.headers };
+  delete headers['x-api-key'];
+  delete headers['authorization'];
+  headers.host = UPSTREAM_HOST;
+  headers['x-api-key'] = KEY;
+
+  const upstream = https.request(
+    { host: UPSTREAM_HOST, port: UPSTREAM_PORT, method: req.method, path: req.url, headers },
+    (upRes) => {
+      console.log(`${req.method} ${req.url} ${upRes.statusCode}`);
+      res.writeHead(upRes.statusCode, upRes.headers);
+      upRes.pipe(res); // stream through — no buffering, so SSE passes intact
+      // A mid-stream upstream failure (after headers) can't become a 502 anymore;
+      // handle its 'error' so it tears down the client response instead of
+      // bubbling to an uncaughtException that takes the whole broker down.
+      upRes.on('error', () => {
+        console.log(`${req.method} ${req.url} upstream-stream-error`);
+        res.destroy();
+      });
+    },
+  );
+
+  upstream.on('error', () => {
+    console.log(`${req.method} ${req.url} 502`);
+    if (!res.headersSent) {
+      res.writeHead(502, { 'content-type': 'text/plain' });
+      res.end('upstream error\n');
+    } else {
+      // Response already began streaming (routine for SSE): a 502 body can't be
+      // sent anymore, and appending 'upstream error' here would splice stray
+      // bytes into the middle of the real body. Tear it down instead so the
+      // client sees a truncated stream rather than a corrupted one.
+      res.destroy();
+    }
+  });
+
+  // Don't let a hung upstream pin a request open forever; tear it down on idle.
+  upstream.setTimeout(120_000, () => upstream.destroy(new Error('upstream timeout')));
+
+  // If the client goes away (aborted/dropped), stop talking to upstream so we
+  // don't leave a half-open request spending against the key.
+  res.on('close', () => {
+    if (!res.writableFinished) upstream.destroy();
+  });
+  // .pipe() forwards neither errors nor a listener onto the client response, so a
+  // client socket error mid-stream (ECONNRESET, write-after-close) would emit an
+  // unhandled 'error' on res and take the whole broker down. Absorb it and tear
+  // down the upstream leg — same crash-proofing as the upRes/upstream handlers.
+  res.on('error', () => upstream.destroy());
+  req.on('error', () => upstream.destroy());
+
+  req.pipe(upstream); // forward the request body streaming too
+});
+
+server.listen(port, '127.0.0.1', () => {
+  console.log(`broker listening on 127.0.0.1:${port} -> ${UPSTREAM_HOST}:${UPSTREAM_PORT}`);
+});