feat!: support PKCE in the oauth2 client's auth/exchange flow #21215
Conversation
Used in coder primary auth
📚 Documentation Check✅ Updates Needed
📝 ReasoningThis PR adds PKCE support for external OAuth2 authentication, which is a user-facing feature that requires administrator configuration. Based on my analysis: What Changed:
Why Documentation is Needed:
What's Already Done:
This comment was generated by an AI Agent through Coder Tasks |
mafredri
left a comment
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Do we need to call this out, link to docs?
There was a problem hiding this comment.
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.
| const providerID = "test-idp" | ||
| fake := oidctest.NewFakeIDP(t, | ||
| append([]oidctest.FakeIDPOpt{}, settings.FakeIDPOpts...)..., | ||
| append([]oidctest.FakeIDPOpt{oidctest.WithPKCE()}, settings.FakeIDPOpts...)..., |
There was a problem hiding this comment.
Does this make it impossible to disable PKCE for tests when using setupOauth2Test?
There was a problem hiding this comment.
It does. PKCE is the new default flow, there are some tests that omit it. But the bulk of our tests should use it.
|
Added docs! |
mafredri
left a comment
There was a problem hiding this comment.
Just a few minor things, but LGTM otherwise (let's see what second doc-check says too).
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 to
nonePKCE challenge used in primary coderd oauth2 flow.
Closes #21213
Implementation from: golang/oauth2@55cd552
What this does
This adds PKCE support when Coder is the OAuth client to an external IdP.
OIDC
PKCE support is automatically detected from the
well-knownconfiguration endpoint. If it is supported, Coder will use it.coder/cli/server.go
Lines 189 to 195 in 53fc864
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:
Manual tests
Tested to work on:
{"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\""}{"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.\""}{"message":"Failed exchanging Oauth code.","detail":"oauth2: \"unauthorized_client\" \"failed PKCE code challenge\""}{"message":"Failed exchanging Oauth code.","detail":"oauth2: \"invalid_grant\" \"Invalid authorization code\""}