mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-08 13:12:08 +00:00
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.
This commit is contained in:
parent
cd2b360d64
commit
3167dbaee2
5 changed files with 174 additions and 0 deletions
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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).
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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_<KEY_U
|
|||
| `TERMINAL_DOCKER_EXTRA_ARGS` | `docker_extra_args` | JSON array |
|
||||
| `TERMINAL_DOCKER_MOUNT_CWD_TO_WORKSPACE` | `docker_mount_cwd_to_workspace` | `true` / `false` |
|
||||
| `TERMINAL_DOCKER_RUN_AS_HOST_USER` | `docker_run_as_host_user` | `true` / `false` |
|
||||
| `TERMINAL_DOCKER_NETWORK` | `docker_network` | `true` / `false` — default `true`; `false` = `--network=none` |
|
||||
| `TERMINAL_DOCKER_PERSIST_ACROSS_PROCESSES` | `docker_persist_across_processes` | `true` / `false` — default `true` |
|
||||
| `TERMINAL_DOCKER_ORPHAN_REAPER` | `docker_orphan_reaper` | `true` / `false` — default `true` |
|
||||
| `TERMINAL_CONTAINER_CPU` | `container_cpu` | CPU cores |
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue