Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

ThomasK33
Copy link
Member

@ThomasK33ThomasK33 commentedJun 24, 2025
edited
Loading

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)

  • Add/.well-known/oauth-authorization-server endpoint for automatic client discovery
  • Returns standardized metadata including supported grant types, response types, and PKCE methods
  • Essential for MCP client compatibility and OAuth2 standards compliance

PKCE Support (RFC 7636)

  • Implement Proof Key for Code Exchange with S256 challenge method
  • Addcode_challenge andcode_challenge_method parameters to authorization flow
  • Addcode_verifier validation in token exchange
  • Provides enhanced security for public clients (mobile apps, CLIs)

Resource Parameter Support (RFC 8707)

  • Addresource parameter to authorization and token endpoints
  • Store resource URI and bind tokens to specific audiences
  • Critical for MCP's resource-bound token model

Enhanced OAuth2 Error Handling

  • Add OAuth2-compliant error responses with proper error codes
  • Use standard error format:{"error": "code", "error_description": "details"}
  • Improve error consistency across OAuth2 endpoints

Authorization UI Improvements

  • Fix authorization flow to use POST-based consent instead of GET redirects
  • Remove dependency on referer headers for security decisions
  • Improve CSRF protection with proper state parameter validation

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

  • Migration 000343: Addscode_challenge,code_challenge_method,resource_uri tooauth2_provider_app_codes
  • Migration 000343: Addsaudience field tooauth2_provider_app_tokens for resource binding
  • Audit Updates: New OAuth2 fields properly tracked in audit system
  • Backward Compatibility: All changes maintain compatibility with existing OAuth2 flows

Test Coverage

  • Comprehensive PKCE test suite incoderd/identityprovider/pkce_test.go
  • OAuth2 metadata endpoint tests incoderd/oauth2_metadata_test.go
  • Integration tests covering PKCE + resource parameter combinations
  • Negative tests for invalid PKCE verifiers and malformed requests

Testing Instructions

# Run the comprehensive OAuth2 test suite./scripts/oauth2/test-mcp-oauth2.shManual Testing with Interactive Server# Start Coder in development mode./scripts/develop.sh# In another terminal, set up test app and run interactive floweval$(./scripts/oauth2/setup-test-app.sh)./scripts/oauth2/test-manual-flow.sh# Opens browser with OAuth2 flow, handles callback automatically# Clean up when done./scripts/oauth2/cleanup-test-app.shIndividual Component Testing# Test metadata endpointcurl -s http://localhost:3000/.well-known/oauth-authorization-server| jq.# Test PKCE generation./scripts/oauth2/generate-pkce.sh# Run specific test suitesgotest -v ./coderd/identityprovider -run TestVerifyPKCEgotest -v ./coderd -run TestOAuth2AuthorizationServerMetadata

Breaking Changes

None. All changes maintain backward compatibility with existing OAuth2 flows.


Change-Id: Ifbd0d9a543d545f9f56ecaa77ff2238542ff954a
Signed-off-by: Thomas Kosiewskitk@coder.com

@ThomasK33Graphite App
Copy link
MemberAuthor

ThomasK33 commentedJun 24, 2025
edited
Loading

This stack of pull requests is managed byGraphite. Learn more aboutstacking.

@ThomasK33ThomasK33 changed the titlefeat(oauth2): add authorization server metadata endpoint and PKCE supportfeat: add authorization server metadata endpoint and PKCE supportJun 24, 2025
@ThomasK33ThomasK33force-pushed thethomask33/06-24-feat_oauth2_add_authorization_server_metadata_endpoint_and_pkce_support branch 8 times, most recently from8f890ec to018694aCompareJune 25, 2025 13:59
@ThomasK33ThomasK33force-pushed thethomask33/06-24-feat_oauth2_add_authorization_server_metadata_endpoint_and_pkce_support branch from018694a to3daa2abCompareJune 25, 2025 14:06
@ThomasK33ThomasK33 marked this pull request as ready for reviewJune 25, 2025 14:07
@ThomasK33ThomasK33 changed the titlefeat: add authorization server metadata endpoint and PKCE supportfeat: oauth2 - add authorization server metadata endpoint and PKCE supportJun 25, 2025
@ThomasK33ThomasK33force-pushed thethomask33/06-24-feat_oauth2_add_authorization_server_metadata_endpoint_and_pkce_support branch from3daa2ab tob50e322CompareJune 25, 2025 18:16
johnstcn
johnstcn previously requested changesJun 26, 2025
Comment on lines -339 to +353
// @Success302
// @Router /oauth2/authorize [post]
// @Success200 "Returns HTML authorization page"
// @Router /oauth2/authorize [get]
Copy link
Member

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.

Copy link
MemberAuthor

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.

Copy link
Member

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.

Copy link
MemberAuthor

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 😅

Copy link
Member

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?

Copy link
MemberAuthor

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.

func (*API)oAuth2ProviderMiddleware(next http.Handler) http.Handler {
returnhttp.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)
})
}

Emyrk reacted with thumbs up emoji
@ThomasK33ThomasK33force-pushed thethomask33/06-24-feat_oauth2_add_authorization_server_metadata_endpoint_and_pkce_support branch 2 times, most recently from3dd6c7e to224784aCompareJune 26, 2025 15:45
@ThomasK33ThomasK33 changed the base branch frommain tographite-base/18548June 26, 2025 16:20
@ThomasK33ThomasK33force-pushed thethomask33/06-24-feat_oauth2_add_authorization_server_metadata_endpoint_and_pkce_support branch from224784a toc2d85d9CompareJune 26, 2025 16:20
@ThomasK33ThomasK33 changed the base branch fromgraphite-base/18548 tothomask33/fix_pin_Nix_version_to_2.28.4_to_avoid_JSON_type_errorJune 26, 2025 16:20
@ThomasK33ThomasK33force-pushed thethomask33/06-24-feat_oauth2_add_authorization_server_metadata_endpoint_and_pkce_support branch fromc2d85d9 to69eb5c8CompareJune 26, 2025 16:23
@ThomasK33ThomasK33force-pushed thethomask33/fix_pin_Nix_version_to_2.28.4_to_avoid_JSON_type_error branch fromce6aacd to0efb35bCompareJune 26, 2025 16:23
@ThomasK33ThomasK33force-pushed thethomask33/06-24-feat_oauth2_add_authorization_server_metadata_endpoint_and_pkce_support branch from69eb5c8 to80c695bCompareJune 26, 2025 16:24
@ThomasK33ThomasK33force-pushed thethomask33/fix_pin_Nix_version_to_2.28.4_to_avoid_JSON_type_error branch from0efb35b to0218619CompareJune 26, 2025 16:24
@graphite-appgraphite-appbot changed the base branch fromgraphite-base/18548 tomainJune 26, 2025 16:34
@ThomasK33ThomasK33force-pushed thethomask33/06-24-feat_oauth2_add_authorization_server_metadata_endpoint_and_pkce_support branch 2 times, most recently from870e5eb todd622d0CompareJune 26, 2025 17:00
Copy link
Member

@EmyrkEmyrk left a 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 👍

Thepost 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?

Comment on lines 149 to 156
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
}
Copy link
Member

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.

ThomasK33 reacted with laugh emoji
Comment on lines -339 to +353
// @Success302
// @Router /oauth2/authorize [post]
// @Success200 "Returns HTML authorization page"
// @Router /oauth2/authorize [get]
Copy link
Member

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?

Comment on lines +38 to +91
### `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
Copy link
Member

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?

Copy link
MemberAuthor

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.

Copy link
Member

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?

Copy link
MemberAuthor

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.

Emyrk reacted with thumbs up emoji
@Emyrk
Copy link
Member

I'm going to pull copilot in as a reviewer. It's caught some typos in my larger PRs in the past.

@EmyrkEmyrk requested a review fromCopilotJune 26, 2025 19:08
Copy link
Contributor

@CopilotCopilotAI left a 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
FileDescription
site/static/oauth2allow.htmlReplaces an anchor link with a POST form for the Allow action to improve security
site/src/api/typesGenerated.tsAdds 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 othersUpdates 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">

Copy link
Member

@johnstcnjohnstcn left a 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

@ThomasK33ThomasK33force-pushed thethomask33/06-24-feat_oauth2_add_authorization_server_metadata_endpoint_and_pkce_support branch 3 times, most recently from0cb471e tof211bddCompareJune 30, 2025 11:49
Copy link
Member

@EmyrkEmyrk left a 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.

ThomasK33 reacted with thumbs up emoji
@ThomasK33ThomasK33force-pushed thethomask33/06-24-feat_oauth2_add_authorization_server_metadata_endpoint_and_pkce_support branch 2 times, most recently froma9550d8 toec5c703CompareJuly 1, 2025 09:27
…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 guidelinesDatabase changes:- Add migration 000341: code_challenge, resource_uri, audience fields- Update audit table for new OAuth2 fieldsOAuth2 provider remains development-only (requires --dev flag).Change-Id: Ifbd0d9a543d545f9f56ecaa77ff2238542ff954aSigned-off-by: Thomas Kosiewski <tk@coder.com>
@ThomasK33ThomasK33force-pushed thethomask33/06-24-feat_oauth2_add_authorization_server_metadata_endpoint_and_pkce_support branch fromec5c703 toe6243ceCompareJuly 1, 2025 13:23
@ThomasK33ThomasK33 merged commit6f2834f intomainJul 1, 2025
41 of 42 checks passed
@ThomasK33Graphite App
Copy link
MemberAuthor

Merge activity

@ThomasK33ThomasK33 deleted the thomask33/06-24-feat_oauth2_add_authorization_server_metadata_endpoint_and_pkce_support branchJuly 1, 2025 13:39
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsJul 1, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@johnstcnjohnstcnjohnstcn left review comments

Copilot code reviewCopilotCopilot left review comments

@EmyrkEmyrkEmyrk approved these changes

Assignees

@ThomasK33ThomasK33

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@ThomasK33@Emyrk@johnstcn

[8]ページ先頭

©2009-2025 Movatter.jp