feat(docker): sandbox egress control (on/off/allowlist) with filtered proxy - #58601
feat(docker): sandbox egress control (on/off/allowlist) with filtered proxy#58601suzu784 wants to merge 6 commits into
Conversation
6ae1dd2 to
82d04bc
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the substantial, fail-closed design and proxy coverage. It needs a few security-boundary fixes before salvage.
Problems
tools/code_execution_tool.py:680-688andtools/file_tools.py:1141-1151independently construct Docker configs but do not receive either new egress key. Unlike the terminal path, those Docker sandboxes fall back toDockerEnvironment's default network mode.- Current reuse protection only inspects a reused container when legacy
networkis false (tools/environments/docker.py:913-917). The new allowlist mode leaves that boolean true, so a labeled bridge container can be reused after an operator changes to allowlist mode. tests/tools/test_egress_proxy.py:436-442reads source text instead of executing configuration behavior.
Suggested changes
- Propagate both settings through file and code-execution paths, with behavioral coverage for all three creation paths.
- Make reuse validate the expected allowlist network and replace stale bridge/different-allowlist containers.
- Replace the source-shape test with CLI/gateway propagation tests; update
website/docs/user-guide/configuration.mdalongside the example config.
Automated hermes-sweeper review.
| # Resolve effective egress mode. ``network_mode`` (on/off/allowlist) is the | ||
| # primary control; the legacy ``network`` bool is kept for backwards compat | ||
| # (network=False == network_mode="off"). See _SECURITY_ARGS / egress_proxy. | ||
| self._network_mode = _resolve_network_mode(network_mode, network) |
There was a problem hiding this comment.
Allowlist mode also needs to update the cross-process reuse guard. On current main that guard only checks not network, so a previously bridge-networked container with the same task/profile labels will be reused after this setting changes to allowlist, bypassing the intended boundary. Validate the expected internal network and replace mismatches.
| """ | ||
| repo = Path(__file__).resolve().parents[2] | ||
| for rel in ("cli.py", "gateway/run.py"): | ||
| src = (repo / rel).read_text(encoding="utf-8") |
There was a problem hiding this comment.
Please replace this source-text assertion with a behavioral propagation test. Repository policy forbids tests that read source files: this can pass while runtime wiring is broken and blocks harmless refactors.
|
Thanks for the review — all three findings were valid. Addressed in cb26d8698 + 98ca78afe: 1. Egress keys not propagated to the code-execution / file-tools Docker paths → fixed in 98ca78afe. Both 2. Reuse guard skipped for allowlist mode → fixed in cb26d8698. The
3. Source-text test → replaced in 98ca78afe with behavioral bridge tests: one executes the real |
… proxy Add a third container egress mode "allowlist" alongside on/off: sandboxes attach to a per-allowlist --internal network and reach the internet only through a shared, dual-homed stdlib forward proxy (default-deny, TLS pass-through, no certificate interception). Raw/non-proxied egress has no route out. - wire terminal.container_network(+_allowlist) end-to-end: cli.py env_mappings, gateway/run.py _terminal_env_map, _get_env_config(); the DockerEnvironment layer alone was inert without these bridges - fail closed everywhere: unknown mode -> "off"; proxy provisioning or outbound-network connect failure -> --network=none (never open network) - IP-literal allowlist entries require an exact match (no subdomain logic) - label egress networks/proxies (hermes.egress) and prune unused ones from cleanup_all_environments(); proxies are shared per-allowlist hash - resolve the outbound network name by runtime (docker: bridge, podman) - skip the client Host header when re-serializing plain-HTTP forwards - warn when docker_env overrides the injected proxy env - show egress mode in `hermes config`; document fail-closed + prune in cli-config.yaml.example - tests: provisioning/restart/race/fail-closed orchestration paths, real proxy-server integration over loopback (403 deny, CONNECT tunnel, plain forward, 502), prune keep/remove, and config-wiring regression guards Verified E2E against a live Docker daemon: allowlisted domain 200 via proxy, unlisted domain 403, raw socket egress unreachable, config propagation from env vars, prune leaves no leftovers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013H5Sx95bUybAUgVk2rZ1pq
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013n8YAnnTrwfzD5oECy6zdi
The reuse guard only inspected NetworkMode when the legacy network bool
was false, so switching to container_network: "allowlist" (which leaves
that bool true) reused stale bridge containers — label-only reuse handed
the agent unfiltered egress despite the lockdown config. A changed
allowlist had the same hole: the old hermes-egress-<hash> network no
longer matches, but the container was reused anyway.
The guard now compares the reused container's NetworkMode against the
effective expectation per mode: "off" and "allowlist" require an exact
match ("none" / the current allowlist's hermes-egress-<hash> network,
failing closed on inspect failure), while "on" only replaces containers
stranded on a hermes-egress-* internal network (no route out under an
open-network config) and keeps deliberate --network=none containers
untouched. expected_network_name() exposes the provisioning name
derivation to the guard without provisioning anything.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NgaecGn2RaqRRmwd6mt4XU
…aths execute_code and the file tools build their container_config dicts independently of terminal_tool, and neither included container_network / container_network_allowlist — so those two Docker sandboxes silently fell back to full network while only the terminal path honored the egress config. Add the two keys, mirroring terminal_tool's own dict. Behavioral coverage replaces the source-shape assertions: - creation-path tests drive the real _get_env_config → container_config → _create_environment chain for terminal / execute_code / file tools and assert the settings reach DockerEnvironment (the two fixed paths fail with network_mode=None without the fix), - the source-grep bridge test is replaced by tests that execute the real cli.load_cli_config() and gateway/run.py import-time bridges and assert the TERMINAL_CONTAINER_NETWORK* env vars get set (config.yaml winning over a stale .env on the gateway side). Document container_network / container_network_allowlist in the user guide alongside the example config. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NgaecGn2RaqRRmwd6mt4XU
…actor Upstream replaced file_tools' _last_known_cwd registry with the session cwd record (get_session_cwd / record_session_cwd), so the creation-path test no longer needs to reset it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NgaecGn2RaqRRmwd6mt4XU
98ca78a to
fa43479
Compare
|
Heads-up on an interaction with a feature that just landed on Both features claim the same container-side surface:
None of this is a rejection signal — default-deny reachability control and credential injection are complementary layers. But the PR needs an explicit answer for |
What does this PR do?
Adds a third egress mode for Docker sandboxes:
terminal.container_network: "allowlist", alongside the existing on/off (network: bool). In allowlist mode, sandboxes attach to a per-allowlist--internalnetwork and reach the internet only through a shared, dual-homed, stdlib-only forward proxy — default-deny, TLS pass-through via CONNECT (no certificate interception). Raw / non-proxied egress has no route out.The threat model is the one already described in
docs/security/network-egress-isolation.md(prompt-injected data exfiltration viacurl/wgetfrom inside a sandbox). That doc covers an ops-level pattern (compose + external squid/envoy); this PR provides the same control in-app, per-sandbox, auto-provisioned, with no external infrastructure. The two are complementary — happy to add a cross-reference to that doc if desired.Related Issue
No linked issue — feature proposal. Closest prior art is the deployment pattern in
docs/security/network-egress-isolation.md.Type of Change
Changes Made
tools/environments/egress_proxy.py(new): network/proxy provisioning — one--internalnetwork + one shared proxy container per allowlist hash, labeledhermes.egress, pruned fromcleanup_all_environments()tools/environments/egress_proxy_server.py(new): self-contained stdlib proxy, mounted read-only intopython:3.13-slim(CONNECT tunnel, absolute-URI plain HTTP forward, 403 deny, 502 upstream-failure)tools/environments/docker.py:DockerEnvironment(network_mode=, network_allowlist=), mode resolution + allowlist normalization; legacydocker_network: falseremains a hard deny even when new defaults are presentcli.pyenv_mappings,gateway/run.py_terminal_env_map,tools/terminal_tool.py_get_env_config()/_create_environment(),hermes_cli/config.pyTERMINAL_CONFIG_ENV_MAP+ defaults +hermes configdisplaycli-config.yaml.example: documentedcontainer_network/container_network_allowlistincl. fail-closed and prune semantics, plus the explicit scope boundary that Codex app-server commands use Codex's separate sandboxscripts/release.py: AUTHOR_MAP entry for this contributor (contributor-check)Design points:
"off"; proxy provisioning or outbound-connect failure →--network=none; legacydocker_network: falsecannot be weakened bycontainer_network: "on"(never silently open)docker_envoverrides the injected proxy env varsHow to Test
bash scripts/run_tests.sh tests/tools/test_egress_proxy.py tests/tools/test_docker_environment.py— 110 tests (provisioning/restart/race/fail-closed orchestration, real proxy-server integration over loopback, prune keep/remove, config-wiring regression guards)terminal.container_network: "allowlist"andcontainer_network_allowlist: ["pypi.org"]→ inside the sandbox,https://pypi.orgreturns 200 via the proxy,https://example.comfails withTunnel connection failed: 403 Forbidden, raw-socket egress is unreachableChecklist
Code
scripts/run_tests.sh, 110 tests); relying on CI for the full matrix — my host lacks some optional deps (whisper/fastapi/mautrix) so a full local run has known env-caused failures unrelated to this changeDocumentation & Housekeeping
cli-config.yaml.example, docstrings)cli-config.yaml.examplefor the new config keysCONTRIBUTING.md/AGENTS.md— N/A (no architecture/workflow change)🤖 Generated with Claude Code
https://claude.ai/code/session_013n8YAnnTrwfzD5oECy6zdi