Repository navigation
Harden the dormant MCP secret-file helper to use exec stdin - #277
Conversation
OpenShellSpawner wrote MCP secret header files by embedding the base64-encoded value in the exec command. The gateway joins exec env and command into one string and logs a prefix of it, so secret material should not travel there. Write MCP secret files with a fixed command (umask 077; cat > "$1") and pass the bytes on exec stdin, with the path as a positional argument. Reject secrets too large to fit in a single exec request with a clear error, which goes through spawn()'s existing sandbox cleanup. Also keep MCP secret values out of the env passed to the sandbox server's exec, matching how provider credentials are filtered. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
exec_stream() does not raise on a nonzero exit, so a failed mkdir or secret-file write previously let spawn() continue without the file. Collect the exec result for both and raise RuntimeError on a nonzero exit, following _materialize_allowed_skills(); spawn()'s existing cleanup then deletes the sandbox. Error messages carry the path and stderr only. Also filter MCP secret names out of spec.environment, the same way provider credentials are filtered, so a caller-supplied secret value reaches the sandbox only as its mounted file. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The sandbox server's exec env merged the spawner's extra_env after the caller env had been filtered, so an extra_env entry whose key matched a provider credential or MCP secret key was passed through. Merge first, then drop credential and MCP secret keys from the final env. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
beesarmy
left a comment
beesarmy
left a comment
There was a problem hiding this comment.
Reviewed merge-result head 3ffb2a1 against base b0104d0. The target-relative diff contains the previously reviewed dormant-helper hardening only; the integration with the exec-output fixes introduces no blocking finding.
The helper sends credential file contents through exec stdin rather than logged command/environment fields, checks write/mkdir exit status, and filters identified secret keys after the server environment merge. Inspected source and regression tests, the installed SDK stdin interface, and the referenced gateway command assembly/logging code. This was read-only review; I did not independently rerun local tests.
Confirmed exact-head build, lint, unit-tests and e2e checks succeeded. The integration workflow is not triggered for these paths: its PR filter covers src/cloud_agents/workflow/** and tests/integration/**, while this diff changes the spawner and its unit tests only. That is not an integration-test pass.
Approval is limited to hardening this existing dormant helper. It does not activate secret-bearing MCP, establish end-to-end credential delivery, or prove gateway/image/Landlock behavior. Current producers do not populate the secret-value input. Existing-file/symlink safety, agent-readable credential files, diagnostic redaction and plain-header exposure remain outside this change; no broader confidentiality or deployment guarantee is implied.
…#277 MCP secret delivery) Conflicts were confined to import blocks and one test pair, all resolved by keeping both sides: - openshell_spawner.py, step_runner.py, activities.py: keep upstream's AGENT_EVENT_LOG_PATH import from spawner.base alongside this branch's spawner.lifetime imports. - tests/unit/workflow/test_step_runner.py: keep both new tests, test_run_step_passes_per_step_lifetime and test_run_step_sets_shared_agent_event_log. lightspeed-core#277's post-create work in _do_spawn (the _inject_mcp_secrets call and the server env filtered after merging extra_env) merged cleanly into _create_and_start, so MCP secret writes now run inside the spawn budget that starts at the expires-at stamp, and a failed or stalled write goes through the bounded BaseException-safe cleanup that deletes the exact created sandbox. The mcp_secret_keys filter on spec.environment stays before CreateSandbox. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Since the merge of lightspeed-core#277, MCP secret files are written within _create_and_start, under the spawn budget. Regressions using the real OpenShellSpawner against a fake gateway: - a failing secret write deletes exactly the created sandbox and restores the active count; - a hanging secret write is cut off by the spawn budget, raising the deadline error, and the sandbox is deleted; - the MCP secret key stays out of spec.environment while other env passes through (read from either the real protobuf map or the session-wide openshell stub that test_openshell_spawner.py may install). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Summary
OpenShellSpawner._inject_mcp_secretswrote each MCP secret file with an exec command that embedded the base64-encoded value in the command string. The OpenShell gateway joins exec env and argv into one command string and logs a preview of it, so secret material in argv or env can reach gateway logs. Stdin is forwarded separately.This PR hardens that helper. It does not fix an active production leak, and it does not enable secret delivery. Neither production caller (
workflow/executor/temporal/activities.py,workflow/core/step_runner.py) passes MCP secret values tospawn()today, so_inject_mcp_secretscurrently logs a warning and skips every secret. The helper is dormant; this makes it safer if it is wired up later.Changes
_write_secret_fileruns a fixed command,sh -c 'umask 077; cat > "$1"' sh <path>, with the secret on exec stdin. The path is a separate argument.mkdircheck the exec exit code and raise on failure. The error message contains no secret content, andspawn()'s existing cleanup deletes the sandbox.spec.environmentand out of the server process's exec env, the same way provider credentials already are. The server env is now filtered after merging the spawner'sextra_env, soextra_envcan't reintroduce those keys.Testing
spec.environment;extra_envre-introduction./var/secrets/mcpin a real sandbox are unverified.Not in scope
headersvalues from catalog entries are serialized intoLIGHTSPEED_MCP_SERVERS, which reachesspec.environmentand the server's exec env. Moving them out needs a matching change in the sandbox image.umask 077only affects newly created files; the helper does not handle an existing file or symlink at the target path, and error messages include raw remote stderr. These need addressing before secret-bearing MCP delivery is enabled._do_write_filestill embeds base64 content in the command; its only caller writes conversation messages, not credentials.🤖 Generated with Claude Code