mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-31 19:16:29 +00:00
fix(egress): v3 round — GodsBoy/stephenschoettler/arshkumarsingh findings
GodsBoy 2nd-round P1 (all 4 addressed):
- _detect_docker_bridge_ip: replace `ip.count('.') == 3` heuristic with
ipaddress.IPv4Address validation + reject unspecified/loopback/multicast/
reserved/link-local/global addresses. Hostile `ip` shim on PATH used to
be able to inject 0.0.0.0 here and re-open INADDR_ANY binding.
- cmd_setup credential_source preservation: re-running `hermes egress
setup` without --from-bitwarden no longer silently downgrades a previous
bitwarden config back to env. Require --no-bitwarden to switch
explicitly; otherwise preserve the existing mode and surface the
decision.
- fail_on_uncovered_providers docstring/default mismatch: docstring used
to claim default=True; behavior was default=False. Resolved by
truth-in-advertising — docstring now correctly states default=False —
AND splitting providers into a strict LLM-specific tier
(_LLM_SPECIFIC_NON_BEARER_PROVIDERS, used by start blocking) and a
generic uncovered tier (used by wizard warnings). Generic cloud creds
(AWS_*, GOOGLE_APPLICATION_CREDENTIALS) no longer trip refuse-start
for operators using terraform/gcloud alongside Hermes. New
discover_blocked_providers() returns the strict subset.
- start_proxy poll-loop must verify listening before pidfile:
previously fell through deadline-expired as success and wrote a
pidfile for a non-listening daemon. Refactored into a do-while
shape, require `listening=True` for success, kill the child + unlink
the pidfile on failure paths.
GodsBoy 2nd-round P2 (the worth-keeping subset):
- O_NOFOLLOW + 0o600 + st_uid check on iron-proxy.log open (symmetric
with the pidfile and audit-log paths the same PR hardens).
- pidfile O_EXCL: refactored pidfile-write into _write_pidfile_safely
which uses O_EXCL to detect concurrent starts. EEXIST with a live
pid means "another start in progress" — refuse with actionable
message; EEXIST with a dead pid means "stale crash" — unlink and
retry once. Discriminates rather than racing.
- _VERSION_CACHE: invalidate on install_iron_proxy success;
don't cache empty stdout (would poison `hermes egress status` for
the lifetime of the process if first probe hit a corrupt binary).
- ensure_audit_log now RAISES on OSError instead of swallowing it as
a warning. Previous behavior let the daemon create the file under
the default umask, exactly the world-readable scenario the helper
was built to prevent. cmd_setup catches the new RuntimeError and
surfaces "✗" with the actionable message.
- SIGINT/SIGTERM handler scoped around the start_proxy poll loop:
Ctrl-C while waiting for `hermes egress start` no longer leaks an
orphan daemon with the port bound. Handler kills the child +
unlinks the pidfile before re-raising.
- pidfile written IMMEDIATELY after Popen, BEFORE the listening
verification. Parent dying during the poll loop now leaves a
pidfile pointing at the orphan so the next `hermes egress stop` can
clean up. Failure paths in the poll loop explicitly unlink.
- _DEFAULT_UPSTREAM_DENY_CIDRS: add ::ffff:0:0/96 (IPv4-mapped IPv6 —
closes the v6-resolved IMDS bypass), 100.64.0.0/10 (CGNAT / cloud
overlays / K8s pod networks), 198.18.0.0/15 (RFC2544 benchmark).
- _NON_BEARER_PROVIDERS split into LLM-specific (Anthropic / Azure /
Gemini — block when strict) vs generic-cloud (AWS_*, GCP appdefault
— warn-only).
- docker.py except narrowing: load_config can raise yaml.YAMLError on
a malformed config.yaml, not just ImportError. Two callsites
(collision check + precedence resolution) now catch yaml.YAMLError
via a sentinel `import yaml` and fail-safe to enforced mode.
GodsBoy 2nd-round P3:
- _reset_for_tests: was a no-op claiming symmetry with bitwarden;
now actually clears _VERSION_CACHE and _proxy_nonce so in-process
callers (notebooks, pytest -p no:xdist) don't see state leakage.
- tests/test_iron_proxy_cli.py: replaced hardcoded Path("/tmp/...")
with hermes_home/-derived fixtures. Matches the same cleanup we
did for test_iron_proxy.py in the previous round.
- --rotate-tokens confirmation gate: when there are existing tokens,
prompt for "rotate" confirmation (skipped when stdin isn't a tty
so CI/scripted use still works) AND back up the mappings to a
timestamped sibling before overwriting. Surface a no-op note when
rotate is requested with no existing tokens.
stephenschoettler (runtime-boundary review):
- #1 BWS silent degrade at proxy start: when credential_source=bitwarden
but the BWS access token or project_id is missing OR the fetch
returns no values for mapped providers, raise instead of silently
falling back to host env. cmd_start also pre-checks at the wizard
layer for actionable error messages. Opt-in escape hatch via new
`proxy.allow_env_fallback: true` config for migration scenarios.
- #2 docker_env collision detection extended: `docker_env:
{OPENROUTER_API_KEY: sk-real}` in config.yaml with enforce_on_docker:
true now raises just like an HTTPS_PROXY collision would. The
collision check pulls mapped provider names from load_mappings() at
call time.
- #3 PID nonce persisted to disk: cross-CLI-invocation stale-pidfile
defense now works. start_proxy writes the nonce next to the pidfile
(sibling 0o600), stop_proxy reads it back via _read_persisted_nonce()
and uses it as a _pid_alive signal in the new process. Falls back
to argv0 basename matching when the file is missing (legacy install).
arshkumarsingh:
- #1 NODE_OPTIONS append-merge: egress dict no longer sets NODE_OPTIONS
directly (would clobber the operator's --max-old-space-size etc.).
Carry the egress flag in a sentinel key
_HERMES_EGRESS_NODE_OPTIONS_APPEND; DockerEnvironment merges into the
existing NODE_OPTIONS in env_args computation with de-duplication.
- #2 docs: structured per-request audit log is at audit.log, not
iron-proxy.log (the latter is daemon stdout/stderr). Diagram and
step-7 text corrected; both file roles are now documented separately.
Tests
- Added 12 new tests in test_iron_proxy.py covering bridge-IP rejection
(parametrized over 8 dangerous inputs), default deny-list adjacency
(IPv4-mapped-v6 + CGNAT), blocked-providers strict-subset property,
_pid_proc_starttime parser with paren-containing comm,
stop_proxy SIGKILL suppression on starttime drift, _reset_for_tests
clear behavior, iron_proxy_version don't-cache-empty, NODE_OPTIONS
sentinel verification, ensure_audit_log raise-on-OSError, and
persisted-nonce roundtrip.
- Added 1 new test in test_iron_proxy_cli.py covering cmd_start
BWS-token-missing fail-loud.
- All 100 tests in test_iron_proxy + test_iron_proxy_cli pass; all 78
tests in test_docker_environment + test_config still pass.
Acknowledged but not addressed:
- GodsBoy P3 dead-code `extra_env` kwarg: kept (removing is a breaking
change for any out-of-tree caller; the kwarg is documented and works).
- Residual risks GodsBoy called out: iron-proxy in-memory secret
zeroisation (Go-binary territory, out of scope); _PROXY_SUBPROCESS_ENV
_ALLOWLIST cosmetic gaps (RUST_LOG, GOMAXPROCS); follow-up.
This commit is contained in:
teknium1
parent
4833acf046
commit
fa4e87b253
7 changed files with 1086 additions and 105 deletions
|
|
@ -72,6 +72,11 @@ def register_cli(parent_parser: argparse.ArgumentParser) -> None:
|
|||
"from secrets.bitwarden config instead of the current env. Fails "
|
||||
"loudly if BW is unreachable rather than silently falling back.",
|
||||
)
|
||||
setup.add_argument(
|
||||
"--no-bitwarden", action="store_true",
|
||||
help="Explicitly switch credential_source back to env on re-setup "
|
||||
"(only meaningful when the previous setup used --from-bitwarden).",
|
||||
)
|
||||
setup.add_argument(
|
||||
"--rotate-tokens", action="store_true",
|
||||
help="Mint fresh proxy tokens for every provider (default is to "
|
||||
|
|
@ -222,10 +227,58 @@ def cmd_setup(args: argparse.Namespace) -> int:
|
|||
# egress setup` from invalidating tokens baked into already-running
|
||||
# sandboxes.
|
||||
existing = ip.load_mappings()
|
||||
rotate = bool(getattr(args, "rotate_tokens", False))
|
||||
|
||||
# P3 confirmation gate: --rotate-tokens invalidates every running
|
||||
# sandbox's proxy tokens immediately. An accidental re-run (history
|
||||
# scroll-back, tmux paste) is unrecoverable, so require explicit
|
||||
# confirmation when there's something to actually rotate. Skipped
|
||||
# when stdin isn't a tty (CI / non-interactive use), in which case
|
||||
# the operator passed the flag deliberately.
|
||||
if rotate and existing:
|
||||
import sys as _sys
|
||||
from datetime import datetime as _dt
|
||||
if _sys.stdin.isatty():
|
||||
console.print(
|
||||
"[yellow]⚠[/yellow] --rotate-tokens will invalidate proxy "
|
||||
"tokens in every running Hermes sandbox. They will start "
|
||||
"401-ing against upstreams until restarted."
|
||||
)
|
||||
try:
|
||||
ans = input("Type 'rotate' to confirm: ").strip().lower()
|
||||
except EOFError:
|
||||
ans = ""
|
||||
if ans != "rotate":
|
||||
console.print("[yellow]Cancelled.[/yellow]")
|
||||
return 1
|
||||
# Backup the existing mappings before we overwrite. The
|
||||
# resulting ``.rotated-<unix>`` sibling is plain JSON and lets
|
||||
# the operator manually recover tokens if they realise the
|
||||
# rotation was a mistake.
|
||||
try:
|
||||
import shutil as _shutil
|
||||
state_dir = ip._proxy_state_dir()
|
||||
mappings_src = state_dir / "mappings.json"
|
||||
if mappings_src.exists():
|
||||
ts = _dt.now().strftime("%Y%m%dT%H%M%S")
|
||||
backup = state_dir / f"mappings.json.rotated-{ts}"
|
||||
_shutil.copy2(str(mappings_src), str(backup))
|
||||
console.print(f" [dim]backup: {backup}[/dim]")
|
||||
except OSError as exc:
|
||||
console.print(
|
||||
f" [yellow]Could not back up mappings before rotation: "
|
||||
f"{exc}[/yellow]"
|
||||
)
|
||||
elif rotate and not existing:
|
||||
console.print(
|
||||
"[dim]Note: --rotate-tokens is a no-op on first-time setup "
|
||||
"(no existing tokens to rotate).[/dim]"
|
||||
)
|
||||
|
||||
mappings = ip.merge_mappings(
|
||||
existing=existing,
|
||||
discovered=discovered,
|
||||
rotate=bool(getattr(args, "rotate_tokens", False)),
|
||||
rotate=rotate,
|
||||
)
|
||||
|
||||
if not mappings:
|
||||
|
|
@ -259,16 +312,6 @@ def cmd_setup(args: argparse.Namespace) -> int:
|
|||
"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")
|
||||
|
|
@ -310,8 +353,15 @@ def cmd_setup(args: argparse.Namespace) -> int:
|
|||
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)
|
||||
# umask (potentially world-readable). Raises on failure (planted
|
||||
# symlink, immutable parent, full disk) — the wizard must surface
|
||||
# that rather than print "✓" for a file the daemon will create
|
||||
# under a slacker umask.
|
||||
try:
|
||||
ip.ensure_audit_log(audit_log_path)
|
||||
except RuntimeError as exc:
|
||||
console.print(f" [red]✗ {exc}[/red]")
|
||||
return 1
|
||||
|
||||
# Allow operator override of the deny list via
|
||||
# ``proxy.upstream_deny_cidrs`` — but the default (None) gives a safe
|
||||
|
|
@ -337,7 +387,30 @@ def cmd_setup(args: argparse.Namespace) -> int:
|
|||
proxy_cfg["enabled"] = True
|
||||
proxy_cfg.setdefault("auto_install", True)
|
||||
proxy_cfg.setdefault("enforce_on_docker", True)
|
||||
proxy_cfg["credential_source"] = "bitwarden" if args.from_bitwarden else "env"
|
||||
# CRITICAL: do NOT silently downgrade credential_source on re-run.
|
||||
# If the operator previously configured `bitwarden` mode (e.g. for
|
||||
# rotation), running `hermes egress setup` again WITHOUT
|
||||
# --from-bitwarden must not rewrite credential_source to "env" —
|
||||
# that silently breaks the Bitwarden rotation guarantee the docs
|
||||
# make. Require an explicit --no-bitwarden to switch back.
|
||||
existing_source = proxy_cfg.get("credential_source")
|
||||
if args.from_bitwarden:
|
||||
proxy_cfg["credential_source"] = "bitwarden"
|
||||
elif getattr(args, "no_bitwarden", False):
|
||||
proxy_cfg["credential_source"] = "env"
|
||||
if existing_source == "bitwarden":
|
||||
console.print(
|
||||
"[yellow]Switched credential_source from bitwarden to env.[/yellow]"
|
||||
)
|
||||
elif existing_source == "bitwarden":
|
||||
# Preserve the existing bitwarden mode. Surface the decision so
|
||||
# the operator knows we kept it.
|
||||
console.print(
|
||||
"[dim]Keeping credential_source=bitwarden from existing config. "
|
||||
"Pass --no-bitwarden to switch to env-based credentials.[/dim]"
|
||||
)
|
||||
else:
|
||||
proxy_cfg["credential_source"] = "env"
|
||||
proxy_cfg.setdefault("fail_on_uncovered_providers", False)
|
||||
save_config(cfg)
|
||||
|
||||
|
|
@ -378,20 +451,32 @@ def cmd_start(args: argparse.Namespace) -> int:
|
|||
and bw_cfg is not None
|
||||
and bool(bw_cfg.get("enabled"))
|
||||
)
|
||||
# Pass the proxy-side allow_env_fallback opt-in through to
|
||||
# start_proxy. This is a deliberate, documented escape hatch: when
|
||||
# set, the daemon silently falls back to host env if BWS is
|
||||
# unreachable, instead of raising. Default is strict (raise).
|
||||
if refresh_bw and bw_cfg is not None:
|
||||
bw_cfg = dict(bw_cfg)
|
||||
bw_cfg["allow_env_fallback"] = bool(
|
||||
proxy_cfg.get("allow_env_fallback", False)
|
||||
)
|
||||
|
||||
# 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.
|
||||
# fail_on_uncovered_providers: when true, refuse to start if any
|
||||
# LLM-specific non-bearer providers (Anthropic native, Azure OpenAI,
|
||||
# Gemini) have env vars set in the host process — those would
|
||||
# otherwise leak real credentials into the sandbox while bypassing
|
||||
# the proxy. Only the strict LLM-specific subset blocks; generic
|
||||
# cloud creds (AWS_*, GOOGLE_APPLICATION_CREDENTIALS) still surface
|
||||
# as warnings via `discover_uncovered_providers` but don't block, to
|
||||
# avoid tripping every operator with terraform / gcloud set up.
|
||||
if bool(proxy_cfg.get("fail_on_uncovered_providers", False)):
|
||||
uncovered = ip.discover_uncovered_providers()
|
||||
if uncovered:
|
||||
blocked = ip.discover_blocked_providers()
|
||||
if blocked:
|
||||
console.print(
|
||||
"[red]✗ Refusing to start: provider env vars present "
|
||||
"that bypass the proxy:[/red]"
|
||||
)
|
||||
for name in uncovered:
|
||||
for name in blocked:
|
||||
console.print(f" - {name}")
|
||||
console.print(
|
||||
" Set `proxy.fail_on_uncovered_providers: false` in "
|
||||
|
|
@ -400,6 +485,37 @@ def cmd_start(args: argparse.Namespace) -> int:
|
|||
)
|
||||
return 1
|
||||
|
||||
# stephenschoettler #1: when `credential_source: bitwarden`, the
|
||||
# operator picked BWS specifically to get the rotation guarantee —
|
||||
# silently falling back to parent-env at start_proxy time reintroduces
|
||||
# exactly the bug class the BW mode is supposed to defeat (host env
|
||||
# is stale / mismatched). Pre-check at the wizard layer so we fail
|
||||
# loud with actionable error messages BEFORE start_proxy degrades.
|
||||
if refresh_bw:
|
||||
bw_access_env = (bw_cfg or {}).get("access_token_env", "BWS_ACCESS_TOKEN")
|
||||
if not os.environ.get(bw_access_env, "").strip():
|
||||
console.print(
|
||||
f"[red]✗ Refusing to start: credential_source=bitwarden but "
|
||||
f"{bw_access_env} is not set in the environment.[/red]"
|
||||
)
|
||||
console.print(
|
||||
" Either export the access token, or run "
|
||||
"`hermes egress setup --no-bitwarden` to switch back to "
|
||||
"env-based credentials."
|
||||
)
|
||||
return 1
|
||||
if not (bw_cfg or {}).get("project_id"):
|
||||
console.print(
|
||||
"[red]✗ Refusing to start: credential_source=bitwarden but "
|
||||
"secrets.bitwarden.project_id is empty.[/red]"
|
||||
)
|
||||
console.print(
|
||||
" Run `hermes secrets bitwarden setup` to configure the "
|
||||
"project, or switch back via `hermes egress setup "
|
||||
"--no-bitwarden`."
|
||||
)
|
||||
return 1
|
||||
|
||||
try:
|
||||
status = ip.start_proxy(
|
||||
refresh_secrets_from_bitwarden=refresh_bw,
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue