diff --git a/agent/secret_sources/_cache.py b/agent/secret_sources/_cache.py new file mode 100644 index 00000000000..03bd4eb7095 --- /dev/null +++ b/agent/secret_sources/_cache.py @@ -0,0 +1,213 @@ +"""Shared substrate for external secret-source backends. + +Every backend (Bitwarden, 1Password, …) needs the same handful of +security-sensitive primitives: + + * a uniform result object (:class:`FetchResult`), + * environment-variable name validation (:func:`is_valid_env_name`), + * a two-layer fetch cache whose disk half writes atomically with ``0600`` + permissions and honours a TTL (:class:`DiskCache`, :class:`CachedFetch`). + +These used to live inline inside ``bitwarden.py``. Pulling them here means +the atomic-write / ``0600`` / TTL logic is audited and fixed in exactly one +place instead of drifting across copy-pasted per-backend modules — each +backend supplies only its own cache-key shape and a serializer for it. + +Nothing in this module ever raises out to the caller's hot path: the disk +layer is strictly best-effort (a miss just triggers a refetch), because a +cache problem must never block Hermes startup. +""" + +from __future__ import annotations + +import json +import os +import tempfile +import time +from dataclasses import dataclass +from pathlib import Path +from typing import Callable, Dict, Generic, Optional, TypeVar + +__all__ = [ + "FetchResult", + "CachedFetch", + "DiskCache", + "is_valid_env_name", + "resolve_cache_home", +] + + +# --------------------------------------------------------------------------- +# Result object + env-name validation — canonical definitions live in +# ``agent.secret_sources.base`` (the SecretSource contract module); re-exported +# here so backends that import from ``_cache`` keep working. +# --------------------------------------------------------------------------- + +from agent.secret_sources.base import ( # noqa: E402 + FetchResult, + is_valid_env_name, +) + + +# --------------------------------------------------------------------------- +# Cache entry +# --------------------------------------------------------------------------- + + +@dataclass +class CachedFetch: + """A set of fetched secret values plus when they were fetched.""" + + secrets: Dict[str, str] + fetched_at: float + + def is_fresh(self, ttl_seconds: float) -> bool: + if ttl_seconds <= 0: + return False + return (time.time() - self.fetched_at) < ttl_seconds + + + + +# --------------------------------------------------------------------------- +# Disk cache +# --------------------------------------------------------------------------- + + +def resolve_cache_home(home_path: Optional[Path] = None) -> Path: + """Resolve the Hermes home used for cache paths. + + ``home_path`` is whatever ``load_hermes_dotenv()`` already resolved; + falling back to ``$HERMES_HOME`` / ``~/.hermes`` keeps direct callers + (and tests that don't thread a home through) working. + """ + if home_path is None: + home_path = Path(os.getenv("HERMES_HOME", Path.home() / ".hermes")) + return home_path + + +K = TypeVar("K") + + +class DiskCache(Generic[K]): + """Best-effort, profile-aware on-disk cache for fetched secret values. + + One JSON object per backend lives at ``/cache/``:: + + {"key": "", "secrets": {...}, "fetched_at": 1.0} + + The file holds only secret *values* keyed by the serialized cache key — + never raw auth material. Backends are responsible for fingerprinting + tokens/sessions *before* they reach ``key_serializer`` so the token can't + land in the key. + + Writes are atomic (``mkstemp`` → ``chmod 0600`` → ``os.replace``) and the + containing ``cache/`` directory is forced to ``0700`` — ``mkdir``'s mode is + umask-subject, so the chmod is the reliable form. Both ``read`` and + ``write`` short-circuit when ``ttl_seconds <= 0``, so setting the TTL to + zero disables *both* cache layers symmetrically: a user opting out never + gets secret values written to disk at all. + """ + + def __init__(self, basename: str, *, key_serializer: Callable[[K], str]) -> None: + self._basename = basename + self._key_serializer = key_serializer + # Temp-file prefix derived from the basename so concurrent writers for + # different backends in the same dir don't collide on the staging name. + stem = basename.split(".", 1)[0] + self._tmp_prefix = f".{stem}_" + + def path(self, home_path: Optional[Path] = None) -> Path: + return resolve_cache_home(home_path) / "cache" / self._basename + + def read( + self, + key: K, + ttl_seconds: float, + home_path: Optional[Path] = None, + ) -> Optional[CachedFetch]: + """Return a fresh cached entry for ``key``, or None. + + Best-effort: any I/O or parse error, a key mismatch, or a stale entry + all return None so the caller re-fetches. + """ + if ttl_seconds <= 0: + return None + path = self.path(home_path) + try: + with open(path, "r", encoding="utf-8") as f: + payload = json.load(f) + except (OSError, json.JSONDecodeError): + return None + if not isinstance(payload, dict): + return None + if payload.get("key") != self._key_serializer(key): + return None + secrets = payload.get("secrets") + fetched_at = payload.get("fetched_at") + if not isinstance(secrets, dict) or not isinstance(fetched_at, (int, float)): + return None + # JSON permits non-string values; env vars need strings, so coerce by + # dropping anything that isn't a str→str pair. + typed: Dict[str, str] = { + k: v for k, v in secrets.items() if isinstance(k, str) and isinstance(v, str) + } + entry = CachedFetch(secrets=typed, fetched_at=float(fetched_at)) + if not entry.is_fresh(ttl_seconds): + return None + return entry + + def write( + self, + key: K, + entry: CachedFetch, + ttl_seconds: float, + home_path: Optional[Path] = None, + ) -> None: + """Persist ``entry`` for ``key`` atomically at mode ``0600``. + + No-op when ``ttl_seconds <= 0`` (so caching is genuinely off) or on any + I/O error — the next invocation just re-fetches. + """ + if ttl_seconds <= 0: + return + path = self.path(home_path) + try: + cache_dir = path.parent + cache_dir.mkdir(parents=True, exist_ok=True) + # mkdir's mode is umask-subject; chmod the dir to 0700 so cache + # metadata isn't exposed if HERMES_HOME is ever made traversable. + try: + os.chmod(cache_dir, 0o700) + except OSError: + pass + payload = { + "key": self._key_serializer(key), + "secrets": entry.secrets, + "fetched_at": entry.fetched_at, + } + # Write to a sibling temp file and atomic-rename. tempfile honours + # os.umask, so we explicitly chmod 0600 before the rename. + fd, tmp = tempfile.mkstemp( + prefix=self._tmp_prefix, suffix=".tmp", dir=str(cache_dir) + ) + try: + with os.fdopen(fd, "w", encoding="utf-8") as f: + json.dump(payload, f) + os.chmod(tmp, 0o600) + os.replace(tmp, path) + except BaseException: + try: + os.unlink(tmp) + except OSError: + pass + raise + except OSError: + pass # best-effort — a disk-cache miss next invocation is fine + + def clear(self, home_path: Optional[Path] = None) -> None: + """Delete the on-disk cache file if present (idempotent).""" + try: + self.path(home_path).unlink() + except (FileNotFoundError, OSError): + pass diff --git a/agent/secret_sources/bitwarden.py b/agent/secret_sources/bitwarden.py index 1fb570ee752..728f0ccd4e7 100644 --- a/agent/secret_sources/bitwarden.py +++ b/agent/secret_sources/bitwarden.py @@ -42,16 +42,16 @@ import time import urllib.error import urllib.request import zipfile -from dataclasses import dataclass from pathlib import Path from typing import Dict, List, Optional, Tuple -from agent.secret_sources.base import ( - ErrorKind, +from agent.secret_sources._cache import ( + CachedFetch as _CachedFetch, + DiskCache, FetchResult, - SecretSource, is_valid_env_name as _is_valid_env_name, ) +from agent.secret_sources.base import ErrorKind, SecretSource logger = logging.getLogger(__name__) @@ -77,7 +77,7 @@ _BWS_RUN_TIMEOUT = 30 # In-process cache so repeated load_hermes_dotenv() calls (CLI startup, # gateway hot-reload, test suites) don't re-fetch from BSM. _CacheKey = Tuple[str, str, str] # (access_token_fingerprint, project_id, server_url) -_CACHE: Dict[_CacheKey, "_CachedFetch"] = {} +_CACHE: Dict[_CacheKey, _CachedFetch] = {} # Disk-persisted cache so back-to-back CLI invocations (e.g. `hermes chat -q ...` # called from scripts, cron, the gateway forking new agents) don't each pay the @@ -88,120 +88,29 @@ _CACHE: Dict[_CacheKey, "_CachedFetch"] = {} # /cache/bws_cache.json. The file holds only the secret VALUES, # never the access token. It's plaintext-equivalent to ~/.hermes/.env (which # we already accept) but kept out of the .env file so users editing it won't -# accidentally commit BSM-sourced secrets. +# accidentally commit BSM-sourced secrets. The atomic-write/0600/TTL mechanics +# live in agent.secret_sources._cache.DiskCache, shared with the other backends. _DISK_CACHE_BASENAME = "bws_cache.json" -def _disk_cache_path(home_path: Optional[Path] = None) -> Path: - """Return the disk cache path under hermes_home/cache/. - - `home_path` is what `load_hermes_dotenv()` already resolved; falling back - to `$HERMES_HOME` / `~/.hermes` keeps direct callers working too. - """ - if home_path is None: - home_path = Path(os.getenv("HERMES_HOME", Path.home() / ".hermes")) - return home_path / "cache" / _DISK_CACHE_BASENAME - - def _cache_key_str(cache_key: _CacheKey) -> str: """Serialize a cache key to a stable string for JSON storage.""" token_fp, project_id, server_url = cache_key return f"{token_fp}|{project_id}|{server_url}" -def _read_disk_cache(cache_key: _CacheKey, ttl_seconds: float, - home_path: Optional[Path] = None) -> Optional["_CachedFetch"]: - """Return a cached entry from disk if fresh, else None. +_DISK_CACHE: DiskCache = DiskCache( + _DISK_CACHE_BASENAME, key_serializer=_cache_key_str +) - Best-effort: any I/O or parse error returns None and we re-fetch. + +def _disk_cache_path(home_path: Optional[Path] = None) -> Path: + """Return the disk cache path under hermes_home/cache/. + + Thin wrapper over the shared DiskCache, kept for tests and any direct + callers; falls back to `$HERMES_HOME` / `~/.hermes` when home is None. """ - if ttl_seconds <= 0: - return None - path = _disk_cache_path(home_path) - try: - with open(path, "r", encoding="utf-8") as f: - payload = json.load(f) - except (OSError, json.JSONDecodeError): - return None - if not isinstance(payload, dict): - return None - if payload.get("key") != _cache_key_str(cache_key): - return None - secrets = payload.get("secrets") - fetched_at = payload.get("fetched_at") - if not isinstance(secrets, dict) or not isinstance(fetched_at, (int, float)): - return None - # Coerce all values to strings — JSON allows numbers but env vars need strings - typed_secrets: Dict[str, str] = { - k: v for k, v in secrets.items() if isinstance(k, str) and isinstance(v, str) - } - entry = _CachedFetch(secrets=typed_secrets, fetched_at=float(fetched_at)) - if not entry.is_fresh(ttl_seconds): - return None - return entry - - -def _write_disk_cache(cache_key: _CacheKey, entry: "_CachedFetch", - home_path: Optional[Path] = None) -> None: - """Persist a cache entry to disk atomically with mode 0600. - - Best-effort: any I/O error is swallowed (the next invocation will just - re-fetch). We never want disk cache failures to break startup. - """ - path = _disk_cache_path(home_path) - try: - path.parent.mkdir(parents=True, exist_ok=True) - payload = { - "key": _cache_key_str(cache_key), - "secrets": entry.secrets, - "fetched_at": entry.fetched_at, - } - # Write to a temp file in the same directory and atomic-rename. - # tempfile honors os.umask, so we explicitly chmod 0600 before rename. - fd, tmp = tempfile.mkstemp( - prefix=".bws_cache_", suffix=".tmp", dir=str(path.parent) - ) - try: - with os.fdopen(fd, "w", encoding="utf-8") as f: - json.dump(payload, f) - os.chmod(tmp, 0o600) - os.replace(tmp, path) - except BaseException: - try: - os.unlink(tmp) - except OSError: - pass - raise - except OSError: - pass # best-effort — disk cache miss on next invocation is fine - - -@dataclass -class _CachedFetch: - secrets: Dict[str, str] - fetched_at: float - - def is_fresh(self, ttl_seconds: float) -> bool: - if ttl_seconds <= 0: - return False - return (time.time() - self.fetched_at) < ttl_seconds - - -# --------------------------------------------------------------------------- -# Public dataclasses -# --------------------------------------------------------------------------- - -# 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", -] + return _DISK_CACHE.path(home_path) # --------------------------------------------------------------------------- @@ -482,7 +391,7 @@ def fetch_bitwarden_secrets( if cached and cached.is_fresh(cache_ttl_seconds): return cached.secrets, [] # L2: disk cache. ~5ms on cache hit vs ~380ms for `bws secret list`. - disk_cached = _read_disk_cache(cache_key, cache_ttl_seconds, home_path) + disk_cached = _DISK_CACHE.read(cache_key, cache_ttl_seconds, home_path) if disk_cached is not None: # Promote into in-process cache so subsequent fetches in the # same process skip the disk read too. @@ -502,7 +411,7 @@ def fetch_bitwarden_secrets( entry = _CachedFetch(secrets=secrets, fetched_at=time.time()) _CACHE[cache_key] = entry if use_cache: - _write_disk_cache(cache_key, entry, home_path) + _DISK_CACHE.write(cache_key, entry, cache_ttl_seconds, home_path) return secrets, warnings @@ -578,15 +487,8 @@ def _run_bws_list( return secrets, warnings -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) - - # --------------------------------------------------------------------------- -# 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. +# Public entry point — called from hermes_cli.env_loader # --------------------------------------------------------------------------- @@ -683,14 +585,14 @@ def apply_bitwarden_secrets( 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 + Thin adapter over the module's 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. + sources (e.g. the 1Password ``env:`` map) outrank it. """ name = "bitwarden" @@ -824,7 +726,4 @@ def _reset_cache_for_tests(home_path: Optional[Path] = None) -> None: writer itself. """ _CACHE.clear() - try: - _disk_cache_path(home_path).unlink() - except (FileNotFoundError, OSError): - pass + _DISK_CACHE.clear(home_path)