Implement OAuth 2.0 Token Exchange (RFC 8693) support #2082
Conversation
|
@claude review this PR. Make sure to review it from the security perspective and read RFC 8693 before the review. |
|
Claude finished @jhrozek's task —— View job OAuth 2.0 Token Exchange (RFC 8693) Security ReviewI've completed a comprehensive security review of this Token Exchange implementation. Here are my findings: Review Tasks:
🔒 Security Analysis SummaryOverall Assessment: SECURE ✅ 🟢 Security Strengths1. Credential Protection Excellence
2. Robust Input Validation
3. DoS Protection Measures
4. Error Handling Security
🟡 Areas for Security Consideration1. SSRF Protection Awareness
2. Transport Security
🟢 RFC 8693 Compliance AnalysisRequired Parameters ✅
Optional Parameters ✅
Response Handling ✅
🟢 Integration Assessmentoauth2.TokenSource Compatibility ✅
Design Patterns ✅
📋 Minor Enhancement Suggestions
🧪 Test Coverage AnalysisExcellent test coverage with 26 comprehensive test scenarios covering:
🚀 ConclusionThis is a high-quality, security-conscious implementation of RFC 8693. The code demonstrates:
Recommendation: APPROVE - This implementation meets enterprise security standards and is ready for production use. Job Run: Link |
There was a problem hiding this comment.
Pull Request Overview
This PR implements OAuth 2.0 Token Exchange (RFC 8693) support for ToolHive by adding a token exchange client library. The implementation allows exchanging incoming access tokens for backend-specific tokens when proxying MCP server requests, following RFC 8693 standards and being compatible with standard oauth2.TokenSource interfaces.
- Adds a comprehensive token exchange client library with Config, TokenSource implementation, and HTTP request handling
- Implements proper error handling for OAuth errors, network failures, and malformed responses
- Includes extensive test coverage for happy path, error scenarios, edge cases, and security considerations
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/auth/tokenexchange/exchange.go | Core implementation of OAuth 2.0 token exchange with Config, TokenSource, and HTTP handling |
| pkg/auth/tokenexchange/exchange_test.go | Comprehensive test suite covering success cases, error handling, and edge cases |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2082 +/- ##
==========================================
+ Coverage 48.38% 48.81% +0.43%
==========================================
Files 238 241 +3
Lines 30034 30496 +462
==========================================
+ Hits 14531 14888 +357
- Misses 14401 14492 +91
- Partials 1102 1116 +14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@claude review only the 2nd patch in this PR, the first one is unchanged |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Claude finished @jhrozek's task —— View job Review of 2nd Patch: Basic Auth ImplementationTask List:
🔒 Security Review: HTTP Basic Auth for Client CredentialsReviewing commit 📋 Summary of ChangesThis patch migrates client authentication from POST body parameters ( Key Changes:
✅ Security Improvements1. Credential Exposure Reduction ⭐
2. Support for Public ClientsThe implementation correctly handles public clients (line 389): if auth.ClientID != "" && auth.ClientSecret != "" {
req.SetBasicAuth(url.QueryEscape(auth.ClientID), url.QueryEscape(auth.ClientSecret))
}
3. Comprehensive Test CoverageNew tests added:
🔴 Critical Issue: Incorrect URL Encoding ImplementationLocation: Problem: The code URL-encodes credentials before passing to req.SetBasicAuth(url.QueryEscape(auth.ClientID), url.QueryEscape(auth.ClientSecret))RFC 6749 Section 2.3.1 Requirements:
The Issue:
Wait, Actually... Looking at the test expectations in assert.Equal(t, url.QueryEscape(specialClientID), username, "ClientID should be URL-encoded")
assert.Equal(t, url.QueryEscape(specialClientSecret), password, "ClientSecret should be URL-encoded")This test verifies that after ✅ Implementation is RFC 6749 CompliantAfter careful analysis, the implementation is correct:
This matches RFC 6749 Section 2.3.1 exactly. ✅ 🟡 Validation Inconsistency (Minor)Location: The if c.ClientID == "" {
return fmt.Errorf("ClientID is required")
}However, the implementation at line 389 supports public clients (empty ClientID/ClientSecret). Assessment:
Suggestion: Add a comment in the validation explaining this design choice: // ClientID is required for confidential clients. Public clients are not supported
// in this implementation as the target IDPs require client authentication.
if c.ClientID == "" {
return fmt.Errorf("ClientID is required")
}🟢 Test Coverage AnalysisExcellent test additions:
📊 RFC 6749 Compliance Summary
🚀 Security Assessment: APPROVED ✅Overall Rating: Secure and RFC-Compliant Strengths:
Minor Suggestions:
No security vulnerabilities identified in this patch. ✅ 📝 Code Quality Notes
Recommendation: APPROVE - This patch successfully implements RFC 6749-compliant Basic Authentication for client credentials with improved security characteristics. Branch: |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Claude finished @jhrozek's task —— View job Review of 2nd Commit: HTTP Basic Auth ImplementationTask List:
🔍 Review SummaryCommit: Overall Assessment: The migration to HTTP Basic Auth is a security improvement, but there is a critical RFC compliance issue that must be fixed. 🔴 Critical Issue: Missing URL Encoding for Client CredentialsLocation: The ProblemThe current implementation: req.SetBasicAuth(auth.ClientID, auth.ClientSecret)RFC 6749 Section 2.3.1 Requirements:
Go Standard Library Documentation (
Why This Matters
The FixRequired change: req.SetBasicAuth(url.QueryEscape(auth.ClientID), url.QueryEscape(auth.ClientSecret))Missing Test CoverageThe commit does not include a test for special characters in client credentials. You must add a test like: func TestExchangeToken_SpecialCharactersInCredentials(t *testing.T) {
specialClientID := "client:with@special/chars"
specialClientSecret := "secret&with%chars="
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
username, password, ok := r.BasicAuth()
require.True(t, ok)
// Verify credentials are URL-encoded
assert.Equal(t, url.QueryEscape(specialClientID), username)
assert.Equal(t, url.QueryEscape(specialClientSecret), password)
// Return success
// ...
}))
// ...
}✅ Security Improvements (Positive Changes)1. Credential Exposure Reduction ⭐Moving credentials from POST body to Authorization header is an excellent security improvement:
2. Clean MigrationThe implementation correctly:
3. Public Client SupportThe implementation properly handles public clients (line 389): if auth.ClientID != "" && auth.ClientSecret != "" {
req.SetBasicAuth(auth.ClientID, auth.ClientSecret)Only sets Basic Auth when both ClientID and ClientSecret are present, which is correct behavior. 🟡 Minor Observations1. Validation ConsistencyThe Recommendation: Add a documentation comment explaining this: // ClientID is required. This implementation only supports confidential clients
// as required by ToolHive's target identity providers.
if c.ClientID == "" {
return fmt.Errorf("ClientID is required")
}2. Comment AccuracyThe comment at line 388 states:
This is partially incorrect. Recommended comment: // Add client authentication via HTTP Basic Auth per RFC 6749 Section 2.3.1
// Per RFC 6749, credentials must be URL-encoded before base64 encoding.
// SetBasicAuth handles the base64 encoding step.
if auth.ClientID != "" && auth.ClientSecret != "" {
req.SetBasicAuth(url.QueryEscape(auth.ClientID), url.QueryEscape(auth.ClientSecret))
}📋 Test Coverage AnalysisGood test additions:
Missing critical test:
🚀 Action ItemsRequired (Blocker)
Optional (Enhancement)
📊 Copilot Review Comments AssessmentCopilot's review comments were correct:
Note: There appears to be confusion in Copilot's latest review. The test expectations ARE correct - after BasicAuth() decodes the base64, the credentials should still be URL-encoded (which the server then URL-decodes). However, the implementation is missing the URL encoding step. 🎯 Final AssessmentStatus: CHANGES REQUIRED Rationale:
Recommendation: REQUEST CHANGES - Fix URL encoding issue and add test coverage before merging. Branch: |
|
The AI reviews are funny when 2 LLMs disagree. But looking at the RFC, Claude was right and Copilot wrong. @claude one more review please. |
|
Claude finished @jhrozek's task —— View job Final Review of OAuth 2.0 Token Exchange ImplementationTask List:
✅ Final Security Assessment: APPROVEDYou were absolutely correct - Claude's previous review was right and Copilot's suggestions were incorrect. The implementation now properly follows RFC 6749 Section 2.3.1. 🟢 RFC 6749 Section 2.3.1 Compliance: CONFIRMEDLocation: The implementation correctly URL-encodes client credentials before passing them to req.SetBasicAuth(url.QueryEscape(auth.ClientID), url.QueryEscape(auth.ClientSecret))Why This is Correct:
🟢 Excellent Test CoverageLocation: The specialClientID := "client:with@special/chars"
specialClientSecret := "secret&with=special%chars"Test validates:
Why the test expectations are correct (lines 1277-1278): assert.Equal(t, url.QueryEscape(specialClientID), username, "ClientID should be URL-encoded")
assert.Equal(t, url.QueryEscape(specialClientSecret), password, "ClientSecret should be URL-encoded")After 🎯 Why Copilot Was WrongCopilot's incorrect suggestion:
Why this is wrong:
The confusion: 🔒 Security Strengths (from previous reviews still apply)
📊 Complete Test SuiteThe PR includes 30 comprehensive test scenarios covering:
🚀 Final VerdictStatus: ✅ APPROVED - RFC COMPLIANT Key Findings:
Recommendation: This implementation is production-ready and should be merged. It correctly implements OAuth 2.0 Token Exchange (RFC 8693) with proper client authentication per RFC 6749 Section 2.3.1. Additional Notes:
Branch: |
Add a token exchange client library that enables ToolHive to exchange incoming access tokens for backend-specific tokens when proxying MCP server requests. The implementation follows RFC 8693 and is compatible with standard oauth2.TokenSource interfaces. Fixes #2064
Change token exchange client authentication from body parameters (client_id/client_secret in POST body) to HTTP Basic Authentication via Authorization header, following RFC 6749 Section 2.3.1 recommendations. This reduces credential exposure risk in HTTP logs and monitoring systems, as the Authorization header receives special handling throughout the HTTP stack while body parameters are commonly logged.
|
I'm sorry one more push, I added the extra checks but some test cases were returning mocked tokens which were not passing the checks |
Add a token exchange client library that enables ToolHive to exchange incoming access tokens for backend-specific tokens when proxying MCP server requests. The implementation follows RFC 8693 and is compatible with standard oauth2.TokenSource interfaces.
Fixes #2064