feat(secrets): add 1Password secret source - #32254
Conversation
There was a problem hiding this comment.
Did a thorough pass on this — a clean, well-scoped, well-tested PR. Detailed findings are left as inline comments on the relevant lines; overview below.
What's done well
- No
shell=Trueanywhere —opis always invoked with an argv list, closing OS-level command injection. - Secret values never hit logs, argv, or stdout (only env-var names/actions are printed).
- The service-account token is handled with care: never written to the disk cache, fingerprinted (truncated SHA-256) rather than stored raw in the cache key, with a regression test; the token env var is never overwritten even under
override_existing=true. - Fail-open discipline + at-rest hygiene are solid: never raises out of apply, atomic
mkstemp -> chmod 0600 -> os.replacedisk writes, 30s subprocess timeout, thorough tests.
Findings (details inline)
| Severity | Location | Issue |
|---|---|---|
| 🔴 Blocking | onepassword.py:110 |
disk-cache substrate copy-pasted from bitwarden.py — centralize |
| 🟠 Major | onepassword.py:389 |
empty op read (rc 0) silently clobbers a good env var |
| 🟠 Major | onepassword.py:398 |
_auth_fingerprint reads literal OP_SESSION (should be OP_SESSION_*) |
| 🟠 Major | env_loader.py:309 |
unguarded float(cache_ttl_seconds) can crash startup |
| 🟠 Minor | onepassword.py:362 |
op child inherits the full os.environ (blast radius) |
| 🟠 Minor | onepassword.py:415 |
_reset_cache_for_tests doesn't clear the disk cache |
| 🧹 Nit | onepassword.py:373 |
subprocess lacks encoding="utf-8" (locale-codepage decode) |
| 🧹 Nit | onepassword.py:384 |
failure message can echo op stdout (200 chars) |
| 🧹 Nit | onepassword.py:119 |
cache dir created without explicit 0700 mode |
| 🧹 Nit | onepassword.py:296 |
empty refs_to_fetch can fall through |
| 🧹 Nit | onepassword.py:173 |
op resolved via PATH only, no pinning (informational) |
| 🧹 Nit | …cli.py:178 |
override_existing default differs (status/sync False vs startup True) |
| 🧹 Nit | …cli.py:192 |
sync --apply reimplements the apply policy |
| 🧹 Nit | …cli.py:261 |
cmd_set persists the unstripped reference |
These are peer notes — we recently built the Proton Pass secret source and hit several of the same questions (env-inheritance scope, fingerprint correctness, the non-blocking guarantee, shared-substrate-vs-copy). Take or leave any of them; you know this codebase and its trust boundaries best. Thanks for a thoughtful PR.
flamerged
left a comment
There was a problem hiding this comment.
Re-filed my earlier review as inline, line-anchored comments below — each tagged by severity (🔴 Blocking / 🟠 Major / 🟠 Minor / 🧹 Nitpick) with a suggested fix. High-level overview is in the review above. Thanks again for a clean, well-tested PR.
flamerged
left a comment
There was a problem hiding this comment.
A few more, surfaced by diffing this against an independent Proton Pass secret source I built in parallel off the same bitwarden template — spots where the two implementations diverged. All low-severity except the first, which is a fail-open break (and the same pattern is in the bitwarden branch). Peer notes — take or leave.
Add generic 1Password op:// secret-reference resolution behind the existing secret-source interface. Wire startup loading, source labels, hermes secrets onepassword CLI commands, docs, and tests.
5967cf6 to
ec5c352
Compare
|
Follow-up pushed in commit cecf6b9 to close the remaining review notes:
Local verification on rebased main: 77 passed for tests/test_bitwarden_secrets.py, tests/test_onepassword_secrets.py, and tests/test_env_loader_secret_sources.py, plus compileall and git diff --check. |
|
Follow-up pushed in commit 923d950 to tighten the shared cache behavior touched by the 1Password review work:
Local verification on the PR branch:
|
|
@kshitijk4poor could you help route/decide the canonical 1Password implementation here? There are now multiple competing 1Password PRs:
I’ve updated this branch through the review feedback, and it is currently mergeable. Local verification is posted above: targeted Happy to adjust this PR if maintainers prefer a detail from #36896, but I think the next step is a maintainer decision on which implementation should be canonical for #36949. |
|
Been looking for a feature like this, really like the approach |
|
1Password support landed on main via PR #59498. Your PR was the earliest 1Password submission in the cluster (May 25) — thank you for kicking this off. The merged implementation is a salvage of #36896 adapted onto a new pluggable SecretSource interface (multiple vaults can now be enabled at once with deterministic precedence), so this PR is superseded. Both you and @hwrdprkns are credited in the merge PR body. |
Add generic 1Password
op://secret-reference resolution behind the existing secret-source interface. Wire startup loading, source labels,hermes secrets onepasswordCLI commands, docs, and tests.What does this PR do?
This adds first-class 1Password support for resolving environment credentials at Hermes startup without storing resolved secret values in
config.yaml.Users configure
secrets.onepassword.envas a mapping from environment variable names to official 1Passwordop://vault/item/fieldreferences. After.envloading, Hermes resolves those references through the officialopCLI and injects them intoos.environ, matching the existing external-secret-source pattern used by Bitwarden Secrets Manager.The implementation is intentionally defensive and startup-safe: missing
op, expired auth, locked desktop app sessions, bad references, or permission failures report warnings and fall back to whatever credentials were already present from.envor the shell.This also adds short-lived-process performance support. Successful reads are cached in-process and on disk under
<hermes_home>/cache/op_cache.json, with0600permissions and a TTL controlled bysecrets.onepassword.cache_ttl_seconds. The cache key uses auth/session fingerprints, account, and configured references; it does not store the service-account token or raw auth material.Related Issue
Fixes #36949.
Related competing PRs: #36896, #45439.
#45439 was called out as a duplicate of this PR and includes unrelated branch contamination; this PR is the narrower, reviewed, and currently mergeable implementation.
Type of Change
Changes Made
agent/secret_sources/onepassword.pyop://...reference validation andop readintegration.--accountselection.service_account_token_env.cache_ttl_seconds.override_existing: false.agent/secret_sources/__init__.pyhermes_cli/env_loader.py.envloading.onepasswordas the credential source for display labels.hermes_homethrough so disk cache paths are profile-aware.hermes_cli/config.pysecrets.onepasswordconfig keys.cli-config.yaml.examplehermes_cli/onepassword_secrets_cli.pyhermes secrets onepasswordcommands:setup,status,sync,set,remove, anddisable.hermes_cli/main.pyhermes secretscommand tree.tests/test_onepassword_secrets.pytests/test_env_loader_secret_sources.pywebsite/docs/user-guide/secrets/onepassword.mdwebsite/docs/user-guide/secrets/index.mdHow to Test
Run the targeted lint checks:
Run the targeted tests:
Optionally smoke-test with a temporary Hermes home and a fake
opbinary to verify startup loading and disk-cache reuse across separate processes.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passuvDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/A