fix: track secret source registration origin
This commit is contained in:
@@ -47,15 +47,12 @@ from agent.secret_sources.base import (
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
# Ordered registry: name → source instance. Python dicts preserve
|
||||
# insertion order, which doubles as the default apply order.
|
||||
# insertion order, which doubles as the default apply order. Origin is
|
||||
# recorded beside each source so consumers never infer ownership from names.
|
||||
_SOURCES: Dict[str, SecretSource] = {}
|
||||
_SOURCE_ORIGINS: Dict[str, str] = {}
|
||||
_BUILTINS_LOADED = False
|
||||
|
||||
# Canonical names of the deliberately-closed bundled source set. Callers
|
||||
# (e.g. the plugin-discovery re-pull) use this to distinguish plugin sources
|
||||
# from in-tree ones without hard-coding a list at each call site.
|
||||
BUILTIN_SOURCE_NAMES: frozenset = frozenset({"bitwarden", "onepassword"})
|
||||
|
||||
|
||||
@dataclass
|
||||
class AppliedVar:
|
||||
@@ -99,7 +96,12 @@ class ApplyReport:
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def register_source(source: SecretSource, *, replace: bool = False) -> bool:
|
||||
def register_source(
|
||||
source: SecretSource,
|
||||
*,
|
||||
replace: bool = False,
|
||||
builtin: bool = False,
|
||||
) -> bool:
|
||||
"""Register a secret source. Returns True on success.
|
||||
|
||||
Rejections are logged, never raised — a bad plugin must not take
|
||||
@@ -145,6 +147,7 @@ def register_source(source: SecretSource, *, replace: bool = False) -> bool:
|
||||
)
|
||||
return False
|
||||
_SOURCES[name] = source
|
||||
_SOURCE_ORIGINS[name] = "builtin" if builtin else "plugin"
|
||||
return True
|
||||
|
||||
|
||||
@@ -158,6 +161,16 @@ def list_sources() -> List[SecretSource]:
|
||||
return list(_SOURCES.values())
|
||||
|
||||
|
||||
def list_plugin_sources() -> List[SecretSource]:
|
||||
"""Return sources registered outside the bundled bootstrap set."""
|
||||
_ensure_builtin_sources()
|
||||
return [
|
||||
source
|
||||
for name, source in _SOURCES.items()
|
||||
if _SOURCE_ORIGINS.get(name) == "plugin"
|
||||
]
|
||||
|
||||
|
||||
def _ensure_builtin_sources() -> None:
|
||||
"""Idempotently register the bundled sources.
|
||||
|
||||
@@ -171,21 +184,21 @@ def _ensure_builtin_sources() -> None:
|
||||
try:
|
||||
from agent.secret_sources.bitwarden import BitwardenSource
|
||||
|
||||
register_source(BitwardenSource())
|
||||
register_source(BitwardenSource(), builtin=True)
|
||||
except Exception: # noqa: BLE001 — never block startup
|
||||
logger.warning("Failed to register bundled Bitwarden secret source",
|
||||
exc_info=True)
|
||||
try:
|
||||
from agent.secret_sources.onepassword import OnePasswordSource
|
||||
|
||||
register_source(OnePasswordSource())
|
||||
register_source(OnePasswordSource(), builtin=True)
|
||||
except Exception: # noqa: BLE001 — never block startup
|
||||
logger.warning("Failed to register bundled 1Password secret source",
|
||||
exc_info=True)
|
||||
try:
|
||||
from agent.secret_sources.command import CommandSource
|
||||
|
||||
register_source(CommandSource())
|
||||
register_source(CommandSource(), builtin=True)
|
||||
except Exception: # noqa: BLE001 — never block startup
|
||||
logger.warning("Failed to register bundled command secret source",
|
||||
exc_info=True)
|
||||
@@ -194,6 +207,7 @@ def _ensure_builtin_sources() -> None:
|
||||
def _reset_registry_for_tests() -> None:
|
||||
global _BUILTINS_LOADED
|
||||
_SOURCES.clear()
|
||||
_SOURCE_ORIGINS.clear()
|
||||
_BUILTINS_LOADED = False
|
||||
|
||||
|
||||
|
||||
@@ -1390,20 +1390,14 @@ class PluginManager:
|
||||
Fail-open: never raise into discover_and_load.
|
||||
"""
|
||||
try:
|
||||
from agent.secret_sources.registry import (
|
||||
BUILTIN_SOURCE_NAMES,
|
||||
list_sources,
|
||||
)
|
||||
from agent.secret_sources.registry import list_plugin_sources
|
||||
from hermes_cli.env_loader import load_hermes_dotenv, reset_secret_source_cache
|
||||
except Exception:
|
||||
return
|
||||
try:
|
||||
sources = list_sources()
|
||||
plugin_sources = list_plugin_sources()
|
||||
except Exception:
|
||||
return
|
||||
plugin_sources = [
|
||||
s for s in sources if getattr(s, "name", "") not in BUILTIN_SOURCE_NAMES
|
||||
]
|
||||
if not plugin_sources:
|
||||
return
|
||||
# Load the secrets config once; hand each source its own section and
|
||||
|
||||
@@ -1,10 +1,9 @@
|
||||
"""Tests for plugin secret-source first-process re-pull (#64177)."""
|
||||
from __future__ import annotations
|
||||
|
||||
import os
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
|
||||
from agent.secret_sources.base import (
|
||||
SECRET_SOURCE_API_VERSION,
|
||||
FetchResult,
|
||||
@@ -40,7 +39,7 @@ def test_refresh_secret_sources_noop_without_plugin_sources(monkeypatch):
|
||||
|
||||
import agent.secret_sources.registry as reg
|
||||
|
||||
monkeypatch.setattr(reg, "list_sources", lambda: [])
|
||||
monkeypatch.setattr(reg, "list_plugin_sources", lambda: [])
|
||||
monkeypatch.setattr(
|
||||
"hermes_cli.env_loader.reset_secret_source_cache",
|
||||
lambda: called.__setitem__("reset", called["reset"] + 1),
|
||||
@@ -61,9 +60,8 @@ def test_refresh_secret_sources_noop_when_only_builtins(monkeypatch):
|
||||
|
||||
import agent.secret_sources.registry as reg
|
||||
|
||||
monkeypatch.setattr(
|
||||
reg, "list_sources", lambda: [_StubSource(name="bitwarden")]
|
||||
)
|
||||
reg._reset_registry_for_tests()
|
||||
assert reg.list_plugin_sources() == []
|
||||
monkeypatch.setattr(
|
||||
"hermes_cli.config.load_config",
|
||||
lambda: {"secrets": {"bitwarden": {"enabled": True}}},
|
||||
@@ -87,7 +85,7 @@ def test_refresh_secret_sources_repulls_when_plugin_enabled(monkeypatch):
|
||||
|
||||
import agent.secret_sources.registry as reg
|
||||
|
||||
monkeypatch.setattr(reg, "list_sources", lambda: [_StubSource()])
|
||||
monkeypatch.setattr(reg, "list_plugin_sources", lambda: [_StubSource()])
|
||||
monkeypatch.setattr(
|
||||
"hermes_cli.config.load_config",
|
||||
lambda: {"secrets": {"myvault": {"enabled": True}}},
|
||||
@@ -112,7 +110,9 @@ def test_refresh_respects_custom_is_enabled(monkeypatch):
|
||||
|
||||
import agent.secret_sources.registry as reg
|
||||
|
||||
monkeypatch.setattr(reg, "list_sources", lambda: [_CustomActivationSource()])
|
||||
monkeypatch.setattr(
|
||||
reg, "list_plugin_sources", lambda: [_CustomActivationSource()]
|
||||
)
|
||||
# No `enabled` key at all — only the source's custom contract decides.
|
||||
monkeypatch.setattr(
|
||||
"hermes_cli.config.load_config",
|
||||
@@ -137,7 +137,9 @@ def test_refresh_skips_custom_source_when_not_activated(monkeypatch):
|
||||
|
||||
import agent.secret_sources.registry as reg
|
||||
|
||||
monkeypatch.setattr(reg, "list_sources", lambda: [_CustomActivationSource()])
|
||||
monkeypatch.setattr(
|
||||
reg, "list_plugin_sources", lambda: [_CustomActivationSource()]
|
||||
)
|
||||
# `enabled: true` but the custom contract ignores it and requires vault_id.
|
||||
monkeypatch.setattr(
|
||||
"hermes_cli.config.load_config",
|
||||
@@ -166,7 +168,7 @@ def test_refresh_skips_source_whose_is_enabled_raises(monkeypatch):
|
||||
|
||||
import agent.secret_sources.registry as reg
|
||||
|
||||
monkeypatch.setattr(reg, "list_sources", lambda: [_Boom()])
|
||||
monkeypatch.setattr(reg, "list_plugin_sources", lambda: [_Boom()])
|
||||
monkeypatch.setattr(
|
||||
"hermes_cli.config.load_config",
|
||||
lambda: {"secrets": {"myvault": {"enabled": True}}},
|
||||
@@ -198,34 +200,57 @@ def test_discover_and_load_invokes_refresh(monkeypatch):
|
||||
|
||||
|
||||
def test_real_plugin_source_discovery_applies_dotenv(monkeypatch, tmp_path):
|
||||
"""End-to-end: a real plugin source registered via discovery triggers a
|
||||
re-pull that flows through the registry's is_enabled contract."""
|
||||
"""A cold process discovers a real plugin and applies its credential."""
|
||||
import agent.secret_sources.registry as reg
|
||||
from hermes_cli import env_loader
|
||||
|
||||
reg._reset_registry_for_tests()
|
||||
|
||||
# Register a real plugin source the way PluginContext.register_secret_source
|
||||
# would (lands in registry.register_source).
|
||||
plugin_source = _StubSource(name="tmpvault", scheme="tmpvault")
|
||||
assert reg.register_source(plugin_source) is True
|
||||
assert any(getattr(s, "name", "") == "tmpvault" for s in reg.list_sources())
|
||||
|
||||
applied = {"reset": 0, "load": 0}
|
||||
monkeypatch.setattr(
|
||||
"hermes_cli.env_loader.reset_secret_source_cache",
|
||||
lambda: applied.__setitem__("reset", applied["reset"] + 1),
|
||||
env_loader.reset_secret_source_cache()
|
||||
home = tmp_path / ".hermes"
|
||||
plugin_dir = home / "plugins" / "fixture-secret-source"
|
||||
plugin_dir.mkdir(parents=True)
|
||||
(home / "config.yaml").write_text(
|
||||
"plugins:\n"
|
||||
" enabled: [fixture-secret-source]\n"
|
||||
"secrets:\n"
|
||||
" fixturevault:\n"
|
||||
" enabled: true\n",
|
||||
encoding="utf-8",
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
"hermes_cli.env_loader.load_hermes_dotenv",
|
||||
lambda **kw: applied.__setitem__("load", applied["load"] + 1),
|
||||
(plugin_dir / "plugin.yaml").write_text(
|
||||
"name: fixture-secret-source\nversion: 0.1.0\n",
|
||||
encoding="utf-8",
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
"hermes_cli.config.load_config",
|
||||
lambda: {"secrets": {"tmpvault": {"enabled": True}}},
|
||||
(plugin_dir / "__init__.py").write_text(
|
||||
"from pathlib import Path\n"
|
||||
"from agent.secret_sources.base import (\n"
|
||||
" SECRET_SOURCE_API_VERSION, FetchResult, SecretSource,\n"
|
||||
")\n\n"
|
||||
"class FixtureVault(SecretSource):\n"
|
||||
" name = 'fixturevault'\n"
|
||||
" label = 'Fixture vault'\n"
|
||||
" api_version = SECRET_SOURCE_API_VERSION\n"
|
||||
" shape = 'bulk'\n\n"
|
||||
" def fetch(self, cfg: dict, home_path: Path) -> FetchResult:\n"
|
||||
" return FetchResult(secrets={\n"
|
||||
" 'HERMES_TEST_PLUGIN_BOOTSTRAP': 'from-plugin',\n"
|
||||
" })\n\n"
|
||||
"def register(ctx):\n"
|
||||
" ctx.register_secret_source(FixtureVault())\n",
|
||||
encoding="utf-8",
|
||||
)
|
||||
|
||||
mgr = PluginManager()
|
||||
mgr._refresh_secret_sources_after_discovery()
|
||||
monkeypatch.setenv("HERMES_HOME", str(home))
|
||||
monkeypatch.delenv("HERMES_TEST_PLUGIN_BOOTSTRAP", raising=False)
|
||||
|
||||
assert applied == {"reset": 1, "load": 1}
|
||||
reg._reset_registry_for_tests()
|
||||
try:
|
||||
PluginManager().discover_and_load()
|
||||
|
||||
assert os.environ["HERMES_TEST_PLUGIN_BOOTSTRAP"] == "from-plugin"
|
||||
assert [source.name for source in reg.list_plugin_sources()] == [
|
||||
"fixturevault"
|
||||
]
|
||||
finally:
|
||||
os.environ.pop("HERMES_TEST_PLUGIN_BOOTSTRAP", None)
|
||||
reg._reset_registry_for_tests()
|
||||
env_loader.reset_secret_source_cache()
|
||||
|
||||
@@ -16,8 +16,10 @@ The bundled set is deliberately closed, same policy as [memory providers](/devel
|
||||
|
||||
`load_hermes_dotenv()` often runs at import time **before** plugins register.
|
||||
Hermes then re-pulls secrets after plugin discovery when any **enabled**
|
||||
plugin secret source is configured (`secrets.<name>.enabled: true`). That
|
||||
closes the "replace Bitwarden with my vault" first-process gap (#64177).
|
||||
plugin secret source is configured. Enablement uses the source's
|
||||
`is_enabled(cfg)` contract; the standard form is
|
||||
`secrets.<name>.enabled: true`, while custom activation remains supported.
|
||||
That closes the "replace Bitwarden with my vault" first-process gap (#64177).
|
||||
|
||||
- Re-pull is idempotent and fail-open (never blocks startup).
|
||||
- Sources only supply env vars through the orchestrator; there is **no**
|
||||
@@ -141,7 +143,7 @@ def register(ctx):
|
||||
Registration is rejected (with a log warning, never a crash) for: non-`SecretSource` instances, invalid/duplicate names, a `scheme` another source owns, wrong `api_version`, or a `shape` outside `mapped`/`bulk`.
|
||||
|
||||
:::note Timing
|
||||
Plugin discovery runs later in startup than the first `load_hermes_dotenv()` call. Immediately after discovery, Hermes re-pulls enabled plugin secret sources (`reset_secret_source_cache()` + `load_hermes_dotenv()`), so the discovering process *does* pick them up — see [First-process bootstrap timing](#first-process-bootstrap-timing) above (#64177). The re-pull is fail-open and skipped when no plugin source is enabled. Any code that read `os.environ` *before* discovery completes (i.e. at pure import time) still sees only the initial load; bundled sources remain the safest choice for the earliest bootstrap. Every subsequently spawned Hermes process (gateway children, cron sessions, subagents) also consults plugin sources on its own initial load.
|
||||
Plugin discovery runs later in startup than the first `load_hermes_dotenv()` call. Immediately after discovery, Hermes re-pulls enabled plugin secret sources (`reset_secret_source_cache()` + `load_hermes_dotenv()`), so the discovering process *does* pick them up — see [First-process bootstrap timing](#first-process-bootstrap-timing) above (#64177). The re-pull is fail-open and skipped when no plugin source is enabled. Any code that reads `os.environ` during the plugin module's import or `register(ctx)` still runs before the re-pull and cannot depend on credentials supplied by that same source; keep credentialed work inside `fetch()`. Gateway, cron, and subagent processes perform the same discovery/re-pull sequence.
|
||||
:::
|
||||
|
||||
## Users configure it like any other source
|
||||
|
||||
Reference in New Issue
Block a user