From 967993b2f5b283abdfa4c954763c83b25b469639 Mon Sep 17 00:00:00 2001 From: khaliqgant Date: Thu, 24 Sep 2026 23:22:17 -0700 Subject: [PATCH 1/2] fix(broker): scope credentials passed to spawned workers Workers inherit the broker's environment so harness CLIs keep PATH, HOME, model API keys and proxies. Relay-owned credentials in that environment are now removed from one centralized list on every worker spawn path before the worker's own credentials are applied. Workspace credentials still reach a worker only through the broker's explicit worker environment. Adds broker unit tests and a fleet e2e that checks the credential variable names a spawned worker can see. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 1 + crates/broker/src/snippets.rs | 13 ++- crates/broker/src/spawner.rs | 98 +++++++++++++++++++- crates/broker/src/worker.rs | 73 ++++++++++++++- tests/e2e/fleet/README.md | 5 ++ tests/e2e/fleet/harness.ts | 1 + tests/e2e/fleet/nodes/env-probe.cjs | 23 +++++ tests/e2e/fleet/nodes/env-probe.ts | 27 ++++++ tests/e2e/fleet/worker-env.test.ts | 133 ++++++++++++++++++++++++++++ 9 files changed, 361 insertions(+), 13 deletions(-) create mode 100644 tests/e2e/fleet/nodes/env-probe.cjs create mode 100644 tests/e2e/fleet/nodes/env-probe.ts create mode 100644 tests/e2e/fleet/worker-env.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index db70a371a7..87cdd0bbd5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Broker-spawned workers no longer inherit the broker's own credentials from its environment; each worker receives only its own agent token and the workspace credentials the broker delegates to it. - Integration subscription setup, listing and retirement use workspace authentication even when a spawned worker also has an agent token, preventing misleading “Workspace key required” failures. - A `teams.json` agent whose `cli` carries an inline `--model`/`-m` now records the model the harness actually runs. The inline override becomes the spawn's effective model before the relay skill prefix is chosen, so worker listings, spawn events, telemetry and small-model guidance describe the running model rather than the superseded pin. - `agent-relay fleet config|enable|disable|inherit` now exit successfully as hidden compatibility no-ops instead of failing on the removed workspace rollout API. diff --git a/crates/broker/src/snippets.rs b/crates/broker/src/snippets.rs index 90ae0084dd..952a10ac72 100644 --- a/crates/broker/src/snippets.rs +++ b/crates/broker/src/snippets.rs @@ -247,16 +247,13 @@ async fn probe_agent_relay_mcp_command_with_timeout( command: &AgentRelayMcpCommand, timeout: Duration, ) -> io::Result<()> { - let mut child = Command::new(&command.command) + let mut probe = Command::new(&command.command); + // Tool discovery is local and must not rotate an agent identity or + // print credentials while diagnosing a broken MCP executable. + crate::spawner::remove_inherited_relay_credentials(&mut probe); + let mut child = probe .args(&command.args) - // Tool discovery is local and must not rotate an agent identity or - // print credentials while diagnosing a broken MCP executable. .env("RELAY_SKIP_BOOTSTRAP", "1") - .env_remove("RELAY_API_KEY") - .env_remove("RELAY_WORKSPACE_KEY") - .env_remove("AGENT_RELAY_WORKSPACE_KEY") - .env_remove("RELAY_AGENT_TOKEN") - .env_remove("RELAY_WORKSPACES_JSON") .stdin(Stdio::piped()) .stdout(Stdio::piped()) .stderr(Stdio::null()) diff --git a/crates/broker/src/spawner.rs b/crates/broker/src/spawner.rs index e90a88c3ca..99e6e26b9e 100644 --- a/crates/broker/src/spawner.rs +++ b/crates/broker/src/spawner.rs @@ -22,6 +22,44 @@ const RELAY_ATTEST_GIT_CONFIG_COUNT: &str = "RELAY_ATTEST_GIT_CONFIG_COUNT"; const RELAY_ATTEST_GIT_CONFIG_INDEX: &str = "RELAY_ATTEST_GIT_CONFIG_INDEX"; const RELAY_ATTEST_BROKER_HOOK_PATH: &str = "RELAY_ATTEST_BROKER_HOOK_PATH"; +/// Relay-owned credentials that an agent process must never pick up from the +/// broker's own (inherited) environment. Agents otherwise inherit the broker's +/// whole environment, which is intentional for PATH, HOME, model API keys and +/// proxies, but these values belong to the broker, the node, or whoever +/// launched the broker. +/// +/// Every worker spawn strips this list first and then injects only what that +/// worker is meant to hold: its own `RELAY_AGENT_TOKEN`, its own result +/// callback token, and the workspace credentials the broker explicitly +/// delegates to agents through its worker environment. +pub(crate) const INHERITED_RELAY_CREDENTIAL_ENV_KEYS: &[&str] = &[ + // The broker's local HTTP API key (set on the broker process at startup). + "RELAY_BROKER_API_KEY", + // The node's control-plane credential. + "RELAY_NODE_TOKEN", + // The broker's own registration identity proof. + "RELAY_AGENT_IDENTITY_KEY", + // Whoever launched the broker; each worker gets its own. + "RELAY_AGENT_TOKEN", + "AGENT_RELAY_RESULT_TOKEN", + // Workspace credentials. Re-added from the broker's explicit worker + // environment when the broker delegates them; never taken from ambient + // environment. + "RELAY_API_KEY", + "RELAY_WORKSPACE_KEY", + "AGENT_RELAY_WORKSPACE_KEY", + "RELAY_WORKSPACES_JSON", +]; + +/// Remove [`INHERITED_RELAY_CREDENTIAL_ENV_KEYS`] from a command's inherited +/// environment. Call before applying the worker's own environment: a later +/// `Command::env` for the same key still wins. +pub(crate) fn remove_inherited_relay_credentials(command: &mut Command) { + for key in INHERITED_RELAY_CREDENTIAL_ENV_KEYS { + command.env_remove(key); + } +} + /// Standard client-side git hooks (see githooks(5)). `core.hooksPath` is a /// single directory that replaces git's entire hook lookup, not just /// `prepare-commit-msg` — so the broker must install a forwarder under every @@ -372,6 +410,7 @@ impl Spawner { .stdout(Stdio::inherit()) .stderr(Stdio::inherit()); + remove_inherited_relay_credentials(&mut cmd); let mut child_env = env_vars.to_vec(); if attestation_env_present(&child_env) { match self.commit_hooks_dir() { @@ -577,11 +616,64 @@ mod tests { use super::{ add_broker_hooks_path, attestation_env_present, git_config_count_with_inherited, - spawn_env_vars, terminate_child, with_commit_attestation_env, write_broker_git_hooks, - Spawner, RELAY_ATTEST_AGENT_ID, RELAY_ATTEST_JTI, RELAY_ATTEST_SESSION_ID, - RELAY_ATTEST_SPONSOR_ID, + remove_inherited_relay_credentials, spawn_env_vars, terminate_child, + with_commit_attestation_env, write_broker_git_hooks, Spawner, + INHERITED_RELAY_CREDENTIAL_ENV_KEYS, RELAY_ATTEST_AGENT_ID, RELAY_ATTEST_JTI, + RELAY_ATTEST_SESSION_ID, RELAY_ATTEST_SPONSOR_ID, }; + #[test] + fn inherited_relay_credentials_cover_broker_node_and_workspace_secrets() { + for key in [ + "RELAY_BROKER_API_KEY", + "RELAY_NODE_TOKEN", + "RELAY_AGENT_IDENTITY_KEY", + "RELAY_AGENT_TOKEN", + "AGENT_RELAY_RESULT_TOKEN", + "RELAY_API_KEY", + "RELAY_WORKSPACE_KEY", + "AGENT_RELAY_WORKSPACE_KEY", + "RELAY_WORKSPACES_JSON", + ] { + assert!( + INHERITED_RELAY_CREDENTIAL_ENV_KEYS.contains(&key), + "{key} must be stripped from worker environments" + ); + } + } + + #[test] + fn remove_inherited_relay_credentials_strips_every_key_and_later_env_wins() { + let mut command = Command::new("true"); + remove_inherited_relay_credentials(&mut command); + command.env("RELAY_AGENT_TOKEN", "worker-own-token"); + + let envs: std::collections::HashMap> = command + .as_std() + .get_envs() + .map(|(key, value)| { + ( + key.to_string_lossy().into_owned(), + value.map(|v| v.to_string_lossy().into_owned()), + ) + }) + .collect(); + for key in INHERITED_RELAY_CREDENTIAL_ENV_KEYS { + if *key == "RELAY_AGENT_TOKEN" { + continue; + } + assert_eq!(envs.get(*key), Some(&None), "{key} should be removed"); + } + assert_eq!( + envs.get("RELAY_AGENT_TOKEN"), + Some(&Some("worker-own-token".to_string())), + "a worker's own token set after the scrub must survive" + ); + // Non-relay environment is left to normal inheritance. + assert!(!envs.contains_key("PATH")); + assert!(!envs.contains_key("ANTHROPIC_API_KEY")); + } + fn git(repo: &Path, args: &[&str], env: &[(String, String)]) -> std::process::Output { StdCommand::new("git") .current_dir(repo) diff --git a/crates/broker/src/worker.rs b/crates/broker/src/worker.rs index 7cd57f069e..c4746a76dc 100644 --- a/crates/broker/src/worker.rs +++ b/crates/broker/src/worker.rs @@ -34,8 +34,9 @@ use crate::{ runtime::headless_provider_cli_name, spawner::{ add_broker_hooks_path, attestation_env_present, is_valid_attestation_value, - resolve_commit_hooks_dir, terminate_child, with_commit_attestation_env, - RELAY_ATTEST_AGENT_ID, RELAY_ATTEST_JTI, RELAY_ATTEST_SESSION_ID, RELAY_ATTEST_SPONSOR_ID, + remove_inherited_relay_credentials, resolve_commit_hooks_dir, terminate_child, + with_commit_attestation_env, RELAY_ATTEST_AGENT_ID, RELAY_ATTEST_JTI, + RELAY_ATTEST_SESSION_ID, RELAY_ATTEST_SPONSOR_ID, }, }; @@ -1208,6 +1209,11 @@ impl WorkerRegistry { // attested child through Command's inherited environment. command.env_remove(key); } + // Every runtime (PTY, headless provider, app-server, native sidecar) + // reaches this point with a command that inherits the broker's + // environment. Drop relay-owned credentials from it; the worker's own + // credentials are injected below. + remove_inherited_relay_credentials(&mut command); child_env.retain(|(key, _)| { !matches!( key.as_str(), @@ -2913,6 +2919,69 @@ mod tests { } } + #[cfg(any(target_os = "linux", target_os = "macos"))] + #[tokio::test] + async fn spawned_worker_holds_only_its_own_and_delegated_relay_credentials() { + let dir = tempfile::tempdir().expect("worker cwd"); + // Record only the NAMES of relay credential variables the worker sees. + let keys = crate::spawner::INHERITED_RELAY_CREDENTIAL_ENV_KEYS.join(" "); + let script = format!( + "for k in {keys}; do if printenv \"$k\" >/dev/null; then echo \"$k\"; fi; done > names.tmp && mv names.tmp names.txt; sleep 30" + ); + let mut spec = sleeping_native_worker("credential-worker", None); + spec.harness_config = Some(ResolvedHarnessConfig::Native(NativeHarnessConfig { + command: "sh".to_string(), + args: vec!["-c".to_string(), script], + cwd: Some(dir.path().to_string_lossy().into_owned()), + env: None, + session_id: "session-credential-worker".to_string(), + metadata: None, + })); + // The broker explicitly delegates a workspace key through its worker + // environment; that delegation must survive the inherited scrub. + let mut registry = make_registry(vec![( + "RELAY_API_KEY".to_string(), + "rk_live_delegated".to_string(), + )]); + + registry + .spawn( + spec, + None, + None, + Some("at_live_worker_own".to_string()), + false, + None, + None, + None, + None, + ) + .await + .expect("credential worker should spawn"); + + let names_path = dir.path().join("names.txt"); + let mut names = None; + for _ in 0..50 { + if let Ok(value) = std::fs::read_to_string(&names_path) { + names = Some(value); + break; + } + tokio::time::sleep(Duration::from_millis(100)).await; + } + registry + .release("credential-worker") + .await + .expect("release credential worker"); + + let mut observed: Vec = names + .expect("worker recorded its credential names") + .lines() + .map(str::to_string) + .collect(); + observed.sort(); + assert_eq!(observed, vec!["RELAY_AGENT_TOKEN", "RELAY_API_KEY"]); + } + #[cfg(any(target_os = "linux", target_os = "macos"))] #[tokio::test] async fn spawned_worker_process_uses_requested_cwd() { diff --git a/tests/e2e/fleet/README.md b/tests/e2e/fleet/README.md index e4d39affc9..5a3916cf79 100644 --- a/tests/e2e/fleet/README.md +++ b/tests/e2e/fleet/README.md @@ -31,6 +31,11 @@ omitted/null heartbeat-load compatibility added in relaycast#307. | delivery seq/dedup | per-agent deliveries carry strictly monotonic `agent_seq` (no duplicates); a resync from a mid-cursor replays only the tail (`gap_detected: false`, no duplicate seqs) — the exactly-once cursor the node-restart reconcile relies on | | mailbox TTL | an undelivered message dead-letters after a short TTL **and the sender is notified** (`delivery.failed` naming the target) | +`worker-env.test.ts` boots a single `env-probe` node and asserts that a spawned +worker sees only its own `RELAY_AGENT_TOKEN` plus the workspace credentials the +broker delegates — never the broker API key, node token, or broker identity key. +The probe records variable names only, never values. + ### Coverage notes (intentionally not re-asserted here) - **overflow reject-new**: `belowDepthCapSql` rejects new deliveries past the per-agent diff --git a/tests/e2e/fleet/harness.ts b/tests/e2e/fleet/harness.ts index 952f327143..f2740f4006 100644 --- a/tests/e2e/fleet/harness.ts +++ b/tests/e2e/fleet/harness.ts @@ -27,6 +27,7 @@ export const REPO_ROOT = path.resolve(HERE, '..', '..', '..'); export const NODE_A_FILE = path.join(HERE, 'nodes', 'node-a.ts'); export const NODE_B_FILE = path.join(HERE, 'nodes', 'node-b.ts'); export const CLOUD_ENROLLED_NODE_FILE = path.join(HERE, 'nodes', 'cloud-enrolled.ts'); +export const ENV_PROBE_NODE_FILE = path.join(HERE, 'nodes', 'env-probe.ts'); const CLI_ENTRY = path.join(REPO_ROOT, 'packages', 'cli', 'dist', 'cli', 'index.js'); diff --git a/tests/e2e/fleet/nodes/env-probe.cjs b/tests/e2e/fleet/nodes/env-probe.cjs new file mode 100644 index 0000000000..73796c3e32 --- /dev/null +++ b/tests/e2e/fleet/nodes/env-probe.cjs @@ -0,0 +1,23 @@ +#!/usr/bin/env node +'use strict'; +// E2E env probe: records the NAMES (never the values) of relay credential-like +// environment variables this spawned worker inherited, then behaves like the +// regular stub agent so the spawn reaches harness readiness. +const { mkdirSync, renameSync, writeFileSync } = require('node:fs'); +const path = require('node:path'); + +const projectDir = process.env.AGENT_RELAY_PROJECT; +const agentName = process.env.RELAY_AGENT_NAME; +if (projectDir && agentName) { + const names = Object.keys(process.env) + .filter((name) => /^(RELAY_|AGENT_RELAY_)/.test(name)) + .filter((name) => /KEY|TOKEN|SECRET|WORKSPACES_JSON/.test(name)) + .sort(); + const dir = path.join(projectDir, '.agentworkforce', 'relay', 'e2e-env-probe'); + mkdirSync(dir, { recursive: true }); + const file = path.join(dir, `${agentName}.json`); + writeFileSync(`${file}.tmp`, JSON.stringify({ agent: agentName, names })); + renameSync(`${file}.tmp`, file); +} + +require('./stub-agent.cjs'); diff --git a/tests/e2e/fleet/nodes/env-probe.ts b/tests/e2e/fleet/nodes/env-probe.ts new file mode 100644 index 0000000000..725a2d7795 --- /dev/null +++ b/tests/e2e/fleet/nodes/env-probe.ts @@ -0,0 +1,27 @@ +import { fileURLToPath } from 'node:url'; +import { z } from 'zod'; +import { definePtyHarness } from '@agent-relay/harnesses'; +import { action, defineNode, spawn } from '@agent-relay/fleet'; + +/** + * E2E node for the worker-environment regression. Its only spawn harness runs + * `env-probe.cjs`, which records which relay credential variable names the + * spawned worker can see. `envprobe:ready` is a sidecar-only action: once it + * is in the roster, the sidecar (and therefore this probe harness) owns + * `spawn:claude` rather than the broker's native provider. + */ +const probe = definePtyHarness({ + runtime: 'pty', + command: process.execPath, + args: [fileURLToPath(new URL('./env-probe.cjs', import.meta.url))], + env: { RELAY_E2E_NODE_NAME: 'env-probe', RELAY_INJECT_RATE_MS: '0' }, +}); + +export default defineNode({ + name: 'env-probe', + maxAgents: 2, + capabilities: { + 'spawn:claude': spawn(probe), + 'envprobe:ready': action({ input: z.object({}) }, async () => ({ ok: true })), + }, +}); diff --git a/tests/e2e/fleet/worker-env.test.ts b/tests/e2e/fleet/worker-env.test.ts new file mode 100644 index 0000000000..43c744e70c --- /dev/null +++ b/tests/e2e/fleet/worker-env.test.ts @@ -0,0 +1,133 @@ +import { readFileSync } from 'node:fs'; +import path from 'node:path'; +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { + cleanupTmp, + createWorkspace, + enrollNode, + ENV_PROBE_NODE_FILE, + FleetNode, + getInvocation, + getNodes, + invokeAction, + makeTmpRoot, + preflight, + registerAgent, + startEngine, + waitFor, + type EngineHandle, +} from './harness.js'; + +/** + * Worker environment regression: a spawned agent must not inherit the + * broker's own credentials. The node is started the way `node up` runs in + * production (node token + workspace key in the broker's environment), and a + * probe harness records only the NAMES of relay credential-like variables it + * can see. Values are never read or printed. + */ +const pre = preflight(); + +describe.skipIf(!pre.ok)('spawned worker environment', () => { + let tmpRoot: string; + let engine: EngineHandle; + let workspaceKey: string; + let driverToken: string; + let probeNode: FleetNode; + + beforeAll(async () => { + tmpRoot = makeTmpRoot(); + engine = await startEngine(pre.engineServe!, tmpRoot); + workspaceKey = await createWorkspace(engine, 'worker-env-e2e'); + const nodeToken = await enrollNode(engine, workspaceKey, 'node_env_probe', 'env-probe', [ + 'spawn:claude', + 'envprobe:ready', + ]); + probeNode = new FleetNode({ + name: 'env-probe', + nodeId: 'node_env_probe', + nodeFile: ENV_PROBE_NODE_FILE, + nodeToken, + workspaceKey, + engineBaseUrl: engine.baseUrl, + brokerBinary: pre.brokerBinary!, + tmpRoot, + capacityHarnesses: 'claude', + }); + probeNode.start(); + driverToken = await registerAgent(engine, workspaceKey, 'env-driver'); + + // Wait for the sidecar-only action: until it is registered the broker's + // native provider could serve `spawn:claude` with a real CLI. + await waitFor( + async () => { + const nodes = await getNodes(engine, workspaceKey, { name: 'env-probe' }); + const match = nodes.find((entry) => entry.name === 'env-probe'); + return match?.live && + match.handlers_live && + match.capabilities.some((capability) => capability.name === 'envprobe:ready') + ? match + : null; + }, + { timeoutMs: 45_000, label: 'env-probe node online with sidecar handlers' } + ); + }, 90_000); + + afterAll(async () => { + await probeNode?.stop(); + await engine?.stop(); + if (tmpRoot && !process.env.CI) cleanupTmp(tmpRoot); + }); + + it('holds only its own token plus the workspace credentials the broker delegates', async () => { + const agent = 'env-probe-worker'; + const spawn = await invokeAction(engine, driverToken, 'spawn', { + cli: 'claude', + name: agent, + target_node: 'env-probe', + }); + expect(spawn.status).toBe(201); + + const done = await waitFor( + async () => { + const invocation = await getInvocation(engine, driverToken, 'spawn', spawn.invocationId!); + return invocation.status === 'completed' || invocation.status === 'failed' ? invocation : null; + }, + { label: `${agent} spawn settled`, timeoutMs: 35_000 } + ); + expect(done.status).toBe('completed'); + + const probePath = path.join( + probeNode.projectDir, + '.agentworkforce', + 'relay', + 'e2e-env-probe', + `${agent}.json` + ); + const probe = await waitFor( + async () => { + try { + return JSON.parse(readFileSync(probePath, 'utf8')) as { agent: string; names: string[] }; + } catch { + return null; + } + }, + { label: `${agent} recorded its environment names`, timeoutMs: 20_000 } + ); + + // The broker's own API key, the node token, and the broker's identity + // proof never reach an agent. + expect(probe.names).not.toContain('RELAY_BROKER_API_KEY'); + expect(probe.names).not.toContain('RELAY_NODE_TOKEN'); + expect(probe.names).not.toContain('RELAY_AGENT_IDENTITY_KEY'); + // Exactly: the agent's own token, plus the workspace credentials the + // broker deliberately delegates so the agent's relay tools can spawn and + // list workers (add_agent, list_agents, query_nodes). + expect(probe.names).toEqual([ + 'AGENT_RELAY_WORKSPACE_KEY', + 'RELAY_AGENT_TOKEN', + 'RELAY_API_KEY', + 'RELAY_WORKSPACES_JSON', + 'RELAY_WORKSPACE_KEY', + ]); + }); +}); From 30fc1af0b3b16bae720b8cb4df5e98c9b9ba7e7d Mon Sep 17 00:00:00 2001 From: khaliqgant Date: Fri, 25 Sep 2026 00:59:17 -0700 Subject: [PATCH 2/2] fix(broker): scrub relay credentials from spawn-time helper processes MCP registration (grok/gemini/droid mcp add/remove), the codex model probe and codex session pre-creation now build their commands through relay_pty::credentials::scrubbed_command, the same list worker spawns strip. The list moves into relay-pty so both crates share one source. Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/broker/src/snippets.rs | 8 +-- crates/broker/src/spawner.rs | 46 ++++------------ crates/broker/src/worker.rs | 8 +-- crates/relay-pty/src/codex_session.rs | 4 +- crates/relay-pty/src/credentials.rs | 78 +++++++++++++++++++++++++++ crates/relay-pty/src/lib.rs | 1 + 6 files changed, 96 insertions(+), 49 deletions(-) create mode 100644 crates/relay-pty/src/credentials.rs diff --git a/crates/broker/src/snippets.rs b/crates/broker/src/snippets.rs index 952a10ac72..6ec83beafc 100644 --- a/crates/broker/src/snippets.rs +++ b/crates/broker/src/snippets.rs @@ -1951,7 +1951,7 @@ fn grok_manual_mcp_add_cmd(cli: &str) -> String { async fn remove_grok_mcp_servers(exe: &str) { for server_name in [AGENT_RELAY_MCP_SERVER, LEGACY_RELAYCAST_SERVER] { - let mut cmd = Command::new(exe); + let mut cmd = crate::spawner::scrubbed_command(exe); cmd.args(["mcp", "remove", server_name]) .stdin(Stdio::null()) .stdout(Stdio::null()) @@ -1979,7 +1979,7 @@ async fn configure_grok_mcp( remove_grok_mcp_servers(&exe).await; - let mut mcp_cmd = Command::new(&exe); + let mut mcp_cmd = crate::spawner::scrubbed_command(&exe); mcp_cmd.args(grok_mcp_add_args( api_key, base_url, @@ -2065,7 +2065,7 @@ async fn configure_gemini_droid_mcp( /// Remove all known relay MCP server names from the gemini/droid shared config. async fn remove_gemini_droid_mcp_servers(exe: &str) { for server_name in [AGENT_RELAY_MCP_SERVER, LEGACY_RELAYCAST_SERVER] { - let mut cmd = Command::new(exe); + let mut cmd = crate::spawner::scrubbed_command(exe); cmd.args(["mcp", "remove", server_name]) .stdin(Stdio::null()) .stdout(Stdio::null()) @@ -2132,7 +2132,7 @@ async fn spawn_mcp_add( cli: &str, manual_cmd: &str, ) -> Result { - let mut mcp_cmd = Command::new(exe); + let mut mcp_cmd = crate::spawner::scrubbed_command(exe); mcp_cmd .args(add_args) .stdin(Stdio::null()) diff --git a/crates/broker/src/spawner.rs b/crates/broker/src/spawner.rs index 99e6e26b9e..65cce975cd 100644 --- a/crates/broker/src/spawner.rs +++ b/crates/broker/src/spawner.rs @@ -22,43 +22,15 @@ const RELAY_ATTEST_GIT_CONFIG_COUNT: &str = "RELAY_ATTEST_GIT_CONFIG_COUNT"; const RELAY_ATTEST_GIT_CONFIG_INDEX: &str = "RELAY_ATTEST_GIT_CONFIG_INDEX"; const RELAY_ATTEST_BROKER_HOOK_PATH: &str = "RELAY_ATTEST_BROKER_HOOK_PATH"; -/// Relay-owned credentials that an agent process must never pick up from the -/// broker's own (inherited) environment. Agents otherwise inherit the broker's -/// whole environment, which is intentional for PATH, HOME, model API keys and -/// proxies, but these values belong to the broker, the node, or whoever -/// launched the broker. -/// -/// Every worker spawn strips this list first and then injects only what that -/// worker is meant to hold: its own `RELAY_AGENT_TOKEN`, its own result -/// callback token, and the workspace credentials the broker explicitly -/// delegates to agents through its worker environment. -pub(crate) const INHERITED_RELAY_CREDENTIAL_ENV_KEYS: &[&str] = &[ - // The broker's local HTTP API key (set on the broker process at startup). - "RELAY_BROKER_API_KEY", - // The node's control-plane credential. - "RELAY_NODE_TOKEN", - // The broker's own registration identity proof. - "RELAY_AGENT_IDENTITY_KEY", - // Whoever launched the broker; each worker gets its own. - "RELAY_AGENT_TOKEN", - "AGENT_RELAY_RESULT_TOKEN", - // Workspace credentials. Re-added from the broker's explicit worker - // environment when the broker delegates them; never taken from ambient - // environment. - "RELAY_API_KEY", - "RELAY_WORKSPACE_KEY", - "AGENT_RELAY_WORKSPACE_KEY", - "RELAY_WORKSPACES_JSON", -]; - -/// Remove [`INHERITED_RELAY_CREDENTIAL_ENV_KEYS`] from a command's inherited -/// environment. Call before applying the worker's own environment: a later -/// `Command::env` for the same key still wins. -pub(crate) fn remove_inherited_relay_credentials(command: &mut Command) { - for key in INHERITED_RELAY_CREDENTIAL_ENV_KEYS { - command.env_remove(key); - } -} +#[cfg(test)] +pub(crate) use relay_pty::credentials::INHERITED_RELAY_CREDENTIAL_ENV_KEYS; +/// Relay-owned credentials no spawned worker or spawn-time helper inherits +/// from the broker's environment; see [`relay_pty::credentials`]. Every worker +/// spawn strips this list first and then injects only what that worker is meant +/// to hold: its own `RELAY_AGENT_TOKEN`, its own result callback token, and the +/// workspace credentials the broker explicitly delegates to agents through its +/// worker environment. +pub(crate) use relay_pty::credentials::{remove_inherited_relay_credentials, scrubbed_command}; /// Standard client-side git hooks (see githooks(5)). `core.hooksPath` is a /// single directory that replaces git's entire hook lookup, not just diff --git a/crates/broker/src/worker.rs b/crates/broker/src/worker.rs index c4746a76dc..0bd3496e6c 100644 --- a/crates/broker/src/worker.rs +++ b/crates/broker/src/worker.rs @@ -2656,12 +2656,8 @@ async fn codex_debug_models_output(resolved_cli: &str) -> std::io::Result diff --git a/crates/relay-pty/src/codex_session.rs b/crates/relay-pty/src/codex_session.rs index 0c65439204..905f439ffa 100644 --- a/crates/relay-pty/src/codex_session.rs +++ b/crates/relay-pty/src/codex_session.rs @@ -11,7 +11,7 @@ use anyhow::{bail, Context, Result}; use serde_json::{json, Value}; use tokio::{ io::{AsyncBufReadExt, AsyncWriteExt, BufReader, Lines}, - process::{ChildStdin, ChildStdout, Command}, + process::{ChildStdin, ChildStdout}, time::timeout, }; @@ -47,7 +47,7 @@ async fn create_resumable_codex_thread_inner( client_version: &str, ) -> Result { let thread_cwd = cwd.canonicalize().unwrap_or_else(|_| cwd.to_path_buf()); - let mut command = Command::new(codex_bin); + let mut command = crate::credentials::scrubbed_command(codex_bin); command .arg("app-server") .arg("--listen") diff --git a/crates/relay-pty/src/credentials.rs b/crates/relay-pty/src/credentials.rs new file mode 100644 index 0000000000..46204f8e3a --- /dev/null +++ b/crates/relay-pty/src/credentials.rs @@ -0,0 +1,78 @@ +//! Relay-owned credentials a spawned process must not inherit from the +//! spawning process's environment. Every agent worker and every spawn-time +//! helper (MCP registration, model probes, session pre-creation) strips this +//! list first; a worker then receives only what it is meant to hold through an +//! explicit `Command::env`, which still wins after the scrub. + +use tokio::process::Command; + +pub const INHERITED_RELAY_CREDENTIAL_ENV_KEYS: &[&str] = &[ + // The broker's local HTTP API key (set on the broker process at startup). + "RELAY_BROKER_API_KEY", + // The node's control-plane credential. + "RELAY_NODE_TOKEN", + // The broker's own registration identity proof. + "RELAY_AGENT_IDENTITY_KEY", + // Whoever launched the broker; each worker gets its own. + "RELAY_AGENT_TOKEN", + "AGENT_RELAY_RESULT_TOKEN", + // Workspace credentials. Re-added from the broker's explicit worker + // environment when the broker delegates them; never taken from ambient + // environment. + "RELAY_API_KEY", + "RELAY_WORKSPACE_KEY", + "AGENT_RELAY_WORKSPACE_KEY", + "RELAY_WORKSPACES_JSON", +]; + +/// Remove [`INHERITED_RELAY_CREDENTIAL_ENV_KEYS`] from a command's inherited +/// environment. Call before applying the command's own environment: a later +/// `Command::env` for the same key still wins. +pub fn remove_inherited_relay_credentials(command: &mut Command) { + for key in INHERITED_RELAY_CREDENTIAL_ENV_KEYS { + command.env_remove(key); + } +} + +/// A `Command` for `program` that does not inherit relay credentials. Use it for +/// every process the broker starts on an agent's behalf, including spawn-time +/// helpers that never become the agent (MCP registration, model probes). +pub fn scrubbed_command(program: impl AsRef) -> Command { + let mut command = Command::new(program); + remove_inherited_relay_credentials(&mut command); + command +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn scrubbed_command_removes_every_relay_credential() { + let mut command = scrubbed_command("true"); + command.env("RELAY_AGENT_TOKEN", "own-token"); + let envs: std::collections::HashMap<_, _> = command + .as_std() + .get_envs() + .map(|(key, value)| { + ( + key.to_string_lossy().into_owned(), + value.map(|v| v.to_owned()), + ) + }) + .collect(); + for key in INHERITED_RELAY_CREDENTIAL_ENV_KEYS { + if *key == "RELAY_AGENT_TOKEN" { + continue; + } + assert_eq!(envs.get(*key), Some(&None), "{key} must be removed"); + } + // An explicit value set after construction still wins. + assert_eq!( + envs.get("RELAY_AGENT_TOKEN"), + Some(&Some("own-token".into())) + ); + // Everything else is left to normal inheritance. + assert!(!envs.contains_key("PATH")); + } +} diff --git a/crates/relay-pty/src/lib.rs b/crates/relay-pty/src/lib.rs index e3e6669b0b..2638c68d0d 100644 --- a/crates/relay-pty/src/lib.rs +++ b/crates/relay-pty/src/lib.rs @@ -40,6 +40,7 @@ pub mod ansi; pub mod codex_session; pub mod crash_insights; +pub mod credentials; pub mod detection; pub mod inject; pub mod pty;