Add scoped and user secret providers with system key isolation#4229
Merged
Add scoped and user secret providers with system key isolation#4229
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4229 +/- ##
==========================================
+ Coverage 69.51% 69.52% +0.01%
==========================================
Files 485 487 +2
Lines 49814 50138 +324
==========================================
+ Hits 34627 34859 +232
- Misses 12503 12593 +90
- Partials 2684 2686 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Large PR Detected
This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.
How to unblock this PR:
Add a section to your PR description with the following format:
## Large PR Justification
[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformationAlternative:
Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.
See our Contributing Guidelines for more details.
This review will be automatically dismissed once you add the justification section.
This was referenced Mar 19, 2026
Introduces ScopedProvider and UserProvider wrappers that isolate system-managed secrets (registry tokens, workload auth, enterprise login) from user-managed secrets using a reserved __thv_<scope>_ key prefix. Also adds BulkDeleteSecrets to the Provider interface so Cleanup operations on both wrappers are handled atomically in a single write on EncryptedManager, consistent with how other lifecycle operations work across read-only providers (no-op) and writable providers. Part of #4192. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add t.Parallel() inside all t.Run closures and move ctx into each subtest to satisfy the paralleltest and tparallel linters. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
e52d21b to
e13fd99
Compare
Large PR justification has been provided. Thank you!
Contributor
|
✅ Large PR justification has been provided. The size review has been dismissed and this PR can now proceed with normal review. |
reyortiz3
reviewed
Mar 26, 2026
reyortiz3
reviewed
Mar 26, 2026
reyortiz3
reviewed
Mar 26, 2026
reyortiz3
reviewed
Mar 26, 2026
reyortiz3
reviewed
Mar 26, 2026
reyortiz3
reviewed
Mar 26, 2026
reyortiz3
reviewed
Mar 26, 2026
reyortiz3
reviewed
Mar 26, 2026
reyortiz3
reviewed
Mar 26, 2026
- Add SecretScope typed string for scope constants (ScopeRegistry, ScopeWorkloads, ScopeAuth) so callers get compile-time safety - Rename inner → provider in ScopedProvider and UserProvider structs - Replace package-level scopedKey() with getScopedKey()/getScopePrefix() methods on ScopedProvider for better encapsulation - Rename BulkDeleteSecrets → DeleteSecrets across all providers, tests, and mocks to better reflect the operation semantics - Include operation name (get/set/delete) in ErrReservedKeyName error messages for clearer diagnostics - Split TestEncryptedManager_BulkDeleteSecrets into four focused parallel tests (deletesSpecifiedKeys, persistsToDisk, emptyListIsNoop, nonExistentKeysNoError) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
aponcedeleonch
previously approved these changes
Mar 30, 2026
Member
aponcedeleonch
left a comment
There was a problem hiding this comment.
Nice foundational work — the two-wrapper approach (ScopedProvider for internal callers, UserProvider for user-facing callers) is clean and the test coverage is thorough. Approving with a few comments around documenting invariants, migration safety, and batch-delete semantics.
…ests - Add invariant doc comment to SecretScope explaining non-empty and no-underscore constraints with rationale - Add TestSecretScopeInvariants validating all declared scope constants - Clarify UserProvider.DeleteSecrets doc with all-or-nothing semantics - Add mixed-input test case for UserProvider.DeleteSecrets confirming inner provider is not called when any key in the list is reserved Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
reyortiz3
approved these changes
Mar 30, 2026
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
3 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.
Summary
ToolHive's secret store currently uses a flat keyspace shared between system-managed secrets (registry OAuth tokens, workload auth credentials) and user-managed secrets. This means system tokens are visible and modifiable via
thv secretcommands, and aCleanup()call from one context could inadvertently wipe secrets owned by another. This PR introduces the foundational isolation layer to fix that.ScopedProvider: wraps anyProviderand namespaces all operations under__thv_<scope>_<name>. Internal callers (registry auth, workload auth, enterprise login) will use this to keep their secrets isolated.UserProvider: wraps anyProviderand blocks access to system-reserved keys (__thv_*). All user-facing entry points (CLI, API, MCP tool server) will use this once callers are wired up (Phase 4, Scoped secret store: wire callers to use ScopedProvider and UserProvider #4227).SystemKeyPrefix = "__thv_", named scopes (ScopeRegistry,ScopeWorkloads,ScopeAuth), andErrReservedKeyNamefor consistent rejection of reserved key access.BulkDeleteSecretsto theProviderinterface so both wrappers'Cleanupmethods operate atomically — no-op on read-only providers (1Password, env), single write onEncryptedManager.Closes #4224
Implementation plan
This is Phase 1 of a 5-phase rollout tracked in #4192:
BulkDeleteSecrets--systemflag)Phases 3 and 4 must ship together — wiring callers before migration runs (or vice versa) would break existing users with secrets stored under bare keys.
Type of change
Test plan
task test)task lint-fix)Changes
pkg/secrets/scoped.goScopedProvider,UserProvider, constants,ErrReservedKeyNamepkg/secrets/scoped_test.gopkg/secrets/types.goBulkDeleteSecretstoProviderinterfacepkg/secrets/encrypted.goBulkDeleteSecrets(single lock, single write)pkg/secrets/environment.goBulkDeleteSecretsno-op (read-only)pkg/secrets/1password.goBulkDeleteSecretsno-op (read-only)pkg/secrets/fallback.goBulkDeleteSecretsdelegates to primarypkg/secrets/mocks/mock_provider.gopkg/environment/environment_test.goDoes this introduce a user-facing change?
No. The wrappers exist but no callers use them yet — that happens in Phase 4 (#4227).
Migration note (Phase 4): When
UserProvideris wired into user-facing code paths, any user whose secrets happen to be named with the__thv_prefix will receiveErrReservedKeyNameinstead of accessing them. This prefix was previously undocumented and unprotected, so real-world conflicts are unlikely, but Phase 4 release notes should explicitly call this out.Special notes for reviewers
The key prefix
__thv_<scope>_<name>(e.g.__thv_registry_mytoken) uses double-underscore and no slashes deliberately — 1Password treats/as a path separator which caused conflicts with an earlierthv_sys/<scope>/design. See the RFC (THV-0056) for the full discussion.BulkDeleteSecretsis included here rather than Phase 2 because bothCleanup()implementations onScopedProviderandUserProviderdepend on it. Adding it to the interface now avoids a follow-up that would touch every provider file again.Large PR Justification
This PR is flagged as large due to tests, but the production code change is well within limits (~265 lines across 6 files excluding tests and generated code). The size cannot be reduced further because:
BulkDeleteSecretsto theProviderinterface requires updating all implementations atomically — splitting across PRs would break the build mid-way.Generated with Claude Code