From 8235f484c947a9ce8a89b7bc2b8bf3453da90020 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Mon, 6 Jul 2026 02:27:44 -0700 Subject: [PATCH] feat(secrets): adapt 1Password onto the SecretSource interface MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up on the cherry-picked #36896 commits, wiring 1Password into the new registry as the reference *mapped* source: - OnePasswordSource adapter (shape=mapped, scheme=op): fetch-only — precedence, override semantics, conflict warnings, and env writes move to the orchestrator; apply_onepassword_secrets kept as legacy shim like Bitwarden's. - Registered in _ensure_builtin_sources; mapped op:// bindings now outrank bulk Bitwarden project dumps on contested vars. - _cache.py FetchResult/is_valid_env_name re-exported from base so there is exactly one canonical definition; bitwarden.py re-adapted onto the contributor's DiskCache substrate. - ErrorKind classification for op failures (auth/binary/empty/network). - Registry + conformance coverage for OnePasswordSource, incl. the headline multi-source test: both vaults claim the same var, mapped 1Password wins, conflict surfaced, provenance correct. - env_loader tests migrated off the legacy apply_* mocks onto the fetch layer; AUTHOR_MAP entry for @hwrdprkns. --- agent/secret_sources/onepassword.py | 151 ++++++++++++++++++ agent/secret_sources/registry.py | 7 + scripts/release.py | 1 + .../test_secret_source_registry.py | 132 +++++++++++++++ tests/test_env_loader_secret_sources.py | 36 +++-- 5 files changed, 312 insertions(+), 15 deletions(-) diff --git a/agent/secret_sources/onepassword.py b/agent/secret_sources/onepassword.py index 6f214d05fe1..a9ec9b313c6 100644 --- a/agent/secret_sources/onepassword.py +++ b/agent/secret_sources/onepassword.py @@ -55,6 +55,7 @@ from agent.secret_sources._cache import ( FetchResult, is_valid_env_name, ) +from agent.secret_sources.base import ErrorKind, SecretSource logger = logging.getLogger(__name__) @@ -477,6 +478,156 @@ def apply_onepassword_secrets( return result +# --------------------------------------------------------------------------- +# SecretSource adapter — the registry-facing wrapper around this module. +# --------------------------------------------------------------------------- + + +class OnePasswordSource(SecretSource): + """1Password as a registered secret source. + + 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``). + + 1Password is a **mapped** source: the user explicitly binds each env + var to an ``op://`` reference under ``secrets.onepassword.env``, so + its claims outrank bulk sources (e.g. a Bitwarden project dump) on + contested vars. + """ + + name = "onepassword" + label = "1Password" + shape = "mapped" + scheme = "op" + + def override_existing(self, cfg: dict) -> bool: + # Default True: an explicit VAR→op:// binding is the strongest + # user intent there is — leaving a stale .env line in place + # should not silently defeat it (same rotation rationale as + # Bitwarden). + return bool(isinstance(cfg, dict) and cfg.get("override_existing", True)) + + def protected_env_vars(self, cfg: dict): + token_env = _DEFAULT_TOKEN_ENV + if isinstance(cfg, dict): + token_env = str(cfg.get("service_account_token_env") or token_env) + return frozenset({token_env}) + + def config_schema(self) -> dict: + return { + "enabled": {"description": "Master switch", "default": False}, + "env": { + "description": "Map of ENV_VAR -> op://vault/item/field reference", + "default": {}, + }, + "account": { + "description": "op --account shorthand (empty = default account)", + "default": "", + }, + "service_account_token_env": { + "description": "Env var holding the service-account token " + "(unset = desktop/interactive session)", + "default": _DEFAULT_TOKEN_ENV, + }, + "binary_path": { + "description": "Pin the op binary (empty = resolve via PATH)", + "default": "", + }, + "cache_ttl_seconds": { + "description": "Disk+memory cache TTL; 0 disables", + "default": 300, + }, + "override_existing": { + "description": "Resolved values overwrite .env/shell values", + "default": True, + }, + } + + def fetch(self, cfg: dict, home_path: Path) -> FetchResult: + cfg = cfg if isinstance(cfg, dict) else {} + result = FetchResult() + + env_map = cfg.get("env") + valid, warnings = _validate_references( + env_map if isinstance(env_map, dict) else None + ) + result.warnings.extend(warnings) + if not valid: + if not warnings: + result.error = ( + "secrets.onepassword.enabled is true but the env: map is " + "empty. Add ENV_VAR: op://vault/item/field entries." + ) + result.error_kind = ErrorKind.NOT_CONFIGURED + return result + + binary_path = str(cfg.get("binary_path") or "") + binary = find_op(binary_path) + result.binary_path = binary + if binary is None: + if binary_path: + result.error = ( + f"secrets.onepassword.binary_path ({binary_path!r}) is " + "not an executable op binary." + ) + else: + result.error = ( + "secrets.onepassword.enabled is true but the op CLI was " + "not found on PATH. Install it " + "(https://developer.1password.com/docs/cli/get-started/) " + "or set secrets.onepassword.binary_path." + ) + 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, fetch_warnings = fetch_onepassword_secrets( + references=valid, + account=str(cfg.get("account") or ""), + token_env=str( + cfg.get("service_account_token_env") or _DEFAULT_TOKEN_ENV + ), + binary=binary, + cache_ttl_seconds=ttl, + home_path=home_path, + ) + except RuntimeError as exc: + result.error = str(exc) + result.error_kind = _classify_op_error(str(exc)) + return result + + result.secrets = secrets + result.warnings.extend(fetch_warnings) + return result + + +def _classify_op_error(message: str) -> ErrorKind: + """Best-effort mapping of op failure text onto the shared taxonomy.""" + lowered = message.lower() + if "timed out" in lowered: + return ErrorKind.TIMEOUT + if "not found on path" in lowered or "not an executable" in lowered \ + or "failed to invoke" in lowered: + return ErrorKind.BINARY_MISSING + if any(tok in lowered for tok in ("unauthorized", "not signed in", + "session expired", "authentication", + "401", "403")): + return ErrorKind.AUTH_FAILED + if "empty value" in lowered: + return ErrorKind.EMPTY_VALUE + if any(tok in lowered for tok in ("network", "connection", "resolve host", + "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 index 993ad4bcda2..7dad8d5d0b8 100644 --- a/agent/secret_sources/registry.py +++ b/agent/secret_sources/registry.py @@ -167,6 +167,13 @@ def _ensure_builtin_sources() -> None: 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()) + except Exception: # noqa: BLE001 — never block startup + logger.warning("Failed to register bundled 1Password secret source", + exc_info=True) def _reset_registry_for_tests() -> None: diff --git a/scripts/release.py b/scripts/release.py index e9ac04b9034..bfceff0c353 100755 --- a/scripts/release.py +++ b/scripts/release.py @@ -45,6 +45,7 @@ ACP_REGISTRY_MANIFEST = REPO_ROOT / "acp_registry" / "agent.json" # Auto-extracted from noreply emails + manual overrides AUTHOR_MAP = { + "taylorhp@gmail.com": "hwrdprkns", # PR #36896 salvage (secrets: 1Password op:// secret source + shared _cache substrate, adapted onto the SecretSource interface) "ishengeqi@163.com": "isheng-eqi", # PR #59428 salvage (cron: reject past one-shot timestamps in update_job fallback + resume_job; #59395). Also PR #59446 salvage (cron: advance one-shot next_run_at before dispatch so concurrent gateway+desktop schedulers can't double-execute; #59229). "derek2000139@qq.com": "derek2000139", # PR #57838 salvage (desktop/windows: pre-write update marker before quit dwell so the renderer's waitForUpdateToFinish gate parks instead of respawning a backend that re-locks venv .pyd files mid-update) "AndreasHiltner@users.noreply.github.com": "AndreasHiltner", # PR #56854 salvage (gateway: route multiplex profile responses through the profile's own adapter — 53-site _adapter_for_source sweep) diff --git a/tests/secret_sources/test_secret_source_registry.py b/tests/secret_sources/test_secret_source_registry.py index 80a67464a0b..2c0d3d5e857 100644 --- a/tests/secret_sources/test_secret_source_registry.py +++ b/tests/secret_sources/test_secret_source_registry.py @@ -466,3 +466,135 @@ class TestBitwardenConformance(SecretSourceConformance): monkeypatch.setattr(bw, "find_bws", lambda **kw: None) monkeypatch.delenv("BWS_ACCESS_TOKEN", raising=False) return BitwardenSource() + + +# --------------------------------------------------------------------------- +# 1Password adapter +# --------------------------------------------------------------------------- + + +class TestOnePasswordSource: + def test_identity(self): + from agent.secret_sources.onepassword import OnePasswordSource + + src = OnePasswordSource() + assert src.name == "onepassword" + assert src.shape == "mapped" + assert src.scheme == "op" + + def test_override_existing_defaults_true(self): + from agent.secret_sources.onepassword import OnePasswordSource + + src = OnePasswordSource() + assert src.override_existing({}) is True + assert src.override_existing({"override_existing": False}) is False + + def test_protected_vars_track_token_env(self): + from agent.secret_sources.onepassword import OnePasswordSource + + src = OnePasswordSource() + assert src.protected_env_vars({}) == frozenset( + {"OP_SERVICE_ACCOUNT_TOKEN"} + ) + assert src.protected_env_vars( + {"service_account_token_env": "MY_OP_TOKEN"} + ) == frozenset({"MY_OP_TOKEN"}) + + def test_fetch_empty_map_not_configured(self, tmp_path): + from agent.secret_sources.onepassword import OnePasswordSource + + result = OnePasswordSource().fetch({"enabled": True}, tmp_path) + assert result.error_kind is ErrorKind.NOT_CONFIGURED + + def test_fetch_missing_binary(self, tmp_path, monkeypatch): + import agent.secret_sources.onepassword as op + + monkeypatch.setattr(op, "find_op", lambda *_a, **_kw: None) + result = op.OnePasswordSource().fetch( + {"enabled": True, "env": {"K": "op://V/I/F"}}, tmp_path + ) + assert result.error_kind is ErrorKind.BINARY_MISSING + + def test_fetch_delegates_and_passes_config(self, tmp_path, monkeypatch): + import agent.secret_sources.onepassword as op + + monkeypatch.setattr(op, "find_op", lambda *_a, **_kw: Path("/fake/op")) + captured = {} + + def _fake_fetch(**kwargs): + captured.update(kwargs) + return {"K": "v"}, ["warn"] + + monkeypatch.setattr(op, "fetch_onepassword_secrets", _fake_fetch) + result = op.OnePasswordSource().fetch( + {"enabled": True, "env": {"K": "op://V/I/F"}, + "account": "team", "service_account_token_env": "MY_TOK"}, + tmp_path, + ) + assert result.ok and result.secrets == {"K": "v"} + assert captured["references"] == {"K": "op://V/I/F"} + assert captured["account"] == "team" + assert captured["token_env"] == "MY_TOK" + + def test_invalid_refs_warned_not_fatal(self, tmp_path, monkeypatch): + import agent.secret_sources.onepassword as op + + monkeypatch.setattr(op, "find_op", lambda *_a, **_kw: Path("/fake/op")) + monkeypatch.setattr(op, "fetch_onepassword_secrets", + lambda **kw: ({"GOOD": "v"}, [])) + result = op.OnePasswordSource().fetch( + {"enabled": True, + "env": {"GOOD": "op://V/I/F", "BAD": "not-a-ref", + "bad name": "op://V/I/F"}}, + tmp_path, + ) + assert result.ok + assert len(result.warnings) == 2 + + def test_mapped_op_beats_bulk_bitwarden_through_orchestrator( + self, tmp_path, monkeypatch + ): + """The headline multi-source scenario: both vaults claim the same var.""" + import agent.secret_sources.bitwarden as bw + import agent.secret_sources.onepassword as op + + monkeypatch.setenv("BWS_ACCESS_TOKEN", "0.token") + monkeypatch.setattr(bw, "find_bws", lambda **kw: Path("/fake/bws")) + monkeypatch.setattr( + bw, "fetch_bitwarden_secrets", + lambda **kw: ({"SHARED_KEY": "from-bitwarden", + "BW_ONLY": "bw-val"}, []), + ) + monkeypatch.setattr(op, "find_op", lambda *_a, **_kw: Path("/fake/op")) + monkeypatch.setattr( + op, "fetch_onepassword_secrets", + lambda **kw: ({"SHARED_KEY": "from-1password"}, []), + ) + reg.register_source(bw.BitwardenSource()) + reg.register_source(op.OnePasswordSource()) + env = {"BWS_ACCESS_TOKEN": "0.token"} + report = reg.apply_all( + { + # bitwarden listed FIRST — mapped 1Password must still win. + "sources": ["bitwarden", "onepassword"], + "bitwarden": {"enabled": True, "project_id": "proj"}, + "onepassword": {"enabled": True, + "env": {"SHARED_KEY": "op://V/I/F"}}, + }, + tmp_path, environ=env, + ) + assert env["SHARED_KEY"] == "from-1password" + assert env["BW_ONLY"] == "bw-val" + assert report.provenance["SHARED_KEY"].source == "onepassword" + assert report.provenance["BW_ONLY"].source == "bitwarden" + assert report.conflicts # the shadowed bitwarden claim is surfaced + + +class TestOnePasswordConformance(SecretSourceConformance): + @pytest.fixture + def source(self, monkeypatch): + import agent.secret_sources.onepassword as op + + monkeypatch.setattr(op, "find_op", lambda *_a, **_kw: None) + monkeypatch.delenv("OP_SERVICE_ACCOUNT_TOKEN", raising=False) + return op.OnePasswordSource() diff --git a/tests/test_env_loader_secret_sources.py b/tests/test_env_loader_secret_sources.py index b836769e9b5..f3291c77cb5 100644 --- a/tests/test_env_loader_secret_sources.py +++ b/tests/test_env_loader_secret_sources.py @@ -185,10 +185,11 @@ def test_apply_external_secret_sources_dedupes_within_process(tmp_path, monkeypa def test_apply_external_secret_sources_records_onepassword_origin(tmp_path, monkeypatch): - """When ``apply_onepassword_secrets`` returns applied keys, they end up in + """When the 1Password source resolves refs, applied vars end up in ``_SECRET_SOURCES`` labeled ``onepassword``.""" monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + monkeypatch.delenv("ANTHROPIC_API_KEY", raising=False) (tmp_path / "config.yaml").write_text( "secrets:\n" " onepassword:\n" @@ -198,16 +199,18 @@ def test_apply_external_secret_sources_records_onepassword_origin(tmp_path, monk encoding="utf-8", ) - from agent.secret_sources.onepassword import FetchResult - - def _fake_apply(**_kwargs): - return FetchResult( - secrets={"ANTHROPIC_API_KEY": "sk-ant-test"}, - applied=["ANTHROPIC_API_KEY"], - ) - import agent.secret_sources.onepassword as op_module - monkeypatch.setattr(op_module, "apply_onepassword_secrets", _fake_apply) + + monkeypatch.setattr(op_module, "find_op", lambda *_a, **_kw: Path("/fake/op")) + monkeypatch.setattr( + op_module, + "fetch_onepassword_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) @@ -255,14 +258,17 @@ def test_apply_external_secret_sources_bad_ttl_does_not_crash(tmp_path, monkeypa captured = {} - from agent.secret_sources.onepassword import FetchResult - - def _fake_apply(**kwargs): + def _fake_fetch(**kwargs): captured.update(kwargs) - return FetchResult() + return {}, [] import agent.secret_sources.onepassword as op_module - monkeypatch.setattr(op_module, "apply_onepassword_secrets", _fake_apply) + monkeypatch.setattr(op_module, "find_op", lambda *_a, **_kw: Path("/fake/op")) + monkeypatch.setattr(op_module, "fetch_onepassword_secrets", _fake_fetch) + + from agent.secret_sources import registry as reg_module + + reg_module._reset_registry_for_tests() env_loader._apply_external_secret_sources(tmp_path)