[PR #290] [MERGED] feat(groom): authenticate agent steps through the local key broker #292

New issue

Closed

opened 2026-10-26 22:38:09 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:38:09 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/290
Author: @mattmillerai
Created: 9/16/2026
Status: ✅ Merged
Merged: 9/16/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-4420-groom-key-broker

📝 Commits (3)

659e49c feat(groom): authenticate agent steps through the local key broker
5abf1fe fix(groom): require the launched broker to own port 8199 before agents run
c437acd docs(groom): correct the broker-step comment on where the real key lives

📊 Changes

2 files changed (+202 additions, -37 deletions)

View changed files

📝 .github/groom/README.md (+9 -2)
📝 .github/workflows/groom.yml (+193 -35)

📄 Description

ELI-5

Groom's three AI agents (finder, verifier, builder) used to run with the real Anthropic API key sitting right in their environment. Because a groom agent reads untrusted repo content and can cat files, a prompt-injected agent that dumped its whole environment could leak that key. This PR moves the key out: each agent job now starts a tiny loopback proxy (the key broker, already merged) that holds the real key, and the agent runs with only a fake key plus a base-URL pointed at the proxy. If the agent dumps its environment now, all it leaks is the fake string groom-broker-dummy and http://127.0.0.1:8199.

What changed

.github/workflows/groom.yml, in all three agent jobs (audit_find, audit_verify, build):

New Start API key broker step right after Install Claude Code. It feeds the real key to key-broker.mjs over stdin (via env -u, never env/argv, so it can't be read back out of /proc/<pid>/{environ,cmdline}), backgrounds it, and waits (up to ~10s) for 127.0.0.1:8199 to accept a connection before the job proceeds — failing the job loudly if the broker never binds.
The agent step env no longer carries secrets.ANTHROPIC_API_KEY. It now sets a fixed ANTHROPIC_API_KEY: groom-broker-dummy (the CLI requires a non-empty key) plus ANTHROPIC_BASE_URL: http://127.0.0.1:8199, so auth flows through the broker, which swaps in the real x-api-key on the way to api.anthropic.com.
The finder/verifier literal-key output scans moved into their own if: always() steps (Scan finder output for leaked key / Scan verifier output for leaked key). They were guarded by [ -n "$ANTHROPIC_API_KEY" ], so with the real key gone from the agent step they would have silently no-op'd; now they run in a step that holds the key in its own env (no agent runs there, so no exfil surface added). The builder's existing Capture patch scan already worked this way and is unchanged.
Comment/doc updates — the header security-boundary block, the finder --bare auth note, the "ONLY the model key" env comments, the sandbox security-posture note, and .github/groom/README.md's key-broker.mjs wiring paragraph — record that the broker is the key-exfil close and the literal scans are now defence in depth.

Security posture

The key now lives only in the broker-start step's env and the broker process heap. The three claude-invoking steps (Run finder/verifier/builder) carry no usable credential. If the broker dies mid-run the CLI gets connection-refused and the job fails closed — there is no fallback that puts the key back in the agent env. Widening groom's caller set beyond trusted-default-branch repos still wants a holistic review before flipping.

Acceptance grep — grep -n 'secrets.ANTHROPIC_API_KEY' .github/workflows/groom.yml now returns 7 lines: the caller-example comment, the three Start API key broker steps, and the three scan steps (finder, verifier, builder patch-capture). Zero are inside a step that invokes claude. (The workflows_call secret declaration is ANTHROPIC_API_KEY: and is retained; it isn't a secrets. reference so it doesn't match the grep.)

Provenance

Authored by: agent-work loop
Verified: actionlint 1.7.12 with shellcheck — no new findings vs the origin/main baseline (16 pre-existing job.workflow_sha false-positives, the documented BE-4169 carve-out; 0 other findings before or after); python3 -m unittest discover -s .github/groom/tests: 380 passed; check_workflow_pins.py: OK; check_agents_md.py --root .: passed; groom.yml parses as valid YAML.
Deviations: the ticket's broker-start snippet used for i in $(seq 1 50), which shellcheck flags SC2034 (the counter is never referenced); changed to for _ in in all three copies — behaviour is identical. Sandbox comments that mention "the model key in this step's env" as a motivating example were left untouched, per the ticket's instruction that the --bare / chmod / env -u sandbox stays exactly as is (the arbitrary-command-execution threat they guard is unchanged by the broker).

Residual

Live pilot NOT run (no dispatch access, and it spends real API budget). The ticket's acceptance names a post-merge step: dispatch ci-groom.yml (dry-run parity pilot) and confirm the finder completes a normal run through the broker. That is the only end-to-end proof that the pinned CLI 2.1.217 honors ANTHROPIC_BASE_URL and accepts the dummy key against the broker — the unit tests (test_key_broker.py, 380-test suite) prove the proxy's own request handling but not the CLI's base-URL behaviour. A human should run that pilot after merge and confirm the finder resolves turns (not auth-starved / no connection-refused).
Broker readiness check is a bare TCP port-connect, matching the ticket snippet. key-broker.mjs's own author recommends keying the wait on the stdout readiness line (groom-key-broker listening on 127.0.0.1:8199 in /tmp/groom-broker.log) instead: a foreign process already holding 8199 would pass a bare connect check while the broker exits EADDRINUSE. On ephemeral CI runners 8199 is effectively never pre-held, so the risk is low, but matching the readiness line would be a strict hardening worth a follow-up.

_{🔄 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/290 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/16/2026 **Status:** ✅ Merged **Merged:** 9/16/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-4420-groom-key-broker` --- ### 📝 Commits (3) - [`659e49c`](https://github.com/Comfy-Org/github-workflows/commit/659e49cdea3f7bea7afb070605864d35bc08c123) feat(groom): authenticate agent steps through the local key broker - [`5abf1fe`](https://github.com/Comfy-Org/github-workflows/commit/5abf1fea89bb7206624039757e04153be12146e9) fix(groom): require the launched broker to own port 8199 before agents run - [`c437acd`](https://github.com/Comfy-Org/github-workflows/commit/c437acd87bed0ab0ff99efa2dc8d688b74f58929) docs(groom): correct the broker-step comment on where the real key lives ### 📊 Changes **2 files changed** (+202 additions, -37 deletions) <details> <summary>View changed files</summary> 📝 `.github/groom/README.md` (+9 -2) 📝 `.github/workflows/groom.yml` (+193 -35) </details> ### 📄 Description ## ELI-5 Groom's three AI agents (finder, verifier, builder) used to run with the real Anthropic API key sitting right in their environment. Because a groom agent reads untrusted repo content and can `cat` files, a prompt-injected agent that dumped its whole environment could leak that key. This PR moves the key out: each agent job now starts a tiny loopback proxy (the key broker, already merged) that holds the real key, and the agent runs with only a fake key plus a base-URL pointed at the proxy. If the agent dumps its environment now, all it leaks is the fake string `groom-broker-dummy` and `http://127.0.0.1:8199`. ## What changed `.github/workflows/groom.yml`, in all three agent jobs (`audit_find`, `audit_verify`, `build`): - **New `Start API key broker` step** right after `Install Claude Code`. It feeds the real key to `key-broker.mjs` over **stdin** (via `env -u`, never env/argv, so it can't be read back out of `/proc/<pid>/{environ,cmdline}`), backgrounds it, and waits (up to ~10s) for `127.0.0.1:8199` to accept a connection before the job proceeds — failing the job loudly if the broker never binds. - **The agent step env no longer carries `secrets.ANTHROPIC_API_KEY`.** It now sets a fixed `ANTHROPIC_API_KEY: groom-broker-dummy` (the CLI requires a non-empty key) plus `ANTHROPIC_BASE_URL: http://127.0.0.1:8199`, so auth flows through the broker, which swaps in the real `x-api-key` on the way to `api.anthropic.com`. - **The finder/verifier literal-key output scans moved into their own `if: always()` steps** (`Scan finder output for leaked key` / `Scan verifier output for leaked key`). They were guarded by `[ -n "$ANTHROPIC_API_KEY" ]`, so with the real key gone from the agent step they would have silently no-op'd; now they run in a step that holds the key in its own env (no agent runs there, so no exfil surface added). The builder's existing `Capture patch` scan already worked this way and is unchanged. - **Comment/doc updates** — the header security-boundary block, the finder `--bare` auth note, the "ONLY the model key" env comments, the sandbox security-posture note, and `.github/groom/README.md`'s `key-broker.mjs` wiring paragraph — record that the broker is the key-exfil close and the literal scans are now defence in depth. ## Security posture The key now lives only in the broker-start step's env and the broker process heap. The three `claude`-invoking steps (`Run finder/verifier/builder`) carry no usable credential. If the broker dies mid-run the CLI gets connection-refused and the job fails closed — there is no fallback that puts the key back in the agent env. Widening groom's caller set beyond trusted-default-branch repos still wants a holistic review before flipping. **Acceptance grep** — `grep -n 'secrets.ANTHROPIC_API_KEY' .github/workflows/groom.yml` now returns 7 lines: the caller-example comment, the three `Start API key broker` steps, and the three scan steps (finder, verifier, builder patch-capture). **Zero are inside a step that invokes `claude`.** (The `workflows_call` secret *declaration* is `ANTHROPIC_API_KEY:` and is retained; it isn't a `secrets.` reference so it doesn't match the grep.) ## Provenance - **Authored by:** agent-work loop - **Verified:** actionlint 1.7.12 with shellcheck — no new findings vs the origin/main baseline (16 pre-existing `job.workflow_sha` false-positives, the documented BE-4169 carve-out; 0 other findings before or after); `python3 -m unittest discover -s .github/groom/tests`: 380 passed; `check_workflow_pins.py`: OK; `check_agents_md.py --root .`: passed; groom.yml parses as valid YAML. - **Deviations:** the ticket's broker-start snippet used `for i in $(seq 1 50)`, which shellcheck flags SC2034 (the counter is never referenced); changed to `for _ in` in all three copies — behaviour is identical. Sandbox comments that mention "the model key in this step's env" as a motivating example were left untouched, per the ticket's instruction that the `--bare` / chmod / `env -u` sandbox stays exactly as is (the arbitrary-command-execution threat they guard is unchanged by the broker). ## Residual - **Live pilot NOT run (no dispatch access, and it spends real API budget).** The ticket's acceptance names a post-merge step: dispatch `ci-groom.yml` (dry-run parity pilot) and confirm the finder completes a normal run through the broker. That is the only end-to-end proof that the pinned CLI 2.1.217 honors `ANTHROPIC_BASE_URL` and accepts the dummy key against the broker — the unit tests (`test_key_broker.py`, 380-test suite) prove the proxy's own request handling but not the CLI's base-URL behaviour. A human should run that pilot after merge and confirm the finder resolves turns (not auth-starved / no connection-refused). - **Broker readiness check is a bare TCP port-connect**, matching the ticket snippet. `key-broker.mjs`'s own author recommends keying the wait on the stdout readiness line (`groom-key-broker listening on 127.0.0.1:8199` in `/tmp/groom-broker.log`) instead: a foreign process already holding 8199 would pass a bare connect check while the broker exits `EADDRINUSE`. On ephemeral CI runners 8199 is effectively never pre-held, so the risk is low, but matching the readiness line would be a strict hardening worth a follow-up. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:38:09 +00:00