Repository navigation
feat(agent): support a trusted launcher credential broker - #7944
shellz-n-stuff wants to merge 3 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b62f9268c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87e4377020
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// Launcher-supplied JSON capability for a fixed loopback credential broker. | ||
| pub const BROKER_ENV: &str = "BUZZ_SANDBOX_AUTH_BROKER"; |
There was a problem hiding this comment.
When an MCP tool runs under the agent's OS user, clearing the child's inherited environment does not keep this capability secret: on Linux, for example, the child can read /proc/$PPID/environ, recover the JSON containing secret and port, and call the loopback /token endpoint to obtain the model bearer. This defeats the stated boundary that MCP tools never receive the credential capability. Consume the capability through a mechanism inaccessible to descendants, or parse and scrub it from the process environment before any MCP process can be spawned (and remove the later environment-presence dependency).
Useful? React with 👍 / 👎.
Signed-off-by: Alex Rosenzweig <arosenzweig@squareup.com>
Signed-off-by: Alex Rosenzweig <arosenzweig@squareup.com>
Signed-off-by: Alex Rosenzweig <arosenzweig@squareup.com>
87e4377 to
34a27c6
Compare
b640a33 to
1815161
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 34a27c6e9c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 hey @shellz-n-stuff, I think the overall shape works. The launcher keeps the PKCE cache and refresh token, and the agent only ever gets an access token through /token. Sending the capability over a length-prefixed stdin frame also doesn't lose ACP input, since tokio::io::stdin() reads through the same buffered std Stdin. The earlier bot findings about secrets in env and the dropped task result look fixed at this head.
The bigger items are the startup race (reply on the existing thread), the fact that serve doesn't validate its own secret, and the new executable tests assuming Linux/macOS. The inline comments cover those plus some smaller things.
A couple more non-inline notes:
serve,BrokerConfigandBROKER_ENVare public, and their only consumer is outside the repo. The frame layout, the/tokenroute and what each status code means together make a protocol, and right now it's only described in scattered comments. If we keep this as a supported launcher API, one doc comment or a short.mdthat spells out the wire contract would help whoever writes the next launcher.- On tests, nothing checked in covers the Databricks broker path end to end. The ACP startup test uses OpenAI with a static key, so
token_source()returnsNonethere. The Databricks startup test only checks the mismatch case, and the handler tests buildBrokerClient/serve_sourcedirectly. I'd want one executable Databricks flow with synthetic credentials: broker bearer → model/catalog request → 401 → refresh → successful retry, plus a service-failure case. It'd also be good to covertoken_source()returningNonefor non-Databricks providers or when a static key is set.broker_timeout_response_is_a_service_errorstubs a 504 route rather than exercising the 55-second handler timeout, so the name claims a bit more than the test does. - It'd be worth saying in the docs that the agent still gets a full
all-apisworkspace access token. The broker keeps the refresh token and cache away from the agent, but it doesn't narrow what that access token can do.
CI: the only real failure is Desktop Smoke E2E (4) (virtualization.spec.ts:859), and I don't think this PR can affect it since it only touches crates/buzz-agent. The rest of the red checks in the rollup come from the cancelled duplicate run.
| Json(request): Json<Request>, | ||
| ) -> Result<Json<Response>, StatusCode> { | ||
| if headers.get("authorization").and_then(|v| v.to_str().ok()) | ||
| != Some(format!("Bearer {}", state.secret).as_str()) |
There was a problem hiding this comment.
🤖 I think the checks here are on the wrong side. serve accepts whatever secret it's given, so a launcher that passes something short like "x" exposes workspace tokens to any local process that can reach the port, including one outside the sandbox or running as another user. The ≥32-char and port != 0 checks only exist on the agent side in token_source, and those don't protect this server.
Suggestions:
- validate the secret in
servebefore building the PKCE source or spawning the task - reject a listener whose
local_addr()isn't loopback, since the caller hands in an already-bound listener - compare in constant time (e.g.
subtle) instead of building aformat!string per request - document that the launcher has to supply real random entropy, since length alone doesn't guarantee that
| .send() | ||
| .await | ||
| .map_err(|_| AgentError::Llm("sandbox sign-in service unavailable".into()))?; | ||
| if response.status() == StatusCode::UNAUTHORIZED { |
There was a problem hiding this comment.
🤖 A 401 from the broker currently means two different things. It's returned for a bad capability (the header check in token) and also for an expired sign-in (LlmAuth). Both end up as "Refresh sign-in from the desktop model picker". A capability mismatch is a launcher bug that re-signing-in can't fix, so I'd return 403 for that case, map it to Llm, and add a test for it.
| { | ||
| return Ok(None); | ||
| } | ||
| if broker.host != cfg.base_url || broker.port == 0 || broker.secret.len() < 32 { |
There was a problem hiding this comment.
🤖 Two small things here:
broker.hostandcfg.base_urlboth come from the launcher, so this comparison catches misconfiguration but isn't really a security boundary. The actual pin is thehostpassed toserve, and/tokenignores this field. I'd soften the "prevents cross-provider reuse" doc onBrokerConfig::hostto match. The exact string compare will also trip on a trailing slash, so maybetrim_end_matches('/')both sides.- The
port/secret.len()checks only fire on the first Databricks request, with a misleading "provider mismatch" error. I'd move them intoread_bootstrapso a bad frame fails at startup with its own message.
| #[cfg(target_os = "linux")] | ||
| nix::sys::prctl::set_dumpable(false) | ||
| .map_err(|_| AgentError::Llm("cannot protect broker process from inspection".into()))?; | ||
| if !cfg!(any(target_os = "linux", target_os = "macos")) { |
There was a problem hiding this comment.
🤖 Question on macOS: the agent doesn't do anything in-process there, so the protection depends entirely on the launcher's sandbox profile denying process-info*, mach-priv-task-port and kern.procargs*. That profile isn't versioned anywhere we can review. I wonder if a ptrace(PT_DENY_ATTACH) here would be worth it as a rough counterpart to set_dumpable(false). It doesn't replace the profile, but it keeps some protection if the profile drifts.
| "sandbox sign-in service unavailable".into(), | ||
| )); | ||
| } | ||
| Ok(response |
There was a problem hiding this comment.
🤖 A few hardening nits for the loopback client and startup read. I don't think any of these are exploitable under a trusted launcher; they just close off the loose ends:
- the successful response goes through an unbounded
.json(), so I'd cap the body bytes .no_proxy()stops proxying, but reqwest still follows redirects by default, so I'd add.redirect(Policy::none())read_bootstrap'sread_exacthas no deadline if the launcher leaves the pipe open mid-frame, so either add a startup timeout or document that the launcher must kill an incomplete startup
| bytes | ||
| } | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
🤖 I think these tests will fail on Windows. There are no platform guards, and on Windows initialize() returns "credential broker unsupported on this platform" before it reads the frame. That breaks the success case, the truncated-frame expectation and the provider-mismatch expectation. The Windows job doesn't catch it because it only runs --test databricks_auth_coordinator for this crate. I'd gate these to Linux/macOS and add an unsupported-platform rejection case.
## Summary Replacing `BUZZ_ACP_AGENT_COMMAND` with a sandbox launcher hides the real adapter from Buzz: for example, Goose loses its default `acp` argument. Add optional `BUZZ_ACP_LAUNCH_PREFIX`, a JSON argument array applied at the shared subprocess boundary, after normal worker configuration. On Unix, every spawn runs `prefix... worker args...` without shell interpolation. Worker identity, environment setup, stdio and existing process cleanup are preserved. Invalid or unavailable prefixes fail launch without falling back to the worker; unset keeps direct launch on every platform. Configured prefixes fail closed on non-Unix because per-worker process-tree cleanup is unavailable. Desktop reserves the key against saved user-environment overrides, and spawn failures name the executable. ### Related issue Prerequisite for [Buzz-App #415](block/buzz-app#415). One commit directly on `main`, independent of #7942–#7944. No duplicate launch-prefix PR found. Policy enforcement, verified launcher staging, protected paths and the supporting runtime pin remain Buzz-App/plugin work. ### Testing - Original head `9ae9d82f8`: complete ACP suite (990 passed, three existing ignored), shipped local-task test, real-Goose probe, and full repository-wide `just ci` passed. - Review fixes: five launch tests and 44 Desktop environment-filter tests pass locally, covering the real production platform gate, save rejection, both merge layers, case variants, and executable error context. - Subprocess tests cover direct/wrapped argument defaults, Pi skills, Hermes/Codex environment, adapter identity, repeated spawns and invalid/missing prefix refusal. - Shipped `buzz-acp run` entrypoint completes a task through the wrapper and deterministic ACP peer. - Real Goose 1.52.0: direct and wrapped `auth-methods` return identical results; the wrapper receives `goose acp` with initially empty worker args. - Independent agent review: no blocking findings. - Required push checks passed on the updated head. The existing native Windows CI job now explicitly runs the platform-contract tests. Its result is pending; the non-Unix refusal branch was not exercised on this macOS host. Repeated-spawn coverage is not a full lazy-pool wake/crash test. Protected Goose in the Buzz-App UI and human acceptance remain pending. To try the hook after building `buzz-acp`, use an installed Goose path: ```sh BUZZ_ACP_LAUNCH_PREFIX='["/usr/bin/env"]' target/debug/buzz-acp auth-methods \ --agent-command /absolute/path/to/goose --agent-args '' --json ``` Expect the same authentication methods as without the prefix. Changing the prefix to `["/missing-launcher"]` must fail. Draft pending human testing. Signed-off-by: Alex Rosenzweig <arosenzweig@squareup.com>
Let a confined agent obtain credentials from a trusted launcher's authenticated loopback broker instead of opening the credential cache. The broker fixes the model service and supports bearer refresh.
The launcher delivers the capability in a bounded private stdin frame before ACP starts. Environment variables contain only a nonsecret protocol marker; the former JSON-in-environment protocol is rejected. Catalog and model requests use initialized broker state. Linux disables process dumpability before reading the capability; macOS launchers must enforce the documented process-inspection restrictions.
Authentication failures remain distinct from temporary service failures. The server task preserves its return value for the launcher.
Stack 3/3: base
codex/launcher-tls. No sandbox engine or plugin build dependency.Validation: broker and executable-startup regressions pass, as do agent/plugin lint checks. An independent agent reviewed and exercised the actual local plugin → confined agent → broker → synthetic TLS model → MCP flow. The MCP inspection attempt failed while an unsandboxed environment-read control succeeded. The local plugin also rejects stale engines and handles early worker exit. Linux was not exercised.
The standalone agent suite encountered a cancellation-test timeout also reproduced on unchanged HEAD. All required push checks passed after restacking onto current main, including Rust tests and desktop native checks. The full
just cigate passed on the restacked tree. Hosted checks are tracked on the PR.Review / merge order: #7942 → #7943 → #7944. Related: #5286. Replaces #7940.
Draft pending human validation of the updated behavior. Companion plugin changes remain in the local plugin folder, which has no Git repository or remote.