Repository navigation
feat(runtime): accept launcher TLS trust snapshots - #7943
shellz-n-stuff wants to merge 1 commit into
Conversation
| // Explicit Buzz-owned OAuth root for named demo builds. The agent may spawn | ||
| // auth-capable child tools after clearing its ambient environment. | ||
| "BUZZ_AGENT_CONFIG_DIR", | ||
| // Read-only launcher trust snapshot; do not forward the auth-broker capability. |
There was a problem hiding this comment.
So taking this path as an env var is the key security piece here. We could require/enforce a specific directory but I think that's explicitly easier to exploit than injecting an env var into what's running Buzz
There was a problem hiding this comment.
🤖 I agree the env var is the right transport for the launcher. My worry is a different injection path: desktop merges user and persona env into managed agents, and BUZZ_SANDBOX_TLS_ROOTS isn't in RESERVED_ENV_KEYS (desktop/src-tauri/src/managed_agents/reserved_env_keys.rs). That list exists for exactly this kind of security gate, and BUZZ_RELAY_URL and BUZZ_AGENT_CONFIG_DIR are already on it. If this key can be set through ordinary agent env config, then setting it together with HTTPS_PROXY would let whoever holds that proxy see model API keys and the CLI's NIP-98 / x-auth-tag headers. Before this PR, HTTPS_PROXY on its own would just fail TLS verification. Would you be open to reserving the key in desktop in this PR? The launcher sets the child env directly, so I don't think that affects Sandpit.
Separately, the "do not forward the auth-broker capability" part of this comment refers to #7944, so it might read better to move that line over there.
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: b640a332a0
ℹ️ 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".
| impl Llm { | ||
| pub fn new(cfg: &Config) -> Result<Self, AgentError> { | ||
| let http = Client::builder() | ||
| let http = crate::sandbox_runtime::http_builder()? |
There was a problem hiding this comment.
When BUZZ_SANDBOX_TLS_ROOTS contains the only CA trusted for a Databricks workspace and DATABRICKS_TOKEN is unset, the LLM client trusts the snapshot but build_token_source constructs PkceOAuthTokenSource with the unchanged Client::builder() in auth.rs. Its OIDC discovery, refresh, and token requests therefore reject the workspace certificate, preventing authentication and model discovery before any LLM request can run. Construct the PKCE HTTP client from the shared snapshot-aware builder as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🤖 +1, this still reproduces at 1815161. PkceOAuthTokenSource::new_with_http_timeout (auth.rs:417) builds from plain Client::builder(), and that's the client used for discovery, code exchange, and refresh in build_token_source, catalog discovery, and buzz-agent auth. #7944 skips PKCE when the broker is configured, but #7943 is meant to work on its own, and with a snapshot-only CA the model call would trust the endpoint while token refresh wouldn't. I'd route it through http_builder() and keep the timeout. It'd also be good to add a private-CA test that goes through the production constructor, so the test breaks if the wiring gets removed.
| let bytes = std::fs::read(path).map_err(|_| fail())?; | ||
| if bytes.len() > 8 * 1024 * 1024 { |
There was a problem hiding this comment.
If the launcher supplies a very large file or a non-terminating device such as /dev/zero, std::fs::read consumes the entire input before the 8 MiB check executes, so malformed input can exhaust memory or hang startup instead of failing closed. Read through a limit of 8 MiB plus one byte, then reject overflow before parsing.
AGENTS.md reference: AGENTS.md:L240-L248
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🤖 +1. I'd cap the read itself with something like File::open(path)?.take(8 MiB + 1) and reject if you get the extra byte. A metadata() check alone wouldn't catch /dev/zero or a file that grows while you're reading it.
Signed-off-by: Alex Rosenzweig <arosenzweig@squareup.com>
1bb547c to
68c0058
Compare
b640a33 to
1815161
Compare
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 hey @shellz-n-stuff, the core TLS piece looks solid to me. As far as I can tell, tls_certs_only on reqwest 0.13.4 really does move the wired clients onto local rustls verification with only the snapshot roots, hostname checks still apply, and a bad snapshot makes client construction fail instead of silently falling back. Unset means nothing changes.
I left the bigger items as replies on the existing threads so they don't get duplicated. The main ones are that the Databricks OAuth client still ignores the snapshot, and whether desktop should reserve BUZZ_SANDBOX_TLS_ROOTS. The inline comments below are smaller.
On coverage, I think it'd help to document which clients honor the snapshot and which don't. It isn't every HTTPS/WSS client in the agent and CLI yet:
- The CLI's ephemeral publish (
publish_ephemeral_event→buzz_ws_client::publish_event→ tokio-tungstenite with webpki roots) doesn't honor it, so something likebuzz users set-presencewon't pick up the snapshot. - Forwarding the var to MCP children doesn't make their clients use it either. For example,
buzz-dev-mcp'sview_imagestill builds a plain reqwest client. DatabricksConnection::newalso uses a plain builder. I wouldn't just swap that one, though, sinceDATABRICKS_REUSE.mddescribes it as runtime-config-independent. Probably the caller should pass trust explicitly, if it needs it at all.
I don't think all of those need fixing in this PR. The doc comment should just say what's covered.
| use std::{path::Path, sync::Arc}; | ||
|
|
||
| /// Path to the immutable certificate snapshot supplied by the launcher. | ||
| pub const ROOTS_ENV: &str = "BUZZ_SANDBOX_TLS_ROOTS"; |
There was a problem hiding this comment.
🤖 nit: this const is a duplicate of buzz_runtime_support::tls::ROOTS_ENV, and nothing uses it. mcp.rs also writes the name out as a string literal. I'd keep just the runtime-support one and reference it from PASSTHROUGH_ENV.
| mod tests { | ||
| use super::*; | ||
| use reqwest::StatusCode; | ||
| #[test] |
There was a problem hiding this comment.
🤖 I think these tests belong in buzz-runtime-support next to the code, which would also let you drop the test-only builder_with_roots wrapper here. A few things aren't covered yet:
- the
http_builder()env branch: unset should give platform trust, set should give the snapshot - each rejection path: bad JSON, empty array, bad DER (which fails at
build(), notfrom_der), an oversized file, and an unreadable path
The untrusted client at the end is mostly testing reqwest rather than this code, so I'd swap it for one of those.
| if bytes.len() > 8 * 1024 * 1024 { | ||
| return Err(fail()); | ||
| } | ||
| let roots: Vec<Vec<u8>> = serde_json::from_slice(&bytes).map_err(|_| fail())?; |
There was a problem hiding this comment.
🤖 Small question: why a JSON array of DER byte arrays rather than a PEM bundle? JSON number arrays are roughly 4x bigger, and the usual SSL_CERT_FILE-style tooling can't produce this format. If the launcher already emits it that's fine. A one-line comment on the format choice would help whoever writes the next launcher.
Allow an external launcher to supply a certificate snapshot for agent and CLI HTTPS requests. Without the environment setting, normal platform trust remains unchanged. Invalid snapshots fail closed and hostname checks remain enabled.
Stack 2/3: base
codex/launcher-endpoints; next: credential broker.Validation: synthetic TLS tests (trusted certificate, wrong hostname, missing snapshot) and 12 CLI regressions pass. Tests are included in this slice. No credential broker or plugin dependency.
Review / merge order: #7942 → #7943 → #7944. Related: #5286. Replaces #7940.
Required push checks passed for this branch. Hosted CI is tracked on the PR.