From d10ef89ee5292c1ffa98258601eff508a01a8abc Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Mon, 31 Aug 2026 11:15:33 -0700 Subject: [PATCH] fix(docker): keep forwarded secret values out of world-readable argv docker run/exec argv previously carried -e KEY=VALUE pairs for every forwarded/passthrough variable. On Linux /proc//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 --- tests/tools/test_docker_environment.py | 135 ++++++++++++++++++++----- tools/environments/docker.py | 63 ++++++++++-- 2 files changed, 161 insertions(+), 37 deletions(-) diff --git a/tests/tools/test_docker_environment.py b/tests/tools/test_docker_environment.py index 8666543227..54082f6ed1 100644 --- a/tests/tools/test_docker_environment.py +++ b/tests/tools/test_docker_environment.py @@ -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//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//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 diff --git a/tools/environments/docker.py b/tools/environments/docker.py index 0d17249f6f..c54d1cf71e 100644 --- a/tools/environments/docker.py +++ b/tools/environments/docker.py @@ -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//cmdline`` (issue #96268). Values live in + ``/proc//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