fix(secrets): hydrate cold multiplex sources locally - #75263
Conversation
(cherry picked from commit 4befef8)
teknium1
left a comment
There was a problem hiding this comment.
Thanks for carrying forward the cold-profile fix and incorporating the .op.env bootstrap path.
Problems
- Blocking:
agent/secret_sources/registry.py:217wrapssource.fetch(cfg, home_path)in a ContextVar, but the publicSecretSourceAPI still passes no environment (agent/secret_sources/base.py:162-169). Third-party sources are supported throughPluginContext.register_secret_source()(agent/secret_sources/registry.py:19-25); an existing source that readsos.environwill still receive process-global/default-profile credentials during cold multiplex hydration. The built-in source conversions do not protect that extension path. hermes_cli/env_loader.py:175writes provenance into global_SECRET_SOURCES;get_secret_source()has no home argument. Two profiles resolving the same key through different sources can produce an incorrect source label for the first profile.
Suggested changes
- Establish an isolation-capable plugin-source contract (or skip legacy sources on multiplex hydration) and add a plugin-style regression proving direct process-environment reads cannot observe another profile.
- Scope provenance by resolved home, matching the existing value snapshots.
Automated hermes-sweeper review.
| ) | ||
| try: | ||
| future = executor.submit(source.fetch, cfg, home_path) | ||
| def _fetch() -> FetchResult: |
There was a problem hiding this comment.
This ContextVar only works for sources that opt in to get_source_environment(). The supported plugin SecretSource.fetch(cfg, home_path) contract still permits an installed source to read os.environ directly, so cold multiplex hydration can expose the process-global/default-profile credentials to that source. Please add an isolation-capable compatibility boundary (or skip legacy sources) and a plugin-style regression.
There was a problem hiding this comment.
Addressed in 2baa23f. Cold multiplex hydration now explicitly requests an isolated environment; legacy plugin sources are skipped unless they declare supports_isolated_environment = True. The bundled Bitwarden, 1Password, and command sources opt in via their existing get_source_environment() contract. Added a plugin-style regression whose legacy source reads os.environ; it is not invoked, so it cannot observe the default-profile canary. Normal startup remains compatible with existing sources.
| value = local_env.get(name) | ||
| if value is None: | ||
| continue | ||
| _SECRET_SOURCES[name] = applied.source |
There was a problem hiding this comment.
_SECRET_SOURCES remains process-global while the values are now per-home. If two profiles hydrate the same variable through different sources, this overwrites the first profile's provenance and get_secret_source() has no home argument to recover it. Please scope provenance by resolved home too.
There was a problem hiding this comment.
Addressed in 2baa23f. Provenance now uses the same resolved-home boundary as secret values. get_secret_source() and format_secret_source_suffix() accept an optional home, and their compatibility fallback returns a label only when every matching loaded home agrees. Added coverage for two homes resolving the same variable through different backends, including correct home-scoped labels and an ambiguous no-home lookup returning no label.
Updated pr and responded to review comments |
Summary
Fixes #74317 by hydrating a cold multiplex profile’s configured external secret sources into an isolated, per-profile mapping before its first runtime secret scope is constructed.
This preserves the existing profile-local
.env/.op.envbootstrap behavior and closes the review-identified extension and metadata boundaries:supports_isolated_environment = Trueand useget_source_environment()/ its mapping for bootstrap and child-process inputs;Why this route
_profile_runtime_scope(), because it can expose a profile’s bootstrap or resolved credentials to siblings.ContextVaralone as sufficient isolation. A third-party source could legally useos.environunder the prior publicSecretSource.fetch(cfg, home_path)contract.Behavior
.envand non-overriding.op.env.os.environ.Regression coverage
.op.envbootstrap, same-home fetch sharing, concurrency, reset/retry, and no-global-env-leak coverage.os.environis not invoked during isolated hydration and cannot observe a default-profile canary.Validation
Prior art / related work
Prepared with AI assistance; implementation and validation were reviewed against the current PR feedback and current source.