mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-08 13:12:08 +00:00
refactor(secrets): extract shared cache/result substrate for secret sources
Pull the disk-cache + FetchResult substrate out of bitwarden.py into a new agent/secret_sources/_cache.py: FetchResult, CachedFetch, is_valid_env_name, and a generic DiskCache (atomic mkstemp -> chmod 0600 -> os.replace write, 0700 cache dir, TTL-gated read AND write). Bitwarden now consumes it via a module-level DiskCache instance and thin wrappers, so the security-sensitive atomic-write/0600/TTL logic lives in exactly one place instead of being copy-pasted per backend (and drifting). Behavior is unchanged — the full Bitwarden suite passes untouched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Taylor H. Perkins
Teknium
parent
2d16ec7fb7
commit
db495b0fba
2 changed files with 238 additions and 126 deletions
213
agent/secret_sources/_cache.py
Normal file
213
agent/secret_sources/_cache.py
Normal file
|
|
@ -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 ``<hermes_home>/cache/<basename>``::
|
||||
|
||||
{"key": "<serialized 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
|
||||
|
|
@ -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"] = {}
|
|||
# <hermes_home>/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)
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue