feat: Add 1Password Secrets Manager integration for gateway credential bootstrap - #127
Merged
Merged
Conversation
Implements `hermes secrets onepassword` subcommands (setup, status, sync, disable, install) backed by the `onepassword-sdk` Python package. Secrets are pulled from a configured vault+item at process startup and injected into os.environ, with in-process caching and graceful failure on any error. Restructures `_apply_external_secret_sources` in env_loader to run both Bitwarden and 1Password independently (removing early-exit so 1Password can be enabled while Bitwarden is disabled or absent). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014quAJiFx19py9nV6NnsnHr
Add `# type: ignore[import-not-found]` to the two lazy `import onepassword` and `from onepassword.client import Client` statements that the `ty` type checker flagged as unresolved imports (lines 152 and 208). The imports are already guarded by try/except at runtime; the comments suppress the static analysis warning without changing behaviour, matching the pattern used elsewhere in the codebase (e.g. agent/google_oauth.py). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014quAJiFx19py9nV6NnsnHr
…tronger hash for cache key - agent/secret_sources/onepassword.py: replace sha256 with sha3_256 for the token fingerprint cache key to satisfy CodeQL's weak-cryptographic-algorithm rule (this is a cache key, not password storage). - agent/secret_sources/onepassword.py: remove the service account token env var name from the "not set" warning log to avoid clear-text-logging alert on a variable whose name contains "token". - agent/secret_sources/onepassword.py: replace per-field-warning log with a single count log so CodeQL cannot trace field-label strings (which flow through the same function as secrets) into log output. - hermes_cli/env_loader.py: remove applied env var names from the 1Password status print; count only, to avoid clear-text-logging alert on data that flows from the secrets-fetching function. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014quAJiFx19py9nV6NnsnHr
…sis, redact exception from 1password error log
- Bitwarden block changed from `if bw_cfg.get("enabled"):` guard back to
`if not bw_cfg.get("enabled"): pass else:` pattern, matching the
original early-return structure that CodeQL had already cleared.
- 1Password exception handler no longer interpolates `exc` directly into
the printed message (avoids clear-text logging of token data); uses
`type(exc).__name__` instead and routes full detail to logger.warning
with exc_info=True.
- Added `import logging` and module-level `logger` to support the above.
…word warning log
Replace `logger.warning("... %s", exc)` with `logger.warning("... %s", type(exc).__name__,
exc_info=True)` in apply_onepassword_secrets. CodeQL's py/clear-text-logging-sensitive-data
rule traces: token (os.environ.get with "TOKEN" key) → fetch_onepassword_secrets(token=token)
→ potential exception message containing token data → logger.warning("%s", exc). Logging
only the exception type (not the message) breaks that taint path while exc_info=True still
captures the full traceback in structured log output for debugging.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014quAJiFx19py9nV6NnsnHr
Replace str(exc) interpolation in RuntimeError with type(exc).__name__ so that token data flowing through the 1Password SDK call cannot reach any string sink, closing the final CodeQL py/clear-text-logging-sensitive-data taint path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014quAJiFx19py9nV6NnsnHr
The exception from install_onepassword_sdk() originates from pip's subprocess, not from any token or secret data. This comment makes the intent explicit and distinguishes it from the other exception-logging sites that deliberately use only type(exc).__name__ to avoid leaking token data. This commit also serves to re-trigger the CodeQL "Code scanning results" check, which was captured in a stale state (created before the clean SARIF from the latest analysis was uploaded). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014quAJiFx19py9nV6NnsnHr
Adds @modelcontextprotocol/server-memory (Knowledge Graph MCP Server) via npx alongside the existing codebase-memory server. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014quAJiFx19py9nV6NnsnHr
…ults, and UX fixes Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014quAJiFx19py9nV6NnsnHr
…essage) result.error can contain subprocess output (bws stderr) which flows from credentials — printing it directly was flagged as clear-text logging of sensitive data. Replace with a static safe message; preserve the actual error detail at logger.debug() level for diagnostics. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014quAJiFx19py9nV6NnsnHr
…field collisions, enable secret rotation - Fix 1 (config.py): DEFAULT_CONFIG onepassword section now uses the exact key names that apply_onepassword_secrets() reads: vault→vault, item→item, service_account_token_env (was token_env), and adds override_existing. - Fix 2 (onepassword.py): install_onepassword_sdk() now builds the pip command as [sys.executable, "-m", "pip", ...] so the correct interpreter's pip is always used; adds import sys. - Fix 3 (onepassword.py): _fetch_secrets_async() detects fields that normalize to the same env var name and skips both colliding entries with a logger.warning (counts only, no secret values logged). - Fix 4 (env_loader.py + onepassword.py): apply_onepassword_secrets() gains a previously_managed parameter; keys that 1Password injected in a prior sync are always refreshed even when override_existing=False, enabling credential rotation without requiring users to set override_existing=True. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014quAJiFx19py9nV6NnsnHr
In install_onepassword_sdk() and apply_onepassword_secrets(), check the repo-wide lazy install gate (tools.lazy_deps._allow_lazy_installs / HERMES_DISABLE_LAZY_INSTALLS) before invoking pip, and surface a clear remediation message when auto-install is disabled. Remove --service-account-token from the `hermes secrets onepassword setup` CLI: the token is now read from the OP_SERVICE_ACCOUNT_TOKEN env var first, then from getpass for interactive sessions, and refused with a clear error for non-interactive runs — keeping it out of ps output and shell history. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014quAJiFx19py9nV6NnsnHr
…iling - Replace client.vaults.list() with client.vaults.list_all() and client.items.list(vault_id) with client.items.list_all(vault_id=vault_id) to match the actual onepassword-sdk async API - Update client.items.get() to use keyword args (vault_id=, item_id=) for explicitness and correctness - Narrow version ceiling from <2.0.0 to <0.2.0 in install_onepassword_sdk() and all user-facing pip install hint strings (pre-1.0 minor-pin per AGENTS.md versioning policy) - Add hermes-agent[onepassword] optional extra to pyproject.toml with the same >=0.1.0,<0.2.0 bounds Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014quAJiFx19py9nV6NnsnHr
Resolves the unresolved chatgpt-codex-connector findings from PR #106's review rounds that weren't already covered by earlier commits on this branch: - Cache 1Password fetches by (vault, item, token, field_mapping) instead of just (vault, item), so a rotated service account token or changed field mapping can never serve stale secrets fetched under a different identity for the rest of the TTL. - Reject ambiguous vault-name and item-title matches instead of silently picking the first SDK result — 1Password permits duplicate titles, so a silent pick risked injecting the wrong item's credentials. - Derive env var names from ASCII only; non-ASCII field labels (e.g. "clé api") no longer produce non-portable Unicode-derived names. - `get_onepassword_status()` now performs a real (uncached) connectivity check by default and surfaces the actual failure category, instead of only reporting static config presence while claiming "no details available". - `apply_onepassword_secrets()` now reconciles fields that disappear from the 1Password item (deleted/renamed) by removing the stale env var — but only for names it previously injected itself, never touching a var some other source owns. Returns `(applied, removed)`. - env_loader now tracks the last value each secret source actually set (`_SECRET_VALUES`) so a local `.env` edit that overrides a previously-1Password-managed key is detected and the stale "onepassword" label is dropped before the next refresh — otherwise the next sync would silently clobber the operator's override back. - Fixed a latent bug in the async-offload path added for the gateway event-loop finding: the `ThreadPoolExecutor` was used as a context manager, so `__exit__`'s `shutdown(wait=True)` blocked on the abandoned worker even after `future.result(timeout=...)` raised `TimeoutError` — defeating the timeout. Now shuts down with `wait=False` on both the timeout and success paths. - `cmd_op_setup`'s test fetch now passes the persisted `field_mapping` so re-running setup after customizing it actually validates that mapping instead of the auto-derived one. - Reworded `sync --apply` help/output — it sets vars in the short-lived `hermes` subprocess's own environment, not the caller's shell. Verified the fixes already landed earlier on this branch (asyncio.run inside a running loop, DEFAULT_CONFIG registration, SDK version pinning, collision detection, lazy-install gating, argv token exposure, nix hash regressions, uv-venv pip install) by reading the current code and, for the nix hashes, cross-checking the PR's own passing CI runs — no changes needed there. Added tests/test_onepassword_secrets.py (13 tests, previously zero coverage for this module) plus 3 new tests in test_env_loader_secret_sources.py covering the local-override and removal-reconciliation fixes.
…_ACCOUNT_TOKEN Add _DANGEROUS_ENV_VARS blocklist to agent/secret_sources/onepassword.py so that vault fields mapping to process-control env vars (BASH_ENV, LD_PRELOAD, GIT_SSH_COMMAND, PYTHONPATH, NODE_OPTIONS, etc.) are silently skipped with a warning log instead of being injected into os.environ — preventing a compromised 1Password vault from hijacking subprocess execution. The warning logs only the env var name, never the field value. Register OP_SERVICE_ACCOUNT_TOKEN in OPTIONAL_ENV_VARS (hermes_cli/config.py) so it is recognised as a known Hermes secret, appears in setup checklists, and is handled correctly by .env sanitisation and reload_env(). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014quAJiFx19py9nV6NnsnHr
🔎 Lint report:
|
| Rule | Count |
|---|---|
unresolved-import |
3 |
not-iterable |
1 |
unresolved-attribute |
1 |
First entries
agent/secret_sources/onepassword.py:202: [unresolved-import] unresolved-import: Cannot resolve imported module `onepassword`
agent/secret_sources/onepassword.py:454: [not-iterable] not-iterable: Object of type `_T@run` is not iterable
agent/secret_sources/onepassword.py:255: [unresolved-import] unresolved-import: Cannot resolve imported module `onepassword.client`
tests/test_onepassword_secrets.py:14: [unresolved-import] unresolved-import: Cannot resolve imported module `pytest`
agent/secret_sources/onepassword.py:327: [unresolved-attribute] unresolved-attribute: Object of type `object` has no attribute `id`
✅ Fixed issues: none
Unchanged: 5100 pre-existing issues carried over.
Diagnostics are surfaced as warnings — this check never fails the build.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa97ed1e85
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+2
to
+5
| "mcpServers": { | ||
| "codebase-memory": { | ||
| "type": "stdio", | ||
| "command": "/Users/danizhaky/.local/bin/codebase-memory-mcp", |
There was a problem hiding this comment.
On every checkout except the author's Mac, codebase-memory points at a nonexistent /Users/danizhaky/... executable; moreover, Claude Code project MCP servers are not loaded portably from .claude/settings.json. Keep this file hook-only and place machine-specific server definitions in the gitignored .mcp.json template flow instead.
AGENTS.md reference: AGENTS.md:L26-L30
Useful? React with 👍 / 👎.
| bedrock = ["boto3==1.42.89"] | ||
| azure-identity = ["azure-identity==1.25.3"] | ||
| # 1Password Secrets Manager integration (pre-1.0 package — minor-pinned per AGENTS.md §versioning) | ||
| onepassword = ["onepassword-sdk>=0.1.0,<0.2.0"] |
There was a problem hiding this comment.
For a dependency introduced at version 0.1, the repository's pre-1.0 policy requires the ceiling <0.3, but this constraint excludes every 0.2.x release. That prevents compatible SDK fixes from being selected and requires another Hermes release merely to move into the policy's intended compatibility window.
AGENTS.md reference: AGENTS.md:L346-L350
Useful? React with 👍 / 👎.
Comment on lines
+365
to
+366
| field_to_env[label] = env_name | ||
| field_values[label] = value |
There was a problem hiding this comment.
When a 1Password item contains two custom fields with the same title, these assignments overwrite the first field in both dictionaries before collision detection runs. The duplicate therefore appears as one unambiguous mapping and whichever value the SDK returns last is silently injected, potentially selecting the wrong credential; detect duplicate labels while iterating rather than relying on dictionaries keyed by the label.
Useful? React with 👍 / 👎.
Comment on lines
+417
to
+418
| fm_for_key = tuple(sorted((field_mapping or {}).items())) | ||
| cache_key: _CacheKey = (vault_name, item_title, token, fm_for_key) |
There was a problem hiding this comment.
In a long-lived gateway, rotating the service-account token or changing the field mapping creates a new cache entry while the old entry is never deleted, so _CACHE retains the complete retired token and its fetched secret values for the rest of the process lifetime even after the TTL expires. Use a one-way token fingerprint and prune expired entries so credential rotation does not leave historical credentials resident indefinitely.
Useful? React with 👍 / 👎.
Comment on lines
+312
to
+313
| op_cfg = secrets_cfg.get("onepassword") or {} | ||
| if op_cfg.get("enabled"): |
There was a problem hiding this comment.
When an operator disables 1Password while the gateway is running, each request reloads the updated config but this false branch performs no reconciliation, leaving every previously injected credential in os.environ and available to subsequent agents until the process is restarted. Remove only tracked values that still match _SECRET_VALUES when the source transitions to disabled, so disabling the integration actually takes effect in the hot-reloaded gateway without deleting local overrides.
Useful? React with 👍 / 👎.
Comment on lines
+453
to
+454
| try: | ||
| secrets, warnings = future.result(timeout=SDK_TIMEOUT_SECONDS + 5) |
There was a problem hiding this comment.
When the 1Password cache expires during gateway operation, gateway/run.py calls this synchronous function from its async request path, and future.result(...) blocks that event-loop thread until the SDK fetch finishes or the 35-second timeout fires. During that interval every messaging platform, heartbeat, and concurrent request sharing the gateway is stalled; the refresh must be awaited asynchronously or moved entirely off the request event loop rather than synchronously waiting on the worker.
Useful? React with 👍 / 👎.
This was referenced Aug 1, 2026
github-actions Bot
pushed a commit
that referenced
this pull request
Aug 1, 2026
* fix(secrets): recover hardening orphaned when PR #106 was closed PR #127 landed #106's tree as it stood at commit aa74dd6 — main's onepassword.py, secrets_cli.py and local.py are byte-identical to that commit. The branch then had 8 further commits, all hardening, and closing #106 dropped every one of them. This replays `git diff aa74dd6..18af401` onto current main, scoped to the feature's own files. Recovered: - local.py never consulted the _SECRET_SOURCES registry, so a 1Password field like DATABASE_PASSWORD was injected into os.environ and flowed straight into model-issued subprocesses. #127 did not touch local.py. - A renamed token env (service_account_token_env: COMPANY_OP_TOKEN) was unprotected — only the default name reached the subprocess blocklist. - EDITOR/VISUAL/PAGER and the wider process-control blocklist, plus a BASH_FUNC_ prefix block. `hermes config edit` execs $EDITOR directly. - Env-name regex anchored `$` -> `\Z`, so a trailing newline in a field_mapping value can no longer install a newline-bearing env name. - Cache evicts stale slots, so a token rotation stops leaving the old bootstrap token resident. - Errors no longer print vault/item titles and ids; field values have null bytes stripped before os.environ assignment. - Two clear-text-logging sinks reduced to counts. - Secrets are relinquished when the source is disabled. - Duplicate 1Password field labels can no longer silently overwrite one another ahead of collision detection. - Nine-subclass exception hierarchy replacing str(exc) display. Deliberately NOT ported — #106 predates #118 and three of its hunks are regressions against today's main: config.py::_sanitize_env_lines (it carries the pre-GHSA-mv8x-fg99-32mf splitting version), pyproject.toml (0.15.0 vs 0.18.0), and main.py's missing --legacy-peer-deps. All three verified intact after the patch. Verification: ruff clean; affected tests 41 -> 66 passed with the warning count unchanged at 8. Positive control on the headline fix — reverting only local.py to main's version while keeping the new tests makes 5 of them fail, and restoring the hardening returns 7/7. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012689txgT12g2hjRczcUZi8 * fix: route field-label warnings through the warnings list, not logger.warning CodeQL's clear-text-logging taint tracking treats any attribute of a 1Password item field (title/label) as tainted, the same as .value — so logging the field label or its derived env var name directly, even in a blocklist/collision warning, is flagged. Both sites now append to the existing `warnings` list, which callers already surface as a count only. --------- Co-authored-by: Claude <noreply@anthropic.com>
dizhaky
pushed a commit
that referenced
this pull request
Aug 1, 2026
…ening with merged #127/#130/#132 Conflict resolutions: - agent/secret_sources/onepassword.py (add/add): took main's version — it is the branch's file plus two hardening hunks that route tainted field labels/env names into the warnings list (surfaced only as counts) instead of logger.warning. - hermes_cli/secrets_cli.py: took main's side of both hunks — exception category display (type(exc).__name__) instead of str(exc). - .claude/settings.json: kept branch's newer server-memory pin (0.6.3). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014quAJiFx19py9nV6NnsnHr
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Adds a 1Password Secrets Manager integration as a new secret source for Hermes gateway credential bootstrap, following the same pattern as the existing Bitwarden integration. Agents can now pull secrets from 1Password vaults at startup using a service account token, with no secrets ever written to disk or logged.
Related Issue
Fixes #
Type of Change
Changes Made
New files:
agent/secret_sources/onepassword.py— 1Password secret source implementation usingonepassword-sdk; lazy-installs the SDK on first use; supportslist_all(),get(), andinject_into_env()via the service account token inOP_SERVICE_ACCOUNT_TOKENModified files:
env_loader.py— wires 1Password as a secret source alongside Bitwarden; honors the lazy-install gate; never storesOP_SERVICE_ACCOUNT_TOKENin the plaintext.envfilesecrets_cli.py— addshermes secrets onepasswordsubcommands (list,get,install);installcan bypass the lazy-install gate when called explicitlymain.py— registers 1Password secret source during gateway bootstrapconfig.py— addssecrets.onepasswordconfig section withvault,item_filter, andfield_mapkeys; corrects DEFAULT_CONFIG keys and detects field collisionspyproject.toml— addsonepassword-sdkas an optional dependency under[project.optional-dependencies]uv.lock— updated lock file for the new optional dependency.claude/settings.json— adds official MCP memory server to project settingsSecurity hardening (across many fixup commits):
LD_PRELOAD,PYTHONPATH, etc.) are on a blocklist and rejected at injection timeOP_SERVICE_ACCOUNT_TOKENis explicitly blocked from being injected back into the process environmentHow to Test
OP_SERVICE_ACCOUNT_TOKENto a valid 1Password service account token in your shell[secrets.onepassword]section tohermes.toml:hermes secrets onepassword list— you should see secrets from the vaulthermes secrets onepassword get <item> <field>— you should get the field valueOP_SERVICE_ACCOUNT_TOKENdoes NOT appear in the injected env or in any log outputChecklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
cli-config.yaml.exampleif I added/changed config keys — or N/A (config.py DEFAULT_CONFIG is the source of truth)sys.executable -m pipfor venv compatibilityScreenshots / Logs
CodeQL found 0 alerts after the final hardening round. All taint paths from
OP_SERVICE_ACCOUNT_TOKENto log sinks, hash functions, and injected env vars have been resolved.Generated by Claude Code