Add middleware to swap the downstream ticket for the upstream ticket #2113
Conversation
|
@claude review with focus on security and with taking the patterns other middlewares, especially the auth one taks |
|
Claude finished @jhrozek's task —— View job Code Review - Security & Middleware PatternsI've completed a thorough review of the token exchange middleware implementation focusing on security concerns and consistency with existing middleware patterns. ✅ StrengthsExcellent security practices:
Strong middleware pattern consistency:
Comprehensive test coverage:
🔒 Security Concerns1. Missing Token Source Validation (High Priority)Location: The middleware doesn't validate that the subject token actually came from the auth middleware. An attacker could potentially bypass authentication by directly sending a request with a Bearer token to an endpoint configured with token exchange but without auth middleware. Current flow: // Line 155: Gets claims from context
claims, ok := r.Context().Value(auth.ClaimsContextKey{}).(jwt.MapClaims)
if !ok {
logger.Debug("No claims found in context, proceeding without token exchange")
next.ServeHTTP(w, r) // ⚠️ Proceeds even without claims
return
}
// Line 162: Extracts token from Authorization header
authHeader := r.Header.Get("Authorization")Problem: If there are no claims (meaning auth middleware didn't run or failed), the middleware still proceeds to the next handler. This creates a potential security gap if token exchange middleware is accidentally configured without auth middleware. Recommendation:
type Config struct {
// ... existing fields ...
// RequireClaims determines behavior when claims are not found
// If true (recommended default), returns 401 when claims are missing
// If false, proceeds without token exchange (current behavior)
RequireClaims bool `json:"require_claims,omitempty"`
}
claims, ok := r.Context().Value(auth.ClaimsContextKey{}).(jwt.MapClaims)
if !ok {
if config.RequireClaims {
logger.Warn("Token exchange middleware requires authentication claims but none found")
http.Error(w, "Authentication required", http.StatusUnauthorized)
return
}
logger.Debug("No claims found in context, proceeding without token exchange")
next.ServeHTTP(w, r)
return
}
2. Token Correlation Gap (Medium Priority)Location: The middleware extracts the token from the Authorization header separately from getting claims from context. There's no verification that the token in the header matches the token that was validated by the auth middleware. Scenario: If an attacker can manipulate the Authorization header after auth validation but before token exchange (unlikely but worth considering in complex proxy scenarios), they could potentially exchange a different token than the one that was validated. Comparison with auth middleware: The auth middleware at Recommendation: Option A: Store the original token in context during auth middleware: // In auth middleware (token.go):
type TokenContextKey struct{}
ctx = context.WithValue(ctx, TokenContextKey{}, tokenString)
ctx = context.WithValue(ctx, ClaimsContextKey{}, claims)
// In tokenexchange middleware:
subjectToken, ok := r.Context().Value(auth.TokenContextKey{}).(string)
if !ok || subjectToken == "" {
// Token must come from context, not header
http.Error(w, "Invalid authentication state", http.StatusInternalServerError)
return
}Option B: Add a hash/signature to claims to verify token hasn't changed: // In auth middleware, add token hash to claims:
tokenHash := sha256.Sum256([]byte(tokenString))
claims["_token_hash"] = hex.EncodeToString(tokenHash[:])
// In tokenexchange middleware, verify:
headerToken := strings.TrimPrefix(authHeader, "Bearer ")
tokenHash := sha256.Sum256([]byte(headerToken))
if claims["_token_hash"] != hex.EncodeToString(tokenHash[:]) {
http.Error(w, "Token mismatch", http.StatusUnauthorized)
return
}Option A is cleaner and more secure. 3. Custom Header Name Validation (Low Priority)Location: The custom header name is only validated for emptiness, not for security issues: if config.HeaderStrategy == HeaderStrategyCustom && config.ExternalTokenHeaderName == "" {
return fmt.Errorf("external_token_header_name must be specified...")
}Concern: Malicious or misconfigured header names could:
Recommendation: func validateHeaderName(name string) error {
// Check length
if len(name) > 256 {
return fmt.Errorf("header name too long (max 256 characters)")
}
// Check for protected headers (case-insensitive)
protected := []string{"Host", "Content-Type", "Content-Length", "Authorization"}
for _, p := range protected {
if strings.EqualFold(name, p) {
return fmt.Errorf("cannot use protected header: %s", name)
}
}
// Check for valid header name characters (RFC 7230)
for _, c := range name {
if !isValidHeaderChar(c) {
return fmt.Errorf("invalid character in header name")
}
}
return nil
}📋 Pattern Consistency Issues4. Error Response Pattern Differs from Auth MiddlewareLocation: Token exchange middleware: if err != nil {
logger.Warnf("Token exchange failed: %v", err)
http.Error(w, "Token exchange failed", http.StatusUnauthorized)
return
}Auth middleware pattern ( if err != nil {
w.Header().Set("WWW-Authenticate", v.buildWWWAuthenticate(true, err.Error()))
http.Error(w, fmt.Sprintf("Invalid token: %v", err), http.StatusUnauthorized)
return
}Observations:
Recommendation:
if err != nil {
logger.Warnf("Token exchange failed for subject %v: %v", sub, err)
// Use 403 instead of 401 - user is authenticated but exchange failed
http.Error(w, "Token exchange failed", http.StatusForbidden)
return
}5. Missing Context TimeoutLocation: The token exchange operation could potentially hang indefinitely if the OAuth server is slow/unresponsive. Auth middleware comparison: The auth middleware has timeout handling in token validator (token.go:557) with a 5-second timeout for JWKS registration. Token exchange: Uses the default HTTP client timeout (30 seconds - exchange.go:31) but doesn't have per-request timeout control. Recommendation: // Create context with timeout for token exchange
exchangeCtx, cancel := context.WithTimeout(r.Context(), 10*time.Second)
defer cancel()
// Get token from token source with timeout context
tokenSource := exchangeConfig.TokenSource(exchangeCtx)
exchangedToken, err := tokenSource.Token()🔍 Minor Issues6. Inconsistent Logger UsageLocation: if sub, exists := claims["sub"]; exists {
logger.Debugf("Performing token exchange for subject: %v", sub)
}Issue: This logs the subject identifier, which could be considered PII in some contexts. The auth middleware is more careful about what it logs. Recommendation: Consider making this configurable or use a hash: if sub, exists := claims["sub"]; exists {
// Hash the subject for logging to avoid PII concerns
subHash := sha256.Sum256([]byte(fmt.Sprint(sub)))
logger.Debugf("Performing token exchange for subject hash: %x", subHash[:8])
}7. Test Coverage for Concurrent RequestsThe test suite is excellent but doesn't include tests for concurrent middleware execution. Token exchange could be CPU/memory intensive under load. Recommendation: Add a test: func TestCreateTokenExchangeMiddlewareFromClaims_Concurrent(t *testing.T) {
// Test that middleware handles concurrent requests safely
// Verify no race conditions in token exchange
}8. Missing Integration Test with Auth MiddlewareThere are no tests that verify the token exchange middleware works correctly when chained after the auth middleware. Recommendation: Add an integration test in func TestTokenExchangeWithAuthMiddleware_Integration(t *testing.T) {
// Test: auth middleware -> token exchange middleware -> handler
// Verify claims flow correctly through the chain
// Verify token is properly exchanged
}✨ Positive Observations
📝 Summary & Recommendations PriorityMust Fix (Security):
Should Fix (Security & Consistency): Nice to Have: Additional NotesThe code is well-written and follows good security practices overall. The main concerns are around the interaction between auth and token exchange middlewares, which should be tightened to prevent misconfiguration vulnerabilities. The pattern consistency with auth middleware is generally good, though error handling could be more aligned. Great work on the comprehensive test coverage and RFC compliance! |
There was a problem hiding this comment.
Pull Request Overview
This pull request adds HTTP middleware for OAuth 2.0 token exchange (RFC 8693) that automatically replaces downstream authentication tokens with backend-specific tokens. The middleware intercepts authenticated requests and exchanges their tokens using configurable injection strategies.
Key changes:
- Implements token exchange middleware with configurable header injection strategies
- Adds comprehensive test coverage for configuration validation, token injection, and error handling
- Updates existing code to rename
ConfigtoExchangeConfigto avoid naming conflicts
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/auth/tokenexchange/middleware.go | Core middleware implementation with configuration validation and token injection logic |
| pkg/auth/tokenexchange/middleware_test.go | Comprehensive test suite covering all middleware functionality and edge cases |
| pkg/auth/tokenexchange/exchange.go | Renames Config struct to ExchangeConfig to avoid conflicts with middleware config |
| pkg/auth/tokenexchange/exchange_test.go | Updates test references to use renamed ExchangeConfig struct |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
I will address the comments tomorrow, but I like the suggestions about making sure the token came through the auth middleware and throwing a 403. The rest seems bogus or not needed. |
Implements HTTP middleware that automatically exchanges downstream authentication tokens for backend-specific tokens using RFC 8693 OAuth 2.0 Token Exchange. The middleware extracts subject tokens from authenticated requests and replaces them with exchanged tokens, supporting two injection strategies: replacing the Authorization header or adding a custom header while preserving the original. Fixes: #2065
…middleware handler
…enExchangeMiddlewareFromClaims
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2113 +/- ##
==========================================
+ Coverage 48.71% 48.87% +0.16%
==========================================
Files 241 242 +1
Lines 30592 30714 +122
==========================================
+ Hits 14902 15013 +111
- Misses 14567 14575 +8
- Partials 1123 1126 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
I will post them as separate patches, but will keep moving with this PR if you don't mind. |
Actually no you're right @blkt I took the suggestion. |
Implements HTTP middleware that automatically exchanges downstream authentication tokens for backend-specific tokens using RFC 8693 OAuth 2.0 Token Exchange.
The middleware extracts subject tokens from authenticated requests and replaces them with exchanged tokens, supporting two injection strategies: replacing the Authorization header or adding a custom header while preserving the original.
Fixes: #2065