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!: support PKCE in the oauth2 client's auth/exchange flow#21215

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

Open
Emyrk wants to merge18 commits intomain
base:main
Choose a base branch
Loading
fromstevenmasley/oauth_pkce

Conversation

@Emyrk
Copy link
Member

@EmyrkEmyrk commentedDec 10, 2025•
edited
Loading

Breaking Change: Existing oauth apps might now use PKCE. If an unknown type was being used, and it does not support PKCE, it will break.

To fix, set the PKCE methods on the external auth tonone

export CODER_EXTERNAL_AUTH_1_PKCE_METHODS=none

PKCE challenge used in primary coderd oauth2 flow.
Closes#21213

Implementation from:golang/oauth2@55cd552

What this does

This addsPKCE support when Coder is the OAuth client to an external IdP.

OIDC

PKCE support is automatically detected from thewell-known configuration endpoint. If it is supported, Coder will use it.

coder/cli/server.go

Lines 189 to 195 in53fc864

varpkceSupportstruct {
CodeChallengeMethodsSupported []promoauth.Oauth2PKCEChallengeMethod`json:"code_challenge_methods_supported"`
}
err=oidcProvider.Claims(&pkceSupport)
iferr!=nil {
returnnil,xerrors.Errorf("pkce detect in claims: %w",err)
}

OAuth (github login & external auth).

Known Oauth types have defaults set based on manual testing. We need to manually test to expand coverage.

We assume PKCE is supported by all unknown oauth IdP's.This can fixed in the configuration

Example to disable:

export CODER_EXTERNAL_AUTH_1_PKCE_METHODS=none
Manual tests

Tested to work on:

  • Github ✔️ uses PKCE
    • Failed code looks like:{"message":"Failed exchanging Oauth code.","detail":"oauth2: \"bad_verification_code\" \"The code passed is incorrect or expired.\" \"https://docs.github.com/apps/managing-oauth-apps/troubleshooting-oauth-app-access-token-request-errors/#bad-verification-code\""}
  • Gitlab ✔️ uses PKCE
    • Failed code looks like:{"message":"Failed exchanging Oauth code.","detail":"oauth2: \"invalid_grant\" \"The provided authorization grant is invalid, expired, revoked, does not match the redirection URI used in the authorization request, or was issued to another client.\""}
  • Gitea ✔️ uses PKCE
  • Failed code looks like:{"message":"Failed exchanging Oauth code.","detail":"oauth2: \"unauthorized_client\" \"failed PKCE code challenge\""}
  • Auth0 ✔️ uses PKCE
    • Failed code looks like:{"message":"Failed exchanging Oauth code.","detail":"oauth2: \"invalid_grant\" \"Invalid authorization code\""}

@EmyrkEmyrk marked this pull request as ready for reviewDecember 11, 2025 16:28
@EmyrkEmyrk changed the titlefeat: oauth2 client to use pkce in auth/exchange flowfeat: oauth2 client support for pkce in auth/exchange flowDec 11, 2025
@EmyrkEmyrk changed the titlefeat: oauth2 client support for pkce in auth/exchange flowfeat: support PKCE in the oauth2 client's auth/exchange flowDec 11, 2025
@EmyrkEmyrk changed the titlefeat: support PKCE in the oauth2 client's auth/exchange flowfeat!: support PKCE in the oauth2 client's auth/exchange flowDec 11, 2025
@EmyrkEmyrk added the release/breakingThis label is applied to PRs to detect breaking changes as part of the release process labelDec 11, 2025
@mafredrimafredri added the doc-checkAssign this label to PRs to check for any doc changes. labelDec 12, 2025
@mafredrimafredri self-requested a reviewDecember 12, 2025 16:06
@github-actions
Copy link

@mafredriCoder
Copy link
Member

📚 Documentation Check

✅ Updates Needed

  • docs/admin/external-auth/index.md - Add documentation for the newCODER_EXTERNAL_AUTH_N_PKCE_METHODS environment variable configuration option. This should explain:
    • What PKCE (Proof Key for Code Exchange) is and why it's used
    • How to configure it (e.g.,CODER_EXTERNAL_AUTH_0_PKCE_METHODS="S256 plain")
    • When administrators should enable it (security best practices, OAuth provider requirements)
    • Which OAuth providers typically require or support PKCE

📝 Reasoning

This PR adds PKCE support for external OAuth2 authentication, which is a user-facing feature that requires administrator configuration. Based on my analysis:

What Changed:

  • AddedCodeChallengeMethodsSupported field toExternalAuthConfig andExternalAuthLinkProvider
  • Added new environment variableCODER_EXTERNAL_AUTH_N_PKCE_METHODS for configuring PKCE methods
  • OIDC providers now auto-detect PKCE support from discovery metadata
  • API schemas were auto-generated and updated in/docs/reference/api/

Why Documentation is Needed:

  • This is a new configuration option that administrators need to know about
  • PKCE affects OAuth2 security and some providers may require it
  • The environment variable follows the existing pattern but isn't documented in/docs/admin/external-auth/index.md
  • Administrators need guidance on when and how to configure this option

What's Already Done:

  • API reference docs (schemas and general) were auto-generated ✅
  • No other documentation changes were included in this PR

This comment was generated by an AI Agent throughCoder Tasks

Copy link
Member

@mafredrimafredri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Minor nits/suggestions, but code changes look good otherwise 👍🏻. I agree with doc check that it would be good to have some added admin documentation.

r.Route("/github",func(r chi.Router) {
r.Use(
httpmw.ExtractOAuth2(options.GithubOAuth2Config,options.HTTPClient,options.DeploymentValues.HTTPCookies,nil),
// Github supports PKCE S256
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Do we need to call this out, link to docs?

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

It's nested in these docs:https://docs.github.com/en/apps/oauth-apps/building-oauth-apps/authorizing-oauth-apps

It is not easy to point out imo.

mafredri reacted with thumbs up emoji
constproviderID="test-idp"
fake:=oidctest.NewFakeIDP(t,
append([]oidctest.FakeIDPOpt{},settings.FakeIDPOpts...)...,
append([]oidctest.FakeIDPOpt{oidctest.WithPKCE()},settings.FakeIDPOpts...)...,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Does this make it impossible to disable PKCE for tests when usingsetupOauth2Test?

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

It does. PKCE is the new default flow, there are some tests that omit it. But the bulk of our tests should use it.

@Emyrk
Copy link
MemberAuthor

Added docs!

@mafredrimafredri added doc-checkAssign this label to PRs to check for any doc changes. and removed doc-checkAssign this label to PRs to check for any doc changes. labelsDec 12, 2025
Copy link
Member

@mafredrimafredri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Just a few minor things, but LGTM otherwise (let's see what second doc-check says too).

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@mafredrimafredrimafredri approved these changes

Assignees

@EmyrkEmyrk

Labels

doc-checkAssign this label to PRs to check for any doc changes.release/breakingThis label is applied to PRs to detect breaking changes as part of the release process

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

feat: Add OIDC nonce parameter support for CSRF protection

3 participants

@Emyrk@mafredri
[8]ページ先頭
©2009-2025 Movatter.jp