Fix logout implementation guide to preserve auth config#49
Merged
ChrisJBurns merged 8 commits intomainfrom Mar 5, 2026
Merged
Conversation
Add RFC for OAuth/OIDC authentication support when accessing remote MCP server registries. Phase 1 covers browser-based OAuth with PKCE, Phase 2 covers bearer tokens for CI/CD environments. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix sequence diagram: auth.Transport calls Token(), not Registry - Resolve callback port question: port 8666 shared intentionally (registry auth and remote MCP auth never run simultaneously) - Document graceful degradation when secrets manager isn't set up - Document get-registry output change (OAuth configured/authenticated) - Clarify callback server timeout behavior in security mitigations - Remove Testing Strategy section (tests pending in implementation) - Remove ClientSecret field (not in scope for Phase 1) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Changes based on review by OAuth/OIDC, ToolHive architecture, MCP protocol, and Security expert panel: OAuth/OIDC: - Remove use_pkce toggle — PKCE with S256 is mandatory, not configurable - Clarify audience vs RFC 8707 resource terminology - Document state parameter crypto/rand generation - Document refresh token rotation handling - Enforce HTTPS for issuer URL (localhost exception) ToolHive Architecture: - Document thv serve limitation (browser flow incompatible with headless) - Document RemoteRegistryProvider auth exclusion as explicit limitation - Add actionable 401/403 error messages with remediation commands MCP Protocol: - Add MCP spec alignment note (registry-level vs server-level auth) - Derive secret keys from registry URL hash to prevent token clobbering - Add Alternative 6: RFC 9728 auto-discovery (deferred) Security: - Specify config file 0600 permissions (programmatic enforcement) - Explicit 127.0.0.1 binding for callback server - Add config file exposure to threat model - Clarify --allow-private-ip scope and risks - Specify 120-second browser flow timeout - Add delimiter to hash input to prevent concatenation ambiguity - Add Future Considerations section (token revocation, ephemeral ports) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Incorporate feedback from @reyortiz3, @JAORMX, and @peppescg: - Switch from custom TokenSource interface to oauth2.TokenSource from golang.org/x/oauth2, aligning with the existing auth infrastructure and eliminating the need for adapter code - Use standard oauth2.Transport instead of custom RoundTripper - Clarify pkg/registry/auth/ separation rationale (thin orchestration layer composing from pkg/auth/ primitives, not re-implementing them) - Add thv registry login/logout commands for explicit authentication - Document MDM/enterprise pre-loaded config support - Add auth status API (auth_status, auth_type fields) to thv serve registry endpoint for ToolHive Studio integration - Add structured registry_auth_required JSON error from thv serve - Add Studio-initiated async auth flow to Future Considerations - Add config validation at load time to Future Considerations Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Implementation Guide (for agent-based implementation): - Constructor signatures for NewOAuthTokenSource - Token() method orchestration pseudocode composing existing primitives - Interactive flag threading through factory → provider → token source - Secrets provider optional handling and graceful degradation - oauth2.Transport wiring into HTTP client chain - thv registry login/logout Cobra command structure - API integration: auth status fields, structured 503 errors, 401 detection, getCurrentProvider integration - Security constraints: secret key derivation (h[:4]), sentinel error, token logging safety rules Address @jhrozek review feedback: - Add issuer-binding validation per OIDC Discovery §4.3 (MITM protection) - Add OIDC discovery hijack to threat model and mitigations table - Add per-IdP audience configuration table (Auth0, Okta, Azure AD, Keycloak) - Clarify nonce not needed (ID tokens not consumed, only access/refresh) - Fix logout to clear tokens only, not auth config (standard convention) - Add thv registry login naming rationale (multiple auth contexts) - Strengthen ephemeral ports note (Okta exact-match incompatibility) - Add registry-served discovery document future consideration - Add thv login alias future consideration - Expand Studio POST /initiate with API contract and security controls Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Address @jhrozek review nit: the implementation guide's logout code was still nuking the entire registry_auth config (c.RegistryAuth = nil) instead of only clearing the token reference. Updated to: - Clear only CachedRefreshTokenRef (not the full auth config) - Update Cobra Long description to match - Print message directing user to 'thv registry login' to re-authenticate Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jhrozek
approved these changes
Mar 5, 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
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.
Addresses @jhrozek's review comment:
Changes:
registryLogoutCmdFuncin the implementation guide to clear onlyCachedRefreshTokenRefinstead of settingc.RegistryAuth = nilLongdescription to match the corrected behaviorthv registry loginto re-authenticateThis aligns the implementation guide with the design text (which was already fixed in the previous commit to say logout preserves auth config).