feat(secrets): add command secret source + unified secrets.provider selector - #44509
feat(secrets): add command secret source + unified secrets.provider selector#445090xr00tf3rr3t wants to merge 3 commits into
command secret source + unified secrets.provider selector#44509Conversation
… selector
Brings the agent's secret-source system to parity with the desktop app's
`command` secrets provider (hermes-desktop src/main/secrets/commandProvider.ts),
so a vault helper configured for the desktop also resolves on the gateway/CLI.
NEW agent/secret_sources/command.py — ports the TS provider's security model:
- Runs a user-configured helper via `/bin/sh -c`; the requested key travels
ONLY in the HERMES_SECRET_KEY env var, never interpolated into the command
string, so a hostile key name is inert data (not code).
- parse_secret_output mirrors the TS parser: exact dotenv-key match wins; >=2
env-shaped lines without the wanted key -> None; otherwise a bare value;
base64 '='-padding disambiguation; cross-key misroute guard (a single
OTHER_KEY=realvalue line never leaks into a different wanted key).
- Hard 3s timeout (kills the whole process group via killpg, so a forking
helper can't keep the pipe open), 1 MiB output cap, POSIX-only (Windows
degrades to an empty result + warning). Every failure degrades to "no value";
it never raises and never blocks startup.
- Logs ONLY structured fields (code=/signal=/errno=) to stderr; the helper's
stderr is piped and DISCARDED; the command string and secret values are
never logged. Reuses bitwarden.py's FetchResult so env_loader consumes both
sources identically.
hermes_cli/env_loader.py — _apply_external_secret_sources now reads a unified
`secrets.provider` selector ("env" | "command" | "bitwarden"):
- provider=command routes to apply_command_secrets, records the provenance as
"command" in _SECRET_SOURCES (so format_secret_source_suffix labels keys
"(from command)" — already generic, not duplicated), and re-runs the ASCII
credential sanitizer like the bitwarden path.
- provider=bitwarden keeps the existing behavior byte-for-byte.
- env / unset is a no-op (today's default — zero change for existing users).
- BACK-COMPAT: a config with only `secrets.bitwarden.enabled: true` and no
`provider` key is treated as provider=bitwarden, so existing Bitwarden users
are unaffected.
Config (the provider selector, command path, timeouts) lives in config.yaml
under `secrets:` per the project rubric — only resolved secret VALUES touch env.
Tests: NEW tests/test_command_secret_source.py — 27 cases, E2E against a real
temp HERMES_HOME with real chmod+x shell helpers (not mocks): bare/dotenv/
base64 round-trip, cross-key misroute, injection-inert key (canary not
created), timeout kill within bound, non-zero-exit degrade, no-secret-in-logs,
precedence/override, dispatch via config.yaml provider:command, idempotency,
and back-compat bitwarden routing. 27 new + 50 baseline green; wider
secrets/env_loader/config surface 229 passed / 5 skipped, no regression.
….example The command-source PR introduces the `secrets.provider` (env|command|bitwarden) selector and `secrets.command` config keys, but cli-config.yaml.example had no `secrets:` section at all. Add a fully-commented block documenting all three providers — the `command` helper (with the HERMES_SECRET_KEY-as-data contract, POSIX-only note, and timeout/cap), the unified selector, and the previously undocumented bitwarden options — so users can discover the feature from the example config. Comments only; no active YAML, no behavior change.
Parity fix for the same gap a review pass (Greptile) caught in the desktop TS provider this source was ported from: _parse_dotenv_map (the list/ enumerate path) stored whitespace-only unquoted values, while parse_secret_output / get_command_secret resolve them to None. A quoted-blank vault placeholder (BLANK=" ") would therefore appear as a configured key in list_command_secrets() but resolve None on read — the two disagreed on whether the key is set. _parse_dotenv_map now drops whitespace-only entries, so the list and get paths agree. (apply_command_secrets already skipped them on apply; this fixes the public list/enumerate API at the source.) Regression test asserts list_command_secrets() omits a quoted-blank entry, keeps the real key, and agrees with get_command_secret(). 28 command-source tests + 50 baseline green.
Verification: Clean implementation, thorough security designThis is a well-executed port of the desktop TS provider. The security model is solid:
Test coverage is comprehensive: 20+ tests covering hostile keys, timeout degradation, precedence, dotenv parsing, dispatch E2E, and back-compat with bitwarden. |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the thorough parser and failure-path coverage. The generic command use case remains unimplemented on current main, but this branch predates the SecretSource consolidation in 2d16ec7fb and cannot be salvaged mechanically.
Problems
- Current startup uses
agent.secret_sources.registry.apply_all()throughhermes_cli/env_loader.py:305-375; it composes all enabled sources with mapped-vs-bulk precedence. The PR's exclusivesecrets.providerbranches athermes_cli/env_loader.py:288-295bypass that behavior. - Current sources must only fetch; the orchestrator owns
os.environwrites (agent/secret_sources/base.py:24-28).apply_command_secrets()writes environment values directly atagent/secret_sources/command.py:380-390. _run_helper()copies the full credential-bearing environment and invokes/bin/sh -c(agent/secret_sources/command.py:181-187), contrary to the shared minimal-environment, argv-only helper inagent/secret_sources/base.py:216-265.
Suggested changes
- Rework this as a mapped
CommandSourceregistered with the existing registry, usingsecrets.command.enabledandsecrets.sources, and return aFetchResultfor the orchestrator to apply. - Use the shared subprocess safety model, update
DEFAULT_CONFIG, and cover conformance plus coexistence with the bundled sources.
Automated hermes-sweeper review.
| if not provider: | ||
| provider = "bitwarden" if bw_cfg.get("enabled") else "env" | ||
|
|
||
| if provider == "command": |
There was a problem hiding this comment.
Current main composes every enabled source through registry.apply_all() with mapped-vs-bulk precedence and conflict reporting. An exclusive provider selector would bypass that architecture; please rework command as a registered source under secrets.sources rather than adding this branch.
| ) | ||
| return None | ||
|
|
||
| env = os.environ.copy() |
There was a problem hiding this comment.
Do not pass the full post-dotenv environment to a secret helper: it contains every credential Hermes knows. Current main's run_secret_cli() uses a minimal allowlisted environment and argv-only execution; the command source needs to adopt that contract.
|
Merged via #69266 — your commit landed as-is (rebase-merged, authorship preserved), with one design change on top: the |
What does this PR do?
Adds a
commandsecret source: at startup Hermes can resolve credentials by running a user-configured helper (e.g.keepassxc-cli,secret-tool,pass, or a script that cats a tmpfs env file) instead of keeping keys in plaintext~/.hermes/.env. The helper prints either a bare value or aKEY=VALUEdotenv blob on stdout; its output is parsed and applied to the process env using the same non-destructive precedence as the existing Bitwarden source (process env /.envwin).It also introduces a single unified selector,
secrets.provider(env|command|bitwarden), so there is one source-of-truth for which backend supplies secrets — rather than each backend having its own ad-hoc enabled flag.envis the default, so existing users see no change, and a config that only setssecrets.bitwarden.enabled: trueis treated asprovider: bitwardenfor full backward compatibility.This extends the existing
agent/secret_sources/framework (the same onebitwarden.pylives in) rather than adding a parallel mechanism — aligning with the secrets roadmap in #3630 and complementing the vendor-specific source requests (Infisical #22791, 1Password #36949, Proton Pass #30649, Vaultwarden #33126). A genericcommandsource is the lowest-common-denominator backend: any secret store with a CLI works through it without bespoke integration.The security model is ported line-for-line from the Hermes desktop app's TypeScript
commandprovider, so the two surfaces resolve secrets identically.Related Issue
Fixes #44507 (generic command/exec secret source backend).
Also relates to #3630 (feat(secrets): Phase 4 — Advanced Security / External Vaults): this adds a generic command/exec backend to the
secret_sourcesframework that complements the vendor-specific source requests (Infisical #22791, 1Password #36949, Proton Pass #30649, Vaultwarden #33126).Type of Change
Changes Made
agent/secret_sources/command.py(new):apply_command_secrets()+get_command_secret()/list_command_secrets()and theparse_secret_output()/unquote_dotenv_value()parsers. Security properties:HERMES_SECRET_KEYenv var — never interpolated into the/bin/sh -cstring — so a hostile key name is inert data, not code.Nonerather than leaking another key's value into the wanted key's Authorization header.=-padding disambiguation so a bare base64 secret isn't misclassified as a dotenv line.killpg, so a forking helper can't hold the pipe open), 1 MiB output cap; every failure degrades to "no value" and never raises.code=/signal=/errno=); the helper's stderr is piped and discarded; the command string and secret values are never logged.bitwarden.py'sFetchResultsoenv_loaderconsumes both sources identically.hermes_cli/env_loader.py:_apply_external_secret_sources()now reads the unifiedsecrets.providerselector and routes tocommand/bitwarden/env(no-op). Bitwarden behavior is unchanged; back-compat maps a baresecrets.bitwarden.enabled: truetoprovider: bitwarden. Provenance is recorded ascommandin_SECRET_SOURCES(the existingformat_secret_source_suffix()already labels it "(from command)" generically — not duplicated).tests/test_command_secret_source.py(new, 27 tests).cli-config.yaml.example: documentedsecrets:block (provider / command / bitwarden) — the config keys this PR introduces.How to Test
~/.hermes/config.yaml:hermes chat -q "hi"); on stderr you'll seeCommand secret source: applied 1 secret (OPENROUTER_API_KEY). The key is now in the process env without ever touching.env.scripts/run_tests.sh(orpytest tests/test_command_secret_source.py tests/test_env_loader_secret_sources.py tests/test_bitwarden_secrets.py -q). Result: 27 new tests pass; the 50 existing secret/env-loader tests stay green. Wider-k "env_loader or secret or secrets"surface: 229 passed / 5 skipped, no regression.The new tests exercise the real path (real
chmod +xhelpers, real tempHERMES_HOME, real subprocess — not mocks) per the rubric: bare/dotenv/base64 round-trip, cross-key misroute, injection-inert key (canary file NOT created), timeout kill within bound, non-zero-exit degrade, no-secret-in-logs, precedence/override, theprovider: commanddispatch via a realconfig.yaml, idempotency, and back-compat bitwarden routing.Checklist
Code
feat(secrets):)Documentation & Housekeeping
cli-config.yaml.example— added a documentedsecrets:block (provider / command / bitwarden), since this PR introduces those config keys. (The file previously had nosecrets:section.)commandsource is POSIX-only (needs/bin/sh). On Windows it degrades cleanly to an empty result with a warning and the user stays on the defaultenvprovider — mirroring the desktop provider's Windows degrade. No behavior change for non-Windows users.Screenshots / Logs
(Resolved values are never printed — only key names and counts.)