diff --git a/agent/command_token_source.py b/agent/command_token_source.py index fac8aa64a4..bebff150b6 100644 --- a/agent/command_token_source.py +++ b/agent/command_token_source.py @@ -1,12 +1,9 @@ """Mint a provider API key by running a command (``key_cmd``). -Static API keys are the exception at enterprise gateways: SSO/OIDC brokers, -cloud IAM, and internal auth proxies all issue SHORT-LIVED bearers instead. -A key copied into ``.env`` (``key_env``) is stale within the hour, so every -request after that 401s and the user has to restart the session. - -``key_cmd`` names a command that PRINTS a token, so the credential is derived -rather than stored:: +Enterprise gateways (SSO/OIDC brokers, cloud IAM, auth proxies) issue +SHORT-LIVED bearers; a key copied into ``.env`` goes stale within the hour. +``key_cmd`` names a command that PRINTS a token (the ``apiKeyHelper`` / +``gcloud auth print-access-token`` / ``databricks auth token`` idiom):: providers: my-gateway: @@ -14,22 +11,14 @@ rather than stored:: api_mode: chat_completions key_cmd: my-auth-cli print-token --profile prod -This is the established pattern for agent tooling — Claude Code's -``apiKeyHelper``, the ``gcloud auth print-access-token`` / ``aws ecr -get-login-password`` idiom, and vendor helpers such as ``databricks auth -token`` all expose exactly this contract. Hermes already accepts a callable -API key on both wire clients (the Entra ID / Azure identity path) and invokes -it per request, so nothing downstream changes: the token is simply always -fresh. It is cached until shortly before expiry, so the command runs about -once per token lifetime rather than once per request. +Both wire clients already accept a callable API key and invoke it per request, +so the token is simply always fresh; it is cached until shortly before expiry. -Output contract: print ONLY the token on stdout, either bare or as JSON with -an ``access_token`` field (``expires_in`` is honoured when present) — the -shape OAuth 2.0 token endpoints and the helpers above already emit. +Output contract: ONLY the token on stdout, bare or as JSON with an +``access_token`` field (``expires_in`` honoured when present). -Precedence: an explicit ``--api-key`` still wins (the one-off recovery escape -hatch); otherwise ``key_cmd`` is preferred over a static ``api_key`` / -``key_env`` on the same entry. +Precedence: an explicit ``--api-key`` still wins (one-off recovery escape +hatch); otherwise ``key_cmd`` beats a static ``api_key`` / ``key_env``. """ from __future__ import annotations @@ -43,19 +32,15 @@ from typing import Callable, Optional logger = logging.getLogger(__name__) -# Treat a cached token as spent slightly before its stated expiry, so a request -# can't be signed with a token that dies in flight. 60s matches the leeway used -# by comparable OAuth token caches. +# Treat a token as spent slightly before expiry so a request can't be signed +# with one that dies in flight (60s = usual OAuth cache leeway). _TOKEN_REFRESH_LEEWAY_SECONDS = 60.0 -# A token helper reads a local credential cache and should answer in -# milliseconds; anything approaching this budget is hung, not slow. +# Helpers answer from a local cache in milliseconds; this long means hung. _MINT_TIMEOUT_SECONDS = 15 -# When a helper advertises NO expiry, the token cannot be cached for the life -# of the process: nothing in the request path re-mints on 401 (the SDK retries -# 429/5xx only), so an expired no-TTL token would 401 every request until -# restart. Re-mint on a bounded window instead — the helper answers from a -# local credential cache in milliseconds, so a periodic re-run is cheap, and a -# helper that wants a longer cache can simply advertise its real expiry. +# No advertised expiry: nothing in the request path re-mints on 401 (the SDK +# retries 429/5xx only), so a process-lifetime cache would 401 forever once the +# token died. Re-mint on a bounded window instead; helpers wanting a longer +# cache advertise their real expiry. _NO_TTL_REFRESH_SECONDS = 900.0 @@ -84,12 +69,8 @@ def _mint(command: str, label: str) -> tuple[str, Optional[float]]: ) from exc if completed.returncode != 0: - # NEVER include stdout/stderr: a partially-successful auth helper can - # print a token or refresh secret there. The command STRING is also - # withheld — a key_cmd can legitimately embed a secret - # (`print-token --client-secret=…`), so echoing it back would leak the - # very credential this module exists to protect. Name the provider so - # the user knows which config entry to run by hand. + # NEVER include stdout/stderr (may hold a token) or the command string + # (may embed `--client-secret=…`); name the provider instead. raise CommandTokenError( f"key_cmd for provider {label!r} exited {completed.returncode}. " f"Run that provider's key_cmd manually to see why " @@ -101,8 +82,6 @@ def _mint(command: str, label: str) -> tuple[str, Optional[float]]: raise CommandTokenError(f"key_cmd for provider {label!r} produced no output") # JSON payload — the shape `databricks auth token --output json` prints. - # Token extraction mirrors databricks/ucode's get_databricks_token: - # json.loads(result.stdout or "{}").get("access_token", "") if stdout.lstrip().startswith("{"): try: payload = json.loads(stdout) @@ -118,12 +97,9 @@ def _mint(command: str, label: str) -> tuple[str, Optional[float]]: ttl = payload.get("expires_in") if isinstance(ttl, (int, float)) and ttl > 0: return token, float(ttl) - # A relative lifetime is the OAuth 2.0 field, but CLI token helpers - # commonly print an absolute ISO 8601 deadline instead. Treating - # that as "no TTL advertised" caches the token for the life of the - # process, so every request 401s once the deadline passes. - # Imported lazily: hermes_cli.auth imports from agent.* at module - # level, so a top-level import here would risk a cycle. + # CLI helpers often print an absolute ISO 8601 deadline instead of + # OAuth's relative lifetime; honour it or the token 401s once past. + # Lazy import: hermes_cli.auth imports agent.* at module level. from hermes_cli.auth import _parse_iso_timestamp for field in ("expiry", "expiresOn"): @@ -134,12 +110,9 @@ def _mint(command: str, label: str) -> tuple[str, Optional[float]]: return token, remaining return token, None - # Bare token. The contract every comparable helper documents is "stdout - # carries the token and nothing else" — extra output would be consumed as - # part of the credential. Strip surrounding whitespace and take the rest - # verbatim; do NOT silently keep one line of several, which converts a - # misconfigured helper (banner, warning, two tokens) into a corrupt-key 401 - # that is far harder to diagnose than an explicit refusal. + # Bare token: stdout carries the token and nothing else. Do NOT keep one + # line of several — that turns a misconfigured helper (banner, warning) into + # a corrupt-key 401 far harder to diagnose than an explicit refusal. token = stdout.strip() if "\n" in token: raise CommandTokenError( @@ -165,12 +138,8 @@ class CommandTokenSource: return self._token token, ttl = _mint(self._command, self._label) self._token = token - self._expires_at = ( - time.monotonic() + max(ttl - _TOKEN_REFRESH_LEEWAY_SECONDS, 5.0) - if ttl - # No advertised TTL: bounded cache (see _NO_TTL_REFRESH_SECONDS) - # — there is no 401-driven re-mint hook to fall back on. - else time.monotonic() + _NO_TTL_REFRESH_SECONDS + self._expires_at = time.monotonic() + ( + max(ttl - _TOKEN_REFRESH_LEEWAY_SECONDS, 5.0) if ttl else _NO_TTL_REFRESH_SECONDS ) logger.debug( "Minted key_cmd token for provider %s (ttl=%s)", diff --git a/agent/secret_scope.py b/agent/secret_scope.py index 86f18f5219..7497d6abb7 100644 --- a/agent/secret_scope.py +++ b/agent/secret_scope.py @@ -1,24 +1,22 @@ """Profile-scoped credential resolution for multi-profile gateway multiplexing. -The multiplexing gateway serves many profiles from one process. Each profile -has its own ``.env`` with its own provider keys and platform tokens, so we -**cannot** union them into the process-global ``os.environ`` (that would leak -profile A's keys to profile B's turns, and to every subprocess spawned with -``env=dict(os.environ)``). +The multiplexing gateway serves many profiles from one process. Each profile +has its own ``.env`` with its own keys, so they **cannot** be unioned into the +process-global ``os.environ`` (profile A's keys would leak into profile B's +turns and into every subprocess spawned with ``env=dict(os.environ)``). -This module provides a fail-closed, context-local secret scope: +This module is a fail-closed, context-local secret scope: - ``set_secret_scope(mapping)`` installs the active profile's secrets for the current task (a contextvar, so it propagates into the agent's worker thread via ``copy_context()`` exactly like the HERMES_HOME override). -- ``get_secret(name)`` reads from that scope. When multiplexing is **active** - and no scope is set, it RAISES rather than silently falling back to - ``os.environ`` — an un-migrated or newly-added call site fails loud at that - exact line instead of leaking another profile's value. When multiplexing is - **off** (the default), it transparently reads ``os.environ`` so the - single-profile gateway and every non-gateway caller behave exactly as before. +- ``get_secret(name)`` reads from that scope. When multiplexing is **active** + and no scope is set it RAISES rather than falling back to ``os.environ`` — + an un-migrated call site fails loud at that line instead of leaking another + profile's value. When multiplexing is **off** (default) it reads + ``os.environ`` so every non-multiplex caller behaves exactly as before. -Design rationale lives in ``docs/design/multiplexing-gateway.md`` (Workstream A). +Design rationale: ``docs/design/multiplexing-gateway.md`` (Workstream A). """ from __future__ import annotations @@ -29,72 +27,52 @@ from pathlib import Path from typing import Dict, Mapping, Optional -# ── multiplex-active flag ──────────────────────────────────────────────── -# Process-global: set once at gateway startup when gateway.multiplex_profiles -# is true. Governs whether get_secret() fails closed on an unscoped read. -# A plain module global (not a contextvar): it describes the deployment mode, -# not a per-task value. +# Process-global (describes the deployment mode, not a per-task value): set once +# at gateway startup when gateway.multiplex_profiles is true. _MULTIPLEX_ACTIVE: bool = False def set_multiplex_active(active: bool) -> None: - """Mark whether the process is running as a profile multiplexer. - - Called once at gateway startup. When True, ``get_secret`` fails closed on - an unscoped read instead of falling back to ``os.environ``. - """ + """Mark whether the process is a profile multiplexer (get_secret fails closed).""" global _MULTIPLEX_ACTIVE _MULTIPLEX_ACTIVE = bool(active) def is_multiplex_active() -> bool: - """Return whether the process is running as a profile multiplexer.""" return _MULTIPLEX_ACTIVE -# ── the secret scope contextvar ────────────────────────────────────────── _SECRET_SCOPE: ContextVar[Optional[Mapping[str, str]]] = ContextVar( "_SECRET_SCOPE", default=None ) class UnscopedSecretError(RuntimeError): - """Raised when a secret is read in multiplex mode with no scope installed. + """A secret was read in multiplex mode with no scope installed. - This is the fail-closed signal: it means a credential read reached - ``get_secret`` without a profile scope active, which in a multiplexer would - otherwise leak whichever profile's value happened to be in ``os.environ``. The fix is to wrap the call path in ``set_secret_scope(...)`` (the per-turn - / per-adapter profile scope), not to widen the allowlist. + / per-adapter profile scope), not to widen the global allowlist. """ def set_secret_scope(secrets: Optional[Mapping[str, str]]) -> Token: - """Install the active profile's secret mapping for the current context. - - Returns a token for ``reset_secret_scope``. Pass ``None`` to clear. - """ + """Install the active profile's secret mapping; ``None`` clears. Returns a reset token.""" return _SECRET_SCOPE.set(secrets) def reset_secret_scope(token: Token) -> None: - """Restore the previous secret scope.""" _SECRET_SCOPE.reset(token) def current_secret_scope() -> Optional[Mapping[str, str]]: - """Return the active secret mapping, or None when no scope is installed.""" + """The active secret mapping, or None when no scope is installed.""" return _SECRET_SCOPE.get() -# ── genuinely-global env vars (NOT per-profile secrets) ────────────────── -# These are process/deployment-level settings, not profile credentials. They -# legitimately live in os.environ and must keep reading from it even in -# multiplex mode — routing them through the fail-closed path would wrongly -# crash. Anything matching is read from os.environ regardless of scope. -# -# Membership test is by exact name OR prefix (see _is_global_env). Keep this -# list tight: when in doubt a value is a profile secret, not a global. +# Genuinely-global env vars: process/deployment settings, NOT profile secrets. +# They keep reading os.environ even in multiplex mode (routing them through the +# fail-closed path would wrongly crash). Keep this tight — when in doubt a +# value is a profile secret. Membership is exact name OR prefix. _GLOBAL_ENV_EXACT = frozenset({ # Hermes runtime / deployment "HERMES_HOME", "HERMES_PROFILE", "HERMES_GATEWAY_LOCK_DIR", @@ -106,26 +84,16 @@ _GLOBAL_ENV_EXACT = frozenset({ "VIRTUAL_ENV", "PYTHONPATH", "SSL_CERT_FILE", # Kanban paths (per-board, not per-profile-secret) "HERMES_KANBAN_DB", "HERMES_KANBAN_WORKSPACES_ROOT", "HERMES_KANBAN_BOARD", - # API-server LISTENER settings — deployment config (Docker compose - # ``environment:`` block, systemd ``Environment=``), not profile secrets. - # The scoped runner reload (#64674) must keep seeing them or container - # deployments silently lose the api_server platform (#69379). NOTE: - # API_SERVER_KEY is deliberately NOT here — it IS a credential and stays - # profile-scoped. + # API-server LISTENER settings — deployment config (compose/systemd env), + # which the scoped runner reload must keep seeing or containers silently + # lose the api_server platform. API_SERVER_KEY is a credential: NOT here. "API_SERVER_ENABLED", "API_SERVER_HOST", "API_SERVER_PORT", "API_SERVER_CORS_ORIGINS", - # Relay-connector ROUTING stamps — deployment config injected into the - # container/process env by managed deploys (the same shape as the - # API_SERVER listener settings above). The scoped runner reload and the - # relay-exclusive sweep in gateway/config.py must keep seeing them, and - # every reader (gateway.config, gateway.relay.relay_url()/registration/ - # self-provision) must resolve the SAME value — a scope-dependent split - # leaves the adapter registered but the platform absent from config (or - # vice versa). Mirrors the non-secret/secret line drawn by the terminal - # env blocklist (tools/environments/local.py): routing hints are global; - # GATEWAY_RELAY_SECRET / GATEWAY_RELAY_ID / GATEWAY_RELAY_DELIVERY_KEY - # and the IDP_* credentials are auth material and deliberately NOT here — - # they stay profile-scoped with the fail-closed multiplex guard. + # Relay-connector ROUTING stamps injected by managed deploys. Every reader + # (gateway.config, relay_url()/registration/self-provision) must resolve + # the SAME value or the adapter registers while the platform is absent + # from config. GATEWAY_RELAY_SECRET/_ID/_DELIVERY_KEY and IDP_* are auth + # material and deliberately stay profile-scoped. "GATEWAY_RELAY_URL", "GATEWAY_RELAY_ENDPOINT", "GATEWAY_RELAY_ALLOW_DIRECT_PLATFORMS", "GATEWAY_RELAY_PLATFORMS", "GATEWAY_RELAY_BOT_IDS", @@ -140,38 +108,31 @@ _GLOBAL_ENV_PREFIXES = ( def _is_global_env(name: str) -> bool: - """Return True for genuinely process-global (non-profile-secret) env vars.""" - if name in _GLOBAL_ENV_EXACT: - return True - return any(name.startswith(p) for p in _GLOBAL_ENV_PREFIXES) + """True for genuinely process-global (non-profile-secret) env vars.""" + return name in _GLOBAL_ENV_EXACT or name.startswith(_GLOBAL_ENV_PREFIXES) + + +def _environ_or(name: str, default: Optional[str]) -> Optional[str]: + val = os.environ.get(name) + return val if val is not None else default def get_secret(name: str, default: Optional[str] = None) -> Optional[str]: """Resolve a credential by env-var name, honoring the active profile scope. - Resolution order: - - 1. Genuinely-global vars (``_is_global_env``) always read ``os.environ`` — - they are deployment settings, not profile secrets. - 2. When a secret scope is installed (multiplexed turn), read from it. Under - multiplexing the scope is authoritative — an absent key returns - ``default`` and we do NOT fall through to ``os.environ``, because in a - multiplexer ``os.environ`` may hold another profile's value. When - multiplexing is OFF, a scope miss falls through to ``os.environ``: - single-profile deployments legitimately provide credentials via the - process environment (systemd ``Environment=``, secret-manager wrappers - like ``pass-cli run`` / ``op run``, plain shell exports) rather than - ``/.env``, and the scope — installed unconditionally around e.g. - every cron job — must stay a ``.env`` overlay, not a blindfold. - 3. No scope installed: - - multiplex INACTIVE (default deployment): read ``os.environ`` — - identical to the legacy ``os.getenv`` behavior every caller had before. - - multiplex ACTIVE: FAIL CLOSED. Raise ``UnscopedSecretError`` so the - missing scope is caught loudly instead of leaking a cross-profile value. + 1. Global vars (``_is_global_env``) always read ``os.environ``. + 2. Scope installed: read from it. Under multiplexing the scope is + authoritative (a miss returns ``default``, never ``os.environ``, which + may hold another profile's value). With multiplexing OFF a miss falls + through to ``os.environ``: single-profile deployments legitimately + inject credentials via the process env (systemd, ``op run``, shell + exports) and the scope — installed around e.g. every cron job — must + stay a ``.env`` overlay, not a blindfold (otherwise cron 401s). + 3. No scope: multiplex INACTIVE reads ``os.environ`` (legacy behavior); + ACTIVE raises ``UnscopedSecretError`` (fail closed). """ if _is_global_env(name): - val = os.environ.get(name) - return val if val is not None else default + return _environ_or(name, default) scope = _SECRET_SCOPE.get() if scope is not None: @@ -180,14 +141,7 @@ def get_secret(name: str, default: Optional[str] = None) -> Optional[str]: return val if _MULTIPLEX_ACTIVE: return default - # Multiplex off: the scope is an overlay over the process environment, - # not an isolation boundary — there is no other profile to leak from. - # Without this fallthrough, credentials injected only into the process - # environment vanish inside any set_secret_scope(...) block (the cron - # scheduler installs one around every job), so cron jobs send a - # placeholder API key and 401 while interactive turns keep working. - val = os.environ.get(name) - return val if val is not None else default + return _environ_or(name, default) if _MULTIPLEX_ACTIVE: raise UnscopedSecretError( @@ -199,25 +153,18 @@ def get_secret(name: str, default: Optional[str] = None) -> Optional[str]: f"(Workstream A)." ) - val = os.environ.get(name) - return val if val is not None else default + return _environ_or(name, default) def _strip_inline_comment(value: str) -> str: """Strip a dotenv-style inline comment from a raw ``.env`` value. - Mirrors python-dotenv (1.2.2) semantics, verified empirically: - - - Quoted values: scan for the matching close quote - (backslash-escape-aware for double quotes, since ``save_env_value`` - writes ``\\"``/``\\\\`` escapes). Everything through the close quote is - kept; a trailing ``# ...`` remainder after it is discarded, so - ``KEY="has # inside" # trailing`` yields ``has # inside``. Non-comment - trailing junk leaves the value untouched (lenient, unlike dotenv's - hard parse error). - - Unquoted values: truncate only at a ``#`` PRECEDED BY WHITESPACE, so - ``KEY=foo#bar`` keeps ``foo#bar`` while ``KEY=value # comment`` keeps - ``value``. A value that *starts* with ``#`` (``KEY=#leading``) is kept. + Mirrors python-dotenv semantics: for quoted values scan to the matching + close quote (backslash-escape-aware for double quotes, since + ``save_env_value`` writes ``\\"``/``\\\\``) and drop a trailing ``# ...``; + other trailing junk (or an unterminated quote) leaves the value untouched. + Unquoted values truncate only at a ``#`` PRECEDED BY WHITESPACE, so + ``foo#bar`` and a leading ``#`` survive while ``value # comment`` → ``value``. """ value = value.strip() if not value: @@ -241,19 +188,13 @@ def _strip_inline_comment(value: str) -> str: def load_env_file(env_path: Path) -> Dict[str, str]: - """Parse a ``.env`` file into a plain dict WITHOUT touching ``os.environ``. + """Parse a ``.env`` file into a dict WITHOUT touching ``os.environ``. - Used to load a profile's secrets into an isolated mapping for - ``set_secret_scope``. Parses the small KEY=VALUE subset Hermes writes - itself (``export`` prefix, ``#`` comments — full-line and - dotenv-compatible inline, matching quotes with the - writer's ``\\"``/``\\\\`` escapes reversed — the same semantics as - ``hermes_cli.config._parse_env_value``) but never mutates the process - environment — that isolation is the whole point. - - Encoding is ``utf-8-sig`` so a leading UTF-8 BOM (Windows Notepad / - PowerShell ``Set-Content -Encoding UTF8``) does not prefix the first - key as ``\\ufeffNAME`` and make ``get_secret('NAME')`` miss under scope. + Handles the subset Hermes writes (``export`` prefix, full-line and inline + ``#`` comments, quotes with the writer's escapes reversed via the canonical + ``_parse_env_value`` — stripping only outer quotes would corrupt credentials + containing ``"`` or ``\\``). ``utf-8-sig`` so a Windows BOM doesn't prefix + the first key as ``\\ufeffNAME``. """ secrets: Dict[str, str] = {} try: @@ -261,12 +202,6 @@ def load_env_file(env_path: Path) -> Dict[str, str]: except (FileNotFoundError, OSError, UnicodeDecodeError): return secrets - # Parse values with the canonical Hermes parser: save_env_value - # escapes " and \ inside double quotes, and every other reader - # (load_env, python-dotenv) reverses those escapes. Stripping only - # the outer quotes here would corrupt credentials containing " - # or \ — they work interactively but fail in scoped (cron / - # multiplex) resolution. from hermes_cli.config import _parse_env_value for raw in text.splitlines(): @@ -287,12 +222,9 @@ def load_env_file(env_path: Path) -> Dict[str, str]: def build_profile_secret_scope(hermes_home: Path) -> Dict[str, str]: - """Build a profile's secret mapping from its ``/.env``. - - Returns a fresh dict (safe to install via ``set_secret_scope``). Genuinely - global vars are intentionally NOT copied in — ``get_secret`` reads those - from ``os.environ`` directly, so the scope holds only profile secrets. - """ + """Build a profile's secret mapping from ``/.env`` plus its external + secret sources. Global vars are NOT copied in — ``get_secret`` reads those + from ``os.environ`` — so the scope holds only profile secrets.""" home = Path(hermes_home) secrets = load_env_file(home / ".env") @@ -303,8 +235,7 @@ def build_profile_secret_scope(hermes_home: Path) -> Dict[str, str]: external_secrets = {} for key, value in external_secrets.items(): - if _is_global_env(key): - continue - secrets[key] = value + if not _is_global_env(key): + secrets[key] = value return secrets