Skip to content

Commit 8fefd91

Browse files
EmyrkEmyrk
authored
feat!: support PKCE in the oauth2 client's auth/exchange flow (#21215)
**Breaking Change:** Existing oauth apps might now use PKCE. If an unknown IdP type was being used, and it does not support PKCE, it will break. To fix, set the PKCE methods on the external auth to `none` ``` export CODER_EXTERNAL_AUTH_1_PKCE_METHODS=none ```
1 parent 3194bcf commit 8fefd91

File tree

26 files changed

+473
-169
lines changed

26 files changed

+473
-169
lines changed

‎cli/server.go‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -186,6 +186,14 @@ func createOIDCConfig(ctx context.Context, logger slog.Logger, vals *codersdk.De
186186
secondaryClaimsSrc = coderd.MergedClaimsSourceAccessToken
187187
}
188188

189+
var pkceSupport struct {
190+
CodeChallengeMethodsSupported []promoauth.Oauth2PKCEChallengeMethod `json:"code_challenge_methods_supported"`
191+
}
192+
err = oidcProvider.Claims(&pkceSupport)
193+
if err != nil {
194+
return nil, xerrors.Errorf("pkce detect in claims: %w", err)
195+
}
196+
189197
return &coderd.OIDCConfig{
190198
OAuth2Config: useCfg,
191199
Provider: oidcProvider,
@@ -206,6 +214,7 @@ func createOIDCConfig(ctx context.Context, logger slog.Logger, vals *codersdk.De
206214
SignupsDisabledText: vals.OIDC.SignupsDisabledText.String(),
207215
IconURL: vals.OIDC.IconURL.String(),
208216
IgnoreEmailVerified: vals.OIDC.IgnoreEmailVerified.Value(),
217+
PKCEMethods: pkceSupport.CodeChallengeMethodsSupported,
209218
}, nil
210219
}
211220

@@ -2761,6 +2770,8 @@ func parseExternalAuthProvidersFromEnv(prefix string, environ []string) ([]coder
27612770
provider.MCPToolAllowRegex = v.Value
27622771
case "MCP_TOOL_DENY_REGEX":
27632772
provider.MCPToolDenyRegex = v.Value
2773+
case "PKCE_METHODS":
2774+
provider.CodeChallengeMethodsSupported = strings.Split(v.Value, " ")
27642775
}
27652776
providers[providerNum] = provider
27662777
}

‎coderd/apidoc/docs.go‎

Lines changed: 7 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/apidoc/swagger.json‎

Lines changed: 7 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/coderd.go‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -941,7 +941,7 @@ func New(options *Options) *API {
941941
r.Route(fmt.Sprintf("/%s/callback", externalAuthConfig.ID), func(r chi.Router) {
942942
r.Use(
943943
apiKeyMiddlewareRedirect,
944-
httpmw.ExtractOAuth2(externalAuthConfig, options.HTTPClient, options.DeploymentValues.HTTPCookies, nil),
944+
httpmw.ExtractOAuth2(externalAuthConfig, options.HTTPClient, options.DeploymentValues.HTTPCookies, nil, externalAuthConfig.CodeChallengeMethodsSupported),
945945
)
946946
r.Get("/", api.externalAuthCallback(externalAuthConfig))
947947
})
@@ -1290,14 +1290,15 @@ func New(options *Options) *API {
12901290
r.Get("/github/device", api.userOAuth2GithubDevice)
12911291
r.Route("/github", func(r chi.Router) {
12921292
r.Use(
1293-
httpmw.ExtractOAuth2(options.GithubOAuth2Config, options.HTTPClient, options.DeploymentValues.HTTPCookies, nil),
1293+
// Github supports PKCE S256
1294+
httpmw.ExtractOAuth2(options.GithubOAuth2Config, options.HTTPClient, options.DeploymentValues.HTTPCookies, nil, options.GithubOAuth2Config.PKCESupported()),
12941295
)
12951296
r.Get("/callback", api.userOAuth2Github)
12961297
})
12971298
})
12981299
r.Route("/oidc/callback", func(r chi.Router) {
12991300
r.Use(
1300-
httpmw.ExtractOAuth2(options.OIDCConfig, options.HTTPClient, options.DeploymentValues.HTTPCookies, oidcAuthURLParams),
1301+
httpmw.ExtractOAuth2(options.OIDCConfig, options.HTTPClient, options.DeploymentValues.HTTPCookies, oidcAuthURLParams, options.OIDCConfig.PKCESupported()),
13011302
)
13021303
r.Get("/", api.userOIDC)
13031304
})

‎coderd/coderdtest/oidctest/idp.go‎

Lines changed: 66 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -169,6 +169,7 @@ type FakeIDP struct {
169169
// clientID to be used by coderd
170170
clientID string
171171
clientSecret string
172+
pkce bool // TODO(Emyrk): Implement for refresh token flow as well
172173
// externalProviderID is optional to match the provider in coderd for
173174
// redirectURLs.
174175
externalProviderID string
@@ -181,6 +182,8 @@ type FakeIDP struct {
181182
// These maps are used to control the state of the IDP.
182183
// That is the various access tokens, refresh tokens, states, etc.
183184
codeToStateMap *syncmap.Map[string, string]
185+
// Code -> PKCE Challenge
186+
codeToChallengeMap *syncmap.Map[string, string]
184187
// Token -> Email
185188
accessTokens *syncmap.Map[string, token]
186189
// Refresh Token -> Email
@@ -239,6 +242,12 @@ func (s statusHookError) Error() string {
239242

240243
type FakeIDPOpt func(idp *FakeIDP)
241244

245+
func WithPKCE() func(*FakeIDP) {
246+
return func(f *FakeIDP) {
247+
f.pkce = true
248+
}
249+
}
250+
242251
func WithAuthorizedRedirectURL(hook func(redirectURL string) error) func(*FakeIDP) {
243252
return func(f *FakeIDP) {
244253
f.hookValidRedirectURL = hook
@@ -450,6 +459,7 @@ func NewFakeIDP(t testing.TB, opts ...FakeIDPOpt) *FakeIDP {
450459
clientSecret: uuid.NewString(),
451460
logger: slog.Make(),
452461
codeToStateMap: syncmap.New[string, string](),
462+
codeToChallengeMap: syncmap.New[string, string](),
453463
accessTokens: syncmap.New[string, token](),
454464
refreshTokens: syncmap.New[string, string](),
455465
refreshTokensUsed: syncmap.New[string, bool](),
@@ -557,8 +567,16 @@ func (f *FakeIDP) realServer(t testing.TB) *httptest.Server {
557567
func (f *FakeIDP) GenerateAuthenticatedToken(claims jwt.MapClaims) (*oauth2.Token, error) {
558568
state := uuid.NewString()
559569
f.stateToIDTokenClaims.Store(state, claims)
560-
code := f.newCode(state)
561-
return f.locked.Config().Exchange(oidc.ClientContext(context.Background(), f.HTTPClient(nil)), code)
570+
571+
exchangeOpts := []oauth2.AuthCodeOption{}
572+
verifier := ""
573+
if f.pkce {
574+
verifier = oauth2.GenerateVerifier()
575+
exchangeOpts = append(exchangeOpts, oauth2.VerifierOption(verifier))
576+
}
577+
code := f.newCode(state, oauth2.S256ChallengeFromVerifier(verifier))
578+
579+
return f.locked.Config().Exchange(oidc.ClientContext(context.Background(), f.HTTPClient(nil)), code, exchangeOpts...)
562580
}
563581

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

777+
opts := []oauth2.AuthCodeOption{}
778+
if f.pkce {
779+
verifier := oauth2.GenerateVerifier()
780+
opts = append(opts, oauth2.S256ChallengeOption(oauth2.S256ChallengeFromVerifier(verifier)))
781+
}
782+
759783
f.stateToIDTokenClaims.Store(state, idTokenClaims)
760784

761785
cli := f.HTTPClient(nil)
762-
u := f.locked.Config().AuthCodeURL(state)
786+
u := f.locked.Config().AuthCodeURL(state, opts...)
763787
req, err := http.NewRequest("GET", u, nil)
764788
require.NoError(t, err)
765789

@@ -790,9 +814,10 @@ type ProviderJSON struct {
790814

791815
// newCode enforces the code exchanged is actually a valid code
792816
// created by the IDP.
793-
func (f *FakeIDP) newCode(state string) string {
817+
func (f *FakeIDP) newCode(state string, challenge string) string {
794818
code := uuid.NewString()
795819
f.codeToStateMap.Store(code, state)
820+
f.codeToChallengeMap.Store(code, challenge)
796821
return code
797822
}
798823

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

946+
challenge := ""
947+
if f.pkce {
948+
method := r.URL.Query().Get("code_challenge_method")
949+
challenge = r.URL.Query().Get("code_challenge")
950+
951+
if method == "" {
952+
httpError(rw, http.StatusBadRequest, xerrors.New("missing code_challenge_method"))
953+
return
954+
}
955+
956+
if challenge == "" {
957+
httpError(rw, http.StatusBadRequest, xerrors.New("missing code_challenge"))
958+
return
959+
}
960+
}
961+
921962
clientID := r.URL.Query().Get("client_id")
922963
if !assert.Equal(t, f.clientID, clientID, "unexpected client_id") {
923964
httpError(rw, http.StatusBadRequest, xerrors.New("invalid client_id"))
@@ -959,7 +1000,7 @@ func (f *FakeIDP) httpHandler(t testing.TB) http.Handler {
9591000

9601001
q := ru.Query()
9611002
q.Set("state", state)
962-
q.Set("code", f.newCode(state))
1003+
q.Set("code", f.newCode(state, challenge))
9631004
ru.RawQuery = q.Encode()
9641005

9651006
http.Redirect(rw, r, ru.String(), http.StatusTemporaryRedirect)
@@ -1009,8 +1050,28 @@ func (f *FakeIDP) httpHandler(t testing.TB) http.Handler {
10091050
http.Error(rw, "invalid code", http.StatusBadRequest)
10101051
return
10111052
}
1053+
1054+
if f.pkce {
1055+
challenge, ok := f.codeToChallengeMap.Load(code)
1056+
if !ok {
1057+
httpError(rw, http.StatusBadRequest, xerrors.New("pkce: challenge not found for code"))
1058+
return
1059+
}
1060+
codeVerifier := values.Get("code_verifier")
1061+
if codeVerifier == "" {
1062+
httpError(rw, http.StatusBadRequest, xerrors.New("pkce: missing code_verifier"))
1063+
return
1064+
}
1065+
expecter := oauth2.S256ChallengeFromVerifier(codeVerifier)
1066+
if challenge != expecter {
1067+
httpError(rw, http.StatusBadRequest, xerrors.New("pkce: invalid code verifier"))
1068+
return
1069+
}
1070+
}
1071+
10121072
// Always invalidate the code after it is used.
10131073
f.codeToStateMap.Delete(code)
1074+
f.codeToChallengeMap.Delete(code)
10141075

10151076
idTokenClaims, ok := f.getClaims(f.stateToIDTokenClaims, stateStr)
10161077
if !ok {

‎coderd/externalauth.go‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import (
1616
"github.com/coder/coder/v2/coderd/externalauth"
1717
"github.com/coder/coder/v2/coderd/httpapi"
1818
"github.com/coder/coder/v2/coderd/httpmw"
19+
"github.com/coder/coder/v2/coderd/util/slice"
1920
"github.com/coder/coder/v2/codersdk"
2021
)
2122

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

418419
func ExternalAuthConfig(cfg *externalauth.Config) codersdk.ExternalAuthLinkProvider {
419420
return codersdk.ExternalAuthLinkProvider{
420-
ID: cfg.ID,
421-
Type: cfg.Type,
422-
Device: cfg.DeviceAuth != nil,
423-
DisplayName: cfg.DisplayName,
424-
DisplayIcon: cfg.DisplayIcon,
425-
AllowRefresh: !cfg.NoRefresh,
426-
AllowValidate: cfg.ValidateURL != "",
427-
SupportsRevocation: cfg.RevokeURL != "",
421+
ID: cfg.ID,
422+
Type: cfg.Type,
423+
Device: cfg.DeviceAuth != nil,
424+
DisplayName: cfg.DisplayName,
425+
DisplayIcon: cfg.DisplayIcon,
426+
AllowRefresh: !cfg.NoRefresh,
427+
AllowValidate: cfg.ValidateURL != "",
428+
SupportsRevocation: cfg.RevokeURL != "",
429+
CodeChallengeMethodsSupported: slice.ToStrings(cfg.CodeChallengeMethodsSupported),
428430
}
429431
}
430432

0 commit comments

Comments
 (0)