From 128a6837b72277d5fb87af7b74823621efdc2e0b Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Sat, 23 May 2026 01:59:01 -0700 Subject: [PATCH] =?UTF-8?q?fix(egress):=20address=20PR=20review=20findings?= =?UTF-8?q?=20=E2=80=94=20P0/P1/P2/P3=20+=20CI=20greens?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P0 — must-fix - iron_proxy: emit default upstream_deny_cidrs (loopback, IMDS 169.254.0.0/16, RFC1918) when caller passes None. Honours the docs promise that cloud-metadata IPs are refused regardless of allowlist. - iron_proxy: bind 127.0.0.1 (+ docker0 bridge IP on Linux) instead of INADDR_ANY (':9090'). LAN peers with a leaked sandbox token could otherwise spend the operator's API quota against any allowlisted upstream. - ensure_ca_cert: write the CA private key via os.open(..., 0o600) instead of shutil.copy2+os.chmod — closes the TOCTOU window where the key existed under the default umask. - discover_uncovered_providers + proxy.fail_on_uncovered_providers config: refuse to start (when strict) if env vars for non-bearer providers (Anthropic native x-api-key, AWS SigV4, Azure OpenAI, etc.) are present. Surfaces a wizard warning in non-strict mode. P1 — should-fix - start_proxy: build a minimal subprocess env (PATH/HOME/locale + only the env names referenced by mappings) instead of os.environ .copy(). Strips proxy-recursion vars (HTTPS_PROXY etc.). Stops the proxy's /proc//environ from leaking every host secret to same-uid local processes. - start_proxy: optional Bitwarden refresh path (refresh_secrets_from_bitwarden=True, bitwarden_config=...). When credential_source=bitwarden, cmd_start wires it in — that's what delivers the rotation guarantee the docs make. - build_proxy_config: wire audit_log into the rendered yaml (log.audit_path). Parameter was accepted but never used. - ensure_audit_log: pre-create the audit log with 0o600 perms so iron-proxy inherits tight permissions instead of relying on umask. - Rename 'hermes proxy ...' → 'hermes egress ...' in user-facing strings (docstring, RuntimeError messages, post-setup banner). - start_proxy: open log file with 0o600 perms and close the parent fd immediately after Popen — fixes the per-restart fd leak. - DockerEnvironment: detect collisions between docker_env and the egress-controlling env vars (HTTPS_PROXY, SSL_CERT_FILE, etc.). When enforce_on_docker=true, fail loud rather than silently inverting the isolation; when false, warn and let docker_env win. - proxy_cli: merge_mappings preserves existing tokens on re-setup; --rotate-tokens flag re-mints all of them. Stops re-running `hermes egress setup` from invalidating tokens baked into already-running sandboxes. - proxy_cli: --from-bitwarden fail-loud on disabled BW config, missing access token, or empty vault. Previously fell through to the env path while still writing credential_source: bitwarden. - docker.py: narrow `except Exception` → `except ImportError`; iron_proxy._read_tunnel_port_from_config: same. Bare excepts were masking real config-load bugs. - start_proxy: write pidfile via os.open with O_NOFOLLOW + 0o600 + st_uid check. Refuses to follow a pre-existing symlink at the pidfile path. - mint_proxy_token docstring: document the 128-bit suffix entropy explicitly (sha256 truncated to 32 hex chars). P2 — follow-up - start_proxy: poll-with-timeout (100ms cadence on _port_listening) instead of an unconditional 5s sleep. Saves several seconds per Docker container create when enforce_on_docker=true. - docker.py: apply enforce_on_docker semantics when CA file vanishes between status.configured check and CA mount. Previously returned empty args silently. - docker.py: refuse to mount when mappings.json is empty/corrupt (was indistinguishable from upstream outage from inside the sandbox). - install_iron_proxy: tarfile.extract(..., filter='data') to silence the PEP 706 deprecation and opt into the 3.14+ default. - _proxy_state_dir: chmod 0o700 unconditionally; add _proxy_state_dir_ro() so read-only callers don't create the dir. - stop_proxy: re-verify pid before SIGKILL via /proc//stat starttime AND _pid_alive. Prevents SIGKILL'ing a recycled pid. - _pid_alive: tightened cmdline check — basename match on argv[0] plus an in-process nonce env var ('iron-proxy' in cmdline matched 'tail iron-proxy.log' and editors with the log open). - docker.py: NODE_OPTIONS=--use-openssl-ca so Node.js routes through the OpenSSL CA store SSL_CERT_FILE controls, narrowing the Python/curl-replace vs Node-add asymmetry waefrebeorn flagged. P3 — polish - proxy_cli: dest='egress_command' (was 'proxy_command' which collided lexically with the inbound OAuth subparser). - iron_proxy_version: cache by binary path — get_status is called per Docker container create, version is constant per binary. - Drop unused `import sys` from iron_proxy. - proxy_cli: `is not None` check on --tunnel-port (was treating 0 as falsy and silently substituting the default). - proxy_cli cmd_disable: use get_status().pid instead of reaching into ip._read_pid() (stale pidfile from a crashed run would have fired a spurious "still running" warning). - Tests: replace hardcoded /tmp/ca.* paths with tmp_path-derived fixtures so tests are hermetic across hosts. CI - Windows footguns scanner: os.kill(pid, 0) is now gated behind platform.system() != 'Windows' with a windows-footgun: ok marker; signal.SIGKILL falls back to SIGTERM on Windows via getattr(signal, 'SIGKILL', signal.SIGTERM). - docs MDX compilation: replace bare `` URLs with `[text](url)` syntax (MDX-jsx parser rejects the angle-bracket form). Tests - 32 new tests covering default deny CIDRs, bind policy, audit log wiring, subprocess env minimization, CA TOCTOU 0o600, state dir 0o700, empty-mappings refusal, CA-vanished refusal, docker_env collision detection, token preservation/rotate, uncovered provider detection, and the proxy_cli command handlers + argparse wiring. - All 156 tests in test_iron_proxy + test_iron_proxy_cli + test_docker_environment + test_config pass locally. Acknowledged but not addressed in this revision - E2E test for HTTPS CONNECT + TLS-MITM path: existing E2E exercises plain HTTP; full MITM coverage needs separate CI infra (real iron- proxy binary + curl with custom CA). Tracked as follow-up. - Cosign-style supply-chain verification for the binary checksum: upstream iron-proxy doesn't sign releases yet. Accepted pattern (same as Bitwarden integration); tracked as follow-up. - CA rotation CLI (`hermes egress rotate-ca`): scope-cut to a follow-up. Reviewers: @annguyenNous @waefrebeorn @GodsBoy @erhnysr --- agent/proxy_sources/iron_proxy.py | 768 +++++++++++++++++-- hermes_cli/config.py | 16 +- hermes_cli/main.py | 4 +- hermes_cli/proxy_cli.py | 247 +++++- tests/test_iron_proxy.py | 566 +++++++++++++- tests/test_iron_proxy_cli.py | 389 ++++++++++ tools/environments/docker.py | 127 ++- website/docs/user-guide/egress/iron-proxy.md | 4 +- 8 files changed, 1990 insertions(+), 131 deletions(-) create mode 100644 tests/test_iron_proxy_cli.py diff --git a/agent/proxy_sources/iron_proxy.py b/agent/proxy_sources/iron_proxy.py index d6f998ca4821..212bf64806f1 100644 --- a/agent/proxy_sources/iron_proxy.py +++ b/agent/proxy_sources/iron_proxy.py @@ -23,7 +23,7 @@ Design summary verifying the SHA-256 against the release's ``checksums.txt``. * A long-lived CA at ``/proxy/ca.{crt,key}`` is generated on - first ``hermes proxy setup``. Sandboxes trust this CA so iron-proxy can + first ``hermes egress setup``. Sandboxes trust this CA so iron-proxy can terminate TLS and rewrite headers. * The proxy config lives at ``/proxy/proxy.yaml``. It enumerates @@ -36,7 +36,7 @@ Design summary Bitwarden Secrets Manager is configured, the real value is pulled there at proxy startup instead. -* The proxy runs as a managed subprocess (``hermes proxy start``), pidfile +* The proxy runs as a managed subprocess (``hermes egress start``), pidfile at ``/proxy/iron-proxy.pid``, structured audit log at ``/proxy/audit.log``. @@ -61,7 +61,6 @@ import shutil import signal import stat import subprocess -import sys import tarfile import tempfile import time @@ -130,6 +129,87 @@ _BEARER_PROVIDERS: Dict[str, Tuple[str, ...]] = { } +# Providers whose env-var names we recognize but whose API uses a non-bearer +# auth scheme (x-api-key, AAD/OAuth, SigV4, custom signatures). When any of +# these env vars are present at proxy-start time AND +# ``proxy.fail_on_uncovered_providers`` is true (default), ``start_proxy`` +# refuses to start. Without this list the sandbox would still hold real +# credentials for these providers and silently bypass the proxy. +# +# Bare strings here are env-var names; the proxy doesn't try to wire them up, +# only flags their presence so the operator knows isolation is incomplete. +_NON_BEARER_PROVIDERS: Tuple[str, ...] = ( + # Anthropic native uses x-api-key, not Authorization: Bearer. + "ANTHROPIC_API_KEY", + # Azure OpenAI: api-key header + optional AAD bearer. + "AZURE_OPENAI_API_KEY", + # AWS Bedrock / SageMaker: SigV4-signed requests. + "AWS_ACCESS_KEY_ID", + "AWS_SECRET_ACCESS_KEY", + # GCP Vertex AI: OAuth bearer from gcloud SDK, not a static env key. + "GOOGLE_APPLICATION_CREDENTIALS", + # Google AI Studio (Gemini): x-goog-api-key OR query param. + "GEMINI_API_KEY", + "GOOGLE_API_KEY", +) + + +# Default SSRF-protection deny list applied to the proxy's outbound traffic. +# Mirrors the public docs promise ("cloud metadata IPs are refused by default +# regardless of allowlist"). Tests / dev setups that need loopback can pass +# an explicit override (e.g. [] to disable, or a smaller subset). +_DEFAULT_UPSTREAM_DENY_CIDRS: Tuple[str, ...] = ( + "127.0.0.0/8", # IPv4 loopback + "::1/128", # IPv6 loopback + "169.254.0.0/16", # IPv4 link-local incl. AWS/GCP/Azure IMDS + "fe80::/10", # IPv6 link-local + "10.0.0.0/8", # RFC1918 + "172.16.0.0/12", # RFC1918 + "192.168.0.0/16", # RFC1918 + "fc00::/7", # IPv6 ULA +) + + +# Min env vars the iron-proxy subprocess actually needs. Everything else +# is stripped — see ``_build_proxy_subprocess_env`` for the rationale. +_PROXY_SUBPROCESS_ENV_ALLOWLIST: Tuple[str, ...] = ( + "PATH", + "HOME", + "TMPDIR", + "TZ", + "LANG", + "LC_ALL", + "LC_CTYPE", + "NO_COLOR", + "SSL_CERT_DIR", + "SSL_CERT_FILE", + "SYSTEMROOT", # Windows + "USERPROFILE", # Windows +) + + +# Env vars that must be stripped from the subprocess env even if they're on +# the allowlist or named in mappings — these would either recurse the proxy +# back through itself or send its traffic through a corporate proxy. +_PROXY_SUBPROCESS_ENV_STRIP: Tuple[str, ...] = ( + "HTTPS_PROXY", "https_proxy", + "HTTP_PROXY", "http_proxy", + "ALL_PROXY", "all_proxy", + "NO_PROXY", "no_proxy", +) + + +# SIGKILL doesn't exist on Windows. We fall back to SIGTERM there, which the +# OS treats as a hard terminate via TerminateProcess() — equivalent semantics. +_KILL_SIGNAL = getattr(signal, "SIGKILL", signal.SIGTERM) + + +# Cached ``iron-proxy --version`` output keyed by binary path. ``get_status`` +# is invoked per Docker-container-create; the version string is constant for +# a given binary so a one-shot subprocess call is plenty. +_VERSION_CACHE: Dict[str, str] = {} + + # --------------------------------------------------------------------------- # Public dataclasses # --------------------------------------------------------------------------- @@ -189,11 +269,36 @@ def _hermes_bin_dir() -> Path: return get_hermes_home() / "bin" -def _proxy_state_dir() -> Path: +def _proxy_state_dir_ro() -> Path: + """Return the proxy state dir without creating it. + + Read-only callers (status probes, pidfile reads, version queries) use + this — there's no reason to materialize ``~/.hermes/proxy/`` just to + check whether a pidfile exists. + """ from hermes_constants import get_hermes_home - d = get_hermes_home() / "proxy" + return get_hermes_home() / "proxy" + + +def _proxy_state_dir() -> Path: + """Return the proxy state dir, creating it with 0o700 if absent. + + Writable callers (CA gen, config write, mappings write, start_proxy) + use this. We force 0o700 — the dir holds the CA signing key, audit + log, and pidfile, so traversal by other local users is undesirable. + The chmod is unconditional so a pre-existing dir with a slack umask + gets tightened on first access. + """ + d = _proxy_state_dir_ro() d.mkdir(parents=True, exist_ok=True) + try: + d.chmod(0o700) + except OSError: + # On Windows the chmod is a no-op for POSIX modes; on shared + # filesystems we may not own the dir. Don't fail here — the + # individual files still get explicit perms. + pass return d @@ -302,7 +407,18 @@ def install_iron_proxy(*, force: bool = False) -> Path: with tarfile.open(archive_path, "r:gz") as tf: member = _pick_tar_member(tf, _platform_binary_name()) - tf.extract(member, tmp) # noqa: S202 — member name is sanitized below + # PEP 706 data filter — strips ownership/mode replay (we set + # chmod explicitly below) AND rejects symlink/hardlink members + # that escape the extraction dir. Required on 3.12+ to silence + # the deprecation warning and on 3.14+ to opt into the + # tarbomb-rejecting default. + try: + tf.extract(member, tmp, filter="data") # noqa: S202 + except TypeError: + # Python < 3.12 — filter kw didn't exist yet; the + # _pick_tar_member sanitization already rejects path + # traversal so this is acceptable. + tf.extract(member, tmp) # noqa: S202 extracted = tmp / member.name # Stage into the final directory then atomically rename so the new @@ -379,7 +495,17 @@ def _pick_tar_member(tf: tarfile.TarFile, binary_name: str) -> tarfile.TarInfo: def iron_proxy_version(binary: Path) -> str: - """Return ``iron-proxy --version`` output, stripped. Empty on failure.""" + """Return ``iron-proxy --version`` output, stripped. Empty on failure. + + Cached by binary path: ``get_status`` is called per Docker container + create, but the version string is constant for a given binary. A + single subprocess invocation is plenty. + """ + + key = str(binary) + cached = _VERSION_CACHE.get(key) + if cached is not None: + return cached try: res = subprocess.run( # noqa: S603 — binary path is trusted @@ -390,7 +516,9 @@ def iron_proxy_version(binary: Path) -> str: ) except (OSError, subprocess.TimeoutExpired): return "" - return (res.stdout or res.stderr or "").strip() + out = (res.stdout or res.stderr or "").strip() + _VERSION_CACHE[key] = out + return out # --------------------------------------------------------------------------- @@ -448,10 +576,43 @@ def ensure_ca_cert(*, force: bool = False) -> Tuple[Path, Path]: timeout=60, ) - # Move into place with private permissions. - shutil.copy2(tmp_key, ca_key) - shutil.copy2(tmp_crt, ca_crt) - os.chmod(ca_key, stat.S_IRUSR | stat.S_IWUSR) + # Move into place with private permissions. CRITICAL: the key + # has to be created with 0o600 from the very first byte — a + # ``shutil.copy2`` followed by ``os.chmod`` leaves a TOCTOU window + # where the private key is world-readable on multi-user hosts. + key_bytes = tmp_key.read_bytes() + crt_bytes = tmp_crt.read_bytes() + + # Stage with explicit 0o600, then atomically rename into place. + # O_NOFOLLOW guards against a symlink at ca_key (defence-in-depth + # — the state dir is 0o700-owned but a malicious local user with + # the same uid could pre-create one). + key_staged = ca_key.with_suffix(ca_key.suffix + ".staged") + open_flags = os.O_WRONLY | os.O_CREAT | os.O_TRUNC + # O_NOFOLLOW exists on POSIX; on Windows we just rely on the + # default semantics. + if hasattr(os, "O_NOFOLLOW"): + open_flags |= os.O_NOFOLLOW + # Best-effort: pre-unlink any existing staged file so the open + # with O_CREAT is always against a fresh inode. + try: + key_staged.unlink() + except FileNotFoundError: + pass + fd = os.open(str(key_staged), open_flags, 0o600) + try: + with os.fdopen(fd, "wb") as f: + f.write(key_bytes) + except Exception: + try: + os.close(fd) + except OSError: + pass + raise + os.replace(key_staged, ca_key) + + # Cert is public — 0o644 is fine and matches typical PEM layout. + ca_crt.write_bytes(crt_bytes) os.chmod(ca_crt, stat.S_IRUSR | stat.S_IWUSR | stat.S_IRGRP | stat.S_IROTH) logger.info("Generated iron-proxy CA at %s", ca_crt) @@ -467,13 +628,67 @@ def mint_proxy_token(prefix: str = "hermes-proxy") -> str: """Mint a fresh opaque token to hand to the sandbox. The token has no internal structure beyond a recognizable prefix — - iron-proxy matches on exact equality. We use a long random suffix - so collisions are infeasible. + iron-proxy matches on exact equality. We use a 128-bit random suffix + (32 hex chars from a SHA-256 of 32 bytes of os.urandom). At that + entropy the birthday-bound collision probability is below 2^-64 for + up to 2^32 tokens, which is plenty for a proxy-scoped namespace. """ return f"{prefix}-{hashlib.sha256(os.urandom(32)).hexdigest()[:32]}" +def _default_http_listen(tunnel_port: int) -> List[str]: + """Build the list of host:port pairs the proxy should bind on. + + Always binds loopback (``127.0.0.1``) so host-side test tooling can hit + the proxy directly. On Linux we also bind the docker bridge gateway + (``172.17.0.1`` by default) so containers can reach the proxy via + ``host.docker.internal:host-gateway``. We do NOT bind ``0.0.0.0`` — + that would expose the proxy (and, with a leaked sandbox token, the + user's API quota) to anyone on the local network. + + On macOS / Windows Docker Desktop the bridge gateway is managed by + Desktop itself and ``host.docker.internal`` resolves via VPNkit, so + a single loopback bind is enough. + """ + + binds = [f"127.0.0.1:{tunnel_port}"] + if platform.system() == "Linux": + bridge_ip = _detect_docker_bridge_ip() + if bridge_ip and bridge_ip != "127.0.0.1": + binds.append(f"{bridge_ip}:{tunnel_port}") + return binds + + +def _detect_docker_bridge_ip() -> Optional[str]: + """Return the docker0 bridge IPv4, if present, else None. + + Best-effort: we try ``ip -4 addr show docker0`` first, then fall back + to parsing ``/proc/net/route`` for the bridge IP. Anything that fails + or doesn't look like an IPv4 returns None — callers handle that as + "no bridge bind". + """ + + try: + res = subprocess.run( # noqa: S603 — ip is a system binary + ["ip", "-4", "-o", "addr", "show", "docker0"], + capture_output=True, text=True, timeout=2, + ) + if res.returncode == 0: + for line in res.stdout.splitlines(): + parts = line.split() + # Expected: ": docker0 inet 172.17.0.1/16 ..." + for i, tok in enumerate(parts): + if tok == "inet" and i + 1 < len(parts): + ip = parts[i + 1].split("/")[0] + # cheap sanity: four dotted parts. + if ip.count(".") == 3: + return ip + except (OSError, subprocess.TimeoutExpired): + pass + return None + + def build_proxy_config( *, mappings: List[TokenMapping], @@ -483,6 +698,7 @@ def build_proxy_config( audit_log: Optional[Path] = None, allowed_hosts: Optional[List[str]] = None, upstream_deny_cidrs: Optional[List[str]] = None, + http_listen: Optional[List[str]] = None, ) -> Dict: """Build the iron-proxy YAML config (as a dict) for a given mapping set. @@ -490,6 +706,19 @@ def build_proxy_config( real secrets from its OWN environment via ``source: {type: env, var: ...}``; the sandbox never sees them. + Bind policy: by default we bind loopback (``127.0.0.1``) plus the + docker bridge gateway IP on Linux (``172.17.0.1`` or whatever + ``docker0`` resolves to). Sandboxes use ``host.docker.internal`` which + Linux Docker maps to the bridge gateway via ``--add-host``; macOS / + Windows Docker Desktop manage their own gateway. We do NOT bind + ``0.0.0.0`` — a LAN peer with a leaked sandbox token could otherwise + spend the operator's API quota against any allowlisted upstream. + + SSRF policy: ``upstream_deny_cidrs`` defaults to a conservative deny + list covering loopback, link-local (incl. AWS/GCP/Azure IMDS at + 169.254.169.254), and RFC1918. Pass an explicit ``[]`` to opt out of + the deny list entirely (only sensible in hermetic tests). + Schema mirrors the official iron-proxy schema as of v0.39.0. Notable points: @@ -527,6 +756,29 @@ def build_proxy_config( "rules": [{"host": h} for h in m.upstream_hosts], }) + # SSRF protection: default-deny cloud metadata + loopback + RFC1918. + # Callers can pass [] to opt out entirely (hermetic tests need this for + # talking to a loopback upstream). None means "use the default". + deny_cidrs: List[str] + if upstream_deny_cidrs is None: + deny_cidrs = list(_DEFAULT_UPSTREAM_DENY_CIDRS) + else: + deny_cidrs = list(upstream_deny_cidrs) + + # Listen addresses. Single canonical "http_listen" for backward compat + # plus a "http_listens" list (iron-proxy v0.39 accepts both; v0.40+ is + # listen-list-only). We always emit both forms so a binary version + # bump can't silently regress the bind policy. + listens = list(http_listen) if http_listen else _default_http_listen(tunnel_port) + primary_listen = listens[0] if listens else f"127.0.0.1:{tunnel_port}" + + log_block: Dict = {"level": "info"} + if audit_log is not None: + # Wire the operator-requested audit-log path into the binary's log + # config. iron-proxy reads ``log.audit_path``; setting it routes + # per-request records there (separately from server-level logs). + log_block["audit_path"] = str(audit_log) + return { # DNS section is required by the binary's config parser, but we run # in tunnel-only mode so the DNS listener never binds an exposed port. @@ -540,9 +792,12 @@ def build_proxy_config( # http_listen is the HTTP-proxy listener that handles both plain # HTTP forwards AND CONNECT tunnels for HTTPS. Sandboxes set # `HTTPS_PROXY=http://host:tunnel_port` and the same listener - # serves both protocols. Bind on all interfaces so containers - # can reach it via host.docker.internal. - "http_listen": f":{tunnel_port}", + # serves both protocols. We bind loopback + the docker bridge + # gateway (Linux) — NOT 0.0.0.0. LAN peers with a leaked + # sandbox token would otherwise be able to spend the operator's + # API quota against any allowlisted upstream. + "http_listen": primary_listen, + "http_listens": listens, # The HTTPS-listener (direct TLS termination, no CONNECT) and # the SOCKS5/CONNECT-only tunnel listener get loopback ephemeral # ports — we don't expose them. @@ -552,13 +807,8 @@ def build_proxy_config( "max_response_body_bytes": 0, "upstream_response_header_timeout": "120s", # SSRF protection: deny outbound to cloud metadata + loopback by - # default. Tests / dev setups that need loopback can pass an - # explicit override (e.g. [] to disable, or just the IMDS subset). - **( - {"upstream_deny_cidrs": list(upstream_deny_cidrs)} - if upstream_deny_cidrs is not None - else {} - ), + # default. An empty list opts out entirely. + "upstream_deny_cidrs": deny_cidrs, }, "tls": { "ca_cert": str(ca_cert), @@ -576,10 +826,36 @@ def build_proxy_config( "config": {"secrets": secrets_rules}, }, ], - "log": {"level": "info"}, + "log": log_block, } +def ensure_audit_log(audit_path: Path) -> None: + """Create the audit log file with private permissions (0o600). + + Called from the wizard right before ``start_proxy``. Without this, + iron-proxy creates the file under the default umask the first time it + writes — meaning every host-side request history is potentially + world-readable. We pre-create the file empty with 0o600 so the daemon + inherits the tight permissions. + """ + + try: + # Use os.open + O_CREAT to avoid races on the chmod. + open_flags = os.O_WRONLY | os.O_CREAT | os.O_APPEND + if hasattr(os, "O_NOFOLLOW"): + open_flags |= os.O_NOFOLLOW + fd = os.open(str(audit_path), open_flags, 0o600) + try: + # Tighten perms even if the file already existed under a + # slacker umask. + os.fchmod(fd, 0o600) + finally: + os.close(fd) + except OSError as exc: + logger.warning("Could not pre-create audit log %s: %s", audit_path, exc) + + def write_proxy_config(config: Dict) -> Path: """Serialize the config dict to ``/proxy/proxy.yaml``. @@ -687,6 +963,69 @@ def discover_provider_mappings( return mappings +def discover_uncovered_providers( + *, + available_env_names: Optional[List[str]] = None, +) -> List[str]: + """Return env-var names for providers we recognize but can't proxy. + + Anthropic native (x-api-key), AWS Bedrock (SigV4), Azure OpenAI + (api-key), etc. When any of these are configured, the sandbox is + holding real credentials that the proxy can't strip — the isolation + guarantee is incomplete for those providers. + + The wizard uses this to print a warning at setup time; ``start_proxy`` + can be configured to refuse to start when ``fail_on_uncovered_providers`` + is true. + """ + + if available_env_names is not None: + names = set(available_env_names) + else: + names = {k for k, v in os.environ.items() if v} + + return [n for n in _NON_BEARER_PROVIDERS if n in names] + + +def merge_mappings( + *, + existing: List[TokenMapping], + discovered: List[TokenMapping], + rotate: bool = False, +) -> List[TokenMapping]: + """Combine an existing mapping set with freshly discovered providers. + + By default this PRESERVES tokens for providers already in ``existing`` — + re-running ``hermes egress setup`` should not invalidate the tokens + baked into containers that are already running. Only newly added + providers get freshly minted tokens. + + When ``rotate=True``, every token in the result is freshly minted + regardless of overlap. The wizard exposes this via ``--rotate-tokens`` + for the rare case where the operator wants to roll all tokens + deliberately (e.g. after a suspected token leak). + + Providers that are in ``existing`` but no longer in ``discovered`` + (operator removed the env var since last setup) are dropped. + """ + + by_name = {m.real_env_name: m for m in existing} + out: List[TokenMapping] = [] + for d in discovered: + prior = by_name.get(d.real_env_name) + if prior is not None and not rotate: + # Preserve the token, refresh the host list in case we added + # new upstreams since last setup. + out.append(TokenMapping( + proxy_token=prior.proxy_token, + real_env_name=prior.real_env_name, + upstream_hosts=d.upstream_hosts, + )) + else: + out.append(d) + return out + + # --------------------------------------------------------------------------- # Subprocess lifecycle # --------------------------------------------------------------------------- @@ -697,7 +1036,10 @@ def _pidfile() -> Path: def _read_pid() -> Optional[int]: - pf = _pidfile() + # Use the read-only path: don't create the proxy dir just to read the + # pidfile. If neither pid file nor dir exists, the daemon is plainly + # not running. + pf = _proxy_state_dir_ro() / "iron-proxy.pid" if not pf.exists(): return None try: @@ -707,40 +1049,115 @@ def _read_pid() -> Optional[int]: return pid if pid > 0 else None +# Nonce env-var set in the iron-proxy subprocess at start_proxy time. Used +# by ``_pid_alive`` to confirm a candidate PID still refers to *our* managed +# binary even across PID recycling (a fresh process can't inherit our +# arbitrary env value). +_HERMES_IRON_PROXY_NONCE_ENV = "HERMES_IRON_PROXY_NONCE" +_proxy_nonce: Optional[str] = None + + +def _pid_proc_starttime(pid: int) -> Optional[str]: + """Return /proc//stat[21] (starttime) on Linux, else None. + + Comparing starttime is the standard cheap way to detect PID recycling + without relying on cmdline scanning. When None, callers fall back to + the cmdline + nonce check. + """ + try: + text = Path(f"/proc/{pid}/stat").read_text(encoding="utf-8") + except OSError: + return None + # /proc//stat: pid (comm-with-parens) state ppid ... fields[21]=starttime + # The "comm" field can contain spaces and parens, so split from the + # right parenthesis instead of using shlex. + rparen = text.rfind(")") + if rparen < 0: + return None + fields = text[rparen + 1:].split() + # field index in the post-")" tail: original 3..n become fields[0..n-3] + # starttime is original field 22 (1-indexed) → tail index 22-3 = 19 + if len(fields) <= 19: + return None + return fields[19] + + def _pid_alive(pid: int) -> bool: """Return True iff ``pid`` is alive AND is an iron-proxy process. - The cmdline check guards against PID reuse — without it, an unrelated - process that happens to have grabbed the same PID after iron-proxy - crashed would look "alive" and we'd refuse to restart. + Defends against PID reuse via three signals (in priority order): + 1. ``/proc//environ`` contains our nonce (most reliable, Linux) + 2. ``/proc//cmdline`` basename matches the managed binary + 3. ``ps -p `` command line contains the binary path + + The legacy ``"iron-proxy" in cmdline`` match was loose enough to match + ``tail iron-proxy.log`` or an editor with that file open. We tighten + on argv[0] basename plus an in-process nonce instead. """ if pid <= 0: return False try: - os.kill(pid, 0) - except (ProcessLookupError, PermissionError, OSError): - return False + # Use psutil.pid_exists when available — it's a no-op on Windows + # whereas os.kill(pid, 0) on Windows is actually a hard kill + # (CTRL_C_EVENT to the target's console process group). See + # bpo-14484. windows-footgun: ok — we explicitly skip the + # os.kill probe on Windows below. + import psutil # type: ignore + if not psutil.pid_exists(pid): + return False + except ImportError: + if platform.system() == "Windows": + # On Windows without psutil we can't safely probe — assume + # the pidfile content is fresh and confirm via the cmdline + # path below. os.kill(pid, 0) is NOT safe here. + pass + else: + try: + os.kill(pid, 0) # windows-footgun: ok — POSIX-only branch + except (ProcessLookupError, PermissionError, OSError): + return False - # Confirm via /proc when available (Linux + WSL). On macOS we fall - # back to ``ps``; on truly exotic platforms the os.kill(pid, 0) result - # is taken at face value. + # Strong proof: nonce env var matches. /proc//environ is null- + # separated KEY=VALUE pairs; substring search is safe. + if _proxy_nonce: + try: + env_bytes = Path(f"/proc/{pid}/environ").read_bytes() + needle = f"{_HERMES_IRON_PROXY_NONCE_ENV}={_proxy_nonce}".encode() + if needle in env_bytes: + return True + except OSError: + pass + + # Fallback: cmdline basename match. argv[0] is the first null- + # separated token in /proc//cmdline. try: cmdline_path = Path(f"/proc/{pid}/cmdline") if cmdline_path.exists(): - cmdline = cmdline_path.read_bytes().decode("utf-8", errors="ignore") - return "iron-proxy" in cmdline + tokens = cmdline_path.read_bytes().split(b"\x00") + if tokens: + argv0 = tokens[0].decode("utf-8", errors="ignore") + argv0_base = os.path.basename(argv0) + if argv0_base.startswith("iron-proxy"): + return True + return False except OSError: pass + + # macOS / non-Linux fallback: ``ps`` command basename. try: res = subprocess.run( # noqa: S603 - ["ps", "-p", str(pid), "-o", "command="], + ["ps", "-p", str(pid), "-o", "comm="], capture_output=True, text=True, timeout=2, ) if res.returncode == 0: - return "iron-proxy" in (res.stdout or "") + comm = (res.stdout or "").strip() + return os.path.basename(comm).startswith("iron-proxy") except (OSError, subprocess.TimeoutExpired): pass + + # Exotic platforms: be conservative — if the OS says alive we believe + # it. This restores the previous behaviour for non-Linux/non-macOS. return True @@ -749,13 +1166,24 @@ def start_proxy( binary: Optional[Path] = None, config_path: Optional[Path] = None, extra_env: Optional[Dict[str, str]] = None, + refresh_secrets_from_bitwarden: bool = False, + bitwarden_config: Optional[Dict] = None, ) -> ProxyStatus: """Spawn iron-proxy as a managed background subprocess. Idempotent — if the proxy is already running with the expected PID, just returns the live status. + + ``refresh_secrets_from_bitwarden=True`` re-fetches upstream secrets + via ``bws secret list`` at startup and injects them into the child + env. This delivers the rotation promise that distinguishes + ``credential_source: bitwarden`` from ``credential_source: env``. + Without this flag (or with ``bitwarden_config=None``) the proxy still + starts but uses whatever the host process env happens to contain. """ + global _proxy_nonce + existing = _read_pid() if existing and _pid_alive(existing): return get_status() @@ -763,42 +1191,96 @@ def start_proxy( bin_path = binary or find_iron_proxy(install_if_missing=True) if bin_path is None: raise RuntimeError( - "iron-proxy binary not available — run `hermes proxy install`." + "iron-proxy binary not available — run `hermes egress install`." ) cfg = config_path or (_proxy_state_dir() / "proxy.yaml") if not cfg.exists(): raise RuntimeError( f"iron-proxy config not found at {cfg}. " - "Run `hermes proxy setup` first." + "Run `hermes egress setup` first." ) - env = os.environ.copy() - if extra_env: - env.update(extra_env) - env.setdefault("NO_COLOR", "1") + # Build a minimal subprocess env. os.environ.copy() would ship every + # secret in the operator's shell to the proxy — /proc//environ + # would then expose OPENAI_API_KEY, AWS keys, etc. to any same-uid + # local process. Defeats the threat model the proxy exists to + # mitigate. + env = _build_proxy_subprocess_env( + extra_env=extra_env, + refresh_from_bitwarden=refresh_secrets_from_bitwarden, + bitwarden_config=bitwarden_config, + ) + + # Plant a per-start nonce in the child env so ``_pid_alive`` can + # confirm a candidate PID still refers to *our* binary across PID + # recycling. Module-global is fine — only one managed proxy per + # Hermes process. + _proxy_nonce = hashlib.sha256(os.urandom(16)).hexdigest() + env[_HERMES_IRON_PROXY_NONCE_ENV] = _proxy_nonce log_path = _proxy_state_dir() / "iron-proxy.log" - log_fp = open(log_path, "ab", buffering=0) + # Keep ownership of the fd tight: open with explicit 0o600 so the + # log doesn't get world-readable under a slack umask, then close it + # immediately after Popen (the child has its own dup). Without the + # close-on-success path, every restart leaked one fd in the Hermes + # process. + log_fd = os.open( + str(log_path), + os.O_WRONLY | os.O_CREAT | os.O_APPEND, + 0o600, + ) + try: + os.fchmod(log_fd, 0o600) # tighten if file pre-existed + except OSError: + pass try: - proc = subprocess.Popen( # noqa: S603 — binary path is trusted - [str(bin_path), "-config", str(cfg)], + # Use the fd directly via the dup mechanism; Popen will dup() it + # into the child so we can close ours unconditionally below. + # NOTE: on Windows ``start_new_session`` is invalid; we don't + # support Windows for the proxy (the binary itself doesn't ship) + # but the kwarg is POSIX-only and silently ignored on Win. + popen_kwargs: Dict = dict( env=env, stdin=subprocess.DEVNULL, - stdout=log_fp, + stdout=log_fd, stderr=subprocess.STDOUT, - start_new_session=True, + ) + if platform.system() != "Windows": + popen_kwargs["start_new_session"] = True + proc = subprocess.Popen( # noqa: S603 — binary path is trusted + [str(bin_path), "-config", str(cfg)], + **popen_kwargs, ) except OSError as exc: - log_fp.close() + os.close(log_fd) raise RuntimeError(f"failed to spawn iron-proxy: {exc}") from exc + finally: + # Close our copy of the fd whether Popen raised or succeeded. + # The child has its own dup via Popen, so it's still writing. + try: + os.close(log_fd) + except OSError: + pass + + # Poll-with-timeout instead of an unconditional 5s sleep. The Go + # binary normally comes up in <200ms; falling through within 100ms + # of liveness keeps Docker container creation snappy. + tunnel_port = _read_tunnel_port_from_config() or _DEFAULT_TUNNEL_PORT + deadline = time.time() + _STARTUP_GRACE_SECONDS + while time.time() < deadline: + if proc.poll() is not None: + tail = _tail_log(log_path, lines=20) + raise RuntimeError( + f"iron-proxy exited immediately (code {proc.returncode}). " + f"Last log lines:\n{tail}" + ) + if _port_listening("127.0.0.1", tunnel_port): + break + time.sleep(0.1) - # Give it a moment to either come up or fail fast. We don't want to - # write a pidfile pointing at a dead process. - time.sleep(_STARTUP_GRACE_SECONDS) if proc.poll() is not None: - log_fp.close() tail = _tail_log(log_path, lines=20) raise RuntimeError( f"iron-proxy exited immediately (code {proc.returncode}). " @@ -806,23 +1288,149 @@ def start_proxy( ) pidfile = _pidfile() - pidfile.write_text(str(proc.pid), encoding="utf-8") + # Use os.open with O_NOFOLLOW to refuse to follow a pre-existing + # symlink at the pidfile path (defence-in-depth; same-uid required to + # plant a symlink but worth defending). O_TRUNC clobbers any stale + # content. + open_flags = os.O_WRONLY | os.O_CREAT | os.O_TRUNC + if hasattr(os, "O_NOFOLLOW"): + open_flags |= os.O_NOFOLLOW + try: + fd = os.open(str(pidfile), open_flags, 0o600) + except OSError as exc: + # If the file existed as a symlink, O_NOFOLLOW returns ELOOP. + # Surface a clear error and let the operator clean up. + raise RuntimeError( + f"Refusing to write pidfile {pidfile}: {exc}. " + "Remove that path manually and retry." + ) from exc + try: + # Verify the file we just opened is owned by us. On POSIX, + # st_uid mismatch means a same-uid race won and we got a hostile + # file — bail rather than write the pid into it. + try: + st = os.fstat(fd) + if hasattr(os, "getuid") and st.st_uid != os.getuid(): + raise RuntimeError( + f"pidfile {pidfile} has unexpected owner uid={st.st_uid}" + ) + except AttributeError: + pass # Windows + os.write(fd, str(proc.pid).encode("utf-8")) + finally: + os.close(fd) + logger.info("Started iron-proxy pid=%s config=%s", proc.pid, cfg) return get_status() +def _build_proxy_subprocess_env( + *, + extra_env: Optional[Dict[str, str]] = None, + refresh_from_bitwarden: bool = False, + bitwarden_config: Optional[Dict] = None, +) -> Dict[str, str]: + """Construct the minimal env for the iron-proxy subprocess. + + Allowlists infrastructure vars (PATH, HOME, locale) plus the env vars + named in ``load_mappings()`` (the real upstream secrets the proxy + needs to do the swap). Everything else is stripped — see + ``_PROXY_SUBPROCESS_ENV_STRIP`` for proxy chain protection. + + When ``refresh_from_bitwarden=True`` AND ``bitwarden_config`` is + populated, fetches upstream secrets via the BSM SDK at startup and + merges them in. This is what delivers the rotation guarantee + promised by ``credential_source: bitwarden`` — without it, rotating + a key in the Bitwarden web app doesn't reach the proxy. + """ + + env: Dict[str, str] = {} + parent = os.environ + for name in _PROXY_SUBPROCESS_ENV_ALLOWLIST: + if name in parent: + env[name] = parent[name] + + # The proxy reads the real upstream secrets from its OWN env, indexed + # by ``m.real_env_name`` in the YAML config's ``secrets.source.var`` + # field. Forward those — but only those. + needed = {m.real_env_name for m in load_mappings()} + for name in needed: + if name in parent: + env[name] = parent[name] + + # Optional Bitwarden refresh path. Pulled lazily so the proxy module + # doesn't hard-depend on the bitwarden module being importable in + # every install. + if refresh_from_bitwarden and bitwarden_config: + try: + from agent.secret_sources import bitwarden as bw + access_token_name = bitwarden_config.get( + "access_token_env", "BWS_ACCESS_TOKEN" + ) + access_token = parent.get(access_token_name, "").strip() + project_id = bitwarden_config.get("project_id", "") + if access_token and project_id: + secrets, warnings = bw.fetch_bitwarden_secrets( + access_token=access_token, + project_id=project_id, + cache_ttl_seconds=0, + use_cache=False, + ) + # Only inject env names we have a mapping for — extra + # secrets in the BW project shouldn't leak into the proxy + # process unless they're going to be used by the swap. + for n in needed: + if n in secrets: + env[n] = secrets[n] + for w in warnings: + logger.warning("Bitwarden refresh: %s", w) + else: + logger.warning( + "credential_source=bitwarden but access_token_env=%s or " + "project_id is empty — proxy will fall back to parent env", + access_token_name, + ) + except (ImportError, RuntimeError) as exc: + logger.warning( + "Bitwarden refresh failed at proxy start, falling back to " + "parent env: %s", exc, + ) + + # Caller-supplied overrides win. This is intentionally last so the + # wizard can inject ad-hoc test secrets without recomputing the BW + # path. + if extra_env: + env.update(extra_env) + + # Strip proxy-recursion-risk vars regardless of how they got in. + for name in _PROXY_SUBPROCESS_ENV_STRIP: + env.pop(name, None) + + env.setdefault("NO_COLOR", "1") + return env + + def stop_proxy() -> bool: """Stop the managed iron-proxy. Returns True if it was running.""" + global _proxy_nonce + pid = _read_pid() if not pid or not _pid_alive(pid): _pidfile().unlink(missing_ok=True) + _proxy_nonce = None return False + # Capture starttime BEFORE signalling so we can compare after the + # grace window — if the pid got recycled mid-wait, the starttime + # changes and we abort the SIGKILL. + starttime_before = _pid_proc_starttime(pid) + try: os.kill(pid, signal.SIGTERM) except ProcessLookupError: _pidfile().unlink(missing_ok=True) + _proxy_nonce = None return False # Wait up to 5s for graceful exit, then SIGKILL. @@ -832,18 +1440,41 @@ def stop_proxy() -> bool: break time.sleep(0.1) else: - try: - os.kill(pid, signal.SIGKILL) - except ProcessLookupError: - pass + # Verify the pid hasn't been recycled before delivering SIGKILL. + # Two checks: + # 1. /proc//stat starttime is unchanged (Linux) + # 2. _pid_alive() still says it's an iron-proxy process + starttime_after = _pid_proc_starttime(pid) + recycled = ( + starttime_before is not None + and starttime_after is not None + and starttime_before != starttime_after + ) or not _pid_alive(pid) + if recycled: + logger.warning( + "iron-proxy pid=%s appears recycled before SIGKILL; " + "not killing.", pid, + ) + else: + try: + os.kill(pid, _KILL_SIGNAL) + except ProcessLookupError: + pass _pidfile().unlink(missing_ok=True) + _proxy_nonce = None logger.info("Stopped iron-proxy pid=%s", pid) return True def get_status() -> ProxyStatus: - """Snapshot the current proxy state — does NOT start anything.""" + """Snapshot the current proxy state — does NOT start anything. + + Crucially, this is called per Docker-container-create when egress + enforcement is on. It must not have side-effects (no mkdir, no + binary version subprocess that takes 30s on a hung binary). The + state dir is read-only here. + """ status = ProxyStatus() status.tunnel_port = _read_tunnel_port_from_config() or _DEFAULT_TUNNEL_PORT @@ -851,9 +1482,12 @@ def get_status() -> ProxyStatus: binary = find_iron_proxy(install_if_missing=False) if binary: status.binary_path = binary + # Cached — see iron_proxy_version(). First call still costs one + # subprocess; subsequent calls in the same process are dict + # lookups. status.binary_version = iron_proxy_version(binary) - state = _proxy_state_dir() + state = _proxy_state_dir_ro() cfg = state / "proxy.yaml" ca = state / "ca.crt" if cfg.exists(): @@ -870,13 +1504,16 @@ def get_status() -> ProxyStatus: def _read_tunnel_port_from_config() -> Optional[int]: - cfg = _proxy_state_dir() / "proxy.yaml" + cfg = _proxy_state_dir_ro() / "proxy.yaml" if not cfg.exists(): return None try: import yaml + except ImportError: + return None + try: data = yaml.safe_load(cfg.read_text(encoding="utf-8")) - except Exception: # noqa: BLE001 + except (OSError, yaml.YAMLError): return None # The CLI/Docker side calls this "the tunnel port" because that's how # sandboxes use it (HTTPS_PROXY), but on the iron-proxy side it's the @@ -930,12 +1567,15 @@ __all__ = [ "TokenMapping", "build_proxy_config", "discover_provider_mappings", + "discover_uncovered_providers", + "ensure_audit_log", "ensure_ca_cert", "find_iron_proxy", "get_status", "install_iron_proxy", "iron_proxy_version", "load_mappings", + "merge_mappings", "mint_proxy_token", "start_proxy", "stop_proxy", diff --git a/hermes_cli/config.py b/hermes_cli/config.py index 2df3838a4aae..c1576ca18204 100644 --- a/hermes_cli/config.py +++ b/hermes_cli/config.py @@ -1787,7 +1787,7 @@ DEFAULT_CONFIG = { # real API credentials at the egress boundary. Compromising the sandbox # leaks tokens that only work from behind the proxy. # - # Configure with `hermes proxy setup`. Disabled by default — the rest of + # Configure with `hermes egress setup`. Disabled by default — the rest of # Hermes works exactly as before with `enabled: false`. "proxy": { # Master switch. When false, iron-proxy is never started, no docker @@ -1811,6 +1811,20 @@ DEFAULT_CONFIG = { # proxy is enabled but not running. False = fall back to direct # outbound with real credentials in the sandbox (the legacy posture). "enforce_on_docker": True, + # When true, `hermes egress start` refuses to start if any provider + # env var is set that the proxy cannot strip (Anthropic native + # `x-api-key`, AWS Bedrock SigV4, Azure OpenAI, etc.). These + # credentials would otherwise leak into the sandbox bypassing the + # proxy. Defaults to false because the false-positive cost + # (operator has the env set but doesn't actually use that provider) + # is higher than the security cost of a warning. + "fail_on_uncovered_providers": False, + # SSRF deny list applied to outbound traffic. Omit / leave empty + # to use the safe default: loopback, link-local (incl. cloud + # metadata IPs at 169.254.169.254), and RFC1918. Set to an + # explicit ``[]`` to opt out entirely (only sensible in hermetic + # tests that need to reach a loopback upstream). + "upstream_deny_cidrs": None, # Extra allowed upstream hosts beyond the bundled defaults (which # cover OpenRouter, OpenAI, Anthropic, Google, xAI, Mistral, Groq, # Together, DeepSeek, Nous). Wildcards (`*.foo.com`) are supported. diff --git a/hermes_cli/main.py b/hermes_cli/main.py index 73bd44e407ab..658fa124dd70 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -11125,7 +11125,9 @@ def main(): _proxy_cli.register_cli(egress_parser) def _dispatch_egress(args): # noqa: ANN001 - sub = getattr(args, "proxy_command", None) + # The egress subparser uses dest='egress_command' to stay disjoint + # from the inbound OAuth ``hermes proxy`` subparser (dest='proxy_command'). + sub = getattr(args, "egress_command", None) if sub is not None and hasattr(args, "func") and args.func is not _dispatch_egress: return args.func(args) egress_parser.print_help() diff --git a/hermes_cli/proxy_cli.py b/hermes_cli/proxy_cli.py index 7e774393a67b..d1f07a900caf 100644 --- a/hermes_cli/proxy_cli.py +++ b/hermes_cli/proxy_cli.py @@ -1,4 +1,4 @@ -"""CLI handlers for ``hermes proxy ...``. +"""CLI handlers for ``hermes egress ...``. Subcommands: install — download the pinned iron-proxy binary @@ -8,6 +8,10 @@ Subcommands: status — show binary version + config presence + listen state + mappings disable — flip ``proxy.enabled`` to False (does not stop a running proxy) config — print the generated proxy.yaml path (for debugging / external review) + +The top-level command is ``hermes egress``. Note that the inbound OAuth +reverse-proxy command (``hermes proxy``) lives elsewhere in +``hermes_cli/main.py`` — different direction, different purpose. """ from __future__ import annotations @@ -31,13 +35,18 @@ from hermes_cli.config import load_config, save_config def register_cli(parent_parser: argparse.ArgumentParser) -> None: - """Attach the proxy subcommand tree to a parent parser. + """Attach the egress subcommand tree to a parent parser. Called from ``hermes_cli.main`` as part of building the top-level - ``hermes proxy`` parser. + ``hermes egress`` parser. """ - sub = parent_parser.add_subparsers(dest="proxy_command") + # dest='egress_command' — keeps this subparser tree disjoint from the + # inbound OAuth ``hermes proxy`` subparser (which uses dest='proxy_command'). + # No runtime collision today since they live in separate parser trees, + # but a future grep-and-refactor on ``proxy_command`` would otherwise + # hit both handlers. + sub = parent_parser.add_subparsers(dest="egress_command") install = sub.add_parser( "install", @@ -60,7 +69,14 @@ def register_cli(parent_parser: argparse.ArgumentParser) -> None: setup.add_argument( "--from-bitwarden", action="store_true", help="Treat secrets as managed by Bitwarden — discover provider keys " - "from secrets.bitwarden config instead of the current env", + "from secrets.bitwarden config instead of the current env. Fails " + "loudly if BW is unreachable rather than silently falling back.", + ) + setup.add_argument( + "--rotate-tokens", action="store_true", + help="Mint fresh proxy tokens for every provider (default is to " + "preserve tokens for providers that already had one — avoids " + "401-ing already-running sandboxes on re-setup).", ) setup.set_defaults(func=cmd_setup) @@ -73,7 +89,8 @@ def register_cli(parent_parser: argparse.ArgumentParser) -> None: status = sub.add_parser("status", help="Show proxy state and mappings") status.add_argument( "--show-tokens", action="store_true", - help="Print the proxy tokens (default: redacted prefix only)", + help="Print the proxy tokens (default: redacted prefix only). " + "Beware: tokens may persist in your shell history.", ) status.set_defaults(func=cmd_status) @@ -93,7 +110,7 @@ def cmd_install(args: argparse.Namespace) -> int: console = Console() try: binary = ip.install_iron_proxy(force=bool(args.force)) - except Exception as exc: # noqa: BLE001 + except Exception as exc: # noqa: BLE001 — top-level user-facing error funnel console.print(f"[red]✗ install failed:[/red] {exc}") console.print( " Manual install: https://github.com/ironsh/iron-proxy/releases" @@ -148,10 +165,12 @@ def cmd_setup(args: argparse.Namespace) -> int: bw_cfg = (cfg.get("secrets") or {}).get("bitwarden") or {} if not bw_cfg.get("enabled"): console.print( - " [yellow]--from-bitwarden requested but secrets.bitwarden.enabled is false.[/yellow]" + " [red]✗ --from-bitwarden requested but " + "secrets.bitwarden.enabled is false.[/red]" ) console.print( - " Run `hermes secrets bitwarden setup` first, or omit --from-bitwarden." + " Run `hermes secrets bitwarden setup` first, or omit " + "--from-bitwarden." ) return 1 try: @@ -159,29 +178,56 @@ def cmd_setup(args: argparse.Namespace) -> int: access_token = os.environ.get( bw_cfg.get("access_token_env", "BWS_ACCESS_TOKEN"), "" ).strip() - if access_token: - secrets, _ = bw.fetch_bitwarden_secrets( - access_token=access_token, - project_id=bw_cfg.get("project_id", ""), - cache_ttl_seconds=0, - use_cache=False, - ) - available_env_names = list(secrets.keys()) + if not access_token: console.print( - f" Pulled {len(available_env_names)} env names from Bitwarden." + f" [red]✗ --from-bitwarden requested but " + f"{bw_cfg.get('access_token_env', 'BWS_ACCESS_TOKEN')} " + "is not set in the environment.[/red]" ) - except Exception as exc: # noqa: BLE001 + return 1 + secrets, _ = bw.fetch_bitwarden_secrets( + access_token=access_token, + project_id=bw_cfg.get("project_id", ""), + cache_ttl_seconds=0, + use_cache=False, + ) + available_env_names = list(secrets.keys()) + if not available_env_names: + console.print( + " [red]✗ Bitwarden returned an empty secrets list.[/red]\n" + " Check the project_id in secrets.bitwarden and the " + "BWS access-token's project scope." + ) + return 1 console.print( - f" [yellow]Could not enumerate Bitwarden secrets: {exc}[/yellow]" + f" Pulled {len(available_env_names)} env names from Bitwarden." + ) + except Exception as exc: # noqa: BLE001 — explicit user-facing error + console.print( + f" [red]✗ Could not enumerate Bitwarden secrets: {exc}[/red]" ) console.print( - " Falling back to current process env for discovery." + " Either fix the Bitwarden config and retry, or rerun setup " + "without --from-bitwarden (the proxy will read secrets from " + "the host process env at start time)." ) + return 1 - mappings = ip.discover_provider_mappings( + discovered = ip.discover_provider_mappings( available_env_names=available_env_names or None, ) + # Preserve tokens for providers we already had unless the operator + # explicitly requested rotation. This prevents re-running `hermes + # egress setup` from invalidating tokens baked into already-running + # sandboxes. + existing = ip.load_mappings() + mappings = ip.merge_mappings( + existing=existing, + discovered=discovered, + rotate=bool(getattr(args, "rotate_tokens", False)), + ) + if not mappings: console.print( " [yellow]No known provider API keys found in env/Bitwarden.[/yellow]" @@ -193,6 +239,36 @@ def cmd_setup(args: argparse.Namespace) -> int: console.print(f" - {env_name}") return 1 + # Warn the operator about providers we recognize but can't proxy + # (Anthropic native, AWS Bedrock, Azure OpenAI, etc). These still + # work — they just bypass the egress isolation. + uncovered = ip.discover_uncovered_providers( + available_env_names=available_env_names or None, + ) + if uncovered: + console.print() + console.print( + " [yellow]⚠[/yellow] Detected provider env vars that the " + "proxy does not yet cover:" + ) + for name in uncovered: + console.print(f" - {name}") + console.print( + " [dim]These providers use non-bearer auth (x-api-key, " + "SigV4, etc.) and will hold real credentials inside the " + "sandbox. Egress isolation is INCOMPLETE for these.[/dim]" + ) + + # If we're rotating existing mappings, also surface that — the + # operator might be running containers that hold the old tokens. + if existing and getattr(args, "rotate_tokens", False): + console.print() + console.print( + " [yellow]⚠[/yellow] --rotate-tokens used: any running " + "Hermes sandbox holds stale tokens and will 401 against " + "upstreams until restarted." + ) + table = Table(show_header=True, header_style="bold") table.add_column("Provider env", style="cyan") table.add_column("Upstream hosts", style="dim") @@ -211,11 +287,19 @@ def cmd_setup(args: argparse.Namespace) -> int: cfg = load_config() proxy_cfg = cfg.setdefault("proxy", {}) - tunnel_port = ( - args.tunnel_port - if args.tunnel_port - else int(proxy_cfg.get("tunnel_port", ip._DEFAULT_TUNNEL_PORT)) - ) + # ``args.tunnel_port`` is None when the flag was not given; ``0`` is + # invalid for a TCP listener so we treat it as an explicit refusal + # and surface a clear error rather than silently substituting the + # default. + if args.tunnel_port is not None: + if args.tunnel_port == 0: + console.print( + " [red]✗ --tunnel-port=0 is not a valid TCP port.[/red]" + ) + return 1 + tunnel_port = int(args.tunnel_port) + else: + tunnel_port = int(proxy_cfg.get("tunnel_port", ip._DEFAULT_TUNNEL_PORT)) proxy_cfg["tunnel_port"] = tunnel_port extra_hosts = list(proxy_cfg.get("extra_allowed_hosts") or []) @@ -223,24 +307,38 @@ def cmd_setup(args: argparse.Namespace) -> int: h for h in extra_hosts if h not in ip._DEFAULT_ALLOWED_HOSTS ] + audit_log_path = ip._proxy_state_dir() / "audit.log" + # Pre-create the audit log with 0o600 so iron-proxy inherits private + # perms instead of letting the daemon create it under the default + # umask (potentially world-readable). + ip.ensure_audit_log(audit_log_path) + + # Allow operator override of the deny list via + # ``proxy.upstream_deny_cidrs`` — but the default (None) gives a safe + # default-deny list (loopback, IMDS, RFC1918) that matches the docs + # promise. + deny_cidrs = proxy_cfg.get("upstream_deny_cidrs") iron_cfg = ip.build_proxy_config( mappings=mappings, ca_cert=ca_crt, ca_key=ca_key, tunnel_port=tunnel_port, - audit_log=ip._proxy_state_dir() / "audit.log", + audit_log=audit_log_path, allowed_hosts=allowed, + upstream_deny_cidrs=deny_cidrs, ) cfg_path = ip.write_proxy_config(iron_cfg) mappings_path = ip.write_mappings(mappings) console.print(f" [green]✓[/green] config: {cfg_path}") console.print(f" [green]✓[/green] mappings: {mappings_path}") + console.print(f" [green]✓[/green] audit log: {audit_log_path}") # ------------------------------------------------------------------ enable proxy_cfg["enabled"] = True proxy_cfg.setdefault("auto_install", True) proxy_cfg.setdefault("enforce_on_docker", True) - proxy_cfg.setdefault("credential_source", "bitwarden" if args.from_bitwarden else "env") + proxy_cfg["credential_source"] = "bitwarden" if args.from_bitwarden else "env" + proxy_cfg.setdefault("fail_on_uncovered_providers", False) save_config(cfg) console.print() @@ -249,10 +347,10 @@ def cmd_setup(args: argparse.Namespace) -> int: "Sandboxes will route outbound traffic through it." ) console.print( - " Start: [cyan]hermes proxy start[/cyan]\n" - " Status: [cyan]hermes proxy status[/cyan]\n" - " Stop: [cyan]hermes proxy stop[/cyan]\n" - " Disable: [cyan]hermes proxy disable[/cyan]" + " Start: [cyan]hermes egress start[/cyan]\n" + " Status: [cyan]hermes egress status[/cyan]\n" + " Stop: [cyan]hermes egress stop[/cyan]\n" + " Disable: [cyan]hermes egress disable[/cyan]" ) return 0 @@ -263,16 +361,59 @@ def cmd_start(args: argparse.Namespace) -> int: proxy_cfg = cfg.get("proxy") or {} if not proxy_cfg.get("enabled"): console.print( - "[yellow]proxy.enabled is false — run `hermes proxy setup` first.[/yellow]" + "[yellow]proxy.enabled is false — run `hermes egress setup` " + "first.[/yellow]" ) return 1 + + # If the operator opted in to Bitwarden-rotation semantics, refresh + # upstream secrets from BSM at startup. This is what delivers the + # rotation guarantee that distinguishes ``credential_source: + # bitwarden`` from ``credential_source: env``. Without it, rotating + # a key in the Bitwarden web app doesn't reach the proxy. + credential_source = proxy_cfg.get("credential_source", "env") + bw_cfg = (cfg.get("secrets") or {}).get("bitwarden") + refresh_bw = ( + credential_source == "bitwarden" + and bw_cfg is not None + and bool(bw_cfg.get("enabled")) + ) + + # fail_on_uncovered_providers: when true, refuse to start if any of + # the recognized non-bearer providers (Anthropic native, AWS Bedrock, + # Azure OpenAI, etc.) have env vars set in the host process — those + # would otherwise leak real credentials into the sandbox while + # bypassing the proxy. + if bool(proxy_cfg.get("fail_on_uncovered_providers", False)): + uncovered = ip.discover_uncovered_providers() + if uncovered: + console.print( + "[red]✗ Refusing to start: provider env vars present " + "that bypass the proxy:[/red]" + ) + for name in uncovered: + console.print(f" - {name}") + console.print( + " Set `proxy.fail_on_uncovered_providers: false` in " + "config.yaml to start anyway (sandbox will hold real " + "credentials for those providers)." + ) + return 1 + try: - status = ip.start_proxy() - except Exception as exc: # noqa: BLE001 + status = ip.start_proxy( + refresh_secrets_from_bitwarden=refresh_bw, + bitwarden_config=bw_cfg, + ) + except Exception as exc: # noqa: BLE001 — top-level user-facing funnel console.print(f"[red]✗ failed to start iron-proxy:[/red] {exc}") return 1 if status.pid: - listening = "[green]listening[/green]" if status.listening else "[yellow]not yet listening[/yellow]" + listening = ( + "[green]listening[/green]" + if status.listening + else "[yellow]not yet listening[/yellow]" + ) console.print( f"[green]✓[/green] iron-proxy running pid={status.pid} " f"port={status.tunnel_port} {listening}" @@ -325,6 +466,25 @@ def cmd_status(args: argparse.Namespace) -> int: tok = m.proxy_token if args.show_tokens else _redact_token(m.proxy_token) m_table.add_row(m.real_env_name, ", ".join(m.upstream_hosts), tok) console.print(m_table) + if args.show_tokens: + console.print( + "[yellow]⚠[/yellow] proxy tokens just printed in full — " + "they may persist in your shell history. Consider clearing " + "it after this command." + ) + + # Surface uncovered providers so the operator knows the isolation + # boundary is incomplete for those upstreams. + uncovered = ip.discover_uncovered_providers() + if uncovered: + console.print() + console.print( + "[yellow]Uncovered providers[/yellow] " + "(real credentials still visible inside the sandbox):" + ) + for name in uncovered: + console.print(f" - {name}") + return 0 @@ -338,10 +498,15 @@ def cmd_disable(args: argparse.Namespace) -> int: proxy_cfg["enabled"] = False save_config(cfg) console.print("[green]✓[/green] proxy.enabled set to false") - if ip._read_pid() is not None: + # Use the public get_status() pid (which already incorporates the + # _pid_alive check) instead of reaching into ip._read_pid(). That + # private accessor only proves the pidfile is non-empty — a stale + # pidfile from a crashed previous run would fire the warning + # spuriously. + if ip.get_status().pid is not None: console.print( " iron-proxy is still running — stop it with " - "[cyan]hermes proxy stop[/cyan] if you want it down too." + "[cyan]hermes egress stop[/cyan] if you want it down too." ) return 0 @@ -350,7 +515,9 @@ def cmd_config(args: argparse.Namespace) -> int: console = Console() status = ip.get_status() if status.config_path is None: - console.print("[yellow](no config generated — run `hermes proxy setup`)[/yellow]") + console.print( + "[yellow](no config generated — run `hermes egress setup`)[/yellow]" + ) return 1 console.print(str(status.config_path)) return 0 diff --git a/tests/test_iron_proxy.py b/tests/test_iron_proxy.py index 8cdc063e023b..cf79140e89f6 100644 --- a/tests/test_iron_proxy.py +++ b/tests/test_iron_proxy.py @@ -17,7 +17,6 @@ import io import os import tarfile from pathlib import Path -from typing import Dict from unittest.mock import MagicMock, patch import pytest @@ -99,12 +98,14 @@ def _sample_mapping(env_name: str = "OPENROUTER_API_KEY") -> ip.TokenMapping: ) -def test_build_proxy_config_shape(): +def test_build_proxy_config_shape(tmp_path): m = _sample_mapping() + ca_crt = tmp_path / "ca.crt" + ca_key = tmp_path / "ca.key" cfg = ip.build_proxy_config( mappings=[m], - ca_cert=Path("/tmp/ca.crt"), - ca_key=Path("/tmp/ca.key"), + ca_cert=ca_crt, + ca_key=ca_key, ) # Top-level sections — note `dns` is required by iron-proxy even when # we only use the CONNECT tunnel. @@ -127,23 +128,154 @@ def test_build_proxy_config_shape(): rule_hosts = {r["host"] for r in rule["rules"]} assert rule_hosts == set(m.upstream_hosts) # TLS section names the CA paths - assert cfg["tls"]["ca_cert"] == "/tmp/ca.crt" + assert cfg["tls"]["ca_cert"] == str(ca_crt) -def test_build_proxy_config_custom_allowed_hosts(): +def test_build_proxy_config_custom_allowed_hosts(tmp_path): m = _sample_mapping("OPENAI_API_KEY") cfg = ip.build_proxy_config( mappings=[m], - ca_cert=Path("/tmp/ca.crt"), - ca_key=Path("/tmp/ca.key"), - allowed_hosts=["only.example.com"], + ca_cert=tmp_path / "ca.crt", + ca_key=tmp_path / "ca.key", + allowed_hosts=["custom-host.test"], ) domains = cfg["transforms"][0]["config"]["domains"] # Custom allowed_hosts wins as the base; mapping's hosts get appended. - assert "only.example.com" in domains + assert "custom-host.test" in domains assert "openrouter.ai" in domains # comes from the mapping +# --------------------------------------------------------------------------- +# Default SSRF deny list (regression: docs promise cloud metadata is denied) +# --------------------------------------------------------------------------- + + +def test_default_deny_cidrs_present_when_unspecified(tmp_path): + """build_proxy_config must emit the default deny list when the caller + passes nothing. The IMDS subnet (169.254.0.0/16) MUST be in the result + or the docs claim that ``upstream_deny_cidrs`` refuses cloud metadata + is a lie.""" + + cfg = ip.build_proxy_config( + mappings=[_sample_mapping()], + ca_cert=tmp_path / "ca.crt", + ca_key=tmp_path / "ca.key", + ) + deny = cfg["proxy"]["upstream_deny_cidrs"] + assert "169.254.0.0/16" in deny # IMDS + assert "127.0.0.0/8" in deny # loopback v4 + assert "::1/128" in deny # loopback v6 + assert "10.0.0.0/8" in deny # RFC1918 + assert "172.16.0.0/12" in deny # RFC1918 + assert "192.168.0.0/16" in deny # RFC1918 + + +def test_explicit_empty_deny_cidrs_disables_default(tmp_path): + """Explicit ``[]`` opts out of the default deny list — needed by + hermetic tests that want to talk to a loopback upstream.""" + + cfg = ip.build_proxy_config( + mappings=[_sample_mapping()], + ca_cert=tmp_path / "ca.crt", + ca_key=tmp_path / "ca.key", + upstream_deny_cidrs=[], + ) + assert cfg["proxy"]["upstream_deny_cidrs"] == [] + + +def test_wizard_rendered_yaml_contains_deny_list(hermes_home, tmp_path): + """End-to-end: cmd_setup writes proxy.yaml; the rendered file must + contain the deny list because the wizard now passes the operator's + config-level setting (None → default) through to build_proxy_config.""" + + # Simulate the wizard's call shape (matches proxy_cli.cmd_setup). + state = ip._proxy_state_dir() + (state / "ca.crt").write_text("fake-ca") + (state / "ca.key").write_text("fake-key") + proxy_yaml = ip.build_proxy_config( + mappings=[_sample_mapping()], + ca_cert=state / "ca.crt", + ca_key=state / "ca.key", + # The wizard passes ``upstream_deny_cidrs`` from the config; when + # the operator hasn't set anything, that's None and we get the + # safe default below. + upstream_deny_cidrs=None, + ) + out = ip.write_proxy_config(proxy_yaml) + text = out.read_text(encoding="utf-8") + assert "169.254.0.0/16" in text + + +# --------------------------------------------------------------------------- +# Bind policy (regression: must not bind 0.0.0.0) +# --------------------------------------------------------------------------- + + +def test_default_bind_is_loopback_not_zero_zero(tmp_path): + """``http_listen`` must NOT be ``0.0.0.0:PORT`` or ``:PORT`` (latter is + INADDR_ANY). Loopback only by default; the docker bridge bind is + optional and added in addition, never instead.""" + + cfg = ip.build_proxy_config( + mappings=[_sample_mapping()], + ca_cert=tmp_path / "ca.crt", + ca_key=tmp_path / "ca.key", + tunnel_port=12345, + http_listen=["127.0.0.1:12345"], # explicit so test is deterministic + ) + primary = cfg["proxy"]["http_listen"] + listens = cfg["proxy"]["http_listens"] + assert primary == "127.0.0.1:12345" + assert listens == ["127.0.0.1:12345"] + # Sentinel: confirm we didn't accidentally serialize a bare-port form + # like ":12345" anywhere in the listen list (that's INADDR_ANY). + for entry in listens: + assert not entry.startswith(":") + assert "0.0.0.0" not in entry + + +def test_default_bind_includes_docker_bridge_on_linux(tmp_path, monkeypatch): + """When http_listen isn't passed AND we're on Linux AND a docker + bridge IP is detected, we should bind that bridge IP in addition to + loopback so containers reach the proxy via host-gateway.""" + + monkeypatch.setattr(ip.platform, "system", lambda: "Linux") + monkeypatch.setattr(ip, "_detect_docker_bridge_ip", lambda: "172.17.0.1") + cfg = ip.build_proxy_config( + mappings=[_sample_mapping()], + ca_cert=tmp_path / "ca.crt", + ca_key=tmp_path / "ca.key", + tunnel_port=9090, + ) + assert "127.0.0.1:9090" in cfg["proxy"]["http_listens"] + assert "172.17.0.1:9090" in cfg["proxy"]["http_listens"] + + +# --------------------------------------------------------------------------- +# audit_log wiring (regression: parameter was accepted but never used) +# --------------------------------------------------------------------------- + + +def test_audit_log_path_lands_in_yaml(tmp_path): + cfg = ip.build_proxy_config( + mappings=[_sample_mapping()], + ca_cert=tmp_path / "ca.crt", + ca_key=tmp_path / "ca.key", + audit_log=tmp_path / "audit.log", + ) + assert cfg["log"]["audit_path"] == str(tmp_path / "audit.log") + + +def test_audit_log_omitted_when_caller_passes_none(tmp_path): + cfg = ip.build_proxy_config( + mappings=[_sample_mapping()], + ca_cert=tmp_path / "ca.crt", + ca_key=tmp_path / "ca.key", + audit_log=None, + ) + assert "audit_path" not in cfg["log"] + + def test_write_and_load_mappings_roundtrip(hermes_home): ms = [_sample_mapping("OPENROUTER_API_KEY"), _sample_mapping("OPENAI_API_KEY")] path = ip.write_mappings(ms) @@ -165,17 +297,111 @@ def test_load_mappings_handles_corrupt_json(hermes_home): assert ip.load_mappings() == [] -def test_write_proxy_config_serializes_yaml(hermes_home): +def test_write_proxy_config_serializes_yaml(hermes_home, tmp_path): + ca_crt = tmp_path / "ca.crt" + ca_key = tmp_path / "ca.key" cfg = ip.build_proxy_config( mappings=[_sample_mapping()], - ca_cert=Path("/tmp/ca.crt"), - ca_key=Path("/tmp/ca.key"), + ca_cert=ca_crt, + ca_key=ca_key, ) out = ip.write_proxy_config(cfg) assert out.exists() text = out.read_text(encoding="utf-8") assert "tunnel_listen" in text - assert "ca_cert: /tmp/ca.crt" in text + assert f"ca_cert: {ca_crt}" in text + + +# --------------------------------------------------------------------------- +# Token-preservation on re-setup (regression: clobbered live sandboxes) +# --------------------------------------------------------------------------- + + +def test_merge_mappings_preserves_existing_tokens(): + """Re-running setup must not invalidate tokens baked into already- + running sandboxes. ``merge_mappings`` keeps the prior token for any + provider that's in both lists.""" + + existing = [ + ip.TokenMapping( + proxy_token="hermes-proxy-original-12345", + real_env_name="OPENROUTER_API_KEY", + upstream_hosts=("openrouter.ai",), + ), + ] + discovered = ip.discover_provider_mappings( + available_env_names=["OPENROUTER_API_KEY", "OPENAI_API_KEY"] + ) + merged = ip.merge_mappings(existing=existing, discovered=discovered) + by_name = {m.real_env_name: m for m in merged} + # Original token preserved. + assert by_name["OPENROUTER_API_KEY"].proxy_token == "hermes-proxy-original-12345" + # New provider got a fresh token. + assert by_name["OPENAI_API_KEY"].proxy_token != "hermes-proxy-original-12345" + # Both providers in the result. + assert set(by_name) == {"OPENROUTER_API_KEY", "OPENAI_API_KEY"} + + +def test_merge_mappings_drops_providers_removed_from_env(): + """When a provider is in `existing` but not in `discovered`, it must + be dropped from the result — the operator removed the env var.""" + + existing = [ + ip.TokenMapping( + proxy_token="stale", real_env_name="OPENROUTER_API_KEY", + upstream_hosts=("openrouter.ai",), + ), + ] + discovered = ip.discover_provider_mappings( + available_env_names=["OPENAI_API_KEY"] + ) + merged = ip.merge_mappings(existing=existing, discovered=discovered) + names = {m.real_env_name for m in merged} + assert names == {"OPENAI_API_KEY"} + + +def test_merge_mappings_rotate_mints_fresh_tokens(): + """``rotate=True`` rolls every token regardless of overlap. The + --rotate-tokens flag uses this.""" + + existing = [ + ip.TokenMapping( + proxy_token="hermes-proxy-original-12345", + real_env_name="OPENROUTER_API_KEY", + upstream_hosts=("openrouter.ai",), + ), + ] + discovered = ip.discover_provider_mappings( + available_env_names=["OPENROUTER_API_KEY"] + ) + merged = ip.merge_mappings(existing=existing, discovered=discovered, rotate=True) + assert merged[0].proxy_token != "hermes-proxy-original-12345" + + +# --------------------------------------------------------------------------- +# Uncovered provider detection (regression: non-bearer providers bypass) +# --------------------------------------------------------------------------- + + +def test_uncovered_providers_detects_anthropic_aws(hermes_home, monkeypatch): + monkeypatch.setenv("ANTHROPIC_API_KEY", "sk-ant-test") + monkeypatch.setenv("AWS_ACCESS_KEY_ID", "AKIAEXAMPLE") + uncovered = ip.discover_uncovered_providers() + assert "ANTHROPIC_API_KEY" in uncovered + assert "AWS_ACCESS_KEY_ID" in uncovered + + +def test_uncovered_providers_explicit_names_empty(): + assert ip.discover_uncovered_providers(available_env_names=[]) == [] + + +def test_uncovered_providers_skips_bearer_providers(hermes_home, monkeypatch): + """OPENROUTER_API_KEY etc. are bearer providers — they should NOT + appear in the uncovered list.""" + + monkeypatch.setenv("OPENROUTER_API_KEY", "sk-or-test") + uncovered = ip.discover_uncovered_providers() + assert "OPENROUTER_API_KEY" not in uncovered # --------------------------------------------------------------------------- @@ -531,3 +757,315 @@ def test_platform_asset_name_rejects_windows(monkeypatch): monkeypatch.setattr("platform.machine", lambda: "AMD64") with pytest.raises(RuntimeError, match="does not ship native Windows"): ip._platform_asset_name() + + +# --------------------------------------------------------------------------- +# Subprocess env minimization (regression: host secrets leaked to proxy) +# --------------------------------------------------------------------------- + + +def test_subprocess_env_strips_unrelated_secrets(hermes_home, monkeypatch): + """``_build_proxy_subprocess_env`` must NOT carry every host secret + over to the proxy. /proc//environ on the proxy would otherwise + expose all of them to same-uid local processes.""" + + # Unrelated env vars that should NOT propagate. + monkeypatch.setenv("MY_PRIVATE_TOKEN", "should-not-leak") + monkeypatch.setenv("DATABASE_URL", "postgres://very-private") + monkeypatch.setenv("SLACK_BOT_TOKEN", "xoxb-very-secret") + # Provider keys that ARE in load_mappings should propagate. + monkeypatch.setenv("OPENROUTER_API_KEY", "sk-or-real") + ip.write_mappings([_sample_mapping("OPENROUTER_API_KEY")]) + + env = ip._build_proxy_subprocess_env() + assert "MY_PRIVATE_TOKEN" not in env + assert "DATABASE_URL" not in env + assert "SLACK_BOT_TOKEN" not in env + assert env.get("OPENROUTER_API_KEY") == "sk-or-real" + + +def test_subprocess_env_strips_proxy_recursion_vars(hermes_home, monkeypatch): + """HTTPS_PROXY etc. in the parent env would otherwise recurse iron-proxy + through itself (or send its traffic through a corporate proxy).""" + + monkeypatch.setenv("HTTPS_PROXY", "http://corporate:3128") + monkeypatch.setenv("HTTP_PROXY", "http://corporate:3128") + monkeypatch.setenv("ALL_PROXY", "socks5://corporate:1080") + env = ip._build_proxy_subprocess_env() + assert "HTTPS_PROXY" not in env + assert "https_proxy" not in env + assert "HTTP_PROXY" not in env + assert "ALL_PROXY" not in env + + +def test_subprocess_env_keeps_infrastructure_vars(hermes_home, monkeypatch): + """PATH / HOME / locale must propagate or the child can't even find + its libs.""" + + monkeypatch.setenv("PATH", "/usr/local/bin:/usr/bin") + monkeypatch.setenv("HOME", "/home/test") + monkeypatch.setenv("LANG", "C.UTF-8") + env = ip._build_proxy_subprocess_env() + assert env.get("PATH") == "/usr/local/bin:/usr/bin" + assert env.get("HOME") == "/home/test" + assert env.get("LANG") == "C.UTF-8" + + +# --------------------------------------------------------------------------- +# CA generation TOCTOU (regression: 0o600 only set AFTER copy) +# --------------------------------------------------------------------------- + + +def test_ca_key_created_with_0o600(hermes_home, monkeypatch): + """The CA private key must NEVER exist on disk with default umask + permissions, even transiently. Fix: open with explicit mode=0o600 + so the very first byte is written under tight perms.""" + + # ensure_ca_cert shells out to openssl; mock the subprocess.run calls + # so we don't need openssl on the test host AND don't depend on its + # output format. + def fake_run(args, **kwargs): + # First call: genrsa → -out is at args[-2] + if args[1] == "genrsa": + out = args[-2] + Path(out).write_bytes(b"-----BEGIN RSA PRIVATE KEY-----\nfake\n-----END RSA PRIVATE KEY-----\n") + elif args[1] == "req": + # Find -out path + i = args.index("-out") + Path(args[i + 1]).write_bytes(b"-----BEGIN CERTIFICATE-----\nfake\n-----END CERTIFICATE-----\n") + result = MagicMock() + result.returncode = 0 + return result + + monkeypatch.setattr(ip.shutil, "which", lambda name: "/usr/bin/openssl" if name == "openssl" else None) + monkeypatch.setattr(ip.subprocess, "run", fake_run) + + ca_crt, ca_key = ip.ensure_ca_cert() + assert ca_key.exists() + mode = ca_key.stat().st_mode & 0o777 + assert mode == 0o600, f"CA key has perms {oct(mode)}, expected 0o600" + + +# --------------------------------------------------------------------------- +# Audit log permissions (regression: depended on umask) +# --------------------------------------------------------------------------- + + +def test_ensure_audit_log_creates_with_0o600(hermes_home, tmp_path): + audit = tmp_path / "audit.log" + ip.ensure_audit_log(audit) + assert audit.exists() + mode = audit.stat().st_mode & 0o777 + assert mode == 0o600 + + +def test_ensure_audit_log_tightens_existing_perms(hermes_home, tmp_path): + audit = tmp_path / "audit.log" + audit.write_text("preexisting content\n") + os.chmod(audit, 0o644) + ip.ensure_audit_log(audit) + mode = audit.stat().st_mode & 0o777 + assert mode == 0o600 + + +# --------------------------------------------------------------------------- +# State dir hardening (regression: world-traversable on multi-user hosts) +# --------------------------------------------------------------------------- + + +def test_proxy_state_dir_is_0o700(hermes_home): + state = ip._proxy_state_dir() + mode = state.stat().st_mode & 0o777 + assert mode == 0o700 + + +def test_proxy_state_dir_ro_does_not_create(hermes_home): + """_proxy_state_dir_ro is for read-only callers — it must NOT + materialize the dir. Pure-status code paths shouldn't have the + side-effect of creating ~/.hermes/proxy/.""" + + # Sanity: rw path creates it. + rw = ip._proxy_state_dir() + assert rw.exists() + # Remove it and confirm the ro path doesn't recreate. + import shutil as _shutil + _shutil.rmtree(str(rw)) + assert not rw.exists() + ro = ip._proxy_state_dir_ro() + assert not ro.exists() + # The path string is the same as the rw one. + assert ro == rw + + +# --------------------------------------------------------------------------- +# Mappings clobber refused when corrupt (regression: silent 403s) +# --------------------------------------------------------------------------- + + +def test_docker_egress_args_raises_on_empty_mappings(hermes_home, monkeypatch): + """If mappings.json is missing / corrupt / empty AND + enforce_on_docker is true, refuse to start the sandbox rather than + silently mounting an unusable proxy config.""" + + from tools.environments.docker import _egress_proxy_args_for_docker + from hermes_cli.config import load_config, save_config + + state = ip._proxy_state_dir() + (state / "ca.crt").write_text("fake-ca") + (state / "ca.key").write_text("fake-key") + proxy_cfg = ip.build_proxy_config( + mappings=[_sample_mapping()], + ca_cert=state / "ca.crt", ca_key=state / "ca.key", tunnel_port=9090, + ) + ip.write_proxy_config(proxy_cfg) + # Note: we deliberately do NOT write mappings.json — that's the + # bug-class this test guards against. + + cfg = load_config() + cfg.setdefault("proxy", {})["enabled"] = True + cfg["proxy"]["enforce_on_docker"] = True + save_config(cfg) + + (state / "iron-proxy.pid").write_text("99999") + monkeypatch.setattr(ip, "_pid_alive", lambda pid: True) + monkeypatch.setattr(ip, "_port_listening", lambda h, p: True) + + with pytest.raises(RuntimeError, match="mappings.json is empty or"): + _egress_proxy_args_for_docker() + + +# --------------------------------------------------------------------------- +# CA missing → enforce_on_docker semantics (regression: silent fail-open) +# --------------------------------------------------------------------------- + + +def test_docker_egress_args_raises_when_ca_vanishes(hermes_home, monkeypatch): + """status.configured was True at check time but the CA file + disappeared between then and now (e.g. operator manually deleted + ~/.hermes/proxy/ca.crt). enforce_on_docker=True must refuse.""" + + from tools.environments.docker import _egress_proxy_args_for_docker + from hermes_cli.config import load_config, save_config + + state = ip._proxy_state_dir() + ca = state / "ca.crt" + ca.write_text("fake-ca") + (state / "ca.key").write_text("fake-key") + proxy_cfg = ip.build_proxy_config( + mappings=[_sample_mapping()], + ca_cert=ca, ca_key=state / "ca.key", tunnel_port=9090, + ) + ip.write_proxy_config(proxy_cfg) + ip.write_mappings([_sample_mapping()]) + + cfg = load_config() + cfg.setdefault("proxy", {})["enabled"] = True + cfg["proxy"]["enforce_on_docker"] = True + save_config(cfg) + + (state / "iron-proxy.pid").write_text("99999") + monkeypatch.setattr(ip, "_pid_alive", lambda pid: True) + monkeypatch.setattr(ip, "_port_listening", lambda h, p: True) + + # Build a fake status: configured=True (because both path fields are + # set), but ca_cert_path.exists() is False — simulating the race + # where the CA file vanished between get_status() and the + # exists() recheck inside _egress_proxy_args_for_docker. + fake_status = ip.ProxyStatus( + binary_path=state / "fake-bin", # truthy + config_path=state / "proxy.yaml", + ca_cert_path=state / "missing-ca.crt", # points at nonexistent path + pid=99999, + listening=True, + tunnel_port=9090, + ) + # ProxyStatus.configured returns True iff config_path AND ca_cert_path + # both exist. We need configured=True but the second exists() check + # in docker.py to return False — force that by writing a placeholder + # config_path that exists and pointing ca_cert_path at a missing file. + (state / "proxy.yaml").write_text("# fake") + # ProxyStatus.configured: config_path.exists() and ca_cert_path.exists(). + # Make ca_cert_path .exists() True for the configured check but the + # explicit .exists() recheck path in docker.py reads the same Path, + # which is missing — so we wrap. + class _CAStub: + """Path-like that toggles .exists() so configured=True but the + defensive recheck in docker.py returns False.""" + _calls = 0 + def __init__(self, real: Path): + self._real = real + def __str__(self): + return str(self._real) + @property + def parent(self): + return self._real.parent + def exists(self): + type(self)._calls += 1 + # First call: configured property check → say yes. + # Second call: docker.py defensive recheck → say no. + return type(self)._calls == 1 + fake_status.ca_cert_path = _CAStub(state / "missing-ca.crt") # type: ignore[assignment] + monkeypatch.setattr(ip, "get_status", lambda: fake_status) + + with pytest.raises(RuntimeError, match="CA cert vanished"): + _egress_proxy_args_for_docker() + + +# --------------------------------------------------------------------------- +# Docker env collision detection (regression: docker_env silently bypassed proxy) +# --------------------------------------------------------------------------- + + +def test_docker_env_collision_with_proxy_raises_when_enforce(hermes_home, monkeypatch): + """Setting docker_env: {HTTPS_PROXY: ''} in config.yaml with + enforce_on_docker=true must fail-loud rather than silently inverting + the egress isolation.""" + + from tools.environments.docker import DockerEnvironment + from hermes_cli.config import load_config, save_config + + # Set up a fully-running proxy. + state = ip._proxy_state_dir() + ca = state / "ca.crt" + ca.write_text("fake-ca") + (state / "ca.key").write_text("fake-key") + proxy_cfg = ip.build_proxy_config( + mappings=[_sample_mapping()], + ca_cert=ca, ca_key=state / "ca.key", tunnel_port=9090, + ) + ip.write_proxy_config(proxy_cfg) + ip.write_mappings([_sample_mapping()]) + cfg = load_config() + cfg.setdefault("proxy", {})["enabled"] = True + cfg["proxy"]["enforce_on_docker"] = True + save_config(cfg) + (state / "iron-proxy.pid").write_text("99999") + monkeypatch.setattr(ip, "_pid_alive", lambda pid: True) + monkeypatch.setattr(ip, "_port_listening", lambda h, p: True) + + # Mock the docker availability check so we never shell out. + monkeypatch.setattr( + "tools.environments.docker._ensure_docker_available", lambda: None, + ) + # Mock find_docker so the resolved docker exe isn't probed. + monkeypatch.setattr( + "tools.environments.docker.find_docker", lambda: "/bin/true", + ) + # Mock subprocess.run so we don't actually run `docker run`. We + # only need the constructor to get past the env merge logic. + monkeypatch.setattr( + "tools.environments.docker.subprocess.run", + lambda *a, **k: MagicMock(stdout="abc123\n", returncode=0), + ) + # init_session is the second outbound subprocess we don't care about. + monkeypatch.setattr( + "tools.environments.docker.DockerEnvironment.init_session", + lambda self: None, + ) + + # The collision: user sets HTTPS_PROXY to empty string in docker_env. + with pytest.raises(RuntimeError, match="overrides egress-proxy variables"): + DockerEnvironment( + image="busybox", + env={"HTTPS_PROXY": ""}, # the collision + ) diff --git a/tests/test_iron_proxy_cli.py b/tests/test_iron_proxy_cli.py new file mode 100644 index 000000000000..6d78d14755a5 --- /dev/null +++ b/tests/test_iron_proxy_cli.py @@ -0,0 +1,389 @@ +"""Unit tests for ``hermes_cli.proxy_cli`` command handlers. + +These tests cover the user-facing CLI surface that was previously +uncovered. We mock the iron_proxy module's side-effect functions +(install / start / stop / discover) and exercise the dispatch + +return-code logic plus the small amount of presentation logic in +each handler (e.g. --from-bitwarden's fail-loud path). +""" + +from __future__ import annotations + +import argparse +import os +from pathlib import Path +from unittest.mock import MagicMock, patch + +import pytest + +from agent.proxy_sources import iron_proxy as ip +from hermes_cli import proxy_cli + + +@pytest.fixture +def hermes_home(tmp_path, monkeypatch): + """Point HERMES_HOME at a temp dir so the wizard doesn't touch the + operator's real config. Also blanks any provider env vars so we + don't accidentally read a real key.""" + + home = tmp_path / "hermes" + home.mkdir() + monkeypatch.setenv("HERMES_HOME", str(home)) + for key in list(os.environ): + if key.endswith("_API_KEY") or key in ( + "BWS_ACCESS_TOKEN", "ANTHROPIC_API_KEY", + "AWS_ACCESS_KEY_ID", "AWS_SECRET_ACCESS_KEY", + ): + monkeypatch.delenv(key, raising=False) + return home + + +def _args(**overrides): + ns = argparse.Namespace( + force=False, + tunnel_port=None, + from_bitwarden=False, + rotate_tokens=False, + show_tokens=False, + ) + for k, v in overrides.items(): + setattr(ns, k, v) + return ns + + +# --------------------------------------------------------------------------- +# cmd_install +# --------------------------------------------------------------------------- + + +def test_cmd_install_success_returns_0(hermes_home, monkeypatch): + monkeypatch.setattr(ip, "install_iron_proxy", lambda **kw: Path("/tmp/iron-proxy")) + monkeypatch.setattr(ip, "iron_proxy_version", lambda b: "v0.39.0-test") + rc = proxy_cli.cmd_install(_args()) + assert rc == 0 + + +def test_cmd_install_failure_returns_1(hermes_home, monkeypatch): + def boom(**kw): + raise RuntimeError("download failed") + monkeypatch.setattr(ip, "install_iron_proxy", boom) + rc = proxy_cli.cmd_install(_args()) + assert rc == 1 + + +# --------------------------------------------------------------------------- +# cmd_setup — --from-bitwarden fail-loud paths +# --------------------------------------------------------------------------- + + +def test_cmd_setup_from_bitwarden_refuses_when_bw_disabled(hermes_home, monkeypatch): + """When --from-bitwarden is passed but secrets.bitwarden.enabled=false, + the wizard must FAIL rather than silently rewriting credential_source + to bitwarden.""" + + from hermes_cli.config import load_config, save_config + + cfg = load_config() + cfg.setdefault("secrets", {})["bitwarden"] = {"enabled": False} + save_config(cfg) + + # Pre-stub install + CA so we get to step 3. + monkeypatch.setattr(ip, "find_iron_proxy", lambda **kw: Path("/tmp/iron-proxy")) + monkeypatch.setattr(ip, "iron_proxy_version", lambda b: "test") + monkeypatch.setattr( + ip, "ensure_ca_cert", + lambda **kw: (hermes_home / "ca.crt", hermes_home / "ca.key"), + ) + + rc = proxy_cli.cmd_setup(_args(from_bitwarden=True)) + assert rc == 1 + # Verify we did NOT write credential_source: bitwarden to config. + cfg2 = load_config() + proxy_cfg = cfg2.get("proxy") or {} + assert proxy_cfg.get("credential_source", "env") != "bitwarden" + + +def test_cmd_setup_from_bitwarden_refuses_when_token_missing(hermes_home, monkeypatch): + """--from-bitwarden with secrets.bitwarden.enabled=true but BWS access + token unset → fail loud, not silent env-fallback.""" + + from hermes_cli.config import load_config, save_config + + cfg = load_config() + cfg.setdefault("secrets", {})["bitwarden"] = { + "enabled": True, + "project_id": "test-proj", + "access_token_env": "BWS_ACCESS_TOKEN", + } + save_config(cfg) + monkeypatch.delenv("BWS_ACCESS_TOKEN", raising=False) + + monkeypatch.setattr(ip, "find_iron_proxy", lambda **kw: Path("/tmp/iron-proxy")) + monkeypatch.setattr(ip, "iron_proxy_version", lambda b: "test") + monkeypatch.setattr( + ip, "ensure_ca_cert", + lambda **kw: (hermes_home / "ca.crt", hermes_home / "ca.key"), + ) + + rc = proxy_cli.cmd_setup(_args(from_bitwarden=True)) + assert rc == 1 + + +def test_cmd_setup_from_bitwarden_refuses_on_empty_vault(hermes_home, monkeypatch): + """If BW returns {} (empty vault / scoped wrong / unreachable), fail + loud rather than silently writing credential_source: bitwarden.""" + + from hermes_cli.config import load_config, save_config + + cfg = load_config() + cfg.setdefault("secrets", {})["bitwarden"] = { + "enabled": True, + "project_id": "test-proj", + "access_token_env": "BWS_ACCESS_TOKEN", + } + save_config(cfg) + monkeypatch.setenv("BWS_ACCESS_TOKEN", "bwsk-test-token") + + monkeypatch.setattr(ip, "find_iron_proxy", lambda **kw: Path("/tmp/iron-proxy")) + monkeypatch.setattr(ip, "iron_proxy_version", lambda b: "test") + monkeypatch.setattr( + ip, "ensure_ca_cert", + lambda **kw: (hermes_home / "ca.crt", hermes_home / "ca.key"), + ) + + # Mock fetch_bitwarden_secrets to return an empty dict (empty vault). + fake_bw = MagicMock() + fake_bw.fetch_bitwarden_secrets = lambda **kw: ({}, []) + monkeypatch.setattr("agent.secret_sources.bitwarden", fake_bw, raising=False) + import sys + sys.modules["agent.secret_sources.bitwarden"] = fake_bw + + rc = proxy_cli.cmd_setup(_args(from_bitwarden=True)) + assert rc == 1 + + +def test_cmd_setup_rejects_tunnel_port_zero(hermes_home, monkeypatch): + """--tunnel-port=0 is rejected explicitly (was silently substituting + the default before the fix).""" + + monkeypatch.setenv("OPENROUTER_API_KEY", "sk-or-test") + monkeypatch.setattr(ip, "find_iron_proxy", lambda **kw: Path("/tmp/iron-proxy")) + monkeypatch.setattr(ip, "iron_proxy_version", lambda b: "test") + monkeypatch.setattr( + ip, "ensure_ca_cert", + lambda **kw: (hermes_home / "ca.crt", hermes_home / "ca.key"), + ) + rc = proxy_cli.cmd_setup(_args(tunnel_port=0)) + assert rc == 1 + + +# --------------------------------------------------------------------------- +# cmd_start — fail_on_uncovered_providers + Bitwarden rotation wire-up +# --------------------------------------------------------------------------- + + +def test_cmd_start_refuses_when_proxy_disabled(hermes_home, monkeypatch): + from hermes_cli.config import load_config, save_config + cfg = load_config() + cfg.setdefault("proxy", {})["enabled"] = False + save_config(cfg) + + rc = proxy_cli.cmd_start(_args()) + assert rc == 1 + + +def test_cmd_start_refuses_on_uncovered_provider_when_strict(hermes_home, monkeypatch): + """fail_on_uncovered_providers=true + ANTHROPIC_API_KEY in env = + refuse to start (real credential would otherwise leak into sandbox).""" + + from hermes_cli.config import load_config, save_config + cfg = load_config() + cfg.setdefault("proxy", {})["enabled"] = True + cfg["proxy"]["fail_on_uncovered_providers"] = True + save_config(cfg) + monkeypatch.setenv("ANTHROPIC_API_KEY", "sk-ant-test") + + rc = proxy_cli.cmd_start(_args()) + assert rc == 1 + + +def test_cmd_start_passes_bitwarden_refresh_flag_when_credential_source_is_bitwarden( + hermes_home, monkeypatch, +): + """When credential_source=bitwarden, cmd_start must wire + refresh_secrets_from_bitwarden=True into start_proxy. That's what + delivers the rotation promise the docs make.""" + + from hermes_cli.config import load_config, save_config + cfg = load_config() + cfg.setdefault("proxy", {})["enabled"] = True + cfg["proxy"]["credential_source"] = "bitwarden" + cfg["proxy"]["fail_on_uncovered_providers"] = False + cfg.setdefault("secrets", {})["bitwarden"] = {"enabled": True} + save_config(cfg) + + captured: dict = {} + def fake_start_proxy(**kw): + captured.update(kw) + s = ip.ProxyStatus() + s.pid = 4242 + s.listening = True + s.tunnel_port = 9090 + return s + monkeypatch.setattr(ip, "start_proxy", fake_start_proxy) + monkeypatch.setattr(ip, "discover_uncovered_providers", lambda **kw: []) + + rc = proxy_cli.cmd_start(_args()) + assert rc == 0 + assert captured.get("refresh_secrets_from_bitwarden") is True + assert captured.get("bitwarden_config") is not None + + +def test_cmd_start_does_not_pass_bitwarden_refresh_when_credential_source_is_env( + hermes_home, monkeypatch, +): + from hermes_cli.config import load_config, save_config + cfg = load_config() + cfg.setdefault("proxy", {})["enabled"] = True + cfg["proxy"]["credential_source"] = "env" + cfg["proxy"]["fail_on_uncovered_providers"] = False + save_config(cfg) + + captured: dict = {} + def fake_start_proxy(**kw): + captured.update(kw) + s = ip.ProxyStatus() + s.pid = 4242 + s.listening = True + return s + monkeypatch.setattr(ip, "start_proxy", fake_start_proxy) + monkeypatch.setattr(ip, "discover_uncovered_providers", lambda **kw: []) + + rc = proxy_cli.cmd_start(_args()) + assert rc == 0 + assert captured.get("refresh_secrets_from_bitwarden") is False + + +# --------------------------------------------------------------------------- +# cmd_stop, cmd_status, cmd_disable, cmd_config +# --------------------------------------------------------------------------- + + +def test_cmd_stop_returns_0_when_running(hermes_home, monkeypatch): + monkeypatch.setattr(ip, "stop_proxy", lambda: True) + rc = proxy_cli.cmd_stop(_args()) + assert rc == 0 + + +def test_cmd_stop_returns_0_when_already_stopped(hermes_home, monkeypatch): + monkeypatch.setattr(ip, "stop_proxy", lambda: False) + rc = proxy_cli.cmd_stop(_args()) + assert rc == 0 + + +def test_cmd_status_returns_0(hermes_home, monkeypatch): + monkeypatch.setattr(ip, "get_status", lambda: ip.ProxyStatus()) + monkeypatch.setattr(ip, "load_mappings", lambda: []) + monkeypatch.setattr(ip, "discover_uncovered_providers", lambda **kw: []) + rc = proxy_cli.cmd_status(_args()) + assert rc == 0 + + +def test_cmd_disable_uses_public_status_pid_not_private_read_pid( + hermes_home, monkeypatch, +): + """cmd_disable must read status.pid (which incorporates the _pid_alive + check) — NOT ip._read_pid() directly (which would fire a spurious + 'still running' warning for a stale pidfile from a crashed run).""" + + from hermes_cli.config import load_config, save_config + + cfg = load_config() + cfg.setdefault("proxy", {})["enabled"] = True + save_config(cfg) + + # Pidfile exists but the process is dead. Old code would have warned + # "still running"; the new code reads status.pid which returns None + # because _pid_alive is False, so no spurious warning. + state = ip._proxy_state_dir() + (state / "iron-proxy.pid").write_text("99999") + # _pid_alive returns False → status.pid is None. + monkeypatch.setattr(ip, "_pid_alive", lambda pid: False) + + # If cmd_disable reads _read_pid() directly (old path), this test + # would still pass — but reading status.pid is the correct + # API. Sentinel: confirm _read_pid is NOT called from cmd_disable. + read_pid_calls = [] + real_read_pid = ip._read_pid + def tracked_read_pid(*a, **kw): + read_pid_calls.append((a, kw)) + return real_read_pid(*a, **kw) + monkeypatch.setattr(ip, "_read_pid", tracked_read_pid) + + rc = proxy_cli.cmd_disable(_args()) + assert rc == 0 + # cmd_disable should call get_status() (which may internally call + # _read_pid), but should NOT call _read_pid from its own body. + # Hard to assert directly without source-introspection — the meatier + # assertion is that no "still running" message fired with a stale + # pidfile. That's covered by inspecting return code + config + # mutation only. + from hermes_cli.config import load_config as _lc + cfg2 = _lc() + assert cfg2["proxy"]["enabled"] is False + + +def test_cmd_config_returns_0_when_present(hermes_home, monkeypatch): + fake = ip.ProxyStatus() + fake.config_path = Path("/tmp/proxy.yaml") + monkeypatch.setattr(ip, "get_status", lambda: fake) + rc = proxy_cli.cmd_config(_args()) + assert rc == 0 + + +def test_cmd_config_returns_1_when_missing(hermes_home, monkeypatch): + monkeypatch.setattr(ip, "get_status", lambda: ip.ProxyStatus()) + rc = proxy_cli.cmd_config(_args()) + assert rc == 1 + + +# --------------------------------------------------------------------------- +# Argparse wiring — dest='egress_command' regression +# --------------------------------------------------------------------------- + + +def test_register_cli_uses_egress_command_dest(): + """The subparser dest must be 'egress_command' to stay disjoint from + the inbound OAuth 'hermes proxy' subparser (dest='proxy_command'). + A future grep-and-refactor on proxy_command should not hit this + subparser by accident.""" + + parser = argparse.ArgumentParser(prog="hermes egress") + proxy_cli.register_cli(parser) + # Parse a no-op invocation and confirm the attribute name. + args = parser.parse_args(["install"]) + assert hasattr(args, "egress_command") + assert not hasattr(args, "proxy_command") + + +def test_egress_subcommands_registered(): + """Smoke test: every documented subcommand parses without error.""" + + parser = argparse.ArgumentParser(prog="hermes egress") + proxy_cli.register_cli(parser) + for sub in ("install", "setup", "start", "stop", "status", "disable", "config"): + args = parser.parse_args([sub]) + assert args.egress_command == sub + + +def test_setup_has_rotate_tokens_flag(): + """--rotate-tokens is the documented escape hatch for re-rolling + every proxy token (used after a suspected token leak). Default is + preserve-existing.""" + + parser = argparse.ArgumentParser(prog="hermes egress") + proxy_cli.register_cli(parser) + args = parser.parse_args(["setup"]) + assert args.rotate_tokens is False + args = parser.parse_args(["setup", "--rotate-tokens"]) + assert args.rotate_tokens is True diff --git a/tools/environments/docker.py b/tools/environments/docker.py index 7bb00feaa205..7963a6bd2e63 100644 --- a/tools/environments/docker.py +++ b/tools/environments/docker.py @@ -198,10 +198,15 @@ def _egress_proxy_args_for_docker() -> tuple[list[str], dict[str, str], list[str backend refuses to start the sandbox. """ + # Narrow except: ImportError is the only legitimate failure here. + # Bare ``except Exception`` would hide AttributeError, SyntaxError in + # the config module, etc. and silently start the sandbox without + # proxy enforcement. We let unexpected exceptions propagate so the + # docker backend visibly fails rather than degrading silently. try: from hermes_cli.config import load_config from agent.proxy_sources import iron_proxy as ip - except Exception as exc: # noqa: BLE001 + except ImportError as exc: logger.debug("Egress proxy plumbing unavailable: %s", exc) return ([], {}, []) @@ -234,7 +239,36 @@ def _egress_proxy_args_for_docker() -> tuple[list[str], dict[str, str], list[str return ([], {}, []) if status.ca_cert_path is None or not status.ca_cert_path.exists(): - # configured says ca exists; defensive double-check + # status.configured was True a moment ago but the CA file has + # disappeared. Treat this with the same enforce semantics as the + # other failure branches — silently dropping the CA mount would + # leave the sandbox with proxy env vars pointing at iron-proxy + # but no trust anchor, so every TLS handshake would 5xx; or + # worse, with enforce_on_docker=false we'd drop both the proxy + # vars AND any other isolation, opening the sandbox. + msg = ( + f"iron-proxy CA cert vanished from {status.ca_cert_path}. " + "Re-run `hermes egress setup` to regenerate it." + ) + if enforce: + raise RuntimeError(msg) + logger.warning("%s — continuing without proxy (enforce_on_docker=false).", msg) + return ([], {}, []) + + # Corrupt or empty mappings.json is a silent failure mode that's + # indistinguishable from an upstream outage from inside the sandbox + # (every request returns 403). Refuse to mount with empty mappings + # rather than ship a broken sandbox. + mappings = ip.load_mappings() + if not mappings: + msg = ( + "iron-proxy is configured but mappings.json is empty or " + "corrupt. Re-run `hermes egress setup` to mint provider " + "tokens before starting a sandbox." + ) + if enforce: + raise RuntimeError(msg) + logger.warning("%s — continuing without proxy (enforce_on_docker=false).", msg) return ([], {}, []) container_ca = "/etc/ssl/certs/hermes-egress-ca.crt" @@ -255,10 +289,21 @@ def _egress_proxy_args_for_docker() -> tuple[list[str], dict[str, str], list[str "no_proxy": "127.0.0.1,localhost,::1", # CA bundle locations for the major language runtimes. iron-proxy # presents a leaf cert signed by our CA on every MITM'd connection. + # + # CRITICAL ASYMMETRY: Python (REQUESTS_CA_BUNDLE / SSL_CERT_FILE) + # and curl (CURL_CA_BUNDLE) REPLACE the system CA store. + # NODE_EXTRA_CA_CERTS ADDS to it. A Node.js process that + # bypasses HTTPS_PROXY by using a raw socket would still see the + # system CA store and succeed where Python/curl fail validation. + # We additionally set NODE_OPTIONS=--use-openssl-ca to force Node + # through the OpenSSL store that SSL_CERT_FILE controls, narrowing + # the asymmetry. Not a complete fix — see the docs caveat — but + # closes the easy case. "REQUESTS_CA_BUNDLE": container_ca, # Python `requests` "SSL_CERT_FILE": container_ca, # Python ssl module / OpenSSL "CURL_CA_BUNDLE": container_ca, # curl - "NODE_EXTRA_CA_CERTS": container_ca, # Node.js + "NODE_EXTRA_CA_CERTS": container_ca, # Node.js: adds to system store + "NODE_OPTIONS": "--use-openssl-ca", # Node.js: route through OpenSSL store # For the agent inside the sandbox to identify itself as proxy-aware. "HERMES_EGRESS_PROXY": "1", } @@ -266,7 +311,7 @@ def _egress_proxy_args_for_docker() -> tuple[list[str], dict[str, str], list[str # Surface the per-provider proxy tokens. The sandbox can swap these into # its provider config (or its env, if it reads the standard names) and the # proxy translates them to the real secrets on egress. - for m in ip.load_mappings(): + for m in mappings: env_overrides[f"HERMES_PROXY_TOKEN_{m.real_env_name}"] = m.proxy_token # On Linux, host.docker.internal isn't populated by default — Docker Desktop @@ -564,11 +609,75 @@ class DockerEnvironment(BaseEnvironment): # Explicit environment variables (docker_env config) — set at container # creation so they're available to all processes (including entrypoint). # Egress proxy env vars (HTTPS_PROXY, CA-bundle paths, proxy tokens) - # are merged here so they're visible to the container's entrypoint as - # well as any subsequent execs. Existing self._env entries win, so - # users can still override the proxy explicitly via docker_env config. - merged_env = dict(egress_env_overrides) - merged_env.update(self._env) + # are merged below. Precedence policy: + # + # - When egress enforcement is on AND the user's docker_env tries + # to override one of the proxy-control vars (HTTPS_PROXY, + # SSL_CERT_FILE, etc.), fail-loud rather than silently inverting + # the isolation. The CA mount + tokens would still ship while + # traffic leaves the sandbox direct with real credentials — + # exactly what enforce_on_docker is meant to prevent. + # - When enforcement is off, the user's docker_env wins (current + # behavior) but we log a warning naming both config sources. + # - When the user override is identical to the egress value, no-op. + if egress_env_overrides: + try: + from hermes_cli.config import load_config as _load_cfg_for_collision + _proxy_cfg = (_load_cfg_for_collision().get("proxy") or {}) + except ImportError: + _proxy_cfg = {} + _enforce_egress = bool(_proxy_cfg.get("enforce_on_docker", True)) + # Only the env vars that *control* the egress posture matter + # for collision detection — auxiliary informational vars like + # HERMES_EGRESS_PROXY can be safely overridden. + _critical = { + "HTTPS_PROXY", "https_proxy", "HTTP_PROXY", "http_proxy", + "NO_PROXY", "no_proxy", + "REQUESTS_CA_BUNDLE", "SSL_CERT_FILE", "CURL_CA_BUNDLE", + "NODE_EXTRA_CA_CERTS", + } + _collisions = sorted( + k for k in _critical + if k in self._env + and k in egress_env_overrides + and self._env[k] != egress_env_overrides[k] + ) + if _collisions: + _msg = ( + f"docker_env in config.yaml overrides egress-proxy " + f"variables {_collisions}; enforce_on_docker is " + f"{'enabled' if _enforce_egress else 'disabled'}." + ) + if _enforce_egress: + raise RuntimeError( + f"{_msg} Remove these keys from docker_env or " + "disable enforce_on_docker to opt out of egress " + "isolation." + ) + logger.warning( + "%s Falling back to docker_env values; sandbox traffic " + "will NOT route through the proxy.", _msg, + ) + + # When enforce_on_docker is true, egress overrides win. When + # false, docker_env wins (back-compat for users who deliberately + # opt out). In both cases the collision check above has already + # surfaced any disagreement. + try: + from hermes_cli.config import load_config as _load_cfg_for_precedence + _enforce_egress_merge = bool( + (_load_cfg_for_precedence().get("proxy") or {}) + .get("enforce_on_docker", True) + ) + except ImportError: + _enforce_egress_merge = True + + if _enforce_egress_merge and egress_env_overrides: + merged_env = dict(self._env) + merged_env.update(egress_env_overrides) + else: + merged_env = dict(egress_env_overrides) + merged_env.update(self._env) env_args = [] for key in sorted(merged_env): env_args.extend(["-e", f"{key}={merged_env[key]}"]) diff --git a/website/docs/user-guide/egress/iron-proxy.md b/website/docs/user-guide/egress/iron-proxy.md index d6b0226830f9..d38260151334 100644 --- a/website/docs/user-guide/egress/iron-proxy.md +++ b/website/docs/user-guide/egress/iron-proxy.md @@ -169,6 +169,6 @@ A request to a non-allowlisted host (e.g. `https://attacker.example.com/leak?key ## See also -- Upstream project: -- Upstream docs: +- Upstream project: [github.com/ironsh/iron-proxy](https://github.com/ironsh/iron-proxy) +- Upstream docs: [docs.iron.sh](https://docs.iron.sh/) - Bitwarden integration: [`hermes secrets bitwarden`](../secrets/bitwarden)