diff --git a/coderd/oauth2_test.go b/coderd/oauth2_test.go index 3a8d5917fda..d2a78bc225c 100644 --- a/coderd/oauth2_test.go +++ b/coderd/oauth2_test.go @@ -498,6 +498,11 @@ func TestOAuth2ProviderTokenExchange(t *testing.T) { var verifier string if test.defaultCode != nil { code = *test.defaultCode + // These subtests exercise malformed/expired code_ + // handling; the code lookup fails before code_verifier is + // ever compared, but it still has to satisfy RFC 7636 + // §4.1's format floor to reach that point. + verifier = strings.Repeat("a", 43) } else { var err error code, verifier, err = authorizationFlow(ctx, userClient, valid) diff --git a/coderd/oauth2provider/authorize.go b/coderd/oauth2provider/authorize.go index 1480259c1fa..8b0e9cceecb 100644 --- a/coderd/oauth2provider/authorize.go +++ b/coderd/oauth2provider/authorize.go @@ -54,12 +54,24 @@ func extractAuthorizeParams(r *http.Request, callbackURL *url.URL) (authorizePar codeChallengeMethod: p.String(vals, "", "code_challenge_method"), } - // PKCE is required for authorization code flow requests. - if params.responseType == codersdk.OAuth2ProviderResponseTypeCode && params.codeChallenge == "" { - p.Errors = append(p.Errors, codersdk.ValidationError{ - Field: "code_challenge", - Detail: `Query param "code_challenge" is required and cannot be empty`, - }) + // PKCE is required for authorization code flow requests. Reject a + // malformed code_challenge here (RFC 7636 §4.4.1) rather than storing it + // verbatim and failing later at token exchange, where the error would + // point at the code_verifier instead of the parameter that was actually + // invalid. + if params.responseType == codersdk.OAuth2ProviderResponseTypeCode { + switch { + case params.codeChallenge == "": + p.Errors = append(p.Errors, codersdk.ValidationError{ + Field: "code_challenge", + Detail: `Query param "code_challenge" is required and cannot be empty`, + }) + case !ValidPKCEFormat(params.codeChallenge): + p.Errors = append(p.Errors, codersdk.ValidationError{ + Field: "code_challenge", + Detail: "must be 43 to 128 characters from the unreserved character set [A-Za-z0-9-._~]", + }) + } } // Validate resource indicator syntax (RFC 8707): must be absolute URI without fragment diff --git a/coderd/oauth2provider/oauth2providertest/fixtures.go b/coderd/oauth2provider/oauth2providertest/fixtures.go index 8dbccb511a3..df9de772beb 100644 --- a/coderd/oauth2provider/oauth2providertest/fixtures.go +++ b/coderd/oauth2provider/oauth2providertest/fixtures.go @@ -13,8 +13,16 @@ const ( // TestResourceURI is used for testing resource parameter TestResourceURI = "https://api.example.com" - // Invalid PKCE verifier for negative testing - InvalidCodeVerifier = "wrong-verifier" + // InvalidCodeVerifier is well-formed (43 characters, RFC 7636 §4.1's + // unreserved set) but does not hash to any issued challenge, so it + // exercises the PKCE comparison failure (invalid_grant) rather than the + // length/charset check (invalid_request). + InvalidCodeVerifier = "wrong-verifier-that-is-well-formed-43-chars" + + // MalformedCodeVerifier is below RFC 7636 §4.1's 43-character floor, so + // it exercises the length/charset check (invalid_request) instead of the + // PKCE comparison. + MalformedCodeVerifier = "too-short" ) // OAuth2ErrorTypes contains standard OAuth2 error codes diff --git a/coderd/oauth2provider/oauth2providertest/oauth2_test.go b/coderd/oauth2provider/oauth2providertest/oauth2_test.go index 22d8ac05341..b7d5649406d 100644 --- a/coderd/oauth2provider/oauth2providertest/oauth2_test.go +++ b/coderd/oauth2provider/oauth2providertest/oauth2_test.go @@ -158,6 +158,113 @@ func TestOAuth2InvalidPKCE(t *testing.T) { ) } +// TestOAuth2PKCEFailureConsumesCode verifies that a code_verifier that fails +// the PKCE hash comparison consumes the authorization code (RFC 6749 §10.5: +// codes are single-use). Without this, a leaked code could be replayed with +// unlimited further code_verifier guesses for the rest of its lifetime. +func TestOAuth2PKCEFailureConsumesCode(t *testing.T) { + t.Parallel() + + client := coderdtest.New(t, &coderdtest.Options{ + IncludeProvisionerDaemon: false, + }) + _ = coderdtest.CreateFirstUser(t, client) + + app, clientSecret := oauth2providertest.CreateTestOAuth2App(t, client) + t.Cleanup(func() { + oauth2providertest.CleanupOAuth2App(t, client, app.ID) + }) + + codeVerifier, codeChallenge := oauth2providertest.GeneratePKCE(t) + state := oauth2providertest.GenerateState(t) + + authParams := oauth2providertest.AuthorizeParams{ + ClientID: app.ID.String(), + ResponseType: "code", + RedirectURI: oauth2providertest.TestRedirectURI, + State: state, + CodeChallenge: codeChallenge, + CodeChallengeMethod: "S256", + } + + code := oauth2providertest.AuthorizeOAuth2App(t, client, client.URL.String(), authParams) + require.NotEmpty(t, code, "should receive authorization code") + + // Attempt the exchange with a well-formed but wrong verifier. This fails + // the PKCE hash comparison (invalid_grant) and must consume the code. + failedParams := oauth2providertest.TokenExchangeParams{ + GrantType: "authorization_code", + Code: code, + ClientID: app.ID.String(), + ClientSecret: clientSecret, + CodeVerifier: oauth2providertest.InvalidCodeVerifier, + RedirectURI: oauth2providertest.TestRedirectURI, + } + oauth2providertest.PerformTokenExchangeExpectingError( + t, client.URL.String(), failedParams, oauth2providertest.OAuth2ErrorTypes.InvalidGrant, + ) + + // The correct verifier can no longer redeem the code: the failed PKCE + // comparison above already consumed it. + retryParams := oauth2providertest.TokenExchangeParams{ + GrantType: "authorization_code", + Code: code, + ClientID: app.ID.String(), + ClientSecret: clientSecret, + CodeVerifier: codeVerifier, + RedirectURI: oauth2providertest.TestRedirectURI, + } + oauth2providertest.PerformTokenExchangeExpectingError( + t, client.URL.String(), retryParams, oauth2providertest.OAuth2ErrorTypes.InvalidGrant, + ) +} + +// TestOAuth2MalformedCodeVerifierIsRejected verifies that a code_verifier +// below the RFC 7636 §4.1 length floor is rejected as invalid_request, +// distinct from a well-formed verifier that fails the PKCE hash comparison +// (invalid_grant, covered by TestOAuth2InvalidPKCE). +func TestOAuth2MalformedCodeVerifierIsRejected(t *testing.T) { + t.Parallel() + + client := coderdtest.New(t, &coderdtest.Options{ + IncludeProvisionerDaemon: false, + }) + _ = coderdtest.CreateFirstUser(t, client) + + app, clientSecret := oauth2providertest.CreateTestOAuth2App(t, client) + t.Cleanup(func() { + oauth2providertest.CleanupOAuth2App(t, client, app.ID) + }) + + _, codeChallenge := oauth2providertest.GeneratePKCE(t) + state := oauth2providertest.GenerateState(t) + + authParams := oauth2providertest.AuthorizeParams{ + ClientID: app.ID.String(), + ResponseType: "code", + RedirectURI: oauth2providertest.TestRedirectURI, + State: state, + CodeChallenge: codeChallenge, + CodeChallengeMethod: "S256", + } + + code := oauth2providertest.AuthorizeOAuth2App(t, client, client.URL.String(), authParams) + require.NotEmpty(t, code, "should receive authorization code") + + tokenParams := oauth2providertest.TokenExchangeParams{ + GrantType: "authorization_code", + Code: code, + ClientID: app.ID.String(), + ClientSecret: clientSecret, + CodeVerifier: oauth2providertest.MalformedCodeVerifier, + RedirectURI: oauth2providertest.TestRedirectURI, + } + + oauth2providertest.PerformTokenExchangeExpectingError( + t, client.URL.String(), tokenParams, oauth2providertest.OAuth2ErrorTypes.InvalidRequest, + ) +} + // TestOAuth2WithoutPKCEIsRejected verifies that authorization requests without // a code_challenge are rejected now that PKCE is mandatory. func TestOAuth2WithoutPKCEIsRejected(t *testing.T) { @@ -189,6 +296,39 @@ func TestOAuth2WithoutPKCEIsRejected(t *testing.T) { ) } +// TestOAuth2MalformedCodeChallengeIsRejected verifies that a code_challenge +// below the RFC 7636 §4.1 length floor is rejected at the authorization +// request, rather than being stored and only failing once a client attempts +// to exchange the resulting code. +func TestOAuth2MalformedCodeChallengeIsRejected(t *testing.T) { + t.Parallel() + + client := coderdtest.New(t, &coderdtest.Options{ + IncludeProvisionerDaemon: false, + }) + _ = coderdtest.CreateFirstUser(t, client) + + app, _ := oauth2providertest.CreateTestOAuth2App(t, client) + t.Cleanup(func() { + oauth2providertest.CleanupOAuth2App(t, client, app.ID) + }) + + state := oauth2providertest.GenerateState(t) + + authParams := oauth2providertest.AuthorizeParams{ + ClientID: app.ID.String(), + ResponseType: "code", + RedirectURI: oauth2providertest.TestRedirectURI, + State: state, + CodeChallenge: "too-short", + CodeChallengeMethod: "S256", + } + + oauth2providertest.AuthorizeOAuth2AppExpectingError( + t, client, client.URL.String(), authParams, http.StatusBadRequest, + ) +} + func TestOAuth2TokenExchangeClientSecretBasic(t *testing.T) { t.Parallel() diff --git a/coderd/oauth2provider/pkce.go b/coderd/oauth2provider/pkce.go index fd759dff889..c43c83c3384 100644 --- a/coderd/oauth2provider/pkce.go +++ b/coderd/oauth2provider/pkce.go @@ -6,6 +6,45 @@ import ( "encoding/base64" ) +// PKCE code verifier bounds from RFC 7636 §4.1. +const ( + pkceVerifierMinLength = 43 + pkceVerifierMaxLength = 128 +) + +// ValidPKCEFormat reports whether s meets RFC 7636 §4.1: 43 to 128 characters +// of the unreserved set [A-Za-z0-9-._~]. RFC 7636 gives code_verifier and +// code_challenge the same ABNF, so this check applies to both: a code_verifier +// directly, and a code_challenge because the S256 method that produces it +// (base64url(SHA256(verifier))) always yields a string within these bounds. +// +// The length floor matters because the challenge and code both travel +// through the authorization URL and redirect, landing in browser history, +// referrer headers, and proxy logs. An attacker who recovers either one +// brute-forces the verifier offline at whatever entropy the client chose, +// with no server-side rate limit to slow them down. A client secret also +// authenticates the token request today, but public clients (#27873) will +// rely on this bound alone, so it must hold on its own merit. The same +// bound on code_challenge keeps a malformed value from being persisted +// verbatim and failing late, at token exchange, instead of at the +// authorization request where RFC 7636 §4.4.1 expects it to be rejected. +func ValidPKCEFormat(s string) bool { + if len(s) < pkceVerifierMinLength || len(s) > pkceVerifierMaxLength { + return false + } + for _, r := range s { + switch { + case r >= 'A' && r <= 'Z', + r >= 'a' && r <= 'z', + r >= '0' && r <= '9', + r == '-', r == '.', r == '_', r == '~': + default: + return false + } + } + return true +} + // VerifyPKCE verifies that the code_verifier matches the code_challenge // using the S256 method as specified in RFC 7636. func VerifyPKCE(challenge, verifier string) bool { diff --git a/coderd/oauth2provider/pkce_test.go b/coderd/oauth2provider/pkce_test.go index da0ff3a9d24..62d92a91fbb 100644 --- a/coderd/oauth2provider/pkce_test.go +++ b/coderd/oauth2provider/pkce_test.go @@ -3,6 +3,7 @@ package oauth2provider_test import ( "crypto/sha256" "encoding/base64" + "strings" "testing" "github.com/stretchr/testify/require" @@ -124,3 +125,76 @@ func TestValidatePKCECodeChallengeMethod(t *testing.T) { }) } } + +func TestValidPKCEFormat(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + verifier string + expectValid bool + }{ + { + name: "Empty", + verifier: "", + expectValid: false, + }, + { + name: "OneCharacter", + verifier: "a", + expectValid: false, + }, + { + name: "OneBelowMinLength", + verifier: strings.Repeat("a", 42), + expectValid: false, + }, + { + name: "AtMinLength", + verifier: strings.Repeat("a", 43), + expectValid: true, + }, + { + name: "AtMaxLength", + verifier: strings.Repeat("a", 128), + expectValid: true, + }, + { + name: "OneAboveMaxLength", + verifier: strings.Repeat("a", 129), + expectValid: false, + }, + { + name: "AllowedCharacters", + verifier: "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-._~", + expectValid: true, + }, + { + name: "PlusIsRejected", + verifier: strings.Repeat("a", 42) + "+", + expectValid: false, + }, + { + name: "SlashIsRejected", + verifier: strings.Repeat("a", 42) + "/", + expectValid: false, + }, + { + name: "EqualsIsRejected", + verifier: strings.Repeat("a", 42) + "=", + expectValid: false, + }, + { + name: "SpaceIsRejected", + verifier: strings.Repeat("a", 42) + " ", + expectValid: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + require.Equal(t, tt.expectValid, oauth2provider.ValidPKCEFormat(tt.verifier)) + }) + } +} diff --git a/coderd/oauth2provider/tokens.go b/coderd/oauth2provider/tokens.go index 3761d1010ca..caa5fb6b77f 100644 --- a/coderd/oauth2provider/tokens.go +++ b/coderd/oauth2provider/tokens.go @@ -13,12 +13,14 @@ import ( "github.com/google/uuid" "golang.org/x/xerrors" + "cdr.dev/slog/v3" "github.com/coder/coder/v2/coderd/apikey" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/coderd/database/dbtime" "github.com/coder/coder/v2/coderd/httpapi" "github.com/coder/coder/v2/coderd/httpmw" + "github.com/coder/coder/v2/coderd/httpmw/loggermw" "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/codersdk" ) @@ -97,6 +99,17 @@ func extractTokenRequest(r *http.Request, callbackURL *url.URL) (codersdk.OAuth2 Detail: "Parameter \"client_secret\" is required and cannot be empty", }) } + // A code_verifier outside RFC 7636 §4.1's bounds is a syntax error + // (RFC 6749 §5.2), distinct from a well-formed verifier that fails the + // PKCE hash comparison in authorizationCodeGrant, which RFC 7636 §4.6 + // maps to invalid_grant instead. Checking it here, alongside the other + // syntax validation, keeps the two failure modes distinguishable. + if !ValidPKCEFormat(req.CodeVerifier) { + p.Errors = append(p.Errors, codersdk.ValidationError{ + Field: "code_verifier", + Detail: "must be 43 to 128 characters from the unreserved character set [A-Za-z0-9-._~] (RFC 7636 §4.1)", + }) + } } // Validate redirect URI - errors are added to p.Errors. @@ -158,6 +171,18 @@ func Tokens(db database.Store, lifetimes codersdk.SessionLifetime) http.HandlerF return } } + + // A malformed code_verifier gets its own message so a client that + // sent a well-formed but wrong verifier (rejected later, as + // invalid_grant, by the PKCE hash comparison) can tell the two + // failures apart instead of retrying the same bad verifier forever. + if slices.ContainsFunc(validationErrs, func(validationError codersdk.ValidationError) bool { + return validationError.Field == "code_verifier" + }) { + httpapi.WriteOAuth2Error(ctx, rw, http.StatusBadRequest, codersdk.OAuth2ErrorCodeInvalidRequest, "The code_verifier parameter must be 43 to 128 characters from the unreserved character set [A-Za-z0-9-._~] (RFC 7636 §4.1)") + return + } + // Generic invalid request for other validation errors httpapi.WriteOAuth2Error(ctx, rw, http.StatusBadRequest, codersdk.OAuth2ErrorCodeInvalidRequest, "The request is missing required parameters or is otherwise malformed") return @@ -211,6 +236,22 @@ func Tokens(db database.Store, lifetimes codersdk.SessionLifetime) http.HandlerF } } +// revokeOAuth2CodeOnPKCEFailure deletes a code that failed PKCE verification +// so it cannot be replayed with further code_verifier guesses (RFC 6749 +// §10.5). Deletion failure does not change the response returned to the +// caller: surfacing it as a different error would let a caller distinguish +// "delete succeeded" from "delete failed," defeating the point of revoking +// the code in the first place. It is instead noted on the request's log line +// so operators can see it happened. +func revokeOAuth2CodeOnPKCEFailure(ctx context.Context, db database.Store, codeID uuid.UUID) { + //nolint:gocritic // OAuth2 system context, no authenticated user during token exchange + if err := db.DeleteOAuth2ProviderAppCodeByID(dbauthz.AsSystemOAuth2(ctx), codeID); err != nil { + if rlogger := loggermw.RequestLoggerFromContext(ctx); rlogger != nil { + rlogger.WithFields(slog.F("oauth2_pkce_failure_code_revoke_error", err.Error())) + } + } +} + func authorizationCodeGrant(ctx context.Context, db database.Store, app database.OAuth2ProviderApp, lifetimes codersdk.SessionLifetime, req codersdk.OAuth2TokenRequest) (codersdk.OAuth2TokenResponse, error) { // Validate the client secret. secret, err := ParseFormattedSecret(req.ClientSecret) @@ -277,18 +318,25 @@ func authorizationCodeGrant(ctx context.Context, db database.Store, app database } } - // PKCE is mandatory for all authorization code flows - // (OAuth 2.1). Verify the code verifier against the stored - // challenge. - if req.CodeVerifier == "" { - return codersdk.OAuth2TokenResponse{}, errInvalidPKCE - } + // PKCE is mandatory for all authorization code flows (OAuth 2.1). Verify + // the code verifier against the stored challenge. extractTokenRequest + // already rejected a malformed verifier as invalid_request, so + // req.CodeVerifier is guaranteed to meet RFC 7636 §4.1's bounds here; a + // mismatch below is a wrong-but-well-formed verifier, RFC 7636 §4.6's + // invalid_grant case. + // + // RFC 6749 §10.5 requires codes to be single-use. A code that survives a + // failed PKCE check would otherwise let a leaked code (the exact threat + // PKCE defends against) be replayed with different code_verifier guesses + // for the rest of its lifetime, unthrottled. if !dbCode.CodeChallenge.Valid || dbCode.CodeChallenge.String == "" { - // Code was issued without a challenge — should not happen + // Code was issued without a challenge, which should not happen // with authorize endpoint enforcement, but defend in depth. + revokeOAuth2CodeOnPKCEFailure(ctx, db, dbCode.ID) return codersdk.OAuth2TokenResponse{}, errInvalidPKCE } if !VerifyPKCE(dbCode.CodeChallenge.String, req.CodeVerifier) { + revokeOAuth2CodeOnPKCEFailure(ctx, db, dbCode.ID) return codersdk.OAuth2TokenResponse{}, errInvalidPKCE } diff --git a/coderd/oauth2provider/tokens_internal_test.go b/coderd/oauth2provider/tokens_internal_test.go index 7f25b68827c..fc2148353cf 100644 --- a/coderd/oauth2provider/tokens_internal_test.go +++ b/coderd/oauth2provider/tokens_internal_test.go @@ -110,6 +110,10 @@ func TestExtractTokenParams_Scopes(t *testing.T) { form.Set("client_id", "test-client") form.Set("client_secret", "test-secret") form.Set("code", "test-code") + // This test only exercises scope parsing, but code_verifier is + // validated unconditionally for this grant type, so use a value + // that satisfies the RFC 7636 §4.1 length floor. + form.Set("code_verifier", strings.Repeat("a", 43)) if tc.scopeParam != "" { form.Set("scope", tc.scopeParam) } @@ -147,22 +151,22 @@ func TestExtractTokenParams_ScopesURLEncoded(t *testing.T) { }{ { name: "PlusEncodedSpaces", - rawQuery: "grant_type=authorization_code&client_id=test&client_secret=secret&code=code&scope=scope1+scope2+scope3", + rawQuery: "grant_type=authorization_code&client_id=test&client_secret=secret&code=code&code_verifier=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa&scope=scope1+scope2+scope3", expectedScopes: []string{"scope1", "scope2", "scope3"}, }, { name: "PercentEncodedSpaces", - rawQuery: "grant_type=authorization_code&client_id=test&client_secret=secret&code=code&scope=scope1%20scope2%20scope3", + rawQuery: "grant_type=authorization_code&client_id=test&client_secret=secret&code=code&code_verifier=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa&scope=scope1%20scope2%20scope3", expectedScopes: []string{"scope1", "scope2", "scope3"}, }, { name: "MixedEncoding", - rawQuery: "grant_type=authorization_code&client_id=test&client_secret=secret&code=code&scope=scope1+scope2%20scope3", + rawQuery: "grant_type=authorization_code&client_id=test&client_secret=secret&code=code&code_verifier=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa&scope=scope1+scope2%20scope3", expectedScopes: []string{"scope1", "scope2", "scope3"}, }, { name: "ColonEncodedInScope", - rawQuery: "grant_type=authorization_code&client_id=test&client_secret=secret&code=code&scope=coder%3Aworkspace.create+coder%3Aworkspace.operate", + rawQuery: "grant_type=authorization_code&client_id=test&client_secret=secret&code=code&code_verifier=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa&scope=coder%3Aworkspace.create+coder%3Aworkspace.operate", expectedScopes: []string{"coder:workspace.create", "coder:workspace.operate"}, }, } @@ -216,6 +220,7 @@ func TestExtractTokenParams_ScopesEdgeCases(t *testing.T) { form.Set("client_id", "test-client") form.Set("client_secret", "test-secret") form.Set("code", "test-code") + form.Set("code_verifier", strings.Repeat("a", 43)) return form }, expectedScopes: []string{}, @@ -229,6 +234,7 @@ func TestExtractTokenParams_ScopesEdgeCases(t *testing.T) { form.Set("client_id", "test-client") form.Set("client_secret", "test-secret") form.Set("code", "test-code") + form.Set("code_verifier", strings.Repeat("a", 43)) form.Set("scope", " ") return form }, @@ -244,6 +250,7 @@ func TestExtractTokenParams_ScopesEdgeCases(t *testing.T) { form.Set("client_id", "test-client") form.Set("client_secret", "test-secret") form.Set("code", "test-code") + form.Set("code_verifier", strings.Repeat("a", 43)) form.Set("scope", longScope) return form }, @@ -318,7 +325,10 @@ func TestExtractAuthorizeParams_Scopes(t *testing.T) { query.Set("response_type", "code") query.Set("client_id", "test-client") query.Set("redirect_uri", "http://localhost:3000/callback") - query.Set("code_challenge", "test-challenge") + // This test only exercises scope parsing, but code_challenge is + // still required for response_type=code and must satisfy the + // RFC 7636 §4.1 length floor, so use a valid-length value. + query.Set("code_challenge", strings.Repeat("a", 43)) if tc.scopeParam != "" { query.Set("scope", tc.scopeParam) } @@ -342,6 +352,74 @@ func TestExtractAuthorizeParams_Scopes(t *testing.T) { } } +// TestExtractAuthorizeParams_CodeChallengeFormat ensures a code_challenge is +// rejected at the authorization request (RFC 7636 §4.4.1) when it does not +// meet the same length and character bounds as a code_verifier, rather than +// being stored and failing later at token exchange. +func TestExtractAuthorizeParams_CodeChallengeFormat(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + codeChallenge string + expectValid bool + }{ + { + name: "ValidLength", + codeChallenge: strings.Repeat("a", 43), + expectValid: true, + }, + { + name: "TooShort", + codeChallenge: strings.Repeat("a", 42), + expectValid: false, + }, + { + name: "TooLong", + codeChallenge: strings.Repeat("a", 129), + expectValid: false, + }, + { + name: "DisallowedCharacter", + codeChallenge: strings.Repeat("a", 42) + "+", + expectValid: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + callbackURL, err := url.Parse("http://localhost:3000/callback") + require.NoError(t, err) + + query := url.Values{} + query.Set("response_type", "code") + query.Set("client_id", "test-client") + query.Set("redirect_uri", "http://localhost:3000/callback") + query.Set("code_challenge", tc.codeChallenge) + + reqURL, err := url.Parse("http://localhost:8080/oauth2/authorize?" + query.Encode()) + require.NoError(t, err) + + req := &http.Request{ + Method: http.MethodGet, + URL: reqURL, + } + + _, validationErrs, err := extractAuthorizeParams(req, callbackURL) + if tc.expectValid { + require.NoError(t, err) + require.Empty(t, validationErrs) + } else { + require.Error(t, err) + require.Len(t, validationErrs, 1) + require.Equal(t, "code_challenge", validationErrs[0].Field) + } + }) + } +} + // TestExtractAuthorizeParams_TokenResponseTypeDoesNotRequirePKCE ensures // response_type=token is parsed without requiring PKCE fields so callers can // return unsupported_response_type instead of invalid_request. diff --git a/docs/admin/integrations/oauth2-provider.md b/docs/admin/integrations/oauth2-provider.md index 51c588e513c..1cbb17a3e1d 100644 --- a/docs/admin/integrations/oauth2-provider.md +++ b/docs/admin/integrations/oauth2-provider.md @@ -182,11 +182,18 @@ PKCE is **required** for all OAuth2 authorization code flows. Coder enforces PKCE in compliance with the OAuth 2.1 specification. Both public and confidential clients must include PKCE parameters: +> [!NOTE] +> `code_verifier` and `code_challenge` must each be 43-128 characters from +> the unreserved character set `[A-Za-z0-9-._~]` (RFC 7636 §4.1). A value +> outside these bounds is rejected with an `invalid_request` error, at the +> token endpoint for `code_verifier` and at the authorization endpoint for +> `code_challenge`. + 1. Generate a code verifier and challenge: ```sh - CODE_VERIFIER=$(openssl rand -base64 96 | tr -d "=+/" | cut -c1-128) - CODE_CHALLENGE=$(echo -n $CODE_VERIFIER | openssl dgst -sha256 -binary | base64 | tr -d "=+/" | cut -c1-43) + CODE_VERIFIER=$(openssl rand -base64 96 | tr -d '\n' | tr '+/' '-_' | tr -d '=') + CODE_CHALLENGE=$(echo -n $CODE_VERIFIER | openssl dgst -sha256 -binary | base64 | tr -d "=" | tr '+/' '-_') ``` 2. Include PKCE parameters in the authorization request: diff --git a/scripts/oauth2/generate-pkce.sh b/scripts/oauth2/generate-pkce.sh index cb94120d569..a0a96cb450a 100755 --- a/scripts/oauth2/generate-pkce.sh +++ b/scripts/oauth2/generate-pkce.sh @@ -4,7 +4,7 @@ # Usage: ./generate-pkce.sh # Generate code verifier (43-128 characters, URL-safe) -CODE_VERIFIER=$(openssl rand -base64 32 | tr -d "=+/" | cut -c -43) +CODE_VERIFIER=$(openssl rand -base64 32 | tr '+/' '-_' | tr -d '=') # Generate code challenge (S256 method) CODE_CHALLENGE=$(echo -n "$CODE_VERIFIER" | openssl dgst -sha256 -binary | base64 | tr -d "=" | tr '+/' '-_') diff --git a/scripts/oauth2/test-manual-flow.sh b/scripts/oauth2/test-manual-flow.sh index 734c3a9c5e0..73dd9de09d6 100755 --- a/scripts/oauth2/test-manual-flow.sh +++ b/scripts/oauth2/test-manual-flow.sh @@ -39,7 +39,7 @@ if ! command -v go &>/dev/null; then fi # Generate PKCE parameters -CODE_VERIFIER=$(openssl rand -base64 32 | tr -d "=+/" | cut -c -43) +CODE_VERIFIER=$(openssl rand -base64 32 | tr '+/' '-_' | tr -d '=') export CODE_VERIFIER CODE_CHALLENGE=$(echo -n "$CODE_VERIFIER" | openssl dgst -sha256 -binary | base64 | tr -d "=" | tr '+/' '-_') export CODE_CHALLENGE diff --git a/scripts/oauth2/test-mcp-oauth2.sh b/scripts/oauth2/test-mcp-oauth2.sh index 9139010f6a3..746faac69b9 100755 --- a/scripts/oauth2/test-mcp-oauth2.sh +++ b/scripts/oauth2/test-mcp-oauth2.sh @@ -66,7 +66,7 @@ echo -e "${GREEN}✓ Created client secret${NC}\n" # Test 2: PKCE Flow echo -e "${YELLOW}Test 2: PKCE Flow${NC}" -CODE_VERIFIER=$(openssl rand -base64 32 | tr -d "=+/" | cut -c -43) +CODE_VERIFIER=$(openssl rand -base64 32 | tr '+/' '-_' | tr -d '=') CODE_CHALLENGE=$(echo -n "$CODE_VERIFIER" | openssl dgst -sha256 -binary | base64 | tr -d "=" | tr '+/' '-_') STATE=$(openssl rand -hex 16) @@ -132,7 +132,7 @@ fi echo -e "${YELLOW}Test 4: Resource Parameter Support${NC}" RESOURCE="https://api.example.com" STATE=$(openssl rand -hex 16) -RESOURCE_CODE_VERIFIER=$(openssl rand -base64 32 | tr -d "=+/" | cut -c -43) +RESOURCE_CODE_VERIFIER=$(openssl rand -base64 32 | tr '+/' '-_' | tr -d '=') RESOURCE_CODE_CHALLENGE=$(echo -n "$RESOURCE_CODE_VERIFIER" | openssl dgst -sha256 -binary | base64 | tr -d "=" | tr '+/' '-_') RESOURCE_AUTH_URL="$BASE_URL/oauth2/authorize?client_id=$CLIENT_ID&response_type=code&redirect_uri=http://localhost:9876/callback&state=$STATE&resource=$RESOURCE&code_challenge=$RESOURCE_CODE_CHALLENGE&code_challenge_method=S256"