Wire all secret callers to scoped and user providers#4343
Closed
Wire all secret callers to scoped and user providers#4343
Conversation
amirejaz
requested review from
ChrisJBurns,
JAORMX,
blkt,
jhrozek,
lujunsan,
rdimitrov and
yrobla
as code owners
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4343 +/- ##
==========================================
- Coverage 69.64% 69.63% -0.02%
==========================================
Files 491 493 +2
Lines 50304 50388 +84
==========================================
+ Hits 35036 35088 +52
- Misses 12580 12616 +36
+ Partials 2688 2684 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
4 tasks
amirejaz
force-pushed
the
scoped-secret-providers
branch
from
e52d21b to
e13fd99
Compare
amirejaz
force-pushed
the
phase4-migration
branch
from
bfbee20 to
1fd9f57
Compare
github-actions
bot
added
size/M
amirejaz
force-pushed
the
phase4-migration
branch
from
1fd9f57 to
1d6f2dc
Compare
github-actions
bot
added
size/L
amirejaz
force-pushed
the
phase4-migration
branch
from
1d6f2dc to
67d36b9
Compare
github-actions
bot
added
size/L
reyortiz3
reviewed
Mar 26, 2026
reyortiz3
reviewed
Mar 26, 2026
reyortiz3
reviewed
Mar 26, 2026
reyortiz3
reviewed
Mar 26, 2026
amirejaz
force-pushed
the
phase4-migration
branch
from
67d36b9 to
4f55374
Compare
github-actions
bot
added
size/L
amirejaz
force-pushed
the
phase4-migration
branch
from
4f55374 to
ec07698
Compare
github-actions
bot
added
size/L
amirejaz
force-pushed
the
phase4-migration
branch
from
e1cbda0 to
5ec8b68
Compare
github-actions
bot
added
size/L
amirejaz
force-pushed
the
phase4-migration
branch
from
5ec8b68 to
f9d6728
Compare
github-actions
bot
added
size/L
When a user upgrades ToolHive, system secrets may still exist under bare keys (e.g. BEARER_TOKEN_foo) until the secret scope migration completes. If migration fails or hasn't run yet, ScopedProvider callers would be unable to find their secrets under the new scoped key (__thv_workloads_BEARER_TOKEN_foo), breaking workload auth. Add a transparent fallback in ScopedProvider.GetSecret: on a not-found response for the scoped key, also try the bare (pre- migration) key. Once migration completes and bare keys are deleted, the fallback finds nothing and becomes a natural no-op — no config check or injection needed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add t.Parallel() to TestScopedProvider_GetSecret_MigrationFallback and its subtests to satisfy the paralleltest linter requirement. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Update every call site that creates a secrets provider to use the appropriate wrapper introduced in Phase 1: - System callers (workload auth tokens, registry credentials, build auth files) now use CreateScopedSecretProvider, placing secrets under the __thv_<scope>_ prefix and hiding them from user-facing commands. - User-facing callers (thv secret commands, REST API, MCP tool server, header secrets, build-env-from-secrets) now use CreateUserSecretProvider, blocking access to __thv_* reserved keys. - RunConfig.WithSecrets and ValidateSecrets now accept separate system and user providers so auth-token resolution and --secret flag resolution use the correct scope independently. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Verify that the UserProvider wiring in CLI secret commands correctly blocks access to __thv_* keys: - set with __thv_ prefix is rejected - get with __thv_ prefix is rejected with "reserved for system use" - delete with __thv_ prefix is rejected - two distinct __thv_ key names are blocked, confirming the check is not tied to a single hardcoded name Each test uses an isolated XDG_CONFIG_HOME/HOME temp directory so the user's real secrets store is never touched. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add GetUserSecretsProvider() to pkg/auth/secrets alongside the existing GetSecretsManager(), centralising the config-read + CreateUserSecretProvider boilerplate in one place - Remove getScopedWorkloadsSecretsManager() from config_buildauthfile.go; it was identical to authsecrets.GetSecretsManager() — use that directly - Replace inline boilerplate in config_buildenv.go/validateSecretExists and secret.go/getSecretsManager with calls to GetUserSecretsProvider(), eliminating duplicated setup-check + provider-create blocks - Rename local variable manager → provider in GetSecretsManager() to match the naming convention used throughout the new provider code Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Existing users may have registry tokens and workload auth secrets stored under bare keys (BEARER_TOKEN_, OAUTH_CLIENT_SECRET_, REGISTRY_OAUTH_, etc.) that pre-date the scoped provider wrappers. This migration renames them into the __thv_<scope>_ namespace on first startup so they are accessible via the new scoped providers and hidden from user-facing secret commands. Key design properties: - Write-before-delete ordering: the new key is written before the old is deleted, so a crash mid-migration leaves the secret reachable. - Idempotent: a missing old key is silently skipped, making retries safe. - One-shot: guarded by the SecretScopeMigration config flag; once set, the migration is a cheap config read and returns immediately. - Discovery-based: DiscoverMigrations lists all secrets and matches known system prefixes, so no static registry of workload names is required. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Follow-up from the SecretScope type change: update SystemKeyPrefixMappings Scope field from string to SecretScope, and replace the removed package-level scopedKey() function with inline key construction. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Rename local variable migrations → keyMigrations in DiscoverMigrations for clarity (distinguishes the slice from the function parameter name used in MigrateSystemKeys) - Replace inline strings.HasPrefix(key, SystemKeyPrefix) with isSystemKey(key) to reuse the existing helper and keep the intent explicit Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace CreateUserSecretProvider and CreateScopedSecretProvider with the new CreateProvider(managerType, WithUserFacing()) and CreateProvider(managerType, WithScope(scope)) API introduced in the phase2-factory-helpers PR. Affected callers: - cmd/thv/app/registry_login.go - cmd/thv/app/header_flags.go - pkg/auth/secrets/secrets.go - pkg/runner/env.go - pkg/runner/runner.go - pkg/runner/protocol.go - pkg/mcp/server/set_secret.go - pkg/mcp/server/list_secrets.go - pkg/workloads/manager.go - pkg/api/v1/secrets.go Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
amirejaz
force-pushed
the
phase4-migration
branch
from
5882adb to
500e0bd
Compare
github-actions
bot
added
size/L
Contributor
Author
|
Closing in favor of a new PR from the correct head branch (phase3-wire-callers). The new PR will show only wire callers changes without including migration commits. |
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.
Summary
CreateSecretProvider, giving every caller access to the full flat keyspace. This PR wires each caller to the appropriate wrapper introduced in Phase 1 (Add scoped and user secret providers with system key isolation #4229), so secrets are isolated by design rather than by convention.CreateScopedSecretProvider, writing and reading under the__thv_<scope>_prefix. These keys are invisible to user-facing secret commands.thv secretcommands, REST API, MCP tool server, header secrets, build-env-from-secrets) now useCreateUserSecretProvider, which blocks access to any__thv_*reserved key.RunConfig.WithSecretsandValidateSecretsnow take separatesystemProvideranduserProviderarguments so auth-token resolution and--secretflag resolution use the correct scope independently.This is Phase 3 of the scoped secret store (part of #4192), tracking issue #4227. It must be released together with Phase 4 (#4244 — migration infrastructure).
Type of change
Test plan
go build ./...— clean buildgo test ./pkg/runner/... ./pkg/auth/secrets/... ./pkg/workloads/... ./pkg/mcp/server/...— all passgolangci-lint run— 0 issuesSpecial notes for reviewers
The
WithSecretssignature change (one provider → two providers) is the most structurally significant change. The split is:systemProvider— used forRemoteAuthConfig.ClientSecret,RemoteAuthConfig.BearerToken(both written by the workload auth subsystem underScopeWorkloads)userProvider— used forc.Secrets(--secretflags) andHeaderForward.AddHeadersFromSecret(both user-specified key names)config_buildauthfile.gogets its owngetScopedWorkloadsSecretsManager()helper rather than reusing the user-facinggetSecretsManager(), since build auth files are system-owned (stored bythv config buildauthfile set, not bythv secret set).Generated with Claude Code