diff --git a/agent/secret_sources/__init__.py b/agent/secret_sources/__init__.py index e1564058ad1..ec1a6e87d3b 100644 --- a/agent/secret_sources/__init__.py +++ b/agent/secret_sources/__init__.py @@ -1,13 +1,37 @@ """External secret source integrations. A secret source is anything that can supply environment-variable-shaped -credentials at process startup, _after_ ~/.hermes/.env has loaded. By -default sources are non-destructive: they only set values for env vars -that aren't already present, so .env and shell exports continue to win. +credentials at process startup, _after_ ~/.hermes/.env has loaded. -Currently shipped: +The contract every source implements is +:class:`agent.secret_sources.base.SecretSource`; the orchestrator that +runs the enabled sources (ordering, mapped-beats-bulk precedence, +first-claim-wins conflicts, ``override_existing`` semantics, provenance) +is :func:`agent.secret_sources.registry.apply_all`. Multiple sources +can be enabled at once — see the registry module docstring for the +precedence ladder. + +Currently bundled: - ``bitwarden`` — Bitwarden Secrets Manager (`bws` CLI). See ``agent.secret_sources.bitwarden`` for the integration and ``hermes_cli.secrets_cli`` for the user-facing setup wizard. + +The bundled set is deliberately closed (policy mirrors memory +providers): new third-party secret managers ship as standalone plugin +repos that subclass ``SecretSource`` and register through +``PluginContext.register_secret_source()`` — they are NOT added to this +package. Exceptions (planned): 1Password, and possibly a generic +``command`` source; OS keystores (Keychain/DPAPI/libsecret) are under +discussion. """ + +from agent.secret_sources.base import ( # noqa: F401 + SECRET_SOURCE_API_VERSION, + ErrorKind, + FetchResult, + SecretSource, + is_valid_env_name, + run_secret_cli, + scrub_ansi, +) diff --git a/agent/secret_sources/base.py b/agent/secret_sources/base.py new file mode 100644 index 00000000000..882e6b21210 --- /dev/null +++ b/agent/secret_sources/base.py @@ -0,0 +1,274 @@ +"""Secret-source contract: the ABC every secret backend implements. + +A *secret source* resolves credentials from an external secret manager +(Bitwarden Secrets Manager, 1Password, an OS keystore, a user script, ...) +into environment-variable-shaped values at process startup, AFTER +``~/.hermes/.env`` has loaded and BEFORE the rest of Hermes reads +``os.environ``. + +Scope of the contract (deliberate, please do not widen): + +* **Read-only.** Sources resolve refs → values. There is no write-back + ("save this key to your vault"), no arbitrary secret objects, and no + mid-session secret API. If a future need for rotation/refresh appears + it will arrive as a versioned optional hook — do not bolt it on. +* **Startup-time, synchronous.** ``fetch()`` is called once per process + (per HERMES_HOME) by the orchestrator in + :mod:`agent.secret_sources.registry`, which enforces a wall-clock + timeout around it. Sources must not spawn background refreshers. +* **Never raises, never prompts.** ``fetch()`` returns a + :class:`FetchResult` — errors go in ``result.error`` with a + machine-readable :class:`ErrorKind`. Interactive auth belongs in the + source's CLI ``setup`` flow, never on the startup path (non-TTY + gateway/cron startup must never block on stdin). +* **Sources fetch; the orchestrator applies.** A source returns the + name→value mapping it *would* contribute. Precedence (mapped-beats-bulk, + first-wins, ``override_existing``, protected vars), conflict warnings, + provenance tracking, and the actual ``os.environ`` writes are owned by + the orchestrator so no backend can get them wrong. + +Versioning: ``SECRET_SOURCE_API_VERSION`` gates plugin compatibility. +New *optional* hooks with default implementations do not bump it; +required-signature changes do, and the registry skips (with a warning) +sources built against a different major version instead of crashing +startup. +""" + +from __future__ import annotations + +import os +import re +import subprocess +from abc import ABC, abstractmethod +from dataclasses import dataclass, field +from enum import Enum +from pathlib import Path +from typing import Dict, FrozenSet, List, Optional, Sequence + +# Bump ONLY for breaking changes to the required contract surface +# (abstract-method signatures, FetchResult required fields). Additive +# optional hooks must ship with defaults and must NOT bump this. +SECRET_SOURCE_API_VERSION = 1 + +# Timeout the orchestrator enforces around fetch() when the source's +# config section doesn't override it. Generous because a first run may +# include a one-time CLI binary auto-install (e.g. bws download+verify). +DEFAULT_FETCH_TIMEOUT_SECONDS = 120.0 + +# Default timeout for run_secret_cli() subprocess invocations. +DEFAULT_CLI_TIMEOUT_SECONDS = 30.0 + + +class ErrorKind(str, Enum): + """Machine-readable failure taxonomy for :class:`FetchResult.error`. + + A fixed vocabulary keeps startup warnings and ``hermes secrets status`` + uniform across backends, and lets the orchestrator implement + kind-dependent policy (e.g. a future stale-cache fallback on + ``NETWORK``/``TIMEOUT`` but not on ``AUTH_FAILED``) exactly once. + """ + + NOT_CONFIGURED = "not_configured" # enabled but missing token/project/map + BINARY_MISSING = "binary_missing" # helper CLI not found / not installed + AUTH_FAILED = "auth_failed" # bad credentials + AUTH_EXPIRED = "auth_expired" # credentials were valid, aren't now + REF_INVALID = "ref_invalid" # a secret reference failed validation + NETWORK = "network" # transport-level failure + EMPTY_VALUE = "empty_value" # backend returned nothing for a ref + TIMEOUT = "timeout" # fetch exceeded its wall-clock budget + INTERNAL = "internal" # anything else (bug, unexpected shape) + + +@dataclass +class FetchResult: + """Outcome of one source's fetch. + + ``secrets`` holds what the source *would* contribute; whether each + var is actually applied is the orchestrator's decision. ``applied`` + and ``skipped`` exist for backward compatibility with the original + Bitwarden fetch-and-apply entry point and are left empty by + conforming ``fetch()`` implementations. + """ + + secrets: Dict[str, str] = field(default_factory=dict) + applied: List[str] = field(default_factory=list) + skipped: List[str] = field(default_factory=list) + warnings: List[str] = field(default_factory=list) + error: Optional[str] = None + error_kind: Optional[ErrorKind] = None + # Path of the helper binary used, when the source is CLI-driven. + # Surfaced by status commands; None for SDK/API-driven sources. + binary_path: Optional[Path] = None + + @property + def ok(self) -> bool: + return self.error is None + + +class SecretSource(ABC): + """One external secret backend. + + Subclasses set the class attributes and implement :meth:`fetch`. + Everything else has a sensible default. + + Attributes: + name: Config-section key under ``secrets:`` in config.yaml. + Lowercase ``[a-z0-9_]+``. Also the provenance label stored + for every var this source supplies. + label: Human-readable name used in startup messages and + ``hermes secrets status`` (e.g. ``"Bitwarden Secrets Manager"``). + shape: ``"mapped"`` when the user explicitly binds env-var names + to refs (1Password ``env:`` map, command source) or + ``"bulk"`` when the backend injects whole projects/folders + of secrets implicitly (Bitwarden BSM). The orchestrator + gives mapped sources precedence over bulk sources: an + explicit binding is stronger intent than a project dump. + scheme: Optional URI scheme this source owns for secret + references (``"op"`` for ``op://...``). Must be unique + across registered sources — refs may eventually appear + outside the ``secrets:`` block (e.g. credential-pool + ``api_key`` fields), so scheme collisions are rejected at + registration time to keep that future possible. + api_version: Contract version this source was built against. + """ + + api_version: int = SECRET_SOURCE_API_VERSION + name: str = "" + label: str = "" + shape: str = "mapped" # "mapped" | "bulk" + scheme: Optional[str] = None + + # -- required ---------------------------------------------------------- + + @abstractmethod + def fetch(self, cfg: dict, home_path: Path) -> FetchResult: + """Resolve this source's secrets. MUST NOT raise or prompt. + + ``cfg`` is the source's raw config section (``secrets.``) + from config.yaml — treat every field defensively, the section + may be malformed. ``home_path`` is the resolved HERMES_HOME. + """ + + # -- optional hooks (defaults are correct for most sources) ------------ + + def is_enabled(self, cfg: dict) -> bool: + """Whether the user turned this source on.""" + return bool(isinstance(cfg, dict) and cfg.get("enabled")) + + def override_existing(self, cfg: dict) -> bool: + """May this source overwrite vars that .env / the shell already set? + + This NEVER extends to vars claimed by another secret source in the + same startup pass — cross-source overrides are a config error the + orchestrator warns about, not a knob. + """ + return bool(isinstance(cfg, dict) and cfg.get("override_existing", False)) + + def protected_env_vars(self, cfg: dict) -> FrozenSet[str]: + """Env vars the orchestrator must never let ANY source overwrite. + + Typically the source's own bootstrap-auth var (e.g. + ``BWS_ACCESS_TOKEN``) so a vault that contains its own access + token can't clobber the credential used to reach it. + """ + return frozenset() + + def fetch_timeout_seconds(self, cfg: dict) -> float: + """Wall-clock budget the orchestrator enforces around fetch().""" + try: + val = float((cfg or {}).get("timeout_seconds", DEFAULT_FETCH_TIMEOUT_SECONDS)) + except (TypeError, ValueError): + return DEFAULT_FETCH_TIMEOUT_SECONDS + return val if val > 0 else DEFAULT_FETCH_TIMEOUT_SECONDS + + def config_schema(self) -> dict: + """Optional description of this source's config keys. + + Shape: ``{key: {"description": str, "default": Any}}``. Used by + setup surfaces to render config without hardcoding per-source + knowledge. Purely informational. + """ + return {} + + +# --------------------------------------------------------------------------- +# Shared helpers — use these instead of hand-rolling per backend +# --------------------------------------------------------------------------- + + +_ENV_NAME_RE = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$") + +# ANSI CSI/OSC escape sequences — helper-CLI stderr often carries color +# codes that must not reach Hermes' own startup output. +_ANSI_RE = re.compile(r"\x1b(?:\[[0-9;?]*[ -/]*[@-~]|\][^\x07\x1b]*(?:\x07|\x1b\\)?)") + + +def is_valid_env_name(name: str) -> bool: + """True when ``name`` is a legal environment-variable name.""" + return bool(name) and bool(_ENV_NAME_RE.match(name)) + + +def scrub_ansi(text: str) -> str: + """Strip ANSI escape sequences (whole CSI/OSC sequences, not just ESC).""" + return _ANSI_RE.sub("", text or "") + + +def run_secret_cli( + argv: Sequence[str], + *, + allow_env: Sequence[str] = (), + extra_env: Optional[Dict[str, str]] = None, + timeout: float = DEFAULT_CLI_TIMEOUT_SECONDS, +) -> subprocess.CompletedProcess: + """Run a secret-manager helper CLI with a minimal, allowlisted env. + + Security posture shared by every subprocess-driven backend: + + * argv list only — never ``shell=True``. Callers pass user-supplied + reference strings AFTER a ``--`` option terminator in their argv. + * The child gets ``PATH``/``HOME``/locale basics plus only the env + vars named in ``allow_env`` (auth/session vars) and ``extra_env`` + — never a copy of the full post-dotenv ``os.environ``, which by + this point holds every credential Hermes knows about. + * ``NO_COLOR=1`` is set and stderr/stdout are ANSI-scrubbed so + helper diagnostics can't smuggle escape sequences into Hermes + output. + * stdin is ``/dev/null`` so a helper that decides to prompt fails + fast instead of hanging startup. + + Raises ``RuntimeError`` on spawn failure or timeout (message safe to + surface); returns the completed process otherwise — callers own + returncode interpretation. + """ + base_keep = ("PATH", "HOME", "USERPROFILE", "SYSTEMROOT", "TMPDIR", "TEMP", + "LANG", "LC_ALL", "XDG_CONFIG_HOME", "XDG_DATA_HOME") + env: Dict[str, str] = {} + for key in (*base_keep, *allow_env): + val = os.environ.get(key) + if val is not None: + env[key] = val + if extra_env: + env.update(extra_env) + env.setdefault("NO_COLOR", "1") + + try: + proc = subprocess.run( # noqa: S603 — argv list, no shell + list(argv), + env=env, + capture_output=True, + text=True, + timeout=timeout, + stdin=subprocess.DEVNULL, + ) + except subprocess.TimeoutExpired as exc: + raise RuntimeError( + f"{Path(str(argv[0])).name} timed out after {timeout:.0f}s" + ) from exc + except OSError as exc: + raise RuntimeError( + f"failed to invoke {Path(str(argv[0])).name}: {exc}" + ) from exc + + proc.stdout = proc.stdout or "" + proc.stderr = scrub_ansi(proc.stderr or "") + return proc diff --git a/agent/secret_sources/bitwarden.py b/agent/secret_sources/bitwarden.py index e025a0ca9b4..1fb570ee752 100644 --- a/agent/secret_sources/bitwarden.py +++ b/agent/secret_sources/bitwarden.py @@ -42,10 +42,17 @@ import time import urllib.error import urllib.request import zipfile -from dataclasses import dataclass, field +from dataclasses import dataclass from pathlib import Path from typing import Dict, List, Optional, Tuple +from agent.secret_sources.base import ( + ErrorKind, + FetchResult, + SecretSource, + is_valid_env_name as _is_valid_env_name, +) + logger = logging.getLogger(__name__) @@ -184,21 +191,17 @@ class _CachedFetch: # Public dataclasses # --------------------------------------------------------------------------- - -@dataclass -class FetchResult: - """Outcome of a single BSM pull.""" - - secrets: Dict[str, str] = field(default_factory=dict) - applied: List[str] = field(default_factory=list) # set into os.environ - skipped: List[str] = field(default_factory=list) # already set, not overridden - warnings: List[str] = field(default_factory=list) # non-fatal issues - error: Optional[str] = None # fatal: nothing was fetched - binary_path: Optional[Path] = None - - @property - def ok(self) -> bool: - return self.error is None +# FetchResult now lives in ``agent.secret_sources.base`` (shared by every +# secret source) and is re-exported here for backward compatibility — +# existing callers/tests import it from this module. +__all__ = [ + "FetchResult", + "BitwardenSource", + "apply_bitwarden_secrets", + "fetch_bitwarden_secrets", + "find_bws", + "install_bws", +] # --------------------------------------------------------------------------- @@ -575,16 +578,15 @@ def _run_bws_list( return secrets, warnings -def _is_valid_env_name(name: str) -> bool: - if not name: - return False - if not (name[0].isalpha() or name[0] == "_"): - return False - return all(c.isalnum() or c == "_" for c in name) +def _is_valid_env_name_local(name: str) -> bool: # pragma: no cover — shim + """Deprecated local alias; use ``agent.secret_sources.base.is_valid_env_name``.""" + return _is_valid_env_name(name) # --------------------------------------------------------------------------- -# Public entry point — called from hermes_cli.env_loader +# Legacy entry point — superseded by BitwardenSource + registry.apply_all(). +# Kept because external scripts/tests call it directly; the env_loader +# startup path no longer does. # --------------------------------------------------------------------------- @@ -673,6 +675,142 @@ def apply_bitwarden_secrets( return result +# --------------------------------------------------------------------------- +# SecretSource adapter — the registry-facing wrapper around this module. +# --------------------------------------------------------------------------- + + +class BitwardenSource(SecretSource): + """Bitwarden Secrets Manager as a registered secret source. + + Thin adapter over the module's existing fetch machinery. ``fetch()`` + only *fetches* — precedence, override semantics, conflict warnings, + and the ``os.environ`` writes are the orchestrator's job + (see ``agent.secret_sources.registry.apply_all``). + + Bitwarden is a **bulk** source: it injects every secret in the + configured BSM project, so explicit per-var bindings from mapped + sources (e.g. a 1Password ``env:`` map) outrank it. + """ + + name = "bitwarden" + label = "Bitwarden Secrets Manager" + shape = "bulk" + scheme = "bws" + + def override_existing(self, cfg: dict) -> bool: + # Default True (matches DEFAULT_CONFIG): the point of BSM is + # centralized rotation — if .env had the final say, rotating a + # key in Bitwarden wouldn't take effect until the stale .env + # line was also deleted. + return bool(isinstance(cfg, dict) and cfg.get("override_existing", True)) + + def protected_env_vars(self, cfg: dict): + token_env = "BWS_ACCESS_TOKEN" + if isinstance(cfg, dict): + token_env = str(cfg.get("access_token_env") or token_env) + return frozenset({token_env}) + + def config_schema(self) -> dict: + return { + "enabled": {"description": "Master switch", "default": False}, + "access_token_env": { + "description": "Env var holding the machine-account access token", + "default": "BWS_ACCESS_TOKEN", + }, + "project_id": {"description": "BSM project UUID", "default": ""}, + "cache_ttl_seconds": { + "description": "Disk+memory cache TTL; 0 disables", + "default": 300, + }, + "override_existing": { + "description": "BSM values overwrite .env/shell values", + "default": True, + }, + "auto_install": { + "description": "Auto-download the pinned bws binary", + "default": True, + }, + "server_url": { + "description": "Region / self-hosted endpoint (empty = US Cloud)", + "default": "", + }, + } + + def fetch(self, cfg: dict, home_path: Path) -> FetchResult: + cfg = cfg if isinstance(cfg, dict) else {} + result = FetchResult() + + access_token_env = str(cfg.get("access_token_env") or "BWS_ACCESS_TOKEN") + access_token = os.environ.get(access_token_env, "").strip() + if not access_token: + result.error = ( + f"secrets.bitwarden.enabled is true but {access_token_env} is " + "not set. Run `hermes secrets bitwarden setup`." + ) + result.error_kind = ErrorKind.NOT_CONFIGURED + return result + + project_id = str(cfg.get("project_id") or "") + if not project_id: + result.error = ( + "secrets.bitwarden.project_id is empty. " + "Run `hermes secrets bitwarden setup`." + ) + result.error_kind = ErrorKind.NOT_CONFIGURED + return result + + auto_install = bool(cfg.get("auto_install", True)) + binary = find_bws(install_if_missing=auto_install) + result.binary_path = binary + if binary is None: + result.error = ( + "bws binary not available and auto-install is disabled. " + "Run `hermes secrets bitwarden setup` to install." + ) + result.error_kind = ErrorKind.BINARY_MISSING + return result + + try: + ttl = float(cfg.get("cache_ttl_seconds", 300)) + except (TypeError, ValueError): + ttl = 300.0 + + try: + secrets, warnings = fetch_bitwarden_secrets( + access_token=access_token, + project_id=project_id, + binary=binary, + cache_ttl_seconds=ttl, + server_url=str(cfg.get("server_url", "") or "").strip(), + home_path=home_path, + ) + except RuntimeError as exc: + result.error = str(exc) + result.error_kind = _classify_bws_error(str(exc)) + return result + + result.secrets = secrets + result.warnings.extend(warnings) + return result + + +def _classify_bws_error(message: str) -> ErrorKind: + """Best-effort mapping of bws failure text onto the shared taxonomy.""" + lowered = message.lower() + if "timed out" in lowered: + return ErrorKind.TIMEOUT + if "binary not available" in lowered or "failed to invoke" in lowered: + return ErrorKind.BINARY_MISSING + if any(tok in lowered for tok in ("unauthorized", "invalid token", + "access token", "401", "403")): + return ErrorKind.AUTH_FAILED + if any(tok in lowered for tok in ("network", "connection", "resolve", + "download", "dns")): + return ErrorKind.NETWORK + return ErrorKind.INTERNAL + + # --------------------------------------------------------------------------- # Test hook — used by hermetic tests to flush the cache between cases. # --------------------------------------------------------------------------- diff --git a/agent/secret_sources/registry.py b/agent/secret_sources/registry.py new file mode 100644 index 00000000000..993ad4bcda2 --- /dev/null +++ b/agent/secret_sources/registry.py @@ -0,0 +1,363 @@ +"""Secret-source registry + apply orchestrator. + +This module owns everything that must be uniform across secret backends +so no individual source can get it wrong: + +* registration (name/scheme uniqueness, API-version gating) +* per-source wall-clock timeout enforcement around ``fetch()`` +* precedence: mapped sources beat bulk sources; within a shape, + ``secrets.sources`` order (or registration order) decides; first + claim wins — later sources never silently clobber an earlier one +* ``override_existing`` semantics (may beat .env/shell, never another + secret source, never a protected var) +* cross-source conflict warnings (shadowed claims are always surfaced) +* provenance: which source supplied every applied var + +The single entry point for startup is :func:`apply_all`, called from +``hermes_cli.env_loader._apply_external_secret_sources()``. + +Plugins register additional sources via +``PluginContext.register_secret_source()`` which lands in +:func:`register_source`. In-tree sources are registered lazily by +:func:`_ensure_builtin_sources` — the set of bundled sources is +deliberately closed (Bitwarden, and 1Password once it lands); new +third-party backends ship as standalone plugin repos implementing +:class:`agent.secret_sources.base.SecretSource`. +""" + +from __future__ import annotations + +import concurrent.futures +import logging +from dataclasses import dataclass, field +from pathlib import Path +from typing import Dict, List, Optional + +from agent.secret_sources.base import ( + SECRET_SOURCE_API_VERSION, + ErrorKind, + FetchResult, + SecretSource, + is_valid_env_name, +) + +logger = logging.getLogger(__name__) + +# Ordered registry: name → source instance. Python dicts preserve +# insertion order, which doubles as the default apply order. +_SOURCES: Dict[str, SecretSource] = {} +_BUILTINS_LOADED = False + + +@dataclass +class AppliedVar: + """Provenance record for one env var the orchestrator set.""" + + name: str + source: str # SecretSource.name + shape: str # "mapped" | "bulk" + overrode_env: bool # replaced a pre-existing .env/shell value + + +@dataclass +class SourceReport: + """One source's outcome within an :class:`ApplyReport`.""" + + name: str + label: str + result: FetchResult + applied: List[str] = field(default_factory=list) + skipped_existing: List[str] = field(default_factory=list) # .env/shell won + skipped_claimed: List[str] = field(default_factory=list) # earlier source won + skipped_protected: List[str] = field(default_factory=list) # bootstrap-auth guard + skipped_invalid: List[str] = field(default_factory=list) # bad env-var name + + +@dataclass +class ApplyReport: + """Merged outcome of one orchestrated apply pass.""" + + sources: List[SourceReport] = field(default_factory=list) + provenance: Dict[str, AppliedVar] = field(default_factory=dict) + conflicts: List[str] = field(default_factory=list) # human-readable warnings + + @property + def applied_any(self) -> bool: + return bool(self.provenance) + + +# --------------------------------------------------------------------------- +# Registration +# --------------------------------------------------------------------------- + + +def register_source(source: SecretSource, *, replace: bool = False) -> bool: + """Register a secret source. Returns True on success. + + Rejections are logged, never raised — a bad plugin must not take + down startup. ``replace`` allows tests / user plugins to override + a bundled source of the same name (last-writer-wins like model + providers), but scheme collisions across *different* names are + always rejected. + """ + if not isinstance(source, SecretSource): + logger.warning( + "Ignoring secret source %r: does not inherit from SecretSource", + source, + ) + return False + name = getattr(source, "name", "") or "" + if not name or not name.replace("_", "").isalnum() or name != name.lower(): + logger.warning("Ignoring secret source with invalid name %r", name) + return False + if getattr(source, "api_version", None) != SECRET_SOURCE_API_VERSION: + logger.warning( + "Ignoring secret source '%s': built against secret-source API v%s, " + "this Hermes speaks v%s", + name, getattr(source, "api_version", "?"), SECRET_SOURCE_API_VERSION, + ) + return False + if getattr(source, "shape", None) not in ("mapped", "bulk"): + logger.warning( + "Ignoring secret source '%s': shape must be 'mapped' or 'bulk', got %r", + name, getattr(source, "shape", None), + ) + return False + if name in _SOURCES and not replace: + logger.warning("Secret source '%s' already registered; ignoring duplicate", name) + return False + scheme = getattr(source, "scheme", None) + if scheme: + for other_name, other in _SOURCES.items(): + if other_name != name and getattr(other, "scheme", None) == scheme: + logger.warning( + "Ignoring secret source '%s': scheme '%s://' is already " + "owned by source '%s'", + name, scheme, other_name, + ) + return False + _SOURCES[name] = source + return True + + +def get_source(name: str) -> Optional[SecretSource]: + _ensure_builtin_sources() + return _SOURCES.get(name) + + +def list_sources() -> List[SecretSource]: + _ensure_builtin_sources() + return list(_SOURCES.values()) + + +def _ensure_builtin_sources() -> None: + """Idempotently register the bundled sources. + + Lazy so importing this module stays cheap and so a broken bundled + source can never break registration of the others. + """ + global _BUILTINS_LOADED + if _BUILTINS_LOADED: + return + _BUILTINS_LOADED = True + try: + from agent.secret_sources.bitwarden import BitwardenSource + + register_source(BitwardenSource()) + except Exception: # noqa: BLE001 — never block startup + logger.warning("Failed to register bundled Bitwarden secret source", + exc_info=True) + + +def _reset_registry_for_tests() -> None: + global _BUILTINS_LOADED + _SOURCES.clear() + _BUILTINS_LOADED = False + + +# --------------------------------------------------------------------------- +# Orchestrated apply +# --------------------------------------------------------------------------- + + +def _fetch_with_timeout( + source: SecretSource, cfg: dict, home_path: Path +) -> FetchResult: + """Run source.fetch() under a wall-clock budget; never raises. + + The budget is enforced with a daemon worker thread: a source that + blows its budget is reported as ``TIMEOUT`` and its (eventual) + result is discarded. The thread itself may linger until process + exit — acceptable for a startup-only path, and strictly better than + an unbounded hang on every ``hermes`` invocation. + """ + timeout = source.fetch_timeout_seconds(cfg) + executor = concurrent.futures.ThreadPoolExecutor( + max_workers=1, thread_name_prefix=f"secret-src-{source.name}" + ) + try: + future = executor.submit(source.fetch, cfg, home_path) + try: + result = future.result(timeout=timeout) + except concurrent.futures.TimeoutError: + future.cancel() + res = FetchResult() + res.error = ( + f"fetch exceeded {timeout:.0f}s budget — startup continued " + "without this source (raise secrets." + f"{source.name}.timeout_seconds if the backend is just slow)" + ) + res.error_kind = ErrorKind.TIMEOUT + return res + except Exception as exc: # noqa: BLE001 — contract violation, contain it + res = FetchResult() + res.error = f"fetch raised {type(exc).__name__}: {exc}" + res.error_kind = ErrorKind.INTERNAL + return res + finally: + executor.shutdown(wait=False) + + if not isinstance(result, FetchResult): + res = FetchResult() + res.error = ( + f"fetch returned {type(result).__name__} instead of FetchResult" + ) + res.error_kind = ErrorKind.INTERNAL + return res + return result + + +def _ordered_enabled_sources(secrets_cfg: dict) -> List[SecretSource]: + """Resolve which sources run, in which order. + + Order: the optional ``secrets.sources`` list wins; sources not named + there follow in registration order. Enabled = the source's own + ``is_enabled`` says so for its config section. Mapped-vs-bulk + precedence is applied on top of this order by :func:`apply_all`. + """ + _ensure_builtin_sources() + + explicit = secrets_cfg.get("sources") + order: List[str] = [] + if isinstance(explicit, list): + for entry in explicit: + if isinstance(entry, str) and entry in _SOURCES and entry not in order: + order.append(entry) + unknown = [e for e in explicit + if isinstance(e, str) and e not in _SOURCES] + if unknown: + logger.warning( + "secrets.sources names unknown source(s): %s (known: %s)", + ", ".join(unknown), ", ".join(_SOURCES) or "none", + ) + for name in _SOURCES: + if name not in order: + order.append(name) + + enabled: List[SecretSource] = [] + for name in order: + source = _SOURCES[name] + cfg = secrets_cfg.get(name) + cfg = cfg if isinstance(cfg, dict) else {} + try: + if source.is_enabled(cfg): + enabled.append(source) + except Exception: # noqa: BLE001 + logger.warning("Secret source '%s' is_enabled() raised; skipping", + name, exc_info=True) + return enabled + + +def apply_all(secrets_cfg: dict, home_path: Path, + environ: Optional[Dict[str, str]] = None) -> ApplyReport: + """Fetch from every enabled source and apply the merged result to env. + + ``environ`` defaults to ``os.environ``; injectable for tests. + + Precedence per env var (most-specific intent wins): + + 1. Pre-existing env (.env / shell) — unless the winning source has + ``override_existing: true``. + 2. Mapped sources, in configured order. + 3. Bulk sources, in configured order. + + First claim wins. A later source that also carries the var gets a + ``skipped_claimed`` entry and a conflict warning — never a silent + clobber, and ``override_existing`` never applies across sources. + """ + import os as _os + + env = environ if environ is not None else _os.environ + report = ApplyReport() + + secrets_cfg = secrets_cfg if isinstance(secrets_cfg, dict) else {} + enabled = _ordered_enabled_sources(secrets_cfg) + if not enabled: + return report + + # Mapped sources outrank bulk sources regardless of list order: + # an explicit VAR→ref binding is stronger intent than a project dump. + ordered = ([s for s in enabled if s.shape == "mapped"] + + [s for s in enabled if s.shape == "bulk"]) + + # Fetch phase. + fetches: List[tuple[SecretSource, dict, FetchResult]] = [] + protected: Dict[str, str] = {} # var → source that protects it + for source in ordered: + cfg = secrets_cfg.get(source.name) + cfg = cfg if isinstance(cfg, dict) else {} + result = _fetch_with_timeout(source, cfg, home_path) + fetches.append((source, cfg, result)) + try: + for var in source.protected_env_vars(cfg): + protected.setdefault(var, source.name) + except Exception: # noqa: BLE001 + pass + + # Apply phase — sequential, first-wins, fully attributed. + claimed: Dict[str, str] = {} # var → source name that won it + for source, cfg, result in fetches: + sr = SourceReport(name=source.name, + label=source.label or source.name, + result=result) + report.sources.append(sr) + if not result.ok: + continue + + try: + override = source.override_existing(cfg) + except Exception: # noqa: BLE001 + override = False + + for var, value in result.secrets.items(): + if not isinstance(var, str) or not isinstance(value, str): + continue + if not is_valid_env_name(var): + sr.skipped_invalid.append(var) + continue + if var in protected: + sr.skipped_protected.append(var) + continue + if var in claimed: + sr.skipped_claimed.append(var) + report.conflicts.append( + f"{var}: kept value from {claimed[var]}; " + f"{source.name} also supplies it (first source wins — " + "remove one binding or reorder secrets.sources)" + ) + continue + existed = bool(env.get(var)) + if existed and not override: + sr.skipped_existing.append(var) + continue + env[var] = value + claimed[var] = source.name + sr.applied.append(var) + report.provenance[var] = AppliedVar( + name=var, + source=source.name, + shape=source.shape, + overrode_env=existed, + ) + + return report diff --git a/cli-config.yaml.example b/cli-config.yaml.example index 8b0769ead3a..6b33a1820d7 100644 --- a/cli-config.yaml.example +++ b/cli-config.yaml.example @@ -1380,3 +1380,30 @@ updates: # # This is a CREDENTIAL: prefer setting HERMES_DASHBOARD_OIDC_CLIENT_SECRET # # in ~/.hermes/.env over putting it here in config.yaml. # # client_secret: "" + +# ============================================================================= +# External secret sources +# ============================================================================= +# Pull provider credentials from external secret managers at process startup +# instead of storing them in ~/.hermes/.env. Only the manager's bootstrap +# token (e.g. BWS_ACCESS_TOKEN) lives in .env; everything else rotates +# centrally in the vault. Multiple sources can be enabled at once: +# - "mapped" sources (explicit VAR -> ref bindings) beat "bulk" sources +# (whole-project dumps like Bitwarden BSM) +# - within a shape, the first source to claim a var wins; later claims +# are skipped with a startup warning (never a silent clobber) +# - a source's override_existing lets it beat .env/shell values, but +# never another secret source's claim +# Docs: https://hermes-agent.nousresearch.com/docs/user-guide/secrets/ +# +# secrets: +# # Optional explicit ordering of enabled sources. +# # sources: [bitwarden] +# bitwarden: +# enabled: false +# project_id: "" # BSM project UUID +# access_token_env: BWS_ACCESS_TOKEN +# cache_ttl_seconds: 300 # 0 disables memory+disk caching +# override_existing: true # BSM wins over .env so rotation works +# auto_install: true # auto-download the pinned bws binary +# server_url: "" # e.g. https://vault.bitwarden.eu (EU cloud) diff --git a/hermes_cli/config.py b/hermes_cli/config.py index 050f1975db9..5597679de4a 100644 --- a/hermes_cli/config.py +++ b/hermes_cli/config.py @@ -3073,6 +3073,15 @@ DEFAULT_CONFIG = { # Pull credentials from external secret managers at process startup # rather than storing them in ~/.hermes/.env. "secrets": { + # Optional explicit ordering of enabled secret sources. When + # omitted, sources run in registration order (bundled first, + # then plugin-registered). Regardless of this list, "mapped" + # sources (explicit VAR→ref bindings, e.g. a future 1Password + # env: map) always take precedence over "bulk" sources + # (project dumps like Bitwarden BSM), and the first source to + # claim a var wins — later claims are skipped with a warning. + # Example: sources: [onepassword, bitwarden] + # "sources": [], "bitwarden": { # Master switch. When false, BSM is never contacted and the # bws binary is never auto-installed — same as not having diff --git a/hermes_cli/env_loader.py b/hermes_cli/env_loader.py index 39ff02657c6..4352e4bdf9e 100644 --- a/hermes_cli/env_loader.py +++ b/hermes_cli/env_loader.py @@ -78,9 +78,17 @@ def format_secret_source_suffix(env_var: str) -> str: return "" if source == "bitwarden": return " (from Bitwarden)" - # Generic fallback — future-proofing for additional secret sources - # (e.g. 1Password, HashiCorp Vault) without having to update every - # call site. + # Ask the registry for the source's human label (e.g. "1Password"). + # Fall back to the raw source name for labels the registry doesn't + # know (stale provenance from an uninstalled plugin, tests). + try: + from agent.secret_sources.registry import get_source + + registered = get_source(source) + if registered is not None and registered.label: + return f" (from {registered.label})" + except Exception: # noqa: BLE001 — label lookup must never raise + pass return f" (from {source})" @@ -281,21 +289,27 @@ def _apply_managed_env() -> None: def _apply_external_secret_sources(home_path: Path) -> None: - """Pull secrets from external sources (currently Bitwarden) into env. + """Pull secrets from every enabled external source into env. - Runs AFTER dotenv loads so .env values are visible (we use them to - locate the access token) but BEFORE the rest of Hermes reads + Runs AFTER dotenv loads so .env values are visible (sources use them + to locate bootstrap tokens) but BEFORE the rest of Hermes reads ``os.environ`` for credentials. Any failure here is logged and swallowed — external secret sources must never block startup. + The heavy lifting (source ordering, mapped-beats-bulk precedence, + first-claim-wins conflict handling, override semantics, provenance) + lives in ``agent.secret_sources.registry.apply_all``; this wrapper + owns the once-per-HERMES_HOME guard, the post-apply ASCII + sanitization sweep, the ``_SECRET_SOURCES`` provenance map that + UI surfaces read, and the startup status lines. + Idempotent within a process: subsequent calls for the same ``home_path`` are no-ops. ``load_hermes_dotenv()`` runs at import time from several hot modules (cli.py, hermes_cli/main.py, run_agent.py, trajectory_compressor.py, ...), so without this guard - the Bitwarden status line would print 3-5x per CLI startup. Use + the status lines would print 3-5x per CLI startup. Use ``reset_secret_source_cache()`` if you need to force a re-pull - (tests, future ``hermes secrets bitwarden sync`` from a long-running - process). + (tests, long-running processes after a config change). """ home_key = str(Path(home_path).resolve()) if home_key in _APPLIED_HOMES: @@ -306,54 +320,45 @@ def _apply_external_secret_sources(home_path: Path) -> None: cfg = _load_secrets_config(home_path) except Exception: # noqa: BLE001 — config errors must not block startup return - - bw_cfg = (cfg or {}).get("bitwarden") or {} - if not bw_cfg.get("enabled"): + if not cfg: return try: - from agent.secret_sources.bitwarden import apply_bitwarden_secrets + from agent.secret_sources.registry import apply_all except ImportError: return - result = apply_bitwarden_secrets( - enabled=True, - access_token_env=bw_cfg.get("access_token_env", "BWS_ACCESS_TOKEN"), - project_id=bw_cfg.get("project_id", ""), - override_existing=bool(bw_cfg.get("override_existing", False)), - cache_ttl_seconds=float(bw_cfg.get("cache_ttl_seconds", 300)), - auto_install=bool(bw_cfg.get("auto_install", True)), - server_url=str(bw_cfg.get("server_url", "") or "").strip(), - home_path=home_path, - ) + try: + report = apply_all(cfg, home_path) + except Exception: # noqa: BLE001 — belt-and-braces; apply_all shouldn't raise + return - if result.applied: - # Re-run the ASCII sanitization pass: BSM values are user-supplied - # and might have the same copy-paste corruption as a manually - # edited .env (see #6843). + if report.applied_any: + # Re-run the ASCII sanitization pass: vault values are + # user-supplied and might have the same copy-paste corruption as + # a manually edited .env (see #6843). _sanitize_loaded_credentials() - # Remember where these came from so the setup / `hermes model` - # flows can label detected credentials with "(from Bitwarden)" — - # otherwise users see "credentials ✓" with no hint that the value - # came from BSM rather than .env. - for name in result.applied: - _SECRET_SOURCES[name] = "bitwarden" - print( - f" Bitwarden Secrets Manager: applied {len(result.applied)} " - f"secret{'s' if len(result.applied) != 1 else ''} " - f"({', '.join(sorted(result.applied))})", - file=sys.stderr, - ) - if result.error: - print( - f" Bitwarden Secrets Manager: {result.error}", - file=sys.stderr, - ) - for warn in result.warnings: - print( - f" Bitwarden Secrets Manager: {warn}", - file=sys.stderr, - ) + # Remember where each var came from so setup / `hermes model` + # flows can label detected credentials with "(from Bitwarden)" / + # "(from 1Password)" — otherwise users see "credentials ✓" with + # no hint the value came from a vault rather than .env. + for name, applied in report.provenance.items(): + _SECRET_SOURCES[name] = applied.source + + for src in report.sources: + if src.applied: + print( + f" {src.label}: applied {len(src.applied)} " + f"secret{'s' if len(src.applied) != 1 else ''} " + f"({', '.join(sorted(src.applied))})", + file=sys.stderr, + ) + if src.result.error: + print(f" {src.label}: {src.result.error}", file=sys.stderr) + for warn in src.result.warnings: + print(f" {src.label}: {warn}", file=sys.stderr) + for conflict in report.conflicts: + print(f" Secret sources: {conflict}", file=sys.stderr) def _load_secrets_config(home_path: Path) -> dict: diff --git a/hermes_cli/plugins.py b/hermes_cli/plugins.py index d5e4b3ff8c1..b95e8e3eeff 100644 --- a/hermes_cli/plugins.py +++ b/hermes_cli/plugins.py @@ -795,6 +795,53 @@ class PluginContext: self.manifest.name, provider.name, ) + # -- secret source registration ------------------------------------------- + + def register_secret_source(self, source) -> None: + """Register an external secret-manager backend. + + ``source`` must be an instance of + :class:`agent.secret_sources.base.SecretSource`. Registered + sources run during ``load_hermes_dotenv()`` startup — after + ``~/.hermes/.env`` loads, before Hermes reads credentials — when + their ``secrets.`` config section is enabled. The + orchestrator (``agent.secret_sources.registry.apply_all``) owns + ordering, mapped-vs-bulk precedence, conflict warnings, and + provenance; the source only fetches. + + NOTE ON TIMING: plugin discovery happens later in startup than + the first ``load_hermes_dotenv()`` call, so a plugin-registered + source is not consulted by the initial env load of the process + that discovers it. It IS consulted by every subsequently + spawned Hermes process (gateway children, cron sessions, + subagents), and immediately after a + ``reset_secret_source_cache()`` re-pull. Plugin sources are + therefore best for supplying credentials to the running fleet; + the bundled sources cover first-process bootstrap. + + Contract requirements (rejected with a warning otherwise): + inherit from ``SecretSource``, ``api_version`` matching + ``SECRET_SOURCE_API_VERSION``, lowercase unique ``name``, + ``shape`` of ``"mapped"`` or ``"bulk"``, unique ``scheme`` (when + set), and a ``fetch()`` that never raises and never prompts. + See the base-module docstring for the full contract. + """ + from agent.secret_sources.base import SecretSource + from agent.secret_sources.registry import register_source + + if not isinstance(source, SecretSource): + logger.warning( + "Plugin '%s' tried to register a secret source that does " + "not inherit from SecretSource. Ignoring.", + self.manifest.name, + ) + return + if register_source(source): + logger.info( + "Plugin '%s' registered secret source: %s", + self.manifest.name, source.name, + ) + # -- TTS provider registration ------------------------------------------- def register_tts_provider(self, provider) -> None: diff --git a/tests/secret_sources/__init__.py b/tests/secret_sources/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/secret_sources/conformance.py b/tests/secret_sources/conformance.py new file mode 100644 index 00000000000..87a3f005614 --- /dev/null +++ b/tests/secret_sources/conformance.py @@ -0,0 +1,123 @@ +"""Conformance kit for :class:`agent.secret_sources.base.SecretSource`. + +Any secret-source backend — bundled or external plugin — can validate +itself against the contract by subclassing :class:`SecretSourceConformance` +and providing a ``source`` fixture (plus optional per-source config +fixtures). Example:: + + from tests.secret_sources.conformance import SecretSourceConformance + + class TestMySourceConformance(SecretSourceConformance): + @pytest.fixture + def source(self): + return MySource() + +The checks encode the parts of the contract that break OTHER people +when violated: never raising, never prompting (stdin closed), respecting +disabled config, valid identity attributes, and orchestrator +compatibility. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from agent.secret_sources.base import ( + SECRET_SOURCE_API_VERSION, + FetchResult, + SecretSource, +) +from agent.secret_sources.registry import ( + _reset_registry_for_tests, + apply_all, + register_source, +) + + +class SecretSourceConformance: + """Base class of contract checks; subclass and provide ``source``.""" + + @pytest.fixture + def source(self) -> SecretSource: # pragma: no cover — must override + raise NotImplementedError("conformance subclasses must provide a source fixture") + + @pytest.fixture + def minimal_cfg(self) -> dict: + """An enabled-but-unconfigured section — the common misconfig case.""" + return {"enabled": True} + + # -- identity ---------------------------------------------------------- + + def test_name_is_lowercase_identifier(self, source): + assert source.name, "source.name must be non-empty" + assert source.name == source.name.lower() + assert source.name.replace("_", "").isalnum() + + def test_label_present(self, source): + assert source.label, "source.label must be a human-readable name" + + def test_shape_valid(self, source): + assert source.shape in ("mapped", "bulk") + + def test_api_version_current(self, source): + assert source.api_version == SECRET_SOURCE_API_VERSION + + # -- contract behavior -------------------------------------------------- + + def test_fetch_never_raises_on_malformed_config(self, source, tmp_path): + """Every degenerate config shape must produce a FetchResult, not a raise.""" + for cfg in ({}, {"enabled": True}, {"enabled": True, "env": "not-a-dict"}, + {"enabled": True, "cache_ttl_seconds": "bogus"}, None): + result = source.fetch(cfg if isinstance(cfg, dict) else {}, tmp_path) + assert isinstance(result, FetchResult), ( + f"fetch() returned {type(result).__name__} for cfg={cfg!r}" + ) + + def test_fetch_unconfigured_reports_error_not_secrets(self, source, tmp_path, + minimal_cfg, monkeypatch): + """enabled=true with nothing else set must fail cleanly with a kind.""" + result = source.fetch(minimal_cfg, tmp_path) + assert isinstance(result, FetchResult) + if not result.ok: + assert result.error_kind is not None, ( + "errors must carry a machine-readable ErrorKind" + ) + assert not result.secrets + + def test_disabled_by_default(self, source): + assert source.is_enabled({}) is False + assert source.is_enabled({"enabled": False}) is False + + def test_timeout_is_positive(self, source, minimal_cfg): + assert source.fetch_timeout_seconds(minimal_cfg) > 0 + # Garbage config must not break the timeout accessor either. + assert source.fetch_timeout_seconds({"timeout_seconds": "junk"}) > 0 + + def test_protected_vars_are_valid_names(self, source, minimal_cfg): + from agent.secret_sources.base import is_valid_env_name + + for var in source.protected_env_vars(minimal_cfg): + assert is_valid_env_name(var) + + # -- orchestrator compatibility ------------------------------------------ + + def test_registers_and_applies_via_orchestrator(self, source, tmp_path, + monkeypatch): + """The source must survive a full apply_all() pass without breaking it.""" + _reset_registry_for_tests() + # Prevent the bundled sources from interfering. + monkeypatch.setattr( + "agent.secret_sources.registry._ensure_builtin_sources", lambda: None + ) + try: + assert register_source(source), "register_source() rejected the source" + env: dict = {} + report = apply_all( + {source.name: {"enabled": True}}, tmp_path, environ=env + ) + names = [sr.name for sr in report.sources] + assert source.name in names + finally: + _reset_registry_for_tests() diff --git a/tests/secret_sources/test_secret_source_registry.py b/tests/secret_sources/test_secret_source_registry.py new file mode 100644 index 00000000000..80a67464a0b --- /dev/null +++ b/tests/secret_sources/test_secret_source_registry.py @@ -0,0 +1,468 @@ +"""Tests for the secret-source contract + orchestrator. + +Covers: registration gating (API version, name/scheme uniqueness, shape), +apply_all precedence (mapped beats bulk, first-wins, override_existing, +protected vars), conflict surfacing, timeout enforcement, provenance, +and Bitwarden's SecretSource adapter — plus the conformance kit run +against the bundled Bitwarden source. +""" + +from __future__ import annotations + +import sys +import time +from pathlib import Path + +import pytest + +ROOT = Path(__file__).resolve().parents[2] +if str(ROOT) not in sys.path: + sys.path.insert(0, str(ROOT)) + +from agent.secret_sources.base import ( # noqa: E402 + SECRET_SOURCE_API_VERSION, + ErrorKind, + FetchResult, + SecretSource, + is_valid_env_name, + run_secret_cli, + scrub_ansi, +) +from agent.secret_sources import registry as reg # noqa: E402 +from agent.secret_sources.bitwarden import BitwardenSource # noqa: E402 +from tests.secret_sources.conformance import SecretSourceConformance # noqa: E402 + + +@pytest.fixture(autouse=True) +def _clean_registry(monkeypatch): + """Each test starts with an empty registry and no builtin auto-load.""" + reg._reset_registry_for_tests() + monkeypatch.setattr(reg, "_ensure_builtin_sources", lambda: None) + yield + reg._reset_registry_for_tests() + + +def _make_source( + name="dummy", + shape="mapped", + secrets=None, + error=None, + error_kind=None, + scheme=None, + override=False, + protected=(), + api_version=SECRET_SOURCE_API_VERSION, + fetch_fn=None, +): + """Build a minimal conforming source for orchestrator tests.""" + + class _Src(SecretSource): + def fetch(self, cfg, home_path): + if fetch_fn is not None: + return fetch_fn(cfg, home_path) + res = FetchResult() + if error: + res.error = error + res.error_kind = error_kind or ErrorKind.INTERNAL + else: + res.secrets = dict(secrets or {}) + return res + + def override_existing(self, cfg): + return override + + def protected_env_vars(self, cfg): + return frozenset(protected) + + _Src.name = name + _Src.label = name.title() + _Src.shape = shape + _Src.scheme = scheme + _Src.api_version = api_version + return _Src() + + +# --------------------------------------------------------------------------- +# Registration gating +# --------------------------------------------------------------------------- + + +class TestRegistration: + def test_registers_conforming_source(self): + assert reg.register_source(_make_source()) is True + assert reg.get_source("dummy") is not None + + def test_rejects_non_secretsource_instance(self): + assert reg.register_source(object()) is False + + def test_rejects_wrong_api_version(self): + src = _make_source(api_version=SECRET_SOURCE_API_VERSION + 1) + assert reg.register_source(src) is False + + def test_rejects_invalid_name(self): + assert reg.register_source(_make_source(name="Bad Name")) is False + assert reg.register_source(_make_source(name="")) is False + assert reg.register_source(_make_source(name="UPPER")) is False + + def test_rejects_invalid_shape(self): + assert reg.register_source(_make_source(shape="sideways")) is False + + def test_rejects_duplicate_name_without_replace(self): + assert reg.register_source(_make_source(name="dup")) is True + assert reg.register_source(_make_source(name="dup")) is False + assert reg.register_source(_make_source(name="dup"), replace=True) is True + + def test_rejects_scheme_collision_across_names(self): + assert reg.register_source(_make_source(name="one", scheme="op")) is True + assert reg.register_source(_make_source(name="two", scheme="op")) is False + + def test_same_name_replace_keeps_scheme(self): + assert reg.register_source(_make_source(name="one", scheme="op")) is True + assert reg.register_source( + _make_source(name="one", scheme="op"), replace=True + ) is True + + +# --------------------------------------------------------------------------- +# apply_all: precedence, conflicts, protection +# --------------------------------------------------------------------------- + + +class TestApplyAll: + def test_disabled_sources_do_not_run(self, tmp_path): + called = [] + + def _fetch(cfg, home): + called.append(True) + return FetchResult(secrets={"A": "1"}) + + reg.register_source(_make_source(fetch_fn=_fetch)) + env: dict = {} + report = reg.apply_all({"dummy": {"enabled": False}}, tmp_path, environ=env) + assert not called + assert not report.sources + assert env == {} + + def test_applies_secrets_and_records_provenance(self, tmp_path): + reg.register_source(_make_source(secrets={"API_KEY": "v1"})) + env: dict = {} + report = reg.apply_all({"dummy": {"enabled": True}}, tmp_path, environ=env) + assert env["API_KEY"] == "v1" + assert report.provenance["API_KEY"].source == "dummy" + assert report.provenance["API_KEY"].shape == "mapped" + assert report.provenance["API_KEY"].overrode_env is False + + def test_existing_env_wins_without_override(self, tmp_path): + reg.register_source(_make_source(secrets={"API_KEY": "vault"})) + env = {"API_KEY": "dotenv"} + report = reg.apply_all({"dummy": {"enabled": True}}, tmp_path, environ=env) + assert env["API_KEY"] == "dotenv" + assert "API_KEY" in report.sources[0].skipped_existing + + def test_override_existing_beats_env_and_is_attributed(self, tmp_path): + reg.register_source(_make_source(secrets={"API_KEY": "vault"}, override=True)) + env = {"API_KEY": "dotenv"} + report = reg.apply_all({"dummy": {"enabled": True}}, tmp_path, environ=env) + assert env["API_KEY"] == "vault" + assert report.provenance["API_KEY"].overrode_env is True + + def test_mapped_beats_bulk_regardless_of_order(self, tmp_path): + reg.register_source( + _make_source(name="bulky", shape="bulk", secrets={"K": "bulk"}) + ) + reg.register_source( + _make_source(name="mappy", shape="mapped", secrets={"K": "mapped"}) + ) + env: dict = {} + # bulk listed first in sources order — mapped must still win. + report = reg.apply_all( + {"sources": ["bulky", "mappy"], + "bulky": {"enabled": True}, "mappy": {"enabled": True}}, + tmp_path, environ=env, + ) + assert env["K"] == "mapped" + assert report.provenance["K"].source == "mappy" + assert report.conflicts, "shadowed bulk claim must surface a warning" + + def test_first_source_wins_within_shape(self, tmp_path): + reg.register_source(_make_source(name="alpha", secrets={"K": "a"})) + reg.register_source(_make_source(name="beta", secrets={"K": "b"})) + env: dict = {} + report = reg.apply_all( + {"sources": ["beta", "alpha"], + "alpha": {"enabled": True}, "beta": {"enabled": True}}, + tmp_path, environ=env, + ) + assert env["K"] == "b" # beta listed first + assert report.provenance["K"].source == "beta" + beta_first = [s for s in report.sources if s.name == "alpha"][0] + assert "K" in beta_first.skipped_claimed + + def test_cross_source_override_never_clobbers_prior_claim(self, tmp_path): + """override_existing beats .env, NEVER another source's claim.""" + reg.register_source(_make_source(name="alpha", secrets={"K": "a"})) + reg.register_source( + _make_source(name="beta", secrets={"K": "b"}, override=True) + ) + env: dict = {} + report = reg.apply_all( + {"sources": ["alpha", "beta"], + "alpha": {"enabled": True}, "beta": {"enabled": True}}, + tmp_path, environ=env, + ) + assert env["K"] == "a" + assert report.conflicts + + def test_protected_vars_never_overwritten_by_any_source(self, tmp_path): + reg.register_source( + _make_source(name="alpha", secrets={"BOOT_TOKEN": "evil"}, + override=True, protected=("BOOT_TOKEN",)) + ) + env = {"BOOT_TOKEN": "real"} + report = reg.apply_all({"alpha": {"enabled": True}}, tmp_path, environ=env) + assert env["BOOT_TOKEN"] == "real" + assert "BOOT_TOKEN" in report.sources[0].skipped_protected + + def test_invalid_env_names_skipped(self, tmp_path): + reg.register_source( + _make_source(secrets={"GOOD_NAME": "v", "bad-name": "v", "1BAD": "v"}) + ) + env: dict = {} + report = reg.apply_all({"dummy": {"enabled": True}}, tmp_path, environ=env) + assert "GOOD_NAME" in env and "bad-name" not in env and "1BAD" not in env + assert set(report.sources[0].skipped_invalid) == {"bad-name", "1BAD"} + + def test_failed_source_does_not_block_others(self, tmp_path): + reg.register_source( + _make_source(name="broken", error="boom", error_kind=ErrorKind.NETWORK) + ) + reg.register_source(_make_source(name="works", secrets={"K": "v"})) + env: dict = {} + report = reg.apply_all( + {"broken": {"enabled": True}, "works": {"enabled": True}}, + tmp_path, environ=env, + ) + assert env["K"] == "v" + broken = [s for s in report.sources if s.name == "broken"][0] + assert broken.result.error_kind is ErrorKind.NETWORK + + def test_raising_fetch_contained_as_internal_error(self, tmp_path): + def _explode(cfg, home): + raise ValueError("plugin bug") + + reg.register_source(_make_source(name="buggy", fetch_fn=_explode)) + env: dict = {} + report = reg.apply_all({"buggy": {"enabled": True}}, tmp_path, environ=env) + assert report.sources[0].result.error_kind is ErrorKind.INTERNAL + assert "plugin bug" in report.sources[0].result.error + + def test_wrong_return_type_contained(self, tmp_path): + reg.register_source( + _make_source(name="liar", fetch_fn=lambda cfg, home: {"not": "a result"}) + ) + report = reg.apply_all({"liar": {"enabled": True}}, tmp_path, environ={}) + assert report.sources[0].result.error_kind is ErrorKind.INTERNAL + + def test_timeout_enforced(self, tmp_path): + def _slow(cfg, home): + time.sleep(5) + return FetchResult(secrets={"K": "late"}) + + src = _make_source(name="slow", fetch_fn=_slow) + src.fetch_timeout_seconds = lambda cfg: 0.2 + reg.register_source(src) + env: dict = {} + start = time.monotonic() + report = reg.apply_all({"slow": {"enabled": True}}, tmp_path, environ=env) + assert time.monotonic() - start < 3 + assert report.sources[0].result.error_kind is ErrorKind.TIMEOUT + assert "K" not in env + + def test_malformed_secrets_cfg_shapes_are_safe(self, tmp_path): + reg.register_source(_make_source(secrets={"K": "v"})) + for cfg in (None, [], "junk", {"dummy": "not-a-dict"}, {"sources": "junk"}): + report = reg.apply_all(cfg, tmp_path, environ={}) + assert isinstance(report, reg.ApplyReport) + + def test_unknown_sources_entry_warns_but_continues(self, tmp_path, caplog): + reg.register_source(_make_source(secrets={"K": "v"})) + env: dict = {} + reg.apply_all( + {"sources": ["ghost", "dummy"], "dummy": {"enabled": True}}, + tmp_path, environ=env, + ) + assert env["K"] == "v" + + +# --------------------------------------------------------------------------- +# Shared helpers +# --------------------------------------------------------------------------- + + +class TestHelpers: + def test_is_valid_env_name(self): + assert is_valid_env_name("GOOD_NAME") + assert is_valid_env_name("_LEADING") + assert not is_valid_env_name("") + assert not is_valid_env_name("1BAD") + assert not is_valid_env_name("bad-name") + assert not is_valid_env_name("has space") + + def test_scrub_ansi_removes_whole_sequences(self): + assert scrub_ansi("\x1b[31mred\x1b[0m plain") == "red plain" + assert scrub_ansi("\x1b]0;title\x07text") == "text" + assert scrub_ansi("") == "" + + def test_run_secret_cli_minimal_env(self): + proc = run_secret_cli( + [sys.executable, "-c", + "import os, json; print(json.dumps(sorted(os.environ)))"], + ) + import json + + child_env = json.loads(proc.stdout) + # No credential-bearing vars from the parent env leak through. + assert not any(k.endswith(("_API_KEY", "_TOKEN", "_SECRET")) + for k in child_env) + assert "NO_COLOR" in child_env + + def test_run_secret_cli_allowlist_passes_named_vars(self, monkeypatch): + monkeypatch.setenv("MY_AUTH_TOKEN", "tok") + monkeypatch.setenv("OTHER_API_KEY", "leak") + proc = run_secret_cli( + [sys.executable, "-c", + "import os; print(os.environ.get('MY_AUTH_TOKEN', '')); " + "print(os.environ.get('OTHER_API_KEY', ''))"], + allow_env=["MY_AUTH_TOKEN"], + ) + lines = proc.stdout.splitlines() + assert lines[0] == "tok" + assert lines[1] == "" + + def test_run_secret_cli_timeout_raises_runtime_error(self): + with pytest.raises(RuntimeError, match="timed out"): + run_secret_cli( + [sys.executable, "-c", "import time; time.sleep(10)"], + timeout=0.3, + ) + + def test_run_secret_cli_stdin_devnull(self): + # A helper that tries to prompt reads EOF immediately. + proc = run_secret_cli( + [sys.executable, "-c", + "import sys; print(repr(sys.stdin.read()))"], + ) + assert proc.stdout.strip() == "''" + + +# --------------------------------------------------------------------------- +# Bitwarden adapter +# --------------------------------------------------------------------------- + + +class TestBitwardenSource: + def test_identity(self): + src = BitwardenSource() + assert src.name == "bitwarden" + assert src.shape == "bulk" + assert src.scheme == "bws" + + def test_override_existing_defaults_true(self): + src = BitwardenSource() + assert src.override_existing({}) is True + assert src.override_existing({"override_existing": False}) is False + + def test_protected_vars_track_token_env(self): + src = BitwardenSource() + assert src.protected_env_vars({}) == frozenset({"BWS_ACCESS_TOKEN"}) + assert src.protected_env_vars( + {"access_token_env": "CUSTOM_TOKEN"} + ) == frozenset({"CUSTOM_TOKEN"}) + + def test_fetch_missing_token_not_configured(self, tmp_path, monkeypatch): + monkeypatch.delenv("BWS_ACCESS_TOKEN", raising=False) + result = BitwardenSource().fetch({"enabled": True}, tmp_path) + assert result.error_kind is ErrorKind.NOT_CONFIGURED + assert "BWS_ACCESS_TOKEN" in result.error + + def test_fetch_missing_project_not_configured(self, tmp_path, monkeypatch): + monkeypatch.setenv("BWS_ACCESS_TOKEN", "0.token") + result = BitwardenSource().fetch({"enabled": True}, tmp_path) + assert result.error_kind is ErrorKind.NOT_CONFIGURED + assert "project_id" in result.error + + def test_fetch_delegates_to_fetch_bitwarden_secrets(self, tmp_path, monkeypatch): + monkeypatch.setenv("BWS_ACCESS_TOKEN", "0.token") + import agent.secret_sources.bitwarden as bw + + monkeypatch.setattr(bw, "find_bws", lambda **kw: Path("/fake/bws")) + captured = {} + + def _fake_fetch(**kwargs): + captured.update(kwargs) + return {"MY_KEY": "val"}, ["a warning"] + + monkeypatch.setattr(bw, "fetch_bitwarden_secrets", _fake_fetch) + result = BitwardenSource().fetch( + {"enabled": True, "project_id": "proj", + "server_url": " https://vault.bitwarden.eu "}, + tmp_path, + ) + assert result.ok + assert result.secrets == {"MY_KEY": "val"} + assert result.warnings == ["a warning"] + assert captured["project_id"] == "proj" + assert captured["server_url"] == "https://vault.bitwarden.eu" + assert captured["home_path"] == tmp_path + + def test_fetch_runtime_error_classified(self, tmp_path, monkeypatch): + monkeypatch.setenv("BWS_ACCESS_TOKEN", "0.token") + import agent.secret_sources.bitwarden as bw + + monkeypatch.setattr(bw, "find_bws", lambda **kw: Path("/fake/bws")) + + def _fail(**kwargs): + raise RuntimeError("bws exited 1: 401 unauthorized") + + monkeypatch.setattr(bw, "fetch_bitwarden_secrets", _fail) + result = BitwardenSource().fetch( + {"enabled": True, "project_id": "proj"}, tmp_path + ) + assert result.error_kind is ErrorKind.AUTH_FAILED + + def test_e2e_through_orchestrator(self, tmp_path, monkeypatch): + """Full path: registry → BitwardenSource → env, with fetch mocked.""" + monkeypatch.setenv("BWS_ACCESS_TOKEN", "0.token") + import agent.secret_sources.bitwarden as bw + + monkeypatch.setattr(bw, "find_bws", lambda **kw: Path("/fake/bws")) + monkeypatch.setattr( + bw, "fetch_bitwarden_secrets", + lambda **kw: ({"ANTHROPIC_API_KEY": "sk-ant", "BWS_ACCESS_TOKEN": "steal"}, []), + ) + reg.register_source(BitwardenSource()) + env = {"BWS_ACCESS_TOKEN": "0.token"} + report = reg.apply_all( + {"bitwarden": {"enabled": True, "project_id": "proj"}}, + tmp_path, environ=env, + ) + assert env["ANTHROPIC_API_KEY"] == "sk-ant" + # The bootstrap token is protected even though BSM carried it. + assert env["BWS_ACCESS_TOKEN"] == "0.token" + assert report.provenance["ANTHROPIC_API_KEY"].source == "bitwarden" + + +# --------------------------------------------------------------------------- +# Conformance kit applied to the bundled source +# --------------------------------------------------------------------------- + + +class TestBitwardenConformance(SecretSourceConformance): + @pytest.fixture + def source(self, monkeypatch): + # Never hit the network / auto-install path in conformance runs. + import agent.secret_sources.bitwarden as bw + + monkeypatch.setattr(bw, "find_bws", lambda **kw: None) + monkeypatch.delenv("BWS_ACCESS_TOKEN", raising=False) + return BitwardenSource() diff --git a/tests/test_bitwarden_secrets.py b/tests/test_bitwarden_secrets.py index ac5057c18b8..fed43b3becb 100644 --- a/tests/test_bitwarden_secrets.py +++ b/tests/test_bitwarden_secrets.py @@ -639,20 +639,23 @@ def test_env_loader_calls_bsm_when_enabled(tmp_path, monkeypatch): monkeypatch.delenv("MY_BSM_KEY", raising=False) called = {"n": 0} - def fake_apply(**kwargs): + + def fake_fetch(**kwargs): called["n"] += 1 - assert kwargs["enabled"] is True assert kwargs["project_id"] == "proj-1" - os.environ["MY_BSM_KEY"] = "from-bsm" - return bw.FetchResult( - secrets={"MY_BSM_KEY": "from-bsm"}, - applied=["MY_BSM_KEY"], - ) + return {"MY_BSM_KEY": "from-bsm"}, [] monkeypatch.setattr( - "agent.secret_sources.bitwarden.apply_bitwarden_secrets", - fake_apply, + "agent.secret_sources.bitwarden.find_bws", + lambda **_kw: Path("/fake/bws"), ) + monkeypatch.setattr( + "agent.secret_sources.bitwarden.fetch_bitwarden_secrets", + fake_fetch, + ) + from agent.secret_sources import registry as reg_module + + reg_module._reset_registry_for_tests() from hermes_cli.env_loader import _apply_external_secret_sources _apply_external_secret_sources(home) diff --git a/tests/test_env_loader_secret_sources.py b/tests/test_env_loader_secret_sources.py index 91c9d4c6e4f..6c5c0d8c780 100644 --- a/tests/test_env_loader_secret_sources.py +++ b/tests/test_env_loader_secret_sources.py @@ -64,10 +64,12 @@ def test_format_secret_source_suffix_generic_label_for_future_sources(): def test_apply_external_secret_sources_records_bitwarden_origin(tmp_path, monkeypatch): - """End-to-end: when ``apply_bitwarden_secrets`` returns applied keys, - they end up in ``_SECRET_SOURCES`` so the UI can label them.""" + """End-to-end: when the Bitwarden source fetches keys, applied vars + end up in ``_SECRET_SOURCES`` so the UI can label them.""" monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + monkeypatch.setenv("BWS_ACCESS_TOKEN", "0.test-token") + monkeypatch.delenv("ANTHROPIC_API_KEY", raising=False) config_path = tmp_path / "config.yaml" config_path.write_text( "secrets:\n" @@ -78,22 +80,19 @@ def test_apply_external_secret_sources_records_bitwarden_origin(tmp_path, monkey encoding="utf-8", ) - # Stub apply_bitwarden_secrets to return a synthetic FetchResult. - from agent.secret_sources.bitwarden import FetchResult - - fake_result = FetchResult( - secrets={"ANTHROPIC_API_KEY": "sk-ant-test"}, - applied=["ANTHROPIC_API_KEY"], - ) - - def _fake_apply(**_kwargs): - return fake_result - - # The import inside _apply_external_secret_sources is lazy, so we - # patch the *module attribute* it will pull in. + # Stub the fetch layer under the SecretSource adapter. import agent.secret_sources.bitwarden as bw_module - monkeypatch.setattr(bw_module, "apply_bitwarden_secrets", _fake_apply) + monkeypatch.setattr(bw_module, "find_bws", lambda **_kw: Path("/fake/bws")) + monkeypatch.setattr( + bw_module, + "fetch_bitwarden_secrets", + lambda **_kw: ({"ANTHROPIC_API_KEY": "sk-ant-test"}, []), + ) + + from agent.secret_sources import registry as reg_module + + reg_module._reset_registry_for_tests() env_loader._apply_external_secret_sources(tmp_path) @@ -131,6 +130,8 @@ def test_apply_external_secret_sources_dedupes_within_process(tmp_path, monkeypa """ monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + monkeypatch.setenv("BWS_ACCESS_TOKEN", "0.test-token") + monkeypatch.delenv("ANTHROPIC_API_KEY", raising=False) config_path = tmp_path / "config.yaml" config_path.write_text( "secrets:\n" @@ -141,19 +142,19 @@ def test_apply_external_secret_sources_dedupes_within_process(tmp_path, monkeypa encoding="utf-8", ) - from agent.secret_sources.bitwarden import FetchResult - call_count = {"n": 0} - def _fake_apply(**_kwargs): + def _fake_fetch(**_kwargs): call_count["n"] += 1 - return FetchResult( - secrets={"ANTHROPIC_API_KEY": "sk-ant-test"}, - applied=["ANTHROPIC_API_KEY"], - ) + return {"ANTHROPIC_API_KEY": "sk-ant-test"}, [] import agent.secret_sources.bitwarden as bw_module - monkeypatch.setattr(bw_module, "apply_bitwarden_secrets", _fake_apply) + monkeypatch.setattr(bw_module, "find_bws", lambda **_kw: Path("/fake/bws")) + monkeypatch.setattr(bw_module, "fetch_bitwarden_secrets", _fake_fetch) + + from agent.secret_sources import registry as reg_module + + reg_module._reset_registry_for_tests() # Five calls in a row, simulating module-import-time invocations from # cli.py, hermes_cli/main.py, run_agent.py, trajectory_compressor.py, diff --git a/website/docs/user-guide/secrets/index.md b/website/docs/user-guide/secrets/index.md index bf8d85cfed6..d2770ddb8b5 100644 --- a/website/docs/user-guide/secrets/index.md +++ b/website/docs/user-guide/secrets/index.md @@ -6,4 +6,28 @@ Supported: - [Bitwarden Secrets Manager](./bitwarden) — `bws` CLI, lazy-installed, free tier works. -More backends (Vault, AWS Secrets Manager, 1Password CLI) are easy to add behind the same interface — the lift is one module in `agent/secret_sources/` and one CLI handler. File a request if you have a specific one in mind. +## Multiple sources at once + +You can enable more than one secret source at the same time — for example a team Bitwarden project alongside a personal vault plugin. Sources compose per env var with a deterministic precedence ladder: + +1. **Your `.env` / shell wins by default.** A source only replaces a pre-existing value when its own `override_existing: true` is set (Bitwarden defaults to true so central rotation works). +2. **Mapped sources beat bulk sources.** A source where you explicitly bind env vars to references (an `env:` map) outranks a source that injects a whole project of secrets implicitly, regardless of ordering. +3. **First source wins.** Within the same shape, the order of the optional `secrets.sources` list (or registration order) decides. Later claims on an already-claimed var are skipped — with a startup warning, never silently. + +`override_existing` never lets one source overwrite a var another source already claimed, and no source can ever overwrite another source's bootstrap token (e.g. `BWS_ACCESS_TOKEN`). + +```yaml +secrets: + sources: [bitwarden] # optional explicit ordering + bitwarden: + enabled: true + project_id: "..." +``` + +Every credential injected by a source is labelled with its origin — setup flows and `hermes model` show `(from Bitwarden)` next to detected keys so you always know where a value came from. + +## Adding your own backend + +Third-party secret managers ship as standalone plugins, not core PRs. A backend subclasses `agent.secret_sources.base.SecretSource` (one required method: `fetch(cfg, home_path) -> FetchResult`) and registers via `ctx.register_secret_source(MySource())` in the plugin's `register(ctx)`. The orchestrator owns precedence, conflict handling, timeouts, and provenance — your source only fetches. Contract rules: `fetch()` never raises, never prompts, and returns within its timeout budget; validate your implementation against the conformance kit in `tests/secret_sources/conformance.py`. + +The bundled set is deliberately closed (same policy as memory providers). Planned in-tree additions: 1Password. Everything else — Infisical, Proton Pass, HashiCorp Vault, AWS Secrets Manager, OS keystores — belongs in plugin repos; share them in the Nous Research Discord (`#plugins-skills-and-skins`).