Merge pull request #17305 from NousResearch/feat/docker-run-as-host-user

feat(docker): run container as host user to avoid root-owned bind mounts

tests/tools/test_docker_environment.py

@@ -45,6 +45,7 @@ def _make_dummy_env(**kwargs):
         host_cwd=kwargs.get("host_cwd"),
         auto_mount_cwd=kwargs.get("auto_mount_cwd", False),
         env=kwargs.get("env"),
+        run_as_host_user=kwargs.get("run_as_host_user", False),
     )
 
 
@@ -384,9 +385,10 @@ def test_normalize_env_dict_rejects_complex_values():
     assert result == {"GOOD": "string"}
 
 
-def test_security_args_include_setuid_setgid_for_gosu_drop():
-    """_SECURITY_ARGS must include SETUID and SETGID so the image entrypoint
-    can drop from root to the non-root `hermes` user via gosu.
+def test_security_args_include_setuid_setgid_for_gosu_drop(monkeypatch):
+    """The default (run_as_host_user=False) invocation must include SETUID and
+    SETGID caps so the image entrypoint can drop from root to the non-root
+    `hermes` user via gosu.
 
     Without these caps gosu exits with
     ``error: failed switching to 'hermes': operation not permitted``
@@ -396,17 +398,117 @@ def test_security_args_include_setuid_setgid_for_gosu_drop():
     after the drop — the drop is a one-way transition performed before the
     `no_new_privs` bit is enforced on the exec boundary.
     """
-    args = docker_env._SECURITY_ARGS
+    monkeypatch.setattr(docker_env, "find_docker", lambda: "/usr/bin/docker")
+    calls = _mock_subprocess_run(monkeypatch)
+
+    _make_dummy_env()
+
+    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]
 
-    # Flatten to set of added caps for clarity.
     added = {
-        args[i + 1]
-        for i, flag in enumerate(args[:-1])
+        run_args[i + 1]
+        for i, flag in enumerate(run_args[:-1])
         if flag == "--cap-add"
     }
     assert "SETUID" in added, "SETUID cap missing — gosu drop in entrypoint will fail"
     assert "SETGID" in added, "SETGID cap missing — gosu drop in entrypoint will fail"
 
-    # Sanity: the hardening posture is still in place.
-    assert "--cap-drop" in args and "ALL" in args
-    assert "--security-opt" in args and "no-new-privileges" in args
+
+# ── run_as_host_user tests ────────────────────────────────────────
+
+
+def test_run_as_host_user_passes_uid_gid(monkeypatch):
+    """With run_as_host_user=True, --user <uid>:<gid> is added to docker run."""
+    monkeypatch.setattr(docker_env, "find_docker", lambda: "/usr/bin/docker")
+    monkeypatch.setattr(docker_env.os, "getuid", lambda: 1234, raising=False)
+    monkeypatch.setattr(docker_env.os, "getgid", lambda: 5678, raising=False)
+    calls = _mock_subprocess_run(monkeypatch)
+
+    _make_dummy_env(run_as_host_user=True)
+
+    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]
+
+    # --user must be present and must be paired with "1234:5678"
+    assert "--user" in run_args, f"--user flag missing from docker run args: {run_args}"
+    idx = run_args.index("--user")
+    assert run_args[idx + 1] == "1234:5678", (
+        f"expected --user 1234:5678, got --user {run_args[idx + 1]}"
+    )
+
+
+def test_run_as_host_user_drops_setuid_setgid_caps(monkeypatch):
+    """When --user is passed, the container never needs gosu, so SETUID/SETGID
+    caps are omitted for a tighter security posture."""
+    monkeypatch.setattr(docker_env, "find_docker", lambda: "/usr/bin/docker")
+    monkeypatch.setattr(docker_env.os, "getuid", lambda: 1000, raising=False)
+    monkeypatch.setattr(docker_env.os, "getgid", lambda: 1000, raising=False)
+    calls = _mock_subprocess_run(monkeypatch)
+
+    _make_dummy_env(run_as_host_user=True)
+
+    run_calls = [c for c in calls if isinstance(c[0], list) and len(c[0]) >= 2 and c[0][1] == "run"]
+    run_args = run_calls[0][0]
+
+    added = {
+        run_args[i + 1]
+        for i, flag in enumerate(run_args[:-1])
+        if flag == "--cap-add"
+    }
+    assert "SETUID" not in added, (
+        "SETUID cap should be dropped when running as host user — no gosu drop is needed"
+    )
+    assert "SETGID" not in added, (
+        "SETGID cap should be dropped when running as host user — no gosu drop is needed"
+    )
+    # Core non-privilege-drop caps must still be there (pip/npm/apt need them).
+    assert "DAC_OVERRIDE" in added
+    assert "CHOWN" in added
+    assert "FOWNER" in added
+
+
+def test_run_as_host_user_default_off(monkeypatch):
+    """Without the opt-in, no --user flag is emitted — preserving existing behavior."""
+    monkeypatch.setattr(docker_env, "find_docker", lambda: "/usr/bin/docker")
+    calls = _mock_subprocess_run(monkeypatch)
+
+    _make_dummy_env()  # run_as_host_user defaults to False
+
+    run_calls = [c for c in calls if isinstance(c[0], list) and len(c[0]) >= 2 and c[0][1] == "run"]
+    run_args = run_calls[0][0]
+    assert "--user" not in run_args, (
+        f"--user should not be in docker run args when opt-in is off: {run_args}"
+    )
+
+
+def test_run_as_host_user_warns_and_skips_when_no_posix_ids(monkeypatch, caplog):
+    """On platforms without POSIX getuid/getgid, log a warning and leave the
+    container at its image default user (no --user flag, full cap set)."""
+    monkeypatch.setattr(docker_env, "find_docker", lambda: "/usr/bin/docker")
+    # Simulate a platform where os.getuid is absent (e.g. Windows host).
+    monkeypatch.delattr(docker_env.os, "getuid", raising=False)
+    monkeypatch.delattr(docker_env.os, "getgid", raising=False)
+    calls = _mock_subprocess_run(monkeypatch)
+
+    with caplog.at_level(logging.WARNING):
+        _make_dummy_env(run_as_host_user=True)
+
+    run_calls = [c for c in calls if isinstance(c[0], list) and len(c[0]) >= 2 and c[0][1] == "run"]
+    run_args = run_calls[0][0]
+
+    assert "--user" not in run_args
+    # Fall back to the full cap set since the container still starts as root.
+    added = {
+        run_args[i + 1]
+        for i, flag in enumerate(run_args[:-1])
+        if flag == "--cap-add"
+    }
+    assert "SETUID" in added
+    assert "SETGID" in added
+    assert any(
+        "does not expose POSIX uid/gid" in rec.getMessage()
+        for rec in caplog.records
+    ), "expected a warning when POSIX ids are unavailable"

tests/tools/test_terminal_config_env_sync.py

@@ -0,0 +1,210 @@
+"""Regression tests for terminal config -> env-var bridging.
+
+terminal_tool._get_env_config() reads ALL terminal settings from os.environ
+(TERMINAL_*).  config.yaml values therefore have to be bridged into env vars
+at startup, by THREE separate code paths:
+
+  1. cli.py            -> ``env_mappings`` dict (CLI / TUI startup)
+  2. gateway/run.py    -> ``_terminal_env_map`` dict (gateway / messaging
+                          platforms)
+  3. hermes_cli/config.py:save_config_value
+                       -> ``_config_to_env_sync`` dict (one-shot when the
+                          user runs ``hermes config set …``)
+
+If any one of these is missing a key, the corresponding config.yaml setting
+silently does nothing for that entry-point.  This bug already shipped once
+for ``docker_run_as_host_user`` (gateway and CLI maps) and once for
+``docker_mount_cwd_to_workspace`` (gateway map).
+
+This test guards against future drift by extracting all three maps via source
+inspection and asserting they all bridge the same set of writable
+``terminal.*`` keys.  Source inspection (rather than importing the live
+dicts) keeps the test independent of the user's ~/.hermes/config.yaml and
+mirrors the pattern used in tests/hermes_cli/test_config_drift.py.
+"""
+
+import ast
+import inspect
+
+
+def _extract_dict_values(source: str, dict_name: str) -> set[str]:
+    """Return the set of *value* strings in `dict_name = { "k": "VALUE", ... }`.
+
+    We parse the source with ast (so multi-line dicts and comments are
+    handled) instead of regex.  The first matching assignment wins.
+    """
+    tree = ast.parse(source)
+    for node in ast.walk(tree):
+        if not isinstance(node, ast.Assign):
+            continue
+        targets = [t for t in node.targets if isinstance(t, ast.Name)]
+        if not any(t.id == dict_name for t in targets):
+            continue
+        if not isinstance(node.value, ast.Dict):
+            continue
+        out: set[str] = set()
+        for k, v in zip(node.value.keys, node.value.values):
+            if isinstance(k, ast.Constant) and isinstance(v, ast.Constant):
+                if isinstance(v.value, str):
+                    out.add(v.value)
+        return out
+    raise AssertionError(f"Could not find `{dict_name} = {{...}}` literal in source")
+
+
+def _extract_dict_keys(source: str, dict_name: str) -> set[str]:
+    """Return the set of *key* strings in `dict_name = { "KEY": "v", ... }`."""
+    tree = ast.parse(source)
+    for node in ast.walk(tree):
+        if not isinstance(node, ast.Assign):
+            continue
+        targets = [t for t in node.targets if isinstance(t, ast.Name)]
+        if not any(t.id == dict_name for t in targets):
+            continue
+        if not isinstance(node.value, ast.Dict):
+            continue
+        out: set[str] = set()
+        for k in node.value.keys:
+            if isinstance(k, ast.Constant) and isinstance(k.value, str):
+                out.add(k.value)
+        return out
+    raise AssertionError(f"Could not find `{dict_name} = {{...}}` literal in source")
+
+
+def _cli_env_map_keys() -> set[str]:
+    """terminal config keys bridged by cli.load_cli_config()."""
+    import cli
+    source = inspect.getsource(cli.load_cli_config)
+    return _extract_dict_keys(source, "env_mappings")
+
+
+def _gateway_env_map_keys() -> set[str]:
+    """terminal config keys bridged by gateway/run.py at module load."""
+    # gateway/run.py builds the dict at module top-level (not inside a
+    # function), so inspect the whole module source.
+    import gateway.run as gr
+    source = inspect.getsource(gr)
+    return _extract_dict_keys(source, "_terminal_env_map")
+
+
+def _save_config_env_sync_keys() -> set[str]:
+    """terminal config keys bridged by ``hermes config set foo bar``."""
+    from hermes_cli import config as hc_config
+    source = inspect.getsource(hc_config.set_config_value)
+    keys = _extract_dict_keys(source, "_config_to_env_sync")
+    # set_config_value uses fully-qualified ``terminal.foo`` keys; strip the
+    # prefix so we can compare against the other two maps which use bare
+    # leaf keys.
+    return {k.split(".", 1)[1] for k in keys if k.startswith("terminal.")}
+
+
+# Keys present in cli.py env_mappings but intentionally absent from
+# gateway/run.py or set_config_value.  Each entry must be justified.
+_CLI_ONLY_OK = frozenset({
+    # `env_type` is a legacy YAML key alias for `backend` that cli.py
+    # accepts for backwards-compat with older cli-config.yaml.  The
+    # gateway path normalizes on the canonical `backend` key, which is
+    # also in the map and handles the same bridging.  See cli.py ~line 515.
+    "env_type",
+    # sudo_password is not a terminal-backend option — it's a credential
+    # used across backends, bridged to $SUDO_PASSWORD (not TERMINAL_*).
+    # Treating it as terminal-only would be misleading.
+    "sudo_password",
+})
+
+
+def _terminal_tool_env_var_names() -> set[str]:
+    """All TERMINAL_* env vars actually consumed by terminal_tool."""
+    import tools.terminal_tool as tt
+    source = inspect.getsource(tt)
+    # Naive scan: every os.getenv("TERMINAL_X", ...) and _parse_env_var("TERMINAL_X", ...).
+    import re
+    pat = re.compile(r'["\'](TERMINAL_[A-Z0-9_]+)["\']')
+    return set(pat.findall(source))
+
+
+def test_cli_and_gateway_env_maps_agree():
+    """cli.py and gateway/run.py must bridge the same set of terminal keys.
+
+    Both feed the same downstream consumer (terminal_tool).  Drift between
+    them means a config.yaml setting that "works in CLI mode but not gateway
+    mode" (or vice-versa) — the bug class that shipped twice already.
+    """
+    cli_keys = _cli_env_map_keys() - _CLI_ONLY_OK
+    gw_keys = _gateway_env_map_keys()
+
+    # Normalize the legacy `env_type` alias: cli.py accepts both `env_type`
+    # and `backend` as source keys for TERMINAL_ENV; gateway only accepts
+    # `backend`.  Since cli.py copies `backend` → `env_type` before the
+    # lookup, they're equivalent.  Remove `backend` from the gateway side
+    # to avoid a spurious "backend missing from cli" failure.
+    gw_keys = gw_keys - {"backend"}
+
+    missing_in_gateway = cli_keys - gw_keys
+    missing_in_cli = gw_keys - cli_keys
+
+    assert not missing_in_gateway, (
+        f"Keys in cli.py env_mappings but missing from gateway/run.py "
+        f"_terminal_env_map: {sorted(missing_in_gateway)}.  Add them to "
+        f"both maps (same bug class as docker_run_as_host_user shipping "
+        f"wired in cli but not gateway in April 2026)."
+    )
+    assert not missing_in_cli, (
+        f"Keys in gateway/run.py _terminal_env_map but missing from cli.py "
+        f"env_mappings: {sorted(missing_in_cli)}.  Add them to both maps."
+    )
+
+
+def test_save_config_set_supports_critical_bridged_keys():
+    """``hermes config set terminal.X true`` must propagate to .env for
+    known-critical keys.  This used to be an all-keys invariant but several
+    pre-existing terminal keys (ssh_*, docker_forward_env, docker_volumes)
+    aren't in _config_to_env_sync and are instead handled via the separate
+    api_keys TERMINAL_SSH_* fallback path or user-edits-yaml-directly.
+
+    Until those gaps are audited and fixed, pin the specific keys that are
+    load-bearing for the docker backend's ownership flag so the bug we just
+    fixed cannot silently regress.
+    """
+    save_keys = _save_config_env_sync_keys()
+    required = {
+        "docker_run_as_host_user",
+        "docker_mount_cwd_to_workspace",
+        "backend",
+        "docker_image",
+        "container_cpu",
+        "container_memory",
+        "container_disk",
+        "container_persistent",
+    }
+    missing = required - save_keys
+    assert not missing, (
+        f"`hermes config set terminal.X` doesn't sync these load-bearing "
+        f"keys to .env: {sorted(missing)}.  Add them to _config_to_env_sync "
+        f"in hermes_cli/config.py:set_config_value."
+    )
+
+
+def test_docker_run_as_host_user_is_bridged_everywhere():
+    """Explicit pin for the bug we just fixed.
+
+    docker_run_as_host_user was added to terminal_tool._get_env_config and
+    DockerEnvironment but NOT to cli.py's env_mappings or gateway/run.py's
+    _terminal_env_map, so ``terminal.docker_run_as_host_user: true`` in
+    config.yaml had no effect at runtime.  This guard makes the regression
+    impossible to reintroduce silently.
+    """
+    assert "docker_run_as_host_user" in _cli_env_map_keys()
+    assert "docker_run_as_host_user" in _gateway_env_map_keys()
+    assert "docker_run_as_host_user" in _save_config_env_sync_keys()
+    assert "TERMINAL_DOCKER_RUN_AS_HOST_USER" in _terminal_tool_env_var_names()
+
+
+def test_docker_mount_cwd_to_workspace_is_bridged_everywhere():
+    """Same regression class — docker_mount_cwd_to_workspace was missing from
+    gateway/run.py's _terminal_env_map until the docker_run_as_host_user
+    audit caught it.
+    """
+    assert "docker_mount_cwd_to_workspace" in _cli_env_map_keys()
+    assert "docker_mount_cwd_to_workspace" in _gateway_env_map_keys()
+    assert "docker_mount_cwd_to_workspace" in _save_config_env_sync_keys()
+    assert "TERMINAL_DOCKER_MOUNT_CWD_TO_WORKSPACE" in _terminal_tool_env_var_names()