From 3167dbaee2aebc257dc61d569ec380b145f35a58 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Sun, 5 Jul 2026 14:13:15 -0700 Subject: [PATCH] fix(docker): widen docker_network to file/code-exec paths + guard container reuse Follow-up to the salvaged toggle commit: - file_tools.py / code_execution_tool.py: carry docker_network in their container_config dicts so those environment-creation paths honor the lockdown instead of silently defaulting back to bridge (the probe/exec asymmetry class reported on #46358). - docker.py: cross-process reuse now inspects HostConfig.NetworkMode when docker_network=false and removes a mismatched (networked) container before starting a fresh air-gapped one. Fails closed when inspect fails. Default-network config never churns containers, so operators using docker_extra_args --network=none are unaffected. - tests: AST invariant that every container_config site carrying docker_run_as_host_user also carries docker_network, plus three reuse guard tests (reject bridge under lockdown / keep matching none / no inspect when network enabled). - docs: configuration.md gains terminal.docker_network + env var row. --- tests/tools/test_docker_network_config.py | 95 +++++++++++++++++++++++ tools/code_execution_tool.py | 1 + tools/environments/docker.py | 73 +++++++++++++++++ tools/file_tools.py | 1 + website/docs/user-guide/configuration.md | 4 + 5 files changed, 174 insertions(+) diff --git a/tests/tools/test_docker_network_config.py b/tests/tools/test_docker_network_config.py index f2c3c13f2cc..776f5ef6d7c 100644 --- a/tests/tools/test_docker_network_config.py +++ b/tests/tools/test_docker_network_config.py @@ -82,3 +82,98 @@ def test_docker_network_config_is_bridged_everywhere(): assert "docker_network" in _gateway_env_map_keys() assert "docker_network" in _save_config_env_sync_keys() assert "TERMINAL_DOCKER_NETWORK" in _terminal_tool_env_var_names() + + +def test_sibling_container_config_sites_carry_docker_network(): + """Every container_config dict that carries docker_run_as_host_user must + also carry docker_network — otherwise that code path silently falls back + to networked containers while the terminal path honors the lockdown + (the probe/exec asymmetry reported on issue #46358). + """ + import ast + import inspect + + import tools.code_execution_tool as code_execution_tool + import tools.file_tools as file_tools + + for module in (terminal_tool, file_tools, code_execution_tool): + tree = ast.parse(inspect.getsource(module)) + sites = 0 + for node in ast.walk(tree): + if not isinstance(node, ast.Dict): + continue + keys = {k.value for k in node.keys if isinstance(k, ast.Constant)} + if "docker_run_as_host_user" in keys: + sites += 1 + assert "docker_network" in keys, ( + f"{module.__name__} builds a container_config with " + f"docker_run_as_host_user but without docker_network " + f"(line {node.lineno})" + ) + assert sites >= 1, f"expected at least one container_config site in {module.__name__}" + + +def _reuse_guard_harness(monkeypatch, *, existing_mode: str, network: bool): + """Drive DockerEnvironment through the cross-process reuse path with a + fake existing container whose NetworkMode is *existing_mode*. + + Returns the list of docker commands issued. + """ + commands = [] + + def fake_run(cmd, *args, **kwargs): + commands.append(cmd) + + class Result: + returncode = 0 + stderr = "" + stdout = "" + + if len(cmd) > 1 and cmd[1] == "ps": + Result.stdout = "existing-container-id\trunning\n" + elif len(cmd) > 1 and cmd[1] == "inspect": + Result.stdout = f"{existing_mode}\n" + elif len(cmd) > 1 and cmd[1] == "run": + Result.stdout = "fresh-container-id\n" + return Result() + + monkeypatch.setattr(docker_env, "find_docker", lambda: "/usr/bin/docker") + monkeypatch.setattr(docker_env.subprocess, "run", fake_run) + monkeypatch.setattr(docker_env.DockerEnvironment, "_storage_opt_supported", lambda self: False) + + docker_env.DockerEnvironment( + image="python:3.11", + cwd="/workspace", + timeout=60, + task_id="reuse-guard-test", + network=network, + persist_across_processes=True, + ) + return commands + + +def test_reuse_rejects_networked_container_when_lockdown_requested(monkeypatch): + commands = _reuse_guard_harness(monkeypatch, existing_mode="bridge", network=False) + + assert any(cmd[1:3] == ["rm", "-f"] for cmd in commands), ( + "bridge-networked container must be removed when docker_network=false" + ) + run_cmd = next(cmd for cmd in commands if len(cmd) > 2 and cmd[1:3] == ["run", "-d"]) + assert "--network=none" in run_cmd + + +def test_reuse_keeps_airgapped_container_when_lockdown_requested(monkeypatch): + commands = _reuse_guard_harness(monkeypatch, existing_mode="none", network=False) + + assert not any(cmd[1] == "rm" for cmd in commands) + assert not any(cmd[1] == "run" for cmd in commands), "matching container must be reused" + + +def test_reuse_skips_inspect_when_network_enabled(monkeypatch): + commands = _reuse_guard_harness(monkeypatch, existing_mode="none", network=True) + + # Default-network config never churns containers, even air-gapped ones + # (operators may have created them via docker_extra_args). + assert not any(cmd[1] == "inspect" for cmd in commands) + assert not any(cmd[1] == "rm" for cmd in commands) + assert not any(cmd[1] == "run" for cmd in commands) diff --git a/tools/code_execution_tool.py b/tools/code_execution_tool.py index 349d4810dfb..707789b9f6b 100644 --- a/tools/code_execution_tool.py +++ b/tools/code_execution_tool.py @@ -684,6 +684,7 @@ def _get_or_create_env(task_id: str): "container_persistent": config.get("container_persistent", True), "docker_volumes": config.get("docker_volumes", []), "docker_run_as_host_user": config.get("docker_run_as_host_user", False), + "docker_network": config.get("docker_network", True), } ssh_config = None diff --git a/tools/environments/docker.py b/tools/environments/docker.py index 74f9fa82c8b..ea4a6ec77e6 100644 --- a/tools/environments/docker.py +++ b/tools/environments/docker.py @@ -897,6 +897,45 @@ class DockerEnvironment(BaseEnvironment): reused = False if persist_across_processes: existing = self._find_reusable_container(task_label, profile_name) + if existing is not None: + container_id, state = existing + # Network-mode guard: reuse must not silently defeat an + # egress lockdown. A container created before the operator + # set ``docker_network: false`` keeps its original bridge + # NetworkMode, so label-only reuse would hand the agent a + # networked container despite the config. On mismatch we + # remove the stale container and start fresh — leaving it in + # place would let the next label-based reuse pick it up again. + # Only the lockdown direction is guarded: a ``none``-mode + # container under a default-network config is left alone so + # operators using ``docker_extra_args: ["--network=none"]`` + # don't get their container churned on every startup. + mode_mismatch = False + actual_mode = None + if not network: + actual_mode = self._container_network_mode(container_id) + mode_mismatch = actual_mode != "none" + if mode_mismatch: + logger.warning( + "Existing container %s has NetworkMode=%s but " + "docker_network=false requests an air-gapped " + "container — removing it and starting fresh " + "(task=%s, profile=%s).", + container_id[:12], actual_mode or "unknown", + task_label, profile_name, + ) + try: + subprocess.run( + [self._docker_exe, "rm", "-f", container_id], + capture_output=True, + text=True, + timeout=30, + check=False, + stdin=subprocess.DEVNULL, + ) + except (subprocess.TimeoutExpired, OSError) as e: + logger.warning("Failed to remove mismatched container %s: %s", container_id[:12], e) + existing = None if existing is not None: container_id, state = existing self._container_id = container_id @@ -1194,6 +1233,40 @@ class DockerEnvironment(BaseEnvironment): logger.debug("Docker --storage-opt support: %s", _storage_opt_ok) return _storage_opt_ok + def _container_network_mode(self, container_id: str) -> Optional[str]: + """Return the container's ``HostConfig.NetworkMode`` (e.g. ``bridge``, + ``none``, ``host``), or ``None`` when inspection fails. + + Used by the reuse path to make sure a persisted container's network + mode still matches the operator's ``docker_network`` setting; callers + treat ``None`` (unknown) as a mismatch when lockdown was requested, + so a failed inspect fails closed rather than open. + """ + try: + result = subprocess.run( + [ + self._docker_exe, "inspect", + "--format", "{{.HostConfig.NetworkMode}}", + container_id, + ], + capture_output=True, + text=True, + timeout=10, + check=False, + stdin=subprocess.DEVNULL, + ) + except (subprocess.TimeoutExpired, OSError) as e: + logger.debug("docker inspect NetworkMode failed: %s", e) + return None + if result.returncode != 0: + logger.debug( + "docker inspect NetworkMode returned %d: %s", + result.returncode, result.stderr.strip(), + ) + return None + mode = result.stdout.strip() + return mode or None + def _find_reusable_container(self, task_label: str, profile_label: str) -> Optional[tuple[str, str]]: """Look for an existing container labeled for this (task, profile). diff --git a/tools/file_tools.py b/tools/file_tools.py index 0ebded2f6f7..299799fcce8 100644 --- a/tools/file_tools.py +++ b/tools/file_tools.py @@ -1073,6 +1073,7 @@ def _get_file_ops(task_id: str = "default") -> ShellFileOperations: "docker_mount_cwd_to_workspace": config.get("docker_mount_cwd_to_workspace", False), "docker_forward_env": config.get("docker_forward_env", []), "docker_run_as_host_user": config.get("docker_run_as_host_user", False), + "docker_network": config.get("docker_network", True), } ssh_config = None diff --git a/website/docs/user-guide/configuration.md b/website/docs/user-guide/configuration.md index 93df14b4b8d..76801083e7a 100644 --- a/website/docs/user-guide/configuration.md +++ b/website/docs/user-guide/configuration.md @@ -219,6 +219,7 @@ terminal: docker_extra_args: # Extra flags appended verbatim to `docker run` - "--gpus=all" - "--network=host" + docker_network: true # false = air-gap the container (--network=none) # Resource limits container_cpu: 1 # CPU cores (0 = unlimited) @@ -240,6 +241,8 @@ terminal: **`terminal.docker_extra_args`** (also overridable via `TERMINAL_DOCKER_EXTRA_ARGS='["--gpus=all"]'`) lets you pass arbitrary `docker run` flags that Hermes doesn't surface as first-class keys — `--gpus`, `--network`, `--add-host`, alternative `--security-opt` overrides, etc. Each entry must be a string; the list is appended last to the assembled `docker run` invocation so it can override Hermes' defaults if needed. Use sparingly — flags that conflict with the sandbox hardening (capability drops, `--user`, the workspace bind mount) will silently weaken isolation. +**`terminal.docker_network`** (default `true`; env: `TERMINAL_DOCKER_NETWORK`) — set to `false` to run the sandbox container with `--network=none`, cutting off all network egress from agent commands. This applies to the execution container used by `terminal`, `execute_code`, and the file tools. Because containers persist across Hermes processes, flipping this to `false` while an older networked container exists will remove that container and start a fresh air-gapped one (a warning is logged); background processes running inside it are lost. Prefer this key over passing `--network=none` through `docker_extra_args`. + **Requirements:** Docker Desktop or Docker Engine installed and running. Hermes probes `$PATH` plus common macOS install locations (`/usr/local/bin/docker`, `/opt/homebrew/bin/docker`, Docker Desktop app bundle). Podman is supported out of the box: set `HERMES_DOCKER_BINARY=podman` (or the full path) to force it when both are installed. #### Container lifecycle @@ -291,6 +294,7 @@ Every key under `terminal:` has an env-var override of the form `TERMINAL_