-
Notifications
You must be signed in to change notification settings - Fork 929
feat: oauth2 - add authorization server metadata endpoint and PKCE support #18548
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for clarifying, I completely missed that!
Confirmed on 2.23 release build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This were not shipped in a release?
I thought backstage relied on these oauth endpoints?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
func (*API) oAuth2ProviderMiddleware(next http.Handler) http.Handler { | |
return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { | |
if !buildinfo.IsDev() { | |
httpapi.Write(r.Context(), rw, http.StatusForbidden, codersdk.Response{ | |
Message: "OAuth2 provider is under development.", | |
}) | |
return | |
} | |
next.ServeHTTP(rw, r) | |
}) | |
} |
3dd6c7e
to
224784a
Compare
224784a
to
c2d85d9
Compare
c2d85d9
to
69eb5c8
Compare
ce6aacd
to
0efb35b
Compare
69eb5c8
to
80c695b
Compare
0efb35b
to
0218619
Compare
870e5eb
to
dd622d0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall things look ok to me 👍
The post
to /authorize
is a good way to prevent the infinite redirect loop issue. I do not see that in the spec, so it is new to me.
Is there a reason some of the testing lives as bash scripts?
coderd/identityprovider/tokens.go
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these tests in bash? They will not be run in CI then right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh there is a browser component to these scripts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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;">
dd622d0
to
3092108
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have any blocking comments, deferring approval to @Emyrk
0cb471e
to
f211bdd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a big diff. Overall I like the changes, and the structure is good. We can iterate ontop of this.
The bash tests feel a bit weak to me, as I don't think they will be manually run. When these feature stabilizes, we might want to get an e2e test in playwright or something.
a9550d8
to
ec5c703
Compare
…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>
ec5c703
to
e6243ce
Compare
Merge activity
|
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-server
endpoint for automatic client discoveryPKCE Support (RFC 7636)
code_challenge
andcode_challenge_method
parameters to authorization flowcode_verifier
validation in token exchangeResource Parameter Support (RFC 8707)
resource
parameter 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_uri
tooauth2_provider_app_codes
audience
field tooauth2_provider_app_tokens
for resource bindingTest Coverage
coderd/identityprovider/pkce_test.go
coderd/oauth2_metadata_test.go
Testing Instructions
Breaking Changes
None. All changes maintain backward compatibility with existing OAuth2 flows.
Change-Id: Ifbd0d9a543d545f9f56ecaa77ff2238542ff954a
Signed-off-by: Thomas Kosiewski tk@coder.com