[P0] CA key TOCTOU race — shutil.copy2 inherits default umask before os.chmod sets 0o600. There's a window where the private key is world-readable.

Fix: use os.open with explicit mode + os.replace:

fd = os.open(str(staged), os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600)
os.write(fd, key_bytes)
os.close(fd)
os.replace(staged, ca_key)

Fixed in ea5e937. CA private key is now written via os.open(..., O_WRONLY | O_CREAT | O_TRUNC | O_NOFOLLOW, 0o600) with an explicit atomic os.replace. New regression test test_ca_key_created_with_0o600 asserts the resulting perms.

[P0] Non-bearer providers bypass proxy — Only 8 providers are covered. Any provider using custom auth (SigV4, x-api-key, AAD tokens) has its real credentials exposed in the sandbox.

Suggest adding proxy.fail_on_uncovered_providers: true config option and a check at start_proxy() time that compares configured providers against this map.

Fixed in ea5e937. Added discover_uncovered_providers() + proxy.fail_on_uncovered_providers config option. When strict, hermes egress start refuses to start if any of {ANTHROPIC,AZURE_OPENAI,AWS_ACCESS_KEY_ID,...} are set in env. In non-strict mode the wizard + status surface them with a warning. Default is non-strict because false positives (operator has the env set but doesn't use that provider) are common.

[P1] Token entropy is 128 bits, not 256 — .hexdigest()[:32] truncates to 32 hex chars = 128 bits. Adequate for proxy-scoped tokens but the docstring should document the actual entropy. Consider using full .hexdigest() or documenting: "128-bit suffix — collision probability < 2^-64 for up to 2^32 tokens".

Documented in ea5e937. The 128-bit truncation is intentional — kept it as-is and updated the docstring with the birthday-bound math explicitly. 32 hex chars × 4 bits = 128 bits; collision probability < 2^-64 up to 2^32 tokens, which is plenty for a proxy-scoped namespace.

[P1] Broad except Exception — catches SyntaxError, AttributeError, etc. Should be except (ImportError, FileNotFoundError) to avoid masking real bugs in config loading or proxy module.

Adjacent: same anti-pattern at agent/proxy_sources/iron_proxy.py:879 in _read_tunnel_port_from_config (bare except Exception falls back to the default port and hides config-file issues). A single grep pass on except Exception in the new module would catch both in one go.

Fixed in ea5e937. Narrowed to except ImportError. The GodsBoy follow-up about the sibling site at iron_proxy.py:_read_tunnel_port_from_config is also fixed — narrowed to (OSError, yaml.YAMLError).

Fixed in ea5e937. Narrowed _read_tunnel_port_from_config's bare except to (OSError, yaml.YAMLError) with a separate ImportError guard around the yaml import. No more masking config-file issues at the default-port fallback.

[P1] PID file not locked — Two concurrent hermes egress start calls will race. Use fcntl.flock(fd, LOCK_EX | LOCK_NB) or O_CREAT | O_EXCL.

Fixed in ea5e937. Pidfile written via os.open(..., O_WRONLY | O_CREAT | O_TRUNC | O_NOFOLLOW, 0o600) with an os.fstat().st_uid == os.getuid() ownership check. O_NOFOLLOW refuses to follow a pre-existing symlink; the uid check catches a same-uid race that won. ELOOP from a pre-existing symlink is surfaced as a clear RuntimeError naming the path.

Severity: P0

build_proxy_config only emits upstream_deny_cidrs when the caller passes a non-None value. cmd_setup in proxy_cli.py:226 never passes it, so the wizard-generated proxy.yaml has no deny list.

User-facing docs in website/docs/user-guide/egress/iron-proxy.md (lines 146, 161) explicitly promise that 169.254.169.254 (cloud metadata) is refused by upstream_deny_cidrs regardless of allowlist. Today the protection depends entirely on iron-proxy upstream's default behavior, which is unverified. DNS rebinding through an allowlisted host reaches IMDS with real keys swapped in.

Fix: default upstream_deny_cidrs to ['127.0.0.0/8','::1/128','169.254.0.0/16','10.0.0.0/8','172.16.0.0/12','192.168.0.0/16','fc00::/7','fe80::/10'] when caller passes None. Treat only an explicit [] as opt-out, and add a test asserting the wizard-rendered yaml contains the deny list. Surfaced by security, correctness, adversarial, and learnings reviewers.

Fixed in ea5e937. build_proxy_config now emits the default deny list when caller passes None — covers loopback (v4 + v6), 169.254.0.0/16 (IMDS), fe80::/10, and RFC1918. Explicit [] opts out. New tests test_default_deny_cidrs_present_when_unspecified and test_wizard_rendered_yaml_contains_deny_list regression-guard this.

Severity: P0

http_listen: ':9090' binds INADDR_ANY. Any host on the local network can reach the proxy. Combined with a leaked HERMES_PROXY_TOKEN_* (which is visible to anyone who can run docker inspect against a Hermes container, including all same-uid local processes), this lets a LAN peer spend the user's API quota against any allowlisted upstream.

The justification 'so containers can reach it via host.docker.internal' is wrong on Linux: the --add-host=host.docker.internal:host-gateway arg added in docker.py:572 resolves to the docker bridge IP (typically 172.17.0.1), not to all interfaces.

Fix: bind to 127.0.0.1:9090; on Linux additionally bind the docker bridge gateway. Do not use INADDR_ANY. Surfaced by security and adversarial reviewers.

Fixed in ea5e937. Default bind is 127.0.0.1:<port> + the docker0 bridge IP on Linux (auto-detected via ip -4 addr show docker0). Never 0.0.0.0 / :PORT. Tests test_default_bind_is_loopback_not_zero_zero and test_default_bind_includes_docker_bridge_on_linux regression-guard both directions.

Severity: P1

cmd_setup --from-bitwarden writes credential_source: bitwarden to config (proxy_cli.py:243). The user-facing docs in website/docs/user-guide/egress/iron-proxy.md promise: 'rotating a key in BW propagates on the next proxy restart'.

Reality: start_proxy() only does os.environ.copy(). There is no bws re-fetch. Rotated keys do not propagate. The rotation guarantee that distinguishes the bitwarden source from the env source is undelivered.

Fix: when credential_source == 'bitwarden', call agent.secret_sources.bitwarden.fetch_bitwarden_secrets() at startup and merge into extra_env. If that is out of scope for v1, revert the config knob and correct the docs. Surfaced by security, correctness, and api-contract reviewers.

Fixed in ea5e937. start_proxy now accepts refresh_secrets_from_bitwarden + bitwarden_config kwargs; when set, fetches via bw.fetch_bitwarden_secrets() at startup and merges the matching env names into the subprocess env. cmd_start wires this in when credential_source == "bitwarden" AND secrets.bitwarden.enabled == true. Rotation now actually propagates to the proxy on restart.

Severity: P1

build_proxy_config(..., audit_log: Optional[Path] = None, ...) (this signature line) accepts the parameter, but the function body never references it. The call site in proxy_cli.py:231 passes audit_log=ip._proxy_state_dir() / 'audit.log' expecting an audit log to materialize. It does not. Operators relying on this for security forensics have no log.

Fix: wire audit_log into the rendered log: block of the yaml, or drop the parameter entirely and update the docstring + caller. Add a test asserting the rendered config contains the audit-log path. Surfaced by 5 reviewers.

Fixed in ea5e937. audit_log now lands in the rendered yaml at log.audit_path. New ensure_audit_log() helper pre-creates the file with 0o600 so iron-proxy inherits tight perms instead of relying on umask. New tests test_audit_log_path_lands_in_yaml and test_ensure_audit_log_creates_with_0o600 cover the wire-up.

Severity: P1

Eleven user-facing strings across iron_proxy.py and proxy_cli.py reference hermes proxy ... but the registered command is hermes egress .... Copy-pasting any of these messages hits the existing inbound OAuth proxy command, which has different verbs, giving a confusing 'argument not recognized' error.

Locations:

• iron_proxy.py:26, 39, 271 (module docstring)
• iron_proxy.py:766, 773 (this RuntimeError and the next)
• proxy_cli.py:~252, ~283 (post-setup banner and start-error string), plus several additional help strings

Fix: find-and-replace hermes proxy -> hermes egress in user-facing strings only (do not touch references to the inbound OAuth hermes proxy command in unrelated code). Surfaced by correctness, maintainability, and api-contract reviewers.

Fixed in ea5e937. All user-facing hermes proxy ... strings in iron_proxy.py and proxy_cli.py renamed to hermes egress .... Did not touch the inbound OAuth hermes proxy references elsewhere in main.py.

Severity: P3

subprocess.run([binary, '--version'], timeout=30) is invoked from get_status, which the docker backend now calls per-container-create. A hung version invocation stalls container creation for 30 seconds. Version is a constant for a given binary path.

Fix: module-level dict cache keyed by binary path. Reliability reviewer flagged.

Fixed in ea5e937. iron_proxy_version now caches by binary path via a module-level _VERSION_CACHE dict. First call costs one subprocess; subsequent calls in the same process are dict lookups. No more 30s stall per Docker container create on a hung binary.

Severity: P3

import sys is not referenced anywhere in this file.

Fix: remove. Safe auto-fix. Kieran-python reviewer flagged.

Removed in ea5e937.

Severity: P3

args.tunnel_port if args.tunnel_port else ... treats 0 as falsy and silently falls back to the config default. Argparse sets the absent-flag default to None.

Fix: args.tunnel_port if args.tunnel_port is not None else .... (Note: 0 is not a valid TCP port, but failing-loud is better than silently substituting.) Kieran-python reviewer flagged.

Fixed in ea5e937. Now args.tunnel_port is not None. 0 is explicitly rejected with a clear error rather than silently substituting the default.

Severity: P3

ip._read_pid() reaches into a private symbol from a different module and trusts the pidfile content without verifying the process is actually alive. A stale pidfile from a crashed previous run causes the 'proxy is still running' warning to fire spuriously.

Fix: use ip.get_status().pid is not None which already incorporates _pid_alive. This also removes the cross-module underscore access. Correctness reviewer flagged.

Fixed in ea5e937. cmd_disable now uses ip.get_status().pid is not None instead of reaching into ip._read_pid(). Status pid already incorporates the _pid_alive check, so a stale pidfile from a crashed run no longer fires the spurious "still running" warning.

Severity: P3

Lines 106, 130, 137, 171 hardcode Path('/tmp/ca.crt') and Path('/tmp/ca.key') for test assertions. AGENTS.md requires tests to use tmp-path fixtures so they're hermetic across host configurations and Windows / CI variants.

Fix: replace with tmp_path-derived paths constructed via the existing hermes_home fixture. Project-standards reviewer flagged.

Fixed in ea5e937. All hardcoded Path("/tmp/ca.*") test paths replaced with tmp_path-derived fixtures. Tests are now hermetic across hosts and Windows / CI variants.

NODE_OPTIONS should append --use-openssl-ca to existing value, not clobber it — user's --max-old-space-size or other Node flags get silently dropped

Fixed in fa4e87b. The egress dict no longer assigns NODE_OPTIONS directly — it carries the flag in a sentinel key _HERMES_EGRESS_NODE_OPTIONS_APPEND and DockerEnvironment merges into the operator's existing NODE_OPTIONS in env_args computation, with de-duplication on identical flags. User's --max-old-space-size=8192 etc. are preserved. New test test_docker_egress_node_options_uses_sentinel regression-guards the sentinel pattern.

"structured audit log" points to iron-proxy.log but the configured audit path is audit.log — iron-proxy.log is daemon stdout, not per-request audit records

Fixed in fa4e87b. Diagram + step 7 of the data-flow section now correctly point per-request audit records at ~/.hermes/proxy/audit.log, with iron-proxy.log (daemon stdout/stderr) called out as a separate file role.

Severity: P1 (security regression risk on the bind-policy fix #2)

The only sanity check before returning the parsed IP is if ip.count(".") == 3. Strings like 0.0.0.0, 999.999.999.999, 224.0.0.1, 255.255.255.255, even aa.bb.cc.dd would pass and get returned as the docker0 bridge bind address.

Failure mode: a hostile ip shim earlier on the operator's PATH, a kernel/iproute2 version that prefixes the inet line differently, or a misconfigured docker0 with 0.0.0.0 could cause start_proxy to add a public-bind address to proxy.http_listen. That re-opens exactly the LAN exposure that prior P0 #2 just closed (the proxy + a leaked sandbox token = quota theft from any LAN peer).

Also note: _PROXY_SUBPROCESS_ENV_ALLOWLIST includes PATH, so a same-uid attacker can plant ip in a writable PATH entry; combined with this parser, that's a path to convert "shell access on host" into "remote unauthenticated proxy access".

Fix: validate with stdlib instead of a digit-count heuristic:

import ipaddress
try:
    addr = ipaddress.IPv4Address(ip)
except ValueError:
    continue
if addr.is_unspecified or addr.is_loopback or addr.is_multicast or addr.is_reserved:
    continue
return str(addr)

Optional defense-in-depth: shutil.which("ip") at module load and skip detection when not in /usr/sbin / /sbin.

Fixed in fa4e87b. Replaced the count(".") == 3 heuristic with ipaddress.IPv4Address validation + reject is_unspecified / is_loopback / is_multicast / is_reserved / is_link_local / is_global. 9 new parametrized tests in test_detect_docker_bridge_ip_rejects_dangerous cover 0.0.0.0, 127.0.0.1, 224.0.0.1, 240.0.0.0, 169.254.0.1, 8.8.8.8, 999.999.999.999, and aa.bb.cc.dd. The hostile ip shim path you flagged is closed.

Severity: P1 (silently breaks the Bitwarden-refresh promise the PR's own docs make)

This line was changed in the fix commit from setdefault to a direct assignment:

proxy_cfg["credential_source"] = "bitwarden" if args.from_bitwarden else "env"

Operator workflow that breaks:

1. Initial hermes egress setup --from-bitwarden → credential_source: bitwarden. Refresh is wired into cmd_start at line 404.
2. Later, operator runs hermes egress setup (no flag) to add a newly-installed provider. This line silently rewrites credential_source to "env".
3. Next hermes egress start no longer passes refresh_secrets_from_bitwarden=True. Rotating a key in Bitwarden no longer reaches the proxy. The docs still promise rotation works.

This is the same class of issue as prior finding #10 (--from-bitwarden silent degrade), except this one is in the re-setup path rather than the first setup path. Adjacent surface, novel location.

Fix: restore setdefault, OR detect the downgrade and either warn loudly or require --no-bitwarden to be explicit:

existing_source = proxy_cfg.get("credential_source")
if args.from_bitwarden:
    proxy_cfg["credential_source"] = "bitwarden"
elif existing_source == "bitwarden":
    console.print("[yellow]Keeping credential_source=bitwarden from existing config. "
                  "Pass --no-bitwarden to switch back to env-based credentials.[/yellow]")
else:
    proxy_cfg["credential_source"] = "env"

Fixed in fa4e87b. Restored preservation semantics — re-running hermes egress setup without --from-bitwarden no longer silently downgrades credential_source back to env. Added an explicit --no-bitwarden flag for the deliberate-switch case (prints a confirmation message when the existing source was bitwarden). Otherwise the existing mode is preserved and the wizard surfaces the decision (dim text) so the operator sees that we kept it.

Severity: P1 (operators reading the module believe non-bearer providers fail-loud; they don't)

Module-level comment at line 135:

"...AND proxy.fail_on_uncovered_providers is true (default), start_proxy refuses to start."

Reality:

• hermes_cli/proxy_cli.py:341 - proxy_cfg.setdefault("fail_on_uncovered_providers", False)
• hermes_cli/proxy_cli.py:387 - if bool(proxy_cfg.get("fail_on_uncovered_providers", False)):
• hermes_cli/config.py DEFAULT_CONFIG - also False.

So the wizard ships fail-OPEN and the runtime check is gated false-by-default. An operator who reads the module docstring believes that having ANTHROPIC_API_KEY in their host env will refuse-to-start; they get the silent-fall-through path instead, and the sandbox boots with real x-api-key bypassing the proxy. This is exactly the threat surface the comment claims to defend.

Fix (pick one - they have different implications, so this needs your judgment):

• Safer default: flip default to True everywhere (setdefault in cmd_setup, .get(..., True) in cmd_start, DEFAULT_CONFIG entry). Matches docstring; matches the spirit of "this is a security feature, default fail-closed". Cost: more operator friction on first run if they happen to have any provider env vars set.
• Truth-in-advertising: leave default at False, but update the docstring and the user docs to say "default false; opt-in via proxy.fail_on_uncovered_providers: true in config".

The current state (docstring promises fail-closed; behavior is fail-open) is the bad option.

Fixed in fa4e87b via the truth-in-advertising path + tier split. The module docstring now correctly states default=False. Separately, _NON_BEARER_PROVIDERS is split into a strict LLM-specific tier (_LLM_SPECIFIC_NON_BEARER_PROVIDERS — Anthropic / Azure / Gemini) that BLOCKS start when fail_on_uncovered_providers: true, vs a generic uncovered tier (AWS_*, GCP appdefault) that's surfaced as warnings only. New discover_blocked_providers() returns the strict subset. Test test_blocked_providers_subset_of_uncovered regression-guards the subset relationship. Operators with terraform/gcloud configured no longer hit refuse-start for unrelated tooling.

Severity: P1

The poll loop at lines 1271-1281:

deadline = time.time() + _STARTUP_GRACE_SECONDS
while time.time() < deadline:
    if proc.poll() is not None:
        ...raise...
    if _port_listening("127.0.0.1", tunnel_port):
        break
    time.sleep(0.1)