diff --git a/agents/duckworth/work/row210/REVIEW-r1.md b/agents/duckworth/work/row210/REVIEW-r1.md new file mode 100644 index 000000000..afb474420 --- /dev/null +++ b/agents/duckworth/work/row210/REVIEW-r1.md @@ -0,0 +1,103 @@ +# Review: Gizmoduck row 210 (#1707) DESIGN.md r1, agent host access with sudo (duckworth-0042) + +Reviewer Duckworth (Opus 5.5), 2026-10-20. Subject: agents/gizmoduck/work/queue-210/DESIGN.md at 7b10bc0ff +on gizmoduck/row210, blob 1514a36d96 (checked), base refactor 9ba31b5b3, per gizmoduck-0041. Against the +brief docs/plans/2026-10-20_host-credentials.md (9db578ff5), REQ-CRED-3 in Honker's 1.7 draft r2 (approved +duckworth-0039), row 199 design r6 (4d3007ec9) and row 199's code on origin/megavolt/row199 at addee664a. +- I checked E1-E11 at 9ba31b5b3: credentials.mjs:22, :84-87, :95-103, :104-117, host.mjs:31 and :270, + broker.mjs:145-147, :790-797 and :1490-1532, server.mjs:87-95, business.mjs:89, vocabulary.mjs:101-103, + sandbox.mjs:29-31, instance-settings.mjs:27. They hold. E12 is stale, see N1. +- 199's `useFor` at addee664a (credentials.mjs:228-256) passes a BusError through and maps anything else to + `service-failed`, so D5's own codes (`host-key-mismatch`, `credential-leak`) reach the caller as designed. +- I ran nothing. The probe in section 2 is the author's, and I didn't repeat it. + +## Verdict: changes, B1-B4 + +What holds: +- The four grant layers, checked on every call, meet REQ-CRED-3's "a grant exists for that agent and that + host before the credential is used". The agent never names another agent: the key comes from the cap + through `useFor`. +- No agent and not the chief can write layers 2-4. `hosts` is outside ORGANIZATION_KEYS and INSTANCE_KEYS, + and `instances.set` refuses credentialed definitions. +- `host.sudo` lands as a hard wait under REQ-WAIT-3, and T25 enforces REQ-WAIT-2. +- The key handoff keeps the key off disk, argv and env (T19). `-F /dev/null`, a bus-written known_hosts + under `HostKeyAlias` and `StrictHostKeyChecking=yes` give no trust on first use. Forwarding is off and + `restrict` is on the host side. +- The command is built from argv with POSIX single quotes and `sudo -n --`, never taken as a string. +- The executor runs in the bus child as a broker.sock verb. The 10 s serial IPC (E5) is a sound reason not + to use a host op. It matches where 199 put the push (199 BUILD.md deviation 11), see N1. +- The brief's two named mutants are there: pin skip (M12, M13) and key leak (M15, M16). +- Section 6 states the limits plainly, including that `"any"` is root and can outlive the grant. + +## Blockers + +B1. The event log doesn't say what ran as root. D5 step 6 records `action.allowed` with "host, argv hash, +stdin size and hash", and T22 checks that the bodies carry hashes. After routing, a within-role call has no +decision, so no record anywhere in the stack names the command, only its sha256. The existing verbs log the +thing done in clear: `requestWeb` records the url or query (broker.mjs:1521), and 1.7's push line says +"Every push is logged with the remote, the ref, the commit and, when one was given, the decision" (draft +:105). argv has already passed `#secretCheck` (step 1), so it holds no known secret, and it's capped at +32 KiB. The design gives no reason to hash it. Fix: record `host`, `user` and `argv` in clear in the +`requested` event, and keep output as sizes and hashes. Change T22 to assert argv is present and stdout, +stderr and the key are not, and M24's partner: a mutant that drops argv from the event. + +B2. A pre-routing decision binds stdin that Jason never sees. D4 binds `stdinSha256`, and the question +shows only "the host id and the argv". `stdin` can be 64 KiB (D6). So `argv: ["bash"]` with a script on +stdin shows Jason the question "… bash", and he approves a script he can't read. Binding a hash he can't +check doesn't give him a decision about it. Fix, either: +- the question shows stdin in full when present (it passed `#secretCheck`), or +- a gated call refuses non-empty stdin, and stdin is allowed only for a routed within-role call. +Add a T9 case for the chosen rule and a mutant that drops it. + +B3. If the bus child dies, the key stays loaded and the call can't be revoked. D7 step 5 cleans up in a +`finally`, and T18 covers success, refusal, timeout and a thrown error. A bus child that is SIGKILLed or +crashes never runs the `finally`. `ssh-agent -D` has no lifetime of its own, so it keeps the key loaded on +its socket until the machine reboots. A detached ssh keeps running the remote command. After the restart +the new bus's kill map is empty, so D9's "revoke also kills a `host.run` in flight" no longer holds for +that call. 199's push leans on git's own time limit for the same case (deviation 11, "git's own time limit +ends it"). ssh-agent has no such limit unless one is given. Fix: +- `ssh-add -t `, so the agent drops the key on its own. +- At bus start, sweep `/secrets/run/`: kill what each leftover dir's recorded pids name (check + they are still that ssh-agent or ssh first) and remove the dir. +- Consider `setpriv --pdeathsig SIGKILL` around ssh-agent and ssh, which the sandbox already uses + (sandbox.mjs:25-27). Probe it from node first. +Add a test that SIGKILLs the bus child mid-call and then asserts the key is gone from the agent by the +bound and the next start leaves no run dir. Add a mutant that drops `-t`. + +B4. Nothing routes section 4 to Jason. REQ-CRED-3 says "Who grants a host credential, and how, is set by +the #424 design, which Jason approves", and section 4 ends "The table above is that answer". Section 10's +only Jason question, Q-J1, is about the first grants at the live window. My approval of the design doesn't +stand in for his. Fix: add a Q-J0 to section 10, sent through Mos before the build lands: approve the +section 4 table. Name where his answer is recorded. + +## Notes (do not block) + +N1. 199 is on origin now: origin/megavolt/row199 at addee664a carries code and BUILD.md. The header's "199 +has no code on origin yet" and E12 are stale. The bus child already spawns git there: runtime.mjs:245 calls +`publishRemote`, which runs `runGit` (seat/src/checkout.mjs:95-138) as a detached group leader with +SIGTERM, a grace period and SIGKILL to the group. Reuse that shape, or the helper, for ssh rather than +writing a second one. Q-S2: deviation 11 keeps the token and the push in the bus child, and the host side +only polls. D5's assumption holds, and since the agent reaches broker.sock directly, 210 needs no poll. +Sage confirms. +N2. Section 6 should say that a registry entry for the stack's own machine, or any host where the stack's +`` lives, gives the agent with `"any"` every agent's keys and tokens, not just root there. +Consider refusing a loopback `address` at load. +N3. D5 says every spawn is `detached: true`, so each gets its own group. D7 step 3 says ssh-agent runs "in +the same process group". Say which group ssh-agent is in, and make sure the timeout and revoke kill reach +both the agent and ssh. +N4. Prefix lists plus stdin: `[["tee"]]` or `[["dd"]]` writes any file as root from stdin. D6 already says +a prefix list limits mistakes, not a determined agent. Say in the doc that stdin goes to the command, so +some prefixes are root on their own. +N5. Pass the user with `-l ` and the address as its own argument rather than `@
`. +Then nothing depends on how ssh splits on `@`, or on an IPv6 literal. +N6. D8 is defense in depth. The remote never holds the private key, and forwarding is off, so output can +carry the key only if the agent already had it. It's still right to keep. Also note that a `host.*` entry +in the shared token map makes today's `assertClean` loop over a multi-line value that never matches +(E3), which is harmless. +N7. My #423 r5 condition (duckworth-0035) applies. If ops-01 changes step 1 to fit this design, for +example per-agent `from=…,restrict` lines for localadmin, that changed step 1 comes to me as a diff before +it runs. That's the live part, not this row. +N8. Q-S1 (route `host.sudo` in 1.7) is Sage's and Jason's call. Without it, B2's rule matters on every +call. + +A round with only B1-B4 and any notes taken gets a diff check, not a full re-read. diff --git a/docs/SESSIONS.md b/docs/SESSIONS.md index f0befa781..a95a5106c 100644 --- a/docs/SESSIONS.md +++ b/docs/SESSIONS.md @@ -1395,3 +1395,4 @@ are never rewritten or removed; corrections are new entries. 2026-10-20T20:16Z | Duckworth (Opus 5.5) | PRD 1.7 draft r2 diff check, honker/row194 11cb0423b sha256 e896746d | approve (duckworth-0039), B1-B2 resolved 2026-10-20T20:18Z | Duckworth (Opus 5.5) | row 202 FED R5 round 1 manifest review, quackerjack/fed-r5 030809e15 manifest 959bfb275 | changes B1 (duckworth-0040), BusinessError from loadAccess/loadBusiness escapes uncaught (probed), N1-N9, 18/18 single-file 2026-10-20T20:24Z | Duckworth (Opus 5.5) | row 202 FED R5 round 2, Quackerjack candidate 48a3fb96e manifest 7fd265033 | approve (duckworth-0041), B1 resolved, 21/21 single-file, N1-N4 +2026-10-20T20:34Z | Duckworth (Opus 5.5) | row 210 (#1707) DESIGN.md r1, Gizmoduck 7b10bc0ff | changes B1-B4 (duckworth-0042): argv not in event log, decision stdin unseen, orphan ssh-agent after bus death, section 4 not routed to Jason; N1-N8