hermes-admin/hermes-agent
1
0
Fork 0
Code Issues Pull Requests Actions 33 Packages Projects Releases Wiki Activity

fix(docker): keep forwarded secret values out of world-readable argv

Browse Source 
docker run/exec argv previously carried -e KEY=VALUE pairs for every
forwarded/passthrough variable. On Linux /proc/<pid>/cmdline is
world-readable regardless of process owner, so every allowlisted secret
was visible to all local users via plain ps for the duration of every
terminal call.

Emit name-only -e KEY flags and supply values via the docker client
subprocess env instead: the docker CLI resolves valueless --env KEY from
its own environment (documented docker/podman behavior), moving secrets
from /proc/*/cmdline (0444) to /proc/*/environ (0400). Covers the docker
run container-start path, the recreation/recovery path, the init-seeding
exec path, and the per-command runtime exec path.

Reported by @sashalab. Fixes #96268
This commit is contained in:
teknium1
2026-08-31 11:15:33 -07:00
committed by Teknium
parent db88b6c97c
commit d10ef89ee5
2 changed files with 161 additions and 37 deletions
Download Patch File Download Diff File Expand all files Collapse all files

135
tests/tools/test_docker_environment.py
View File

@@ -185,9 +185,12 @@ def test_init_env_args_uses_hermes_dotenv_for_allowlisted_env(monkeypatch):
monkeypatch.setattr(docker_env, "_load_hermes_env_vars", lambda: {"DATABASE_URL": "value_from_dotenv"})
args = env._build_init_env_args()
args_str = " ".join(args)
assert "DATABASE_URL=value_from_dotenv" in args_str
assert "-e" in args and "DATABASE_URL" in args
# Value must NOT be in argv (world-readable /proc/*/cmdline, #96268) —
# it travels via the docker client subprocess env instead.
assert not any("value_from_dotenv" in a for a in args)
assert env._init_env_values["DATABASE_URL"] == "value_from_dotenv"
def test_init_env_args_prefers_shell_env_over_hermes_dotenv(monkeypatch):
@@ -198,10 +201,10 @@ def test_init_env_args_prefers_shell_env_over_hermes_dotenv(monkeypatch):
monkeypatch.setattr(docker_env, "_load_hermes_env_vars", lambda: {"DATABASE_URL": "value_from_dotenv"})
args = env._build_init_env_args()
args_str = " ".join(args)
assert "DATABASE_URL=value_from_shell" in args_str
assert "value_from_dotenv" not in args_str
assert "DATABASE_URL" in args
assert env._init_env_values["DATABASE_URL"] == "value_from_shell"
assert not any("value_from_dotenv" in a for a in args)
def test_init_env_args_uses_hermes_dotenv_for_empty_shell_env(monkeypatch):
@@ -219,9 +222,9 @@ def test_init_env_args_uses_hermes_dotenv_for_empty_shell_env(monkeypatch):
args = env._build_init_env_args()
# Assert on the resolved value, not the printed -e flag: the disk value
# must win and a blank "MY_SECRET=" flag must never be emitted.
assert "MY_SECRET=value_from_dotenv" in args
assert "MY_SECRET=" not in args
# must win and a blank value must never be forwarded.
assert "MY_SECRET" in args
assert env._init_env_values["MY_SECRET"] == "value_from_dotenv"
def test_init_env_args_uses_active_profile_for_forwarded_env(monkeypatch):
@@ -239,8 +242,9 @@ def test_init_env_args_uses_active_profile_for_forwarded_env(monkeypatch):
ss.reset_secret_scope(token)
ss.set_multiplex_active(False)
assert "SERVICE_TOKEN=token-for-routed-profile" in args
assert "SERVICE_TOKEN=token-for-default" not in args
assert "SERVICE_TOKEN" in args
assert env._init_env_values["SERVICE_TOKEN"] == "token-for-routed-profile"
assert not any("token-for-default" in a for a in args)
def test_init_env_args_omits_missing_scoped_forwarded_env(monkeypatch):
@@ -258,8 +262,9 @@ def test_init_env_args_omits_missing_scoped_forwarded_env(monkeypatch):
ss.reset_secret_scope(token)
ss.set_multiplex_active(False)
assert "SERVICE_TOKEN=token-for-default" not in args
assert not any("token-for-default" in a for a in args)
assert "SERVICE_TOKEN" not in args
assert "SERVICE_TOKEN" not in env._init_env_values
def test_runtime_exec_tracks_scope_and_clears_missing_value(monkeypatch):
@@ -273,7 +278,7 @@ def test_runtime_exec_tracks_scope_and_clears_missing_value(monkeypatch):
monkeypatch.setattr(
docker_env,
"_popen_bash",
lambda cmd, stdin_data=None: calls.append((cmd, stdin_data)) or object(),
lambda cmd, stdin_data=None, **kw: calls.append((cmd, stdin_data, kw)) or object(),
)
ss.set_multiplex_active(True)
token = ss.set_secret_scope({"SERVICE_TOKEN": "token-for-profile-a"})
@@ -290,9 +295,16 @@ def test_runtime_exec_tracks_scope_and_clears_missing_value(monkeypatch):
ss.set_multiplex_active(False)
first_cmd = calls[0][0]
assert "SERVICE_TOKEN=token-for-profile-a" in first_cmd
# Name-only flag in argv; the value rides in the client subprocess env
# (issue #96268: keep secrets out of world-readable /proc/*/cmdline).
assert "SERVICE_TOKEN" in first_cmd
assert not any("token-for-profile-a" in str(a) for a in first_cmd)
first_env = calls[0][2].get("env") or {}
assert first_env.get("SERVICE_TOKEN") == "token-for-profile-a"
second_cmd = calls[1][0]
assert "SERVICE_TOKEN=token-for-profile-a" not in second_cmd
second_env = (calls[1][2].get("env") or {})
assert second_env.get("SERVICE_TOKEN") != "token-for-profile-a"
assert not any("token-for-profile-a" in str(a) for a in second_cmd)
assert "unset SERVICE_TOKEN" in second_cmd[-1]
@@ -312,15 +324,19 @@ def test_wrapped_exec_scopes_explicit_forward_env_across_profiles(monkeypatch, t
monkeypatch.setenv("EXPLICIT_TOKEN", "token-for-default")
monkeypatch.setattr(docker_env, "_load_hermes_env_vars", lambda: {})
def _run_fake_docker_exec(cmd, stdin_data=None):
def _run_fake_docker_exec(cmd, stdin_data=None, **kwargs):
"""Execute the generated docker exec command in a real local bash."""
container_index = cmd.index(env._container_id)
# Name-only -e flags (#96268): values come from the client env kwarg.
client_env = kwargs.get("env") or os.environ
child_env = os.environ.copy()
index = 2
while index < container_index:
assert cmd[index] == "-e"
key, value = cmd[index + 1].split("=", 1)
child_env[key] = value
key = cmd[index + 1]
assert "=" not in key, f"secret value leaked into argv: {key}"
if key in client_env:
child_env[key] = client_env[key]
index += 2
assert cmd[container_index + 1 : container_index + 3] == ["bash", "-c"]
return subprocess.Popen(
@@ -362,7 +378,7 @@ def test_wrapped_exec_scopes_explicit_forward_env_across_profiles(monkeypatch, t
def test_docker_env_appears_in_run_command(monkeypatch):
"""Explicit docker_env values should be passed via -e at docker run time."""
"""Explicit docker_env values pass via name-only -e + client env (#96268)."""
monkeypatch.setattr(docker_env, "find_docker", lambda: "/usr/bin/docker")
calls = _mock_subprocess_run(monkeypatch)
@@ -370,19 +386,23 @@ def test_docker_env_appears_in_run_command(monkeypatch):
run_calls = [c for c in calls if isinstance(c[0], list) and len(c[0]) >= 2 and c[0][1] == "run"]
assert run_calls, "docker run should have been called"
run_args = run_calls[0][0]
run_args, run_kwargs = run_calls[0]
run_args_str = " ".join(run_args)
assert "SSH_AUTH_SOCK=/run/user/1000/ssh-agent.sock" in run_args_str
assert "GNUPGHOME=/root/.gnupg" in run_args_str
# Names in argv, values ONLY in the client subprocess env (#96268).
assert "SSH_AUTH_SOCK" in run_args and "GNUPGHOME" in run_args
assert "/run/user/1000/ssh-agent.sock" not in run_args_str
assert "/root/.gnupg" not in run_args_str
client_env = run_kwargs.get("env") or {}
assert client_env.get("SSH_AUTH_SOCK") == "/run/user/1000/ssh-agent.sock"
assert client_env.get("GNUPGHOME") == "/root/.gnupg"
def _node_options_from_run(calls):
run_calls = [c for c in calls if isinstance(c[0], list) and len(c[0]) >= 2 and c[0][1] == "run"]
assert run_calls, "docker run should have been called"
args = run_calls[0][0]
for i, a in enumerate(args):
if a == "-e" and i + 1 < len(args) and args[i + 1].startswith("NODE_OPTIONS="):
return args[i + 1].split("=", 1)[1]
args, kwargs = run_calls[0]
if "NODE_OPTIONS" in args and "-e" in args:
return (kwargs.get("env") or {}).get("NODE_OPTIONS")
return None
@@ -415,10 +435,10 @@ def test_forward_env_overrides_docker_env_in_init_args(monkeypatch):
monkeypatch.setattr(docker_env, "_load_hermes_env_vars", lambda: {})
args = env._build_init_env_args()
args_str = " ".join(args)
assert "MY_KEY=dynamic_value" in args_str
assert "MY_KEY=static_value" not in args_str
assert "MY_KEY" in args
assert env._init_env_values["MY_KEY"] == "dynamic_value"
assert not any("static_value" in a for a in args)
def test_normalize_env_dict_filters_invalid_keys():
@@ -1646,3 +1666,62 @@ def test_extra_args_set_shm_size_helper():
assert docker_env._extra_args_set_shm_size(None) is False
# non-string entries must not crash (config.yaml can be malformed)
assert docker_env._extra_args_set_shm_size([42, None, "--shm-size=1g"]) is True
# ── issue #96268: secrets must never appear in docker argv ────────────
def test_forwarded_secret_values_never_in_argv(monkeypatch):
"""Regression #96268: values must not ride in `-e KEY=VALUE` argv.
/proc/<pid>/cmdline is world-readable on Linux, so any forwarded secret
placed in the docker client's argv is visible to every local user via
plain `ps`. Names go in argv (`-e KEY`); values go in the client
subprocess env (owner-only /proc/<pid>/environ).
"""
secret = "s3cr3t-gitlab-token-value"
env = _make_execute_only_env(["GITLAB_TOKEN"])
monkeypatch.setenv("GITLAB_TOKEN", secret)
monkeypatch.setattr(docker_env, "_load_hermes_env_vars", lambda: {})
# init path
init_args = env._build_init_env_args()
assert "GITLAB_TOKEN" in init_args
assert all(secret not in a for a in init_args)
assert env._init_env_values["GITLAB_TOKEN"] == secret
# runtime path
run_args, _unsets, values = env._build_runtime_env_args_with_unsets()
assert "GITLAB_TOKEN" in run_args
assert all(secret not in a for a in run_args)
assert values["GITLAB_TOKEN"] == secret
# _run_bash must put the value into the spawned client's env kwarg
calls = []
monkeypatch.setattr(
docker_env,
"_popen_bash",
lambda cmd, stdin_data=None, **kw: calls.append((cmd, kw)) or object(),
)
env._init_env_args = init_args
env._init_env_values = dict(env._init_env_values)
env._run_bash("true", login=True)
cmd, kw = calls[0]
assert all(secret not in str(a) for a in cmd)
assert (kw.get("env") or {}).get("GITLAB_TOKEN") == secret
def test_docker_run_secret_values_never_in_argv(monkeypatch):
"""Regression #96268 for the `docker run -d` container-start path."""
secret = "run-time-secret-value"
monkeypatch.setattr(docker_env, "find_docker", lambda: "/usr/bin/docker")
calls = _mock_subprocess_run(monkeypatch)
_make_dummy_env(env={"MY_TOKEN": secret})
run_calls = [c for c in calls if isinstance(c[0], list) and len(c[0]) >= 2 and c[0][1] == "run"]
assert run_calls, "docker run should have been called"
args, kwargs = run_calls[0]
assert "MY_TOKEN" in args
assert all(secret not in str(a) for a in args)
assert (kwargs.get("env") or {}).get("MY_TOKEN") == secret

63
tools/environments/docker.py
View File

@@ -939,6 +939,8 @@ class DockerEnvironment(BaseEnvironment):
self._container_name: str = ""
self._image_uses_s6_init: bool = False
self._all_run_args: list[str] = []
self._run_env_values: dict[str, str] = {}
self._init_env_values: dict[str, str] = {}
logger.info("DockerEnvironment volumes: %s", volumes)
# Ensure volumes is a list (config.yaml could be malformed)
if volumes is not None and not isinstance(volumes, list):
@@ -1308,9 +1310,16 @@ class DockerEnvironment(BaseEnvironment):
if not merged_env["NODE_OPTIONS"]:
merged_env.pop("NODE_OPTIONS", None)
# Name-only -e flags: the docker CLI resolves a valueless `--env KEY`
# from its own process environment, so the secret values travel via
# the spawned client's env (owner-readable /proc/*/environ) instead of
# its argv (world-readable /proc/*/cmdline). See issue #96268.
env_args = []
for key in sorted(merged_env):
env_args.extend(["-e", f"{key}={merged_env[key]}"])
env_args.extend(["-e", key])
# Values injected into the docker-client subprocess env at run time
# (also reused verbatim by the container-recreation recovery path).
self._run_env_values = dict(merged_env)
# Optional: run the container as the host user so files written into
# bind-mounted dirs (/workspace, /root, docker_volumes entries) are
@@ -1529,6 +1538,7 @@ class DockerEnvironment(BaseEnvironment):
timeout=120, # image pull may take a while
check=True,
stdin=subprocess.DEVNULL,
env=self._docker_client_env(self._run_env_values),
)
except (subprocess.CalledProcessError, subprocess.TimeoutExpired) as e:
# Docker may create the container object before `docker run`
@@ -1557,11 +1567,29 @@ class DockerEnvironment(BaseEnvironment):
# Initialize session snapshot inside the container
self.init_session()
def _docker_client_env(self, values: dict[str, str]) -> dict[str, str] | None:
"""Env for a spawned docker-client subprocess carrying forwarded values.
Name-only ``-e KEY`` flags make the docker CLI read each value from
its own process environment, keeping secrets out of the client's
world-readable ``/proc/<pid>/cmdline`` (issue #96268). Values live in
``/proc/<pid>/environ`` instead, which is owner/root-only.
Returns ``None`` (inherit as before) when there is nothing to add.
"""
if not values:
return None
client_env = dict(os.environ)
client_env.update(values)
return client_env
def _build_init_env_args(self) -> list[str]:
"""Build -e KEY=VALUE args for injecting host env vars into init_session.
"""Build name-only -e args for injecting host env vars into init_session.
These are used during init_session() so that export -p captures the
configured environment and the current profile's forwarded values.
The VALUES intentionally do not appear in the argv — they are passed
via the docker client subprocess env (see _docker_client_env and
issue #96268); the flags here are name-only ``-e KEY``.
"""
passthrough_env, unset_names = self._resolve_passthrough_env()
exec_env: dict[str, str] = dict(self._env)
@@ -1569,10 +1597,11 @@ class DockerEnvironment(BaseEnvironment):
for name in unset_names:
exec_env.pop(name, None)
self._init_unset_passthrough_names = tuple(sorted(unset_names))
self._init_env_values = dict(exec_env)
args = []
for key in sorted(exec_env):
args.extend(["-e", f"{key}={exec_env[key]}"])
args.extend(["-e", key])
return args
def _build_passthrough_env(self) -> dict[str, str]:
@@ -1619,16 +1648,22 @@ class DockerEnvironment(BaseEnvironment):
unset_names.add(key)
return exec_env, unset_names
def _build_runtime_env_args_with_unsets(self) -> tuple[list[str], tuple[str, ...]]:
"""Build runtime forwarding args plus names absent from the active scope."""
def _build_runtime_env_args_with_unsets(
self,
) -> tuple[list[str], tuple[str, ...], dict[str, str]]:
"""Build runtime forwarding args, names absent from scope, and values.
The returned args are name-only ``-e KEY`` flags (issue #96268); the
values dict must be injected into the docker client subprocess env.
"""
passthrough_env, unset_names = self._resolve_passthrough_env()
args = []
for key in sorted(passthrough_env):
args.extend(["-e", f"{key}={passthrough_env[key]}"])
return args, tuple(sorted(unset_names))
args.extend(["-e", key])
return args, tuple(sorted(unset_names)), dict(passthrough_env)
def _build_runtime_env_args(self) -> list[str]:
"""Build only dynamic forwarded values for a non-login command."""
"""Build only dynamic forwarded names for a non-login command."""
return self._build_runtime_env_args_with_unsets()[0]
def _run_bash(self, cmd_string: str, *, login: bool = False,
@@ -1643,11 +1678,17 @@ class DockerEnvironment(BaseEnvironment):
# Init seeds the snapshot. Profile-scoped passthrough values are also
# injected on every later command because this container can be shared
# by multiple routed profiles in one gateway process.
# Env flags are name-only; values travel via the client subprocess
# env so they never hit world-readable /proc/*/cmdline (#96268).
unset_names: tuple[str, ...] = ()
env_values: dict[str, str] = {}
if login:
cmd.extend(self._init_env_args)
env_values = dict(getattr(self, "_init_env_values", {}))
elif self._profile_scoped_passthrough:
runtime_args, unset_names = self._build_runtime_env_args_with_unsets()
runtime_args, unset_names, env_values = (
self._build_runtime_env_args_with_unsets()
)
cmd.extend(runtime_args)
if login:
@@ -1663,6 +1704,9 @@ class DockerEnvironment(BaseEnvironment):
else:
cmd.extend(["bash", "-c", cmd_string])
client_env = self._docker_client_env(env_values)
if client_env is not None:
return _popen_bash(cmd, stdin_data, env=client_env)
return _popen_bash(cmd, stdin_data)
# ------------------------------------------------------------------
@@ -1741,6 +1785,7 @@ class DockerEnvironment(BaseEnvironment):
result = subprocess.run(
run_cmd, capture_output=True, text=True, encoding='utf-8', errors='replace', timeout=120, check=True,
stdin=subprocess.DEVNULL,
env=self._docker_client_env(self._run_env_values),
)
self._container_id = result.stdout.strip()
self._container_name = new_name