feat: add 1Password secrets backend - #38569
Conversation
20b01b8 to
551ca4f
Compare
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Changes
This PR adds a 1Password CLI (op) secret-source integration alongside the existing Bitwarden integration. It introduces:
agent/secret_sources/onepassword.py— full 1Password reference resolution with subprocess-drivenop read, SHA-256 fingerprint-based caching, timeout handling, and a structuredFetchResultdataclass.hermes_cli/onepassword_secrets_cli.py— CLI commands (setup,status,sync,disable) for the new backend.hermes_cli/env_loader.py— extended to call both Bitwarden and 1Password from_apply_external_secret_sources.hermes_cli/main.py— adds parser forhermes secrets onepassword ....
Security Observations
- Token handling is correct — the service-account token is never logged or cached in plain mappable form (only a SHA-256 fingerprint is used as the cache key).
- Startup-block failure path —
apply_onepassword_secretscalls are wrapped inexcept Exceptionsoopfailures never block Hermes startup, matching the existing Bitwarden pattern. - Subprocess call —
_run_op_readpasses the token viaenv["OP_SERVICE_ACCOUNT_TOKEN"]rather than the command line, avoiding process-table leakage. - No secrets in diff — no actual credential values are present in the code, only op:// reference strings.
Code Quality
- Consistent with the existing Bitwarden architecture (
FetchResult,find_<tool>(), apply function signature). - Test scaffolding (
_reset_cache_for_tests()) present for cache isolation. - Docstrings and design summary are clear.
Suggestions
- Missing test coverage: No unit tests for
onepassword.pyitself (only mocked CLI setup). Consider adding a smalltests/agent/test_secret_sources_onepassword.pythat patchessubprocess.runto verify the happy-path, timeout, timeout-expired, non-zero-returncode, andop not foundbranches. The Bitwarden module has tests; parity here would help. noqa: BLE001usage is consistent with existing exceptions; not a concern.- Consider whether the
20second_OP_RUN_TIMEOUTshould be documented as a tunable in the module docstring or made configurable — it's fine hard-coded for now, but users on slow networks may hit it.
Reviewed by Hermes Agent
|
Thanks for the review @tonydwb. Do you want me to work on the suggestions before we run the tests and merge? |
|
1Password support landed on main via PR #59498, which also ships a pluggable SecretSource interface — an ABC + orchestrator that lets multiple secret managers run simultaneously with deterministic precedence, conflict warnings, and per-var provenance. The merged backend is a salvage of #36896 (earliest full-cluster credit to #32254). This PR is superseded by that interface; thanks for the contribution and sorry we couldn't land them all — six independent 1Password PRs is exactly why the interface now exists. If you'd like to build further secret-manager backends, they now plug in as standalone plugins via |
Summary
opCLI andOP_SERVICE_ACCOUNT_TOKENsecrets.onepassword.mappingso env vars can resolve fromop://...secret references at startuphermes secrets onepassword/op/1passwordCLI commands for setup, status, sync, and disableBehavior
os.environduringload_hermes_dotenv()onepasswordfor UI suffixesTest Plan
python -m pytest tests/test_bitwarden_secrets.py tests/test_env_loader_secret_sources.py tests/test_onepassword_secrets.py -qpython -m py_compile agent/secret_sources/onepassword.py hermes_cli/onepassword_secrets_cli.py hermes_cli/env_loader.py hermes_cli/main.py