diff --git a/pkg/auth/tokenexchange/exchange.go b/pkg/auth/tokenexchange/exchange.go index b6a8e3fb68..75c422863b 100644 --- a/pkg/auth/tokenexchange/exchange.go +++ b/pkg/auth/tokenexchange/exchange.go @@ -21,19 +21,13 @@ import ( "github.com/stacklok/toolhive/pkg/oauthproto" ) -const ( - // defaultHTTPTimeout is the timeout for HTTP requests - defaultHTTPTimeout = 30 * time.Second - - // maxResponseBodySize is the maximum size for reading response bodies (1 MB) - maxResponseBodySize = 1 << 20 - - // redactedPlaceholder is used to redact sensitive values in string representations - redactedPlaceholder = "[REDACTED]" - - // emptyPlaceholder is used to indicate empty/missing values in string representations - emptyPlaceholder = "" -) +// maxResponseBodySize bounds io.LimitReader in executeTokenExchangeRequest so +// a pathological server cannot exhaust memory. The shared pkg/oauthproto +// package has an identical unexported constant, but we cannot import it yet — +// the shared one is consumed by oauthproto.DoTokenRequest, which will replace +// executeTokenExchangeRequest in a follow-up commit. +// TODO: drop when executeTokenExchangeRequest is replaced by oauthproto.DoTokenRequest. +const maxResponseBodySize = 1 << 20 // NormalizeTokenType converts a short token type name to its full URN. // Accepts both short forms ("access_token", "id_token", "jwt") and full URNs. @@ -66,11 +60,6 @@ func NormalizeTokenType(tokenType string) (string, error) { } } -// defaultHTTPClient is the default HTTP client used for token exchange requests. -var defaultHTTPClient = &http.Client{ - Timeout: defaultHTTPTimeout, -} - // actingParty represents the acting party in a token exchange delegation scenario. // When present, it indicates that the actor token holder is acting on behalf of the subject token holder. type actingParty struct { @@ -96,64 +85,41 @@ type exchangeRequest struct { // String implements fmt.Stringer for exchangeRequest, redacting sensitive tokens. func (r exchangeRequest) String() string { - subjectToken := redactedPlaceholder - if r.SubjectToken == "" { - subjectToken = emptyPlaceholder - } - actorToken := "" if r.ActingParty != nil { - actorToken = redactedPlaceholder - if r.ActingParty.ActorToken == "" { - actorToken = emptyPlaceholder - } + actorToken = oauthproto.Redact(r.ActingParty.ActorToken) } return fmt.Sprintf("exchangeRequest{GrantType: %s, Audience: %s, Resource: %s, Scope: %v, SubjectToken: %s, ActorToken: %s}", - r.GrantType, r.Audience, r.Resource, r.Scope, subjectToken, actorToken) + r.GrantType, r.Audience, r.Resource, r.Scope, oauthproto.Redact(r.SubjectToken), actorToken) } // response is used to decode the remote server response during an OAuth 2.0 token exchange. type response struct { - AccessToken string `json:"access_token"` //nolint:gosec // G117: field legitimately holds sensitive data + AccessToken string `json:"access_token"` IssuedTokenType string `json:"issued_token_type"` TokenType string `json:"token_type"` ExpiresIn int `json:"expires_in"` Scope string `json:"scope"` - RefreshToken string `json:"refresh_token"` //nolint:gosec // G117: field legitimately holds sensitive data + RefreshToken string `json:"refresh_token"` } // String implements fmt.Stringer for response, redacting sensitive tokens. func (r response) String() string { - accessToken := redactedPlaceholder - if r.AccessToken == "" { - accessToken = emptyPlaceholder - } - - refreshToken := redactedPlaceholder - if r.RefreshToken == "" { - refreshToken = emptyPlaceholder - } - return fmt.Sprintf("response{AccessToken: %s, TokenType: %s, ExpiresIn: %d, RefreshToken: %s}", - accessToken, r.TokenType, r.ExpiresIn, refreshToken) + oauthproto.Redact(r.AccessToken), r.TokenType, r.ExpiresIn, oauthproto.Redact(r.RefreshToken)) } // clientAuthentication represents OAuth client credentials for token exchange. type clientAuthentication struct { ClientID string - ClientSecret string //nolint:gosec // G117 + ClientSecret string } // String implements fmt.Stringer for clientAuthentication, redacting the client secret. func (c clientAuthentication) String() string { - clientSecret := redactedPlaceholder - if c.ClientSecret == "" { - clientSecret = emptyPlaceholder - } - return fmt.Sprintf("clientAuthentication{ClientID: %s, ClientSecret: %s}", - c.ClientID, clientSecret) + c.ClientID, oauthproto.Redact(c.ClientSecret)) } // ExchangeConfig holds the configuration for token exchange. @@ -165,7 +131,7 @@ type ExchangeConfig struct { ClientID string // ClientSecret is the OAuth 2.0 client secret - ClientSecret string //nolint:gosec // G117 + ClientSecret string // Audience is the target audience for the exchanged token (optional per RFC 8693) Audience string @@ -195,7 +161,7 @@ type ExchangeConfig struct { SubjectTokenProvider func() (string, error) // HTTPClient is the HTTP client to use for token exchange requests. - // If nil, defaultHTTPClient will be used. + // If nil, oauthproto.DefaultHTTPClient() will be used. HTTPClient *http.Client } @@ -358,7 +324,7 @@ func exchangeToken( } if client == nil { - client = defaultHTTPClient + client = oauthproto.DefaultHTTPClient() } body, err := executeTokenExchangeRequest(client, req) @@ -457,14 +423,17 @@ func createTokenExchangeRequest( // executeTokenExchangeRequest sends the HTTP request and returns the response body. func executeTokenExchangeRequest(client *http.Client, req *http.Request) ([]byte, error) { - resp, err := client.Do(req) // #nosec G704 -- URL is the configured token exchange endpoint + resp, err := client.Do(req) if err != nil { return nil, fmt.Errorf("token exchange request failed: %w", err) } defer func() { - if err := resp.Body.Close(); err != nil { - // Non-fatal: response body cleanup failure - slog.Debug("Failed to close response body", "error", err) + // Close without draining — matches oauthproto.DoTokenRequest. The + // LimitReader below caps how much we read; draining the remainder + // would be unbounded on oversized or never-terminating bodies and + // defeat the 1 MiB memory cap. Connection reuse is the tradeoff. + if closeErr := resp.Body.Close(); closeErr != nil { + slog.Debug("token exchange: close response body", "error", closeErr) } }() diff --git a/pkg/auth/tokenexchange/exchange_test.go b/pkg/auth/tokenexchange/exchange_test.go index abc5a0fe01..468847982b 100644 --- a/pkg/auth/tokenexchange/exchange_test.go +++ b/pkg/auth/tokenexchange/exchange_test.go @@ -216,7 +216,7 @@ func TestTokenSource_Token_Success(t *testing.T) { err = json.NewEncoder(w).Encode(resp) require.NoError(t, err) })) - defer server.Close() + t.Cleanup(server.Close) // Create config with test server config := &ExchangeConfig{ @@ -258,7 +258,7 @@ func TestTokenSource_Token_WithRefreshToken(t *testing.T) { w.WriteHeader(http.StatusOK) _ = json.NewEncoder(w).Encode(resp) })) - defer server.Close() + t.Cleanup(server.Close) config := &ExchangeConfig{ TokenURL: server.URL, @@ -290,7 +290,7 @@ func TestTokenSource_Token_NoExpiry(t *testing.T) { w.WriteHeader(http.StatusOK) _ = json.NewEncoder(w).Encode(resp) })) - defer server.Close() + t.Cleanup(server.Close) config := &ExchangeConfig{ TokenURL: server.URL, @@ -343,7 +343,7 @@ func TestTokenSource_Token_ContextCancellation(t *testing.T) { time.Sleep(100 * time.Millisecond) w.WriteHeader(http.StatusOK) })) - defer server.Close() + t.Cleanup(server.Close) config := &ExchangeConfig{ TokenURL: server.URL, @@ -418,7 +418,7 @@ func TestExchangeToken_HTTPErrorResponses(t *testing.T) { w.WriteHeader(tt.statusCode) _, _ = w.Write([]byte(tt.responseBody)) })) - defer server.Close() + t.Cleanup(server.Close) request := &exchangeRequest{ GrantType: "urn:ietf:params:oauth:grant-type:token-exchange", @@ -483,7 +483,7 @@ func TestExchangeToken_MalformedJSON(t *testing.T) { w.WriteHeader(http.StatusOK) _, _ = w.Write([]byte(tt.responseBody)) })) - defer server.Close() + t.Cleanup(server.Close) request := &exchangeRequest{ SubjectToken: "test-token", @@ -507,7 +507,7 @@ func TestExchangeToken_MissingRequiredFields(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) { t.Fatal("should not reach server") })) - defer server.Close() + t.Cleanup(server.Close) request := &exchangeRequest{ // Missing SubjectToken @@ -542,7 +542,7 @@ func TestExchangeToken_DefaultValues(t *testing.T) { w.WriteHeader(http.StatusOK) _ = json.NewEncoder(w).Encode(resp) })) - defer server.Close() + t.Cleanup(server.Close) request := &exchangeRequest{ SubjectToken: "test-token", @@ -577,7 +577,7 @@ func TestExchangeToken_OptionalFields(t *testing.T) { w.WriteHeader(http.StatusOK) _ = json.NewEncoder(w).Encode(resp) })) - defer server.Close() + t.Cleanup(server.Close) request := &exchangeRequest{ SubjectToken: "test-token", @@ -618,7 +618,7 @@ func TestExchangeToken_ActorTokenWithoutType(t *testing.T) { w.WriteHeader(http.StatusOK) _ = json.NewEncoder(w).Encode(resp) })) - defer server.Close() + t.Cleanup(server.Close) request := &exchangeRequest{ SubjectToken: "test-token", @@ -686,7 +686,7 @@ func TestExchangeToken_ResponseSizeLimit(t *testing.T) { w.WriteHeader(http.StatusOK) _ = json.NewEncoder(w).Encode(resp) })) - defer server.Close() + t.Cleanup(server.Close) request := &exchangeRequest{ SubjectToken: "test-token", @@ -742,7 +742,7 @@ func TestExchangeToken_NoCredentialLeakage(t *testing.T) { t.Parallel() server := tt.setupServer() - defer server.Close() + t.Cleanup(server.Close) request := &exchangeRequest{ SubjectToken: tt.subjectToken, @@ -784,7 +784,7 @@ func TestExchangeToken_FormEncoding(t *testing.T) { w.WriteHeader(http.StatusOK) _ = json.NewEncoder(w).Encode(resp) })) - defer server.Close() + t.Cleanup(server.Close) request := &exchangeRequest{ SubjectToken: specialChars, @@ -817,7 +817,7 @@ func TestExchangeToken_ContentLength(t *testing.T) { w.WriteHeader(http.StatusOK) _ = json.NewEncoder(w).Encode(resp) })) - defer server.Close() + t.Cleanup(server.Close) request := &exchangeRequest{ SubjectToken: "test-token", @@ -901,7 +901,7 @@ func TestSubjectTokenProvider_Variants(t *testing.T) { w.WriteHeader(http.StatusOK) _ = json.NewEncoder(w).Encode(resp) })) - defer server.Close() + t.Cleanup(server.Close) config := &ExchangeConfig{ TokenURL: server.URL, @@ -950,7 +950,7 @@ func TestExchangeToken_EmptyClientCredentials(t *testing.T) { w.WriteHeader(http.StatusOK) _ = json.NewEncoder(w).Encode(resp) })) - defer server.Close() + t.Cleanup(server.Close) request := &exchangeRequest{ SubjectToken: "test-token", @@ -988,7 +988,7 @@ func TestExchangeToken_OnlyClientID(t *testing.T) { w.WriteHeader(http.StatusOK) _ = json.NewEncoder(w).Encode(resp) })) - defer server.Close() + t.Cleanup(server.Close) request := &exchangeRequest{ SubjectToken: "test-token", @@ -1020,7 +1020,7 @@ func TestExchangeToken_ResponseFields(t *testing.T) { w.WriteHeader(http.StatusOK) _ = json.NewEncoder(w).Encode(resp) })) - defer server.Close() + t.Cleanup(server.Close) request := &exchangeRequest{ SubjectToken: "test-token", @@ -1052,7 +1052,7 @@ func TestExchangeToken_MinimalResponse(t *testing.T) { w.WriteHeader(http.StatusOK) _ = json.NewEncoder(w).Encode(resp) })) - defer server.Close() + t.Cleanup(server.Close) request := &exchangeRequest{ SubjectToken: "test-token", @@ -1121,7 +1121,7 @@ func TestExchangeToken_ScopeArray(t *testing.T) { w.WriteHeader(http.StatusOK) _ = json.NewEncoder(w).Encode(resp) })) - defer server.Close() + t.Cleanup(server.Close) request := &exchangeRequest{ SubjectToken: "test-token", @@ -1328,7 +1328,7 @@ func TestExchangeToken_URLValues(t *testing.T) { w.WriteHeader(http.StatusOK) _ = json.NewEncoder(w).Encode(resp) })) - defer server.Close() + t.Cleanup(server.Close) request := &exchangeRequest{ GrantType: "urn:ietf:params:oauth:grant-type:token-exchange", @@ -1393,7 +1393,7 @@ func TestExchangeToken_BasicAuthURLEncoding(t *testing.T) { w.WriteHeader(http.StatusOK) _ = json.NewEncoder(w).Encode(resp) })) - defer server.Close() + t.Cleanup(server.Close) request := &exchangeRequest{ SubjectToken: "test-token", diff --git a/pkg/auth/tokenexchange/middleware.go b/pkg/auth/tokenexchange/middleware.go index 0903393949..a9b7f437d5 100644 --- a/pkg/auth/tokenexchange/middleware.go +++ b/pkg/auth/tokenexchange/middleware.go @@ -36,7 +36,7 @@ const ( const ( // EnvClientSecret is the environment variable name for the OAuth client secret // This corresponds to the "client_secret" field in the token exchange configuration - //nolint:gosec // G101: This is an environment variable name, not a credential + //nolint:gosec // G101: this is an environment variable name, not a credential value EnvClientSecret = "TOOLHIVE_TOKEN_EXCHANGE_CLIENT_SECRET" ) @@ -63,7 +63,7 @@ type Config struct { ClientID string `json:"client_id"` // ClientSecret is the OAuth 2.0 client secret - ClientSecret string `json:"client_secret"` //nolint:gosec // G117: field legitimately holds sensitive data + ClientSecret string `json:"client_secret"` // Audience is the target audience for the exchanged token Audience string `json:"audience"` @@ -333,7 +333,6 @@ func createTokenExchangeMiddleware( // Log some claim information for debugging if sub, exists := claims["sub"]; exists { - //nolint:gosec // G706: subject claim is from validated JWT slog.Debug("Performing token exchange for subject", "subject", sub) }