feat(agent): add keyless broker runtime - #6967
Conversation
🔐 Codex Security Review
|
a101b80 to
1087586
Compare
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Two credential-handling defects make the keyless boundary unsafe, and one request-bound bug makes a compromised broker unnecessarily expensive to survive:
-
High:
session/newwrites the broker bearer credential to ACP debug logs. Broker mode placesBUZZ_BROKER_CREDENTIALin the MCP server env (crates/buzz-acp/src/lib.rs:5231-5248), whichsession_new_fullembeds underparams.mcpServers(crates/buzz-acp/src/acp.rs:699-724). Genericsend_requestthen logs the original message atacp::wire=debug(acp.rs:1145-1166). The new redactor only protects observer telemetry inwrite_ndjsonand the separate prompt path, so it does not protect this log. Apply the redactor to every outbound wire diagnostic, especiallysend_request, and pinsession/newwith a log-capture regression test. -
High: loopback HTTP can send the bearer through a system proxy in plaintext.
HttpBrokerClientpermitshttp://localhostand loopback literals, but its default reqwest client is created without.no_proxy()(crates/buzz-broker-client/src/lib.rs:52-95). Reqwest enables system/environment proxy discovery by default, so a machine withHTTP_PROXY/ALL_PROXYand no matchingNO_PROXYroutes the request, including theAuthorization: Bearerheader at lines 134-143, through that proxy. For plaintext loopback, structurally force a proxy-free client (and preventwith_clientfrom weakening that invariant), or require HTTPS universally. Add a capture-proxy regression proving a loopback credential never reaches the proxy. -
Medium:
channel.readbuffers for the protocol-wide maximum instead of the requested page size.max_response_bytesgrants every read 64 MiB solely from its action (crates/buzz-broker-client/src/lib.rs:14-25,128-169), while request-specific enforcement happens only after the entire body is parsed (crates/buzz-sdk/src/broker/mod.rs:731-765). The ACP poll always asks for 100 events (crates/buzz-acp/src/runtime_transport.rs:297-307), so a hostile or faulty broker can force repeated 64 MiB allocations for pages that will ultimately be rejected. Derive the transport cap from the validated request limit and test the limit-100 case.
The explicit-channel ACP polling path itself looks sound: signed-event verification, h-tag isolation, rotating dedup, terminal unauthenticated handling, and fail-closed unknown channel metadata are wired consistently. CI is green, and focused source/contract review was against exact head 937e446cf8fce5d1b7db5de4a78b87e6b6524370 and stacked base fcf2e9bc753e9ce047b21710c4262e106c2f706b. git diff --check passed. I did not run the external broker harness; the submitted head does not include one.
Client-side [C1] of the agent broker (#6790): lets the buzz CLI run without an agent nsec, routing reads and writes through a host so relay traffic looks like a normal Buzz client. - broker_client.rs: HttpBrokerClient implements the BrokerClient transport primitive the contract crate omits — POST /v1/action with an opaque bearer credential, parse an envelope regardless of HTTP status, never interpret a verdict. Correlation stays in execute(). - backend.rs: AgentBackend trait spoken in broker vocabulary, with two impls behind a Backend enum — BrokerBackend (keyless) and LocalBackend (nsec + relay, today's path). LocalBackend reuses the shared buzz_sdk::build_message builder, so there is no parallel message path. - lib.rs: register the two modules. Not yet wired into the command surface or provisioning. 8/8 unit tests pass (4 transport, 4 backend); clippy -D warnings clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Joel Robotham <jrobotham@squareup.com>
Add the mode toggle for the keyless client. --agent-mode (env BUZZ_AGENT_MODE, default "local") picks the backend; "broker" diverges before any key is read and routes the wake->reply slice through the host: - lib.rs: AgentMode enum + --agent-mode / --broker-url / --broker-credential flags; run() branches to run_broker(), which builds Backend::broker from the endpoint + credential. Broker mode fails closed if a private key is present (supplying one is a provisioning error, not silently ignored). --mentions-only added to `messages get` for the wake path. - messages.rs: dispatch_broker maps `messages get` -> channel.read and `messages send`/reply -> message.post/message.reply, taking explicit pubkey mentions. Relay-coupled extras (auto @mention resolution, file upload, forum kinds, broadcast, time/kind windowing) are refused in broker mode rather than silently dropped; the local path is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Joel Robotham <jrobotham@squareup.com>
- examples/mock_broker.rs: a throwaway host that speaks just enough of the contract (POST /v1/action, echoes requestId/action, canned outcomes) to exercise the keyless CLI end to end before a real broker exists. Signs nothing, touches no relay. - KEYLESS.md: brief instructions — build, run against the mock, and point the CLI at your own broker. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Joel Robotham <jrobotham@squareup.com>
Extend the AgentBackend seam with reaction_add / profile_set and route two more command groups through it in keyless mode: - reactions add -> reaction.add (requires --channel, the host's scope; refuses --emoji-url, which is host-owned custom-emoji handling) - users set-profile -> profile.set (maps name/about/avatar; refuses --nip05, absent from the contract; requires >=1 field) BrokerBackend maps each to its ActionArgs and unwraps the EventPublished outcome, same as message.post. LocalBackend implements both for parity (reaction via build_reaction; profile as read-merge-write over the current kind:0, emulating the contract's 'absent fields left as-is'). Local command paths are untouched. Mock host and KEYLESS.md gain the two actions; two BrokerBackend round-trip tests added. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Joel Robotham <jrobotham@squareup.com>
Add an addressing-only bridge for encrypted memory while the end-to-end runtime storage semantics remain deferred. - add mem address <slug> and normalize shorthand to the NIP-AE slug - route storage.address through AgentBackend in broker mode and print its validated JSON outcome - derive the same address locally for command parity - extend the mock host, backend round-trip coverage, and keyless docs The returned coordinates identify a record but do not yet fetch, decrypt, encrypt, or publish it; KEYLESS.md records that deliberate temporary limit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Joel Robotham <jrobotham@squareup.com>
Promote the HTTP broker transport into a shared crate and add an explicit runtime transport seam to buzz-acp. Broker mode rejects local keys, derives its identity through storage.address, polls configured channels through channel.read, validates returned events, and provisions agent subprocesses with broker-only credentials. Relay-only housekeeping and enrichment stay disabled until the frozen contract grows an agreed host-owned path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Joel Robotham <jrobotham@squareup.com>
Remove empty local-credential tombstones from spawned agent environments and defensively treat an empty CLI private-key value as absent, while continuing to reject all real key material. Bound broker HTTP actions, terminate polling on rejected credentials, and document the cursor, restart, and thread-context limits that remain for real-host integration. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Joel Robotham <jrobotham@squareup.com>
Signed-off-by: Joel Robotham <jrobotham@squareup.com>
Signed-off-by: Joel Robotham <jrobotham@squareup.com>
Signed-off-by: Joel Robotham <jrobotham@squareup.com>
Signed-off-by: Joel Robotham <jrobotham@squareup.com>
Signed-off-by: Joel Robotham <jrobotham@squareup.com>
Signed-off-by: Joel Robotham <jrobotham@squareup.com>
937e446 to
f42a4a3
Compare
|
🤖 Updated by Codex in
The rebase also accounts for the updated observer payload contract by including outer JSON escaping in the broker-frame budget. Local pre-push checks and CI are green on the new head. Ready for re-review. |
Stack Info: Stacked on #6922
Why
Allow agents to run without a signing key or direct relay access by delegating authorized operations to a broker host.
What
localandbrokermodes.buzz-acpthrough broker-polled reads with bounded replay deduplication and safe child provisioning.Risk Assessment
Low — broker mode is opt-in and local mode remains covered by existing tests. Basic end-to-end validation has exercised the keyless path through a signing broker against a real Buzz relay.
References
Update Aug 29, 10:11: Hardened broker boundaries following review.
session/new.channel.readresponse buffering from the validated page limit.Generated with Codex