Skip to content

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.

Sign up for GitHub

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

Open
Emyrk wants to merge 11 commits into
base: main
Choose a base branch
from
Open

feat!: support PKCE in the oauth2 client's auth/exchange flow #21215

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
08f52e9
feat: oauth2 client to use pkce in auth/exchange flow
Emyrk Dec 10, 2025
40283ea
force pkce
Emyrk Dec 10, 2025
5eb6e9f
chore: pkce defaults
Emyrk Dec 10, 2025
53fc864
frontend test
Emyrk Dec 10, 2025
ead92c8
chore: sane defaults and make gen
Emyrk Dec 11, 2025
0be0bb2
make some tests use pkce
Emyrk Dec 11, 2025
303ea30
fmt
Emyrk Dec 11, 2025
7af6c3a
fix tests
Emyrk Dec 11, 2025
7f11ab7
make gen
Emyrk Dec 11, 2025
65b08c8
fix tests
Emyrk Dec 11, 2025
c6b3b24
fix tests
Emyrk Dec 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions cli/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,14 @@ func createOIDCConfig(ctx context.Context, logger slog.Logger, vals *codersdk.De
secondaryClaimsSrc = coderd.MergedClaimsSourceAccessToken
}

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

return &coderd.OIDCConfig{
OAuth2Config: useCfg,
Provider: oidcProvider,
Expand All @@ -206,6 +214,7 @@ func createOIDCConfig(ctx context.Context, logger slog.Logger, vals *codersdk.De
SignupsDisabledText: vals.OIDC.SignupsDisabledText.String(),
IconURL: vals.OIDC.IconURL.String(),
IgnoreEmailVerified: vals.OIDC.IgnoreEmailVerified.Value(),
PKCEMethods: pkceSupport.CodeChallengeMethodsSupported,
}, nil
}

Expand Down Expand Up @@ -2761,6 +2770,8 @@ func parseExternalAuthProvidersFromEnv(prefix string, environ []string) ([]coder
provider.MCPToolAllowRegex = v.Value
case "MCP_TOOL_DENY_REGEX":
provider.MCPToolDenyRegex = v.Value
case "PKCE_METHODS":
provider.CodeChallengeMethodsSupported = strings.Split(v.Value, " ")
}
providers[providerNum] = provider
}
Expand Down
6 changes: 6 additions & 0 deletions coderd/apidoc/docs.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions coderd/apidoc/swagger.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 5 additions & 3 deletions coderd/coderd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"github.com/coder/coder/v2/coderd/oauth2provider"
"github.com/coder/coder/v2/coderd/pproflabel"
"github.com/coder/coder/v2/coderd/prebuilds"
"github.com/coder/coder/v2/coderd/promoauth"
"github.com/coder/coder/v2/coderd/usage"
"github.com/coder/coder/v2/coderd/wsbuilder"

Expand Down Expand Up @@ -940,7 +941,7 @@ func New(options *Options) *API {
r.Route(fmt.Sprintf("/%s/callback", externalAuthConfig.ID), func(r chi.Router) {
r.Use(
apiKeyMiddlewareRedirect,
httpmw.ExtractOAuth2(externalAuthConfig, options.HTTPClient, options.DeploymentValues.HTTPCookies, nil),
httpmw.ExtractOAuth2(externalAuthConfig, options.HTTPClient, options.DeploymentValues.HTTPCookies, nil, externalAuthConfig.CodeChallengeMethodsSupported),
)
r.Get("/", api.externalAuthCallback(externalAuthConfig))
})
Expand Down Expand Up @@ -1289,14 +1290,15 @@ func New(options *Options) *API {
r.Get("/github/device", api.userOAuth2GithubDevice)
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

@mafredri mafredri Dec 12, 2025

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?

httpmw.ExtractOAuth2(options.GithubOAuth2Config, options.HTTPClient, options.DeploymentValues.HTTPCookies, nil, []promoauth.Oauth2PKCEChallengeMethod{promoauth.PKCEChallengeMethodSha256}),
Copy link
Member

@mafredri mafredri Dec 12, 2025

Choose a reason for hiding this comment

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

Suggestion: PKCESupported() on GithubOAuth2Config?

)
r.Get("/callback", api.userOAuth2Github)
})
})
r.Route("/oidc/callback", func(r chi.Router) {
r.Use(
httpmw.ExtractOAuth2(options.OIDCConfig, options.HTTPClient, options.DeploymentValues.HTTPCookies, oidcAuthURLParams),
httpmw.ExtractOAuth2(options.OIDCConfig, options.HTTPClient, options.DeploymentValues.HTTPCookies, oidcAuthURLParams, options.OIDCConfig.PKCESupported()),
)
r.Get("/", api.userOIDC)
})
Expand Down
66 changes: 61 additions & 5 deletions coderd/coderdtest/oidctest/idp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,7 @@ type FakeIDP struct {
// clientID to be used by coderd
clientID string
clientSecret string
pkce bool // TODO: Implement for refresh token flow as well
Copy link
Member

@mafredri mafredri Dec 12, 2025

Choose a reason for hiding this comment

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

Nit: TODO(name):, or perhaps track via GH ticket.

// externalProviderID is optional to match the provider in coderd for
// redirectURLs.
externalProviderID string
Expand All @@ -181,6 +182,8 @@ type FakeIDP struct {
// These maps are used to control the state of the IDP.
// That is the various access tokens, refresh tokens, states, etc.
codeToStateMap *syncmap.Map[string, string]
// Code -> PKCE Challenge
codeToChallengeMap *syncmap.Map[string, string]
// Token -> Email
accessTokens *syncmap.Map[string, token]
// Refresh Token -> Email
Expand Down Expand Up @@ -239,6 +242,12 @@ func (s statusHookError) Error() string {

type FakeIDPOpt func(idp *FakeIDP)

func WithPKCE() func(*FakeIDP) {
return func(f *FakeIDP) {
f.pkce = true
}
}

func WithAuthorizedRedirectURL(hook func(redirectURL string) error) func(*FakeIDP) {
return func(f *FakeIDP) {
f.hookValidRedirectURL = hook
Expand Down Expand Up @@ -450,6 +459,7 @@ func NewFakeIDP(t testing.TB, opts ...FakeIDPOpt) *FakeIDP {
clientSecret: uuid.NewString(),
logger: slog.Make(),
codeToStateMap: syncmap.New[string, string](),
codeToChallengeMap: syncmap.New[string, string](),
accessTokens: syncmap.New[string, token](),
refreshTokens: syncmap.New[string, string](),
refreshTokensUsed: syncmap.New[string, bool](),
Expand Down Expand Up @@ -557,8 +567,16 @@ func (f *FakeIDP) realServer(t testing.TB) *httptest.Server {
func (f *FakeIDP) GenerateAuthenticatedToken(claims jwt.MapClaims) (*oauth2.Token, error) {
state := uuid.NewString()
f.stateToIDTokenClaims.Store(state, claims)
code := f.newCode(state)
return f.locked.Config().Exchange(oidc.ClientContext(context.Background(), f.HTTPClient(nil)), code)

exchangeOpts := []oauth2.AuthCodeOption{}
verifier := ""
if f.pkce {
verifier = oauth2.GenerateVerifier()
exchangeOpts = append(exchangeOpts, oauth2.VerifierOption(verifier))
}
code := f.newCode(state, oauth2.S256ChallengeFromVerifier(verifier))

return f.locked.Config().Exchange(oidc.ClientContext(context.Background(), f.HTTPClient(nil)), code, exchangeOpts...)
}

// Login does the full OIDC flow starting at the "LoginButton".
Expand Down Expand Up @@ -756,10 +774,16 @@ func (f *FakeIDP) OIDCCallback(t testing.TB, state string, idTokenClaims jwt.Map
panic("cannot use OIDCCallback with WithServing. This is only for the in memory usage")
}

opts := []oauth2.AuthCodeOption{}
if f.pkce {
verifier := oauth2.GenerateVerifier()
opts = append(opts, oauth2.S256ChallengeOption(oauth2.S256ChallengeFromVerifier(verifier)))
}

f.stateToIDTokenClaims.Store(state, idTokenClaims)

cli := f.HTTPClient(nil)
u := f.locked.Config().AuthCodeURL(state)
u := f.locked.Config().AuthCodeURL(state, opts...)
req, err := http.NewRequest("GET", u, nil)
require.NoError(t, err)

Expand Down Expand Up @@ -790,9 +814,10 @@ type ProviderJSON struct {

// newCode enforces the code exchanged is actually a valid code
// created by the IDP.
func (f *FakeIDP) newCode(state string) string {
func (f *FakeIDP) newCode(state string, challenge string) string {
code := uuid.NewString()
f.codeToStateMap.Store(code, state)
f.codeToChallengeMap.Store(code, challenge)
return code
}

Expand Down Expand Up @@ -918,6 +943,22 @@ func (f *FakeIDP) httpHandler(t testing.TB) http.Handler {
mux.Handle(authorizePath, http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
f.logger.Info(r.Context(), "http call authorize", slogRequestFields(r)...)

challenge := ""
if f.pkce {
method := r.URL.Query().Get("code_challenge_method")
challenge = r.URL.Query().Get("code_challenge")

if method == "" {
httpError(rw, http.StatusBadRequest, xerrors.New("missing code_challenge_method"))
return
}

if challenge == "" {
httpError(rw, http.StatusBadRequest, xerrors.New("missing code_challenge"))
return
}
}

clientID := r.URL.Query().Get("client_id")
if !assert.Equal(t, f.clientID, clientID, "unexpected client_id") {
httpError(rw, http.StatusBadRequest, xerrors.New("invalid client_id"))
Expand Down Expand Up @@ -959,7 +1000,7 @@ func (f *FakeIDP) httpHandler(t testing.TB) http.Handler {

q := ru.Query()
q.Set("state", state)
q.Set("code", f.newCode(state))
q.Set("code", f.newCode(state, challenge))
ru.RawQuery = q.Encode()

http.Redirect(rw, r, ru.String(), http.StatusTemporaryRedirect)
Expand Down Expand Up @@ -1009,6 +1050,21 @@ func (f *FakeIDP) httpHandler(t testing.TB) http.Handler {
http.Error(rw, "invalid code", http.StatusBadRequest)
return
}

if f.pkce {
challenge, ok := f.codeToChallengeMap.Load(code)
Copy link
Member

@mafredri mafredri Dec 12, 2025

Choose a reason for hiding this comment

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

We should also clean up this map after use.

if !ok {
httpError(rw, http.StatusBadRequest, xerrors.New("code challenge not found for code"))
Copy link
Member

@mafredri mafredri Dec 12, 2025

Choose a reason for hiding this comment

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

Suggested change
httpError(rw, http.StatusBadRequest, xerrors.New("code challenge not found for code"))
httpError(rw, http.StatusBadRequest, xerrors.New("challenge not found for code"))

This read a bit clearer to me, but if the original is more correct, keep it. Should we mention pkce here to give a hint pkce: challenge ...?

return
}
codeVerifier := values.Get("code_verifier")
Copy link
Member

@mafredri mafredri Dec 12, 2025

Choose a reason for hiding this comment

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

Empty handling?

expecter := oauth2.S256ChallengeFromVerifier(codeVerifier)
if challenge != expecter {
httpError(rw, http.StatusBadRequest, xerrors.New("invalid code verifier"))
Copy link
Member

@mafredri mafredri Dec 12, 2025

Choose a reason for hiding this comment

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

Suggested change
httpError(rw, http.StatusBadRequest, xerrors.New("invalid code verifier"))
httpError(rw, http.StatusBadRequest, xerrors.New("pkce: invalid code verifier"))

?

return
}
}

// Always invalidate the code after it is used.
f.codeToStateMap.Delete(code)

Expand Down
18 changes: 10 additions & 8 deletions coderd/externalauth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
"github.com/coder/coder/v2/coderd/externalauth"
"github.com/coder/coder/v2/coderd/httpapi"
"github.com/coder/coder/v2/coderd/httpmw"
"github.com/coder/coder/v2/coderd/util/slice"
"github.com/coder/coder/v2/codersdk"
)

Expand Down Expand Up @@ -417,14 +418,15 @@ func ExternalAuthConfigs(auths []*externalauth.Config) []codersdk.ExternalAuthLi

func ExternalAuthConfig(cfg *externalauth.Config) codersdk.ExternalAuthLinkProvider {
return codersdk.ExternalAuthLinkProvider{
ID: cfg.ID,
Type: cfg.Type,
Device: cfg.DeviceAuth != nil,
DisplayName: cfg.DisplayName,
DisplayIcon: cfg.DisplayIcon,
AllowRefresh: !cfg.NoRefresh,
AllowValidate: cfg.ValidateURL != "",
SupportsRevocation: cfg.RevokeURL != "",
ID: cfg.ID,
Type: cfg.Type,
Device: cfg.DeviceAuth != nil,
DisplayName: cfg.DisplayName,
DisplayIcon: cfg.DisplayIcon,
AllowRefresh: !cfg.NoRefresh,
AllowValidate: cfg.ValidateURL != "",
SupportsRevocation: cfg.RevokeURL != "",
CodeChallengeMethodsSupported: slice.ToStrings(cfg.CodeChallengeMethodsSupported),
}
}

Expand Down
Loading
Loading