feat: oauth2 - add authorization server metadata endpoint and PKCE support #18548
Conversation
8f890ec to
018694a
Compare
018694a to
3daa2ab
Compare
3daa2ab to
b50e322
Compare
| // @Success 302 | ||
| // @Router /oauth2/authorize [post] | ||
| // @Success 200 "Returns HTML authorization page" | ||
| // @Router /oauth2/authorize [get] |
There was a problem hiding this comment.
This looks like a breaking change to me. We should still support the GET /oauth2/authorize -> 302
I'm happy for us to support both methods, but I'd like the existing GET endpoint to behave the same.
There was a problem hiding this comment.
The OAuth 2 endpoints were never part of a stable release, only added to the muxer/router in dev builds, so not a breaking change per se.
There was a problem hiding this comment.
Thanks for clarifying, I completely missed that!
Confirmed on 2.23 release build.
There was a problem hiding this comment.
I think the redirect here would be the spec compliant thing to do, but then I'm a bit too lazy to have to redirect to another page, to do the same thing.
I'm not against it if you think it makes more sense though... just lazy 😅
There was a problem hiding this comment.
This were not shipped in a release?
I thought backstage relied on these oauth endpoints?
There was a problem hiding this comment.
Nope, you have the OAuth 2 middleware applied to all OAuth 2-related routes, which just returned a status forbidden for non-dev builds.
Lines 20 to 31 in 7a3a6d4
| origin := r.Header.Get(httpmw.OriginHeader) | ||
| originU, err := url.Parse(origin) | ||
| if err != nil { | ||
| httpapi.Write(r.Context(), rw, http.StatusBadRequest, codersdk.Response{ | ||
| Message: "Invalid origin header.", | ||
| Detail: err.Error(), | ||
| }) | ||
| return | ||
| } | ||
|
|
||
| referer := r.Referer() | ||
| refererU, err := url.Parse(referer) | ||
| if err != nil { | ||
| httpapi.Write(r.Context(), rw, http.StatusBadRequest, codersdk.Response{ | ||
| Message: "Invalid referer header.", | ||
| Detail: err.Error(), | ||
| }) | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
Why are these checks being removed?
There was a problem hiding this comment.
The checks were removed due to a redesign of the OAuth2 flow for improved standardization and security.
The old approach (removed code), relied on fragile referer and origin header checks. There used header parsing to verify if requests "came from self." and added a redirected=true parameter for loop detection. (Which isn't not part of any OAuth spec). And it rendered us vulnerable to header spoofing.
The new approach (current code) uses standard HTTP methods (GET for consent and POST for authorization), and when users clicks the "Allow" button, a POST request is submitted from the form.Allowing us to be compliant with the standard OAuth2 authorization flow, and also allowing us to potentially reuse the CSRF middleware in the future if necessary.
Also, it simplifies the logic by removing a good bit of complexity with the redirects and state encoding with URL query params and potentially error-prone header validation.
There was a problem hiding this comment.
Ahhh GET and POST to prevent the infinite loop issue.
I would need to do more manual testing of other providers, but I don't recall seeing a POST request to /authorize in the spec. Do you have a link or anything I could refer to ?
There was a problem hiding this comment.
The authorization server MUST support the use of the HTTP "GET"
method [RFC2616] for the authorization endpoint and MAY support the
use of the "POST" method as well.
| // url.Parse() allows empty URLs, which is fine because the origin is not | ||
| // always set by browsers (or other tools like cURL). If the origin does | ||
| // exist, we will make sure it matches. We require `referer` to be set at | ||
| // a minimum in order to detect whether "allow" has been pressed, however. | ||
| cameFromSelf := (origin == "" || originU.Hostname() == accessURL.Hostname()) && | ||
| refererU.Hostname() == accessURL.Hostname() && | ||
| refererU.Path == "/oauth2/authorize" | ||
|
|
||
| // If we were redirected here from this same page it means the user | ||
| // pressed the allow button so defer to the authorize handler which | ||
| // generates the code, otherwise show the HTML allow page. | ||
| // TODO: Skip this step if the user has already clicked allow before, and | ||
| // we can just reuse the token. | ||
| if cameFromSelf { |
There was a problem hiding this comment.
Same, why is this being removed?
3dd6c7e to
224784a
Compare
224784a to
c2d85d9
Compare
c2d85d9 to
69eb5c8
Compare
ce6aacd to
0efb35b
Compare
69eb5c8 to
80c695b
Compare
0efb35b to
0218619
Compare
80c695b to
03c4724
Compare
0218619 to
1b1d091
Compare
870e5eb to
dd622d0
Compare
| // errorWriter interface abstracts different error response formats. | ||
| // This uses the Strategy pattern to avoid a control flag (useOAuth2Errors bool) | ||
| // which was flagged by the linter as an anti-pattern. Instead of duplicating | ||
| // the entire function logic or using a boolean parameter, we inject the error | ||
| // handling behavior through this interface. | ||
| type errorWriter interface { | ||
| writeMissingClientID(ctx context.Context, rw http.ResponseWriter) | ||
| writeInvalidClientID(ctx context.Context, rw http.ResponseWriter, err error) | ||
| writeInvalidClient(ctx context.Context, rw http.ResponseWriter) | ||
| } |
There was a problem hiding this comment.
This is kind of nifty. Why do we need to send codersdk.Response errors at all though?
Can /authorize just return the oauth compliant errors?
| // TestCodeChallenge2 is the generated challenge for TestCodeVerifier2 | ||
| var TestCodeChallenge2 = GenerateCodeChallenge(TestCodeVerifier2) |
| func AuthorizeOAuth2App(t *testing.T, client *codersdk.Client, baseURL string, params AuthorizeParams) string { | ||
| t.Helper() | ||
|
|
||
| ctx := testutil.Context(t, testutil.WaitLong) | ||
|
|
||
| // Build authorization URL | ||
| authURL, err := url.Parse(baseURL + "/oauth2/authorize") | ||
| require.NoError(t, err, "failed to parse authorization URL") | ||
|
|
||
| query := url.Values{} | ||
| query.Set("client_id", params.ClientID) | ||
| query.Set("response_type", params.ResponseType) | ||
| query.Set("redirect_uri", params.RedirectURI) | ||
| query.Set("state", params.State) | ||
|
|
||
| if params.CodeChallenge != "" { | ||
| query.Set("code_challenge", params.CodeChallenge) | ||
| query.Set("code_challenge_method", params.CodeChallengeMethod) | ||
| } | ||
| if params.Resource != "" { | ||
| query.Set("resource", params.Resource) | ||
| } | ||
| if params.Scope != "" { | ||
| query.Set("scope", params.Scope) | ||
| } | ||
|
|
||
| authURL.RawQuery = query.Encode() | ||
|
|
||
| // Create POST request to authorize endpoint (simulating user clicking "Allow") | ||
| req, err := http.NewRequestWithContext(ctx, "POST", authURL.String(), nil) | ||
| require.NoError(t, err, "failed to create authorization request") | ||
|
|
||
| // Add session token | ||
| req.Header.Set("Coder-Session-Token", client.SessionToken()) | ||
|
|
||
| // Perform request | ||
| httpClient := &http.Client{ | ||
| CheckRedirect: func(_ *http.Request, _ []*http.Request) error { | ||
| // Don't follow redirects, we want to capture the redirect URL | ||
| return http.ErrUseLastResponse | ||
| }, | ||
| } | ||
|
|
||
| resp, err := httpClient.Do(req) | ||
| require.NoError(t, err, "failed to perform authorization request") | ||
| defer resp.Body.Close() | ||
|
|
||
| // Should get a redirect response (either 302 Found or 307 Temporary Redirect) | ||
| require.True(t, resp.StatusCode == http.StatusFound || resp.StatusCode == http.StatusTemporaryRedirect, | ||
| "expected redirect response, got %d", resp.StatusCode) | ||
|
|
||
| // Extract redirect URL | ||
| location := resp.Header.Get("Location") | ||
| require.NotEmpty(t, location, "missing Location header in redirect response") | ||
|
|
||
| // Parse redirect URL to extract authorization code | ||
| redirectURL, err := url.Parse(location) | ||
| require.NoError(t, err, "failed to parse redirect URL") | ||
|
|
||
| code := redirectURL.Query().Get("code") | ||
| require.NotEmpty(t, code, "missing authorization code in redirect URL") | ||
|
|
||
| // Verify state parameter | ||
| returnedState := redirectURL.Query().Get("state") | ||
| require.Equal(t, params.State, returnedState, "state parameter mismatch") | ||
|
|
||
| return code | ||
| } |
There was a problem hiding this comment.
Some of this exists in oidctest
Might be worth merging this package with that?
| origin := r.Header.Get(httpmw.OriginHeader) | ||
| originU, err := url.Parse(origin) | ||
| if err != nil { | ||
| httpapi.Write(r.Context(), rw, http.StatusBadRequest, codersdk.Response{ | ||
| Message: "Invalid origin header.", | ||
| Detail: err.Error(), | ||
| }) | ||
| return | ||
| } | ||
|
|
||
| referer := r.Referer() | ||
| refererU, err := url.Parse(referer) | ||
| if err != nil { | ||
| httpapi.Write(r.Context(), rw, http.StatusBadRequest, codersdk.Response{ | ||
| Message: "Invalid referer header.", | ||
| Detail: err.Error(), | ||
| }) | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
Ahhh GET and POST to prevent the infinite loop issue.
I would need to do more manual testing of other providers, but I don't recall seeing a POST request to /authorize in the spec. Do you have a link or anything I could refer to ?
| if ve.Field == "grant_type" { | ||
| writeOAuth2Error(ctx, rw, http.StatusBadRequest, "unsupported_grant_type", "The grant type is missing or unsupported") | ||
| return | ||
| } | ||
| // Check for missing required parameters for authorization_code grant | ||
| if ve.Field == "code" || ve.Field == "client_id" || ve.Field == "client_secret" { | ||
| writeOAuth2Error(ctx, rw, http.StatusBadRequest, "invalid_request", fmt.Sprintf("Missing required parameter: %s", ve.Field)) | ||
| return | ||
| } |
There was a problem hiding this comment.
Does it matter which comes first? Does 1 have any priority?
I just wonder if this can be made more deterministic, rather than relying on the validation order. (Which I guess has some static order based on the function extractTokenParameters)
| if ve.Field == "grant_type" { | |
| writeOAuth2Error(ctx, rw, http.StatusBadRequest, "unsupported_grant_type", "The grant type is missing or unsupported") | |
| return | |
| } | |
| // Check for missing required parameters for authorization_code grant | |
| if ve.Field == "code" || ve.Field == "client_id" || ve.Field == "client_secret" { | |
| writeOAuth2Error(ctx, rw, http.StatusBadRequest, "invalid_request", fmt.Sprintf("Missing required parameter: %s", ve.Field)) | |
| return | |
| } | |
| if slices.ContainsFunc(validationErrs, func(validationError codersdk.ValidationError) bool { | |
| return validationError.Field == "grant_type" | |
| }) { | |
| writeOAuth2Error(ctx, rw, http.StatusBadRequest, "unsupported_grant_type", "The grant type is missing or unsupported") | |
| return | |
| } | |
| if slices.ContainsFunc(validationErrs, func(validationError codersdk.ValidationError) bool { | |
| return validationError.Field == ve.Field == "code" || ve.Field == "client_id" || ve.Field == "client_secret" | |
| }) { | |
| writeOAuth2Error(ctx, rw, http.StatusBadRequest, "invalid_request", fmt.Sprintf("Missing required parameter: %s", ve.Field)) | |
| return | |
| } |
| if errors.Is(err, errBadSecret) { | ||
| writeOAuth2Error(ctx, rw, http.StatusUnauthorized, "invalid_client", "The client credentials are invalid") | ||
| return | ||
| } | ||
| if errors.Is(err, errBadCode) { | ||
| writeOAuth2Error(ctx, rw, http.StatusBadRequest, "invalid_grant", "The authorization code is invalid or expired") | ||
| return | ||
| } | ||
| if errors.Is(err, errInvalidPKCE) { | ||
| writeOAuth2Error(ctx, rw, http.StatusBadRequest, "invalid_grant", "The PKCE code verifier is invalid") | ||
| return | ||
| } | ||
| if errors.Is(err, errBadToken) { | ||
| writeOAuth2Error(ctx, rw, http.StatusBadRequest, "invalid_grant", "The refresh token is invalid or expired") | ||
| return | ||
| } |
There was a problem hiding this comment.
This is unfortunate, but I get it.
| // @Success 302 | ||
| // @Router /oauth2/authorize [post] | ||
| // @Success 200 "Returns HTML authorization page" | ||
| // @Router /oauth2/authorize [get] |
There was a problem hiding this comment.
This were not shipped in a release?
I thought backstage relied on these oauth endpoints?
| ### `setup-test-app.sh` | ||
|
|
||
| Creates a test OAuth2 application and outputs environment variables. | ||
|
|
||
| Usage: | ||
|
|
||
| ```bash | ||
| eval $(./scripts/oauth2/setup-test-app.sh) | ||
| echo "Client ID: $CLIENT_ID" | ||
| ``` | ||
|
|
||
| ### `cleanup-test-app.sh` | ||
|
|
||
| Deletes a test OAuth2 application. | ||
|
|
||
| Usage: | ||
|
|
||
| ```bash | ||
| ./scripts/oauth2/cleanup-test-app.sh $CLIENT_ID | ||
| # Or if CLIENT_ID is set as environment variable: | ||
| ./scripts/oauth2/cleanup-test-app.sh | ||
| ``` | ||
|
|
||
| ### `generate-pkce.sh` | ||
|
|
||
| Generates PKCE code verifier and challenge for manual testing. | ||
|
|
||
| Usage: | ||
|
|
||
| ```bash | ||
| ./scripts/oauth2/generate-pkce.sh | ||
| ``` | ||
|
|
||
| ### `test-manual-flow.sh` | ||
|
|
||
| Launches a local Go web server to test the OAuth2 flow interactively. The server automatically handles the OAuth2 callback and token exchange, providing a user-friendly web interface with results. | ||
|
|
||
| Usage: | ||
|
|
||
| ```bash | ||
| # First set up an app | ||
| eval $(./scripts/oauth2/setup-test-app.sh) | ||
|
|
||
| # Then run the test server | ||
| ./scripts/oauth2/test-manual-flow.sh | ||
| ``` | ||
|
|
||
| Features: | ||
|
|
||
| - Starts a local web server on port 9876 | ||
| - Automatically captures the authorization code | ||
| - Performs token exchange without manual intervention | ||
| - Displays results in a clean web interface | ||
| - Shows example API calls you can make with the token |
There was a problem hiding this comment.
Why are these tests in bash? They will not be run in CI then right?
There was a problem hiding this comment.
That's right, they won't be run in CI.
They are in Bash because I am testing the actual browser-based flow of a user authorizing and clicking the button on the authorize page. Automating that manual e2e test with something like Puppeteer or other headless browsers seemed a bit overkill to me, at least for now.
There was a problem hiding this comment.
Oh there is a browser component to these scripts?
There was a problem hiding this comment.
Yeah, it launches a local Go web server that "receives" the authorization code and exchanges it for a coder token after a user clicks on authorize in the browser.
|
I'm going to pull copilot in as a reviewer. It's caught some typos in my larger PRs in the past. |
There was a problem hiding this comment.
Pull Request Overview
This PR implements critical OAuth2 features to improve standards compliance in Coder’s authorization server. Key changes include adding a /.well-known/oauth-authorization-server metadata endpoint, full PKCE support within the authorization and token exchange flows, and enhanced handling of the resource parameter for token binding.
Reviewed Changes
Copilot reviewed 40 out of 40 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| site/static/oauth2allow.html | Replaces an anchor link with a POST form for the Allow action to improve security |
| site/src/api/typesGenerated.ts | Adds the OAuth2AuthorizationServerMetadata type definition for client discovery |
| scripts/oauth2/* | Introduces extensive test scripts for OAuth2 metadata, PKCE, resource parameter flows, and manual testing |
| coderd/* | Updates OAuth2 flows, authorization handlers, token exchange logic and introduces PKCE and resource parameter support |
| enterprise/audit/table.go, docs/*, and others | Updates database models, queries, and documentation to support new OAuth2 fields and behavior |
Comments suppressed due to low confidence (3)
coderd/identityprovider/authorize.go:90
- [nitpick] Consider adding an inline comment to explain that when a code_challenge is provided but no code_challenge_method is specified, the method defaults to 'S256'. This improves code clarity for future maintainers.
if params.codeChallenge != "" {
coderd/oauth2_test.go:433
- [nitpick] Verify that the error message ‘invalid_client’ is used consistently for client credential failures and aligns with OAuth2 standards. Consistency in error responses will aid clients in correctly interpreting error scenarios.
tokenError: "invalid_client",
site/static/oauth2allow.html:163
- Replacing the anchor link with a POST form for the 'Allow' action enhances security by mitigating CSRF risks. Ensure that the server-side endpoint handling this form validates POST requests appropriately.
<form method="POST" style="display: inline;">
…port - Add /.well-known/oauth-authorization-server metadata endpoint (RFC 8414) - Implement PKCE support with S256 method for enhanced security - Add resource parameter support (RFC 8707) for token binding - Add OAuth2-compliant error responses with proper error codes - Fix authorization UI to use POST-based consent instead of GET redirects - Add comprehensive OAuth2 test scripts and interactive test server - Update CLAUDE.md with OAuth2 development guidelines Database changes: - Add migration 000341: code_challenge, resource_uri, audience fields - Update audit table for new OAuth2 fields OAuth2 provider remains development-only (requires --dev flag). Change-Id: Ifbd0d9a543d545f9f56ecaa77ff2238542ff954a Signed-off-by: Thomas Kosiewski <tk@coder.com>
dd622d0 to
3092108
Compare
Summary
This PR implements critical MCP OAuth2 compliance features for Coder's authorization server, adding PKCE support, resource parameter handling, and OAuth2 server metadata discovery. This brings Coder's OAuth2 implementation significantly closer to production readiness for MCP (Model Context Protocol)
integrations.
What's Added
OAuth2 Authorization Server Metadata (RFC 8414)
/.well-known/oauth-authorization-serverendpoint for automatic client discoveryPKCE Support (RFC 7636)
code_challengeandcode_challenge_methodparameters to authorization flowcode_verifiervalidation in token exchangeResource Parameter Support (RFC 8707)
resourceparameter to authorization and token endpointsEnhanced OAuth2 Error Handling
{"error": "code", "error_description": "details"}Authorization UI Improvements
Why This Matters
For MCP Integration: MCP requires OAuth2 authorization servers to support PKCE, resource parameters, and metadata discovery. Without these features, MCP clients cannot securely authenticate with Coder.
For Security: PKCE prevents authorization code interception attacks, especially critical for public clients. Resource binding ensures tokens are only valid for intended services.
For Standards Compliance: These are widely adopted OAuth2 extensions that improve interoperability with modern OAuth2 clients.
Database Changes
code_challenge,code_challenge_method,resource_uritooauth2_provider_app_codesaudiencefield tooauth2_provider_app_tokensfor resource bindingTest Coverage
coderd/identityprovider/pkce_test.gocoderd/oauth2_metadata_test.goTesting Instructions
Breaking Changes
None. All changes maintain backward compatibility with existing OAuth2 flows.
Change-Id: Ifbd0d9a543d545f9f56ecaa77ff2238542ff954a
Signed-off-by: Thomas Kosiewski tk@coder.com