Skip to content

Commit 2a1664a

Browse files
amirejazCopilot
andauthored
RFC: Client ID Metadata Document (CIMD) support (#71)
* Add RFC for Client ID Metadata Document (CIMD) support Proposes CIMD support on both sides of the ToolHive proxy: as an OAuth client preferring CIMD over DCR when connecting to remote MCP servers (thv run), and as an authorization server accepting HTTPS URLs as client_id from MCP clients (embedded AS). Closes stacklok/toolhive#2728 Related: stacklok/toolhive#4825, stacklok/toolhive#4826 Signed-off-by: amirejaz <amir@stacklok.com> * Rename RFC to THV-0071 to match PR number Signed-off-by: amirejaz <amir@stacklok.com> * Update rfcs/THV-0071-cimd-support.md Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Address Copilot review comments - Fix spelling: defence → defense (two occurrences) - Align config naming: CIMDEnabled → authServer.cimd.enabled - Resolve redirect/non-200 contradiction in Input Validation - Add IPv6 ULA (fc00::/7) to SSRF blocked ranges; frame as globally routable IPs only for future-proofing Signed-off-by: amirejaz <amir@stacklok.com> * Fix redirect URI: use correct default callback port 8666 Remove inaccurate port-agnostic http://localhost/callback entry. The actual default callback port is 8666, defined as DefaultCallbackPort in pkg/auth/remote/config.go and configurable via --remote-auth-callback-port. Signed-off-by: amirejaz <amir@stacklok.com> * Address specialist review findings Blocker: - Explicitly list ClientIDMetadataDocumentSupported field addition in Phase 1 task list; note both RFC 8414 and OIDC discovery must be parsed High severity: - Add PKCE S256 requirement for CIMD flows (mandatory per MCP 2025-11-25) - Expand SSRF protection: use net.IP.IsGlobalUnicast() as allowlist baseline; add 100.64.0.0/10, IPv4-mapped IPv6, documentation ranges - Add DisableKeepAlives and dual-stack DialContext requirements to SSRF transport to prevent keep-alive and Happy Eyeballs bypasses - Change self-referential binding from strict string comparison to RFC 3986 canonical normalization (lowercase host/scheme, strip default ports) - Expand DCR fallback trigger: invalid_client, unauthorized_client, invalid_request, raw HTTP 400/401 at authorization stage only - Add cache floor (60s), ceiling (24h), and no-store handling - Document refresh token behavior: document not re-fetched on refresh - Document air-gapped limitation for Phase 2 self-fetch scenario - Add Content-Type validation before JSON parsing Low/informational: - Note SHOULD vs MUST normative strength of priority chain in Summary - Add LRU cache flooding threat to threat model - Add compromised hosting as accepted residual risk in threat model - Note IETF draft-01 version in References Signed-off-by: amirejaz <amir@stacklok.com> * Address jhrozek review feedback - Move cimd.go to pkg/oauth (protocol-level, not auth-specific) - Replace new ssrf.go with enhancement to existing pkg/networking HttpClientBuilder; CIMD fetches reuse existing SSRF infrastructure with missing ranges added (100.64.0.0/10, IPv4-mapped, doc ranges) - Clarify CIMD carries no credentials; rejection = AS compatibility issue, not expiry - Note logo_uri is a URL reference, not inline data; 10 KB cap is for the JSON document only - Note custom metadata URL override as a follow-up enhancement - Clarify opt-in applies to Phase 2 only (outbound fetch gating); Phase 1 is automatic with no config flag Signed-off-by: amirejaz <amir@stacklok.com> * Address jhrozek second-round review comments - Revert to strict string comparison for self-referential binding (spec-mandated; full IDN normalization required to be safe, adds complexity for a case well-implemented clients never produce) - Fix PerformOAuthFlow sketch: set config.ClientID before the DCR gate (shouldDynamicallyRegisterClient checks ClientID == empty) - Narrow DCR fallback: remove invalid_request (request-shape error, not identity rejection; keeping it could mask errors or enable forced downgrade to DCR) - Expand decorator: embed full storage.Storage (~15 methods), add singleflight.Group to deduplicate concurrent fetches on anonymous /authorize, expose Unwrap() for legacy RedisStorage type assertion - Document second GetClient call site in callback.go; wired via storage embedding, no extra work needed - Document RedisStorage type assertion landmine in server_impl.go; must run migration before wrapping or use Unwrap() - Expand Phase 2 implementation plan with all files touched by config threading (RunConfig -> Config -> handler -> decorator) - Fix AuthorizationServerMetadata field location: pkg/oauth/discovery.go (shared package), not the authserver handler - Fix SSRF threat row to match expanded pkg/networking IP ranges - Narrow Phase 1 test description to match updated fallback conditions - Update Last Updated date Signed-off-by: amirejaz <amir@stacklok.com> --------- Signed-off-by: amirejaz <amir@stacklok.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
1 parent 8c1292d commit 2a1664a

1 file changed

Lines changed: 499 additions & 0 deletions

File tree

0 commit comments

Comments
 (0)