From b24bb99c262cecefc6a1c5ec23758f4e96813879 Mon Sep 17 00:00:00 2001 From: "blink-so[bot]" <211532188+blink-so[bot]@users.noreply.github.com> Date: Wed, 10 Dec 2025 09:49:19 +0000 Subject: [PATCH] feat: add PKCE support for OIDC authentication This adds support for PKCE (Proof Key for Code Exchange) when Coder acts as an OIDC client. PKCE is an OAuth 2.0 extension that prevents authorization code interception attacks, making authentication more secure especially for public clients. ## Changes - Add `CODER_OIDC_PKCE` environment variable (default: false) - Add `--oidc-pkce` flag to enable PKCE for OIDC authentication - Modify ExtractOAuth2 middleware to: - Generate PKCE code verifier when initiating auth flow - Store verifier in HttpOnly cookie - Include code_challenge (S256) in authorization request - Pass code_verifier during token exchange - Update all ExtractOAuth2 call sites with pkceEnabled parameter ## Configuration To enable PKCE for OIDC authentication: ```bash export CODER_OIDC_PKCE=true ``` Or via CLI: ```bash coder server --oidc-pkce ``` ## Security - Uses S256 challenge method (SHA256 hash of verifier) - Verifier stored in HttpOnly cookie for CSRF protection - Compatible with all major IdPs that support PKCE (Okta, Azure AD, Keycloak, etc.) Related: RFC 7636 - Proof Key for Code Exchange --- coderd/coderd.go | 9 ++++++--- coderd/httpmw/oauth2.go | 33 ++++++++++++++++++++++++++++++--- coderd/httpmw/oauth2_test.go | 18 +++++++++--------- codersdk/client.go | 2 ++ codersdk/deployment.go | 13 +++++++++++++ 5 files changed, 60 insertions(+), 15 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index b356f372dc56c..ab21d8607af1d 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -940,7 +940,8 @@ 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), + // External auth providers don't use PKCE currently + httpmw.ExtractOAuth2(externalAuthConfig, options.HTTPClient, options.DeploymentValues.HTTPCookies, nil, false), ) r.Get("/", api.externalAuthCallback(externalAuthConfig)) }) @@ -1289,14 +1290,16 @@ 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 OAuth2 does not use PKCE + httpmw.ExtractOAuth2(options.GithubOAuth2Config, options.HTTPClient, options.DeploymentValues.HTTPCookies, nil, false), ) 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), + // PKCE is configurable via CODER_OIDC_PKCE + httpmw.ExtractOAuth2(options.OIDCConfig, options.HTTPClient, options.DeploymentValues.HTTPCookies, oidcAuthURLParams, options.DeploymentValues.OIDC.PKCEEnabled.Value()), ) r.Get("/", api.userOIDC) }) diff --git a/coderd/httpmw/oauth2.go b/coderd/httpmw/oauth2.go index 28e6400c8a5a4..54569fd15be7b 100644 --- a/coderd/httpmw/oauth2.go +++ b/coderd/httpmw/oauth2.go @@ -40,7 +40,8 @@ func OAuth2(r *http.Request) OAuth2State { // a "code" URL parameter will be redirected. // AuthURLOpts are passed to the AuthCodeURL function. If this is nil, // the default option oauth2.AccessTypeOffline will be used. -func ExtractOAuth2(config promoauth.OAuth2Config, client *http.Client, cookieCfg codersdk.HTTPCookieConfig, authURLOpts map[string]string) func(http.Handler) http.Handler { +// pkceEnabled enables PKCE (Proof Key for Code Exchange) for enhanced security. +func ExtractOAuth2(config promoauth.OAuth2Config, client *http.Client, cookieCfg codersdk.HTTPCookieConfig, authURLOpts map[string]string, pkceEnabled bool) func(http.Handler) http.Handler { opts := make([]oauth2.AuthCodeOption, 0, len(authURLOpts)+1) opts = append(opts, oauth2.AccessTypeOffline) for k, v := range authURLOpts { @@ -133,7 +134,20 @@ func ExtractOAuth2(config promoauth.OAuth2Config, client *http.Client, cookieCfg HttpOnly: true, })) - http.Redirect(rw, r, config.AuthCodeURL(state, opts...), http.StatusTemporaryRedirect) + // Build auth URL options, adding PKCE if enabled + authOpts := opts + if pkceEnabled { + verifier := oauth2.GenerateVerifier() + http.SetCookie(rw, cookieCfg.Apply(&http.Cookie{ + Name: codersdk.OAuth2PKCECookie, + Value: verifier, + Path: "/", + HttpOnly: true, + })) + authOpts = append(authOpts, oauth2.S256ChallengeOption(verifier)) + } + + http.Redirect(rw, r, config.AuthCodeURL(state, authOpts...), http.StatusTemporaryRedirect) return } @@ -163,7 +177,20 @@ func ExtractOAuth2(config promoauth.OAuth2Config, client *http.Client, cookieCfg redirect = stateRedirect.Value } - oauthToken, err := config.Exchange(ctx, code) + // Build exchange options, including PKCE verifier if present + var exchangeOpts []oauth2.AuthCodeOption + if pkceEnabled { + pkceCookie, err := r.Cookie(codersdk.OAuth2PKCECookie) + if err != nil { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "PKCE is enabled but verifier cookie is missing.", + }) + return + } + exchangeOpts = append(exchangeOpts, oauth2.VerifierOption(pkceCookie.Value)) + } + + oauthToken, err := config.Exchange(ctx, code, exchangeOpts...) if err != nil { errorCode := http.StatusInternalServerError detail := err.Error() diff --git a/coderd/httpmw/oauth2_test.go b/coderd/httpmw/oauth2_test.go index 9739735f3eaf7..ccccf90e20db2 100644 --- a/coderd/httpmw/oauth2_test.go +++ b/coderd/httpmw/oauth2_test.go @@ -50,7 +50,7 @@ func TestOAuth2(t *testing.T) { t.Parallel() req := httptest.NewRequest("GET", "/", nil) res := httptest.NewRecorder() - httpmw.ExtractOAuth2(nil, nil, codersdk.HTTPCookieConfig{}, nil)(nil).ServeHTTP(res, req) + httpmw.ExtractOAuth2(nil, nil, codersdk.HTTPCookieConfig{}, nil, false)(nil).ServeHTTP(res, req) require.Equal(t, http.StatusBadRequest, res.Result().StatusCode) }) t.Run("RedirectWithoutCode", func(t *testing.T) { @@ -58,7 +58,7 @@ func TestOAuth2(t *testing.T) { req := httptest.NewRequest("GET", "/?redirect="+url.QueryEscape("/dashboard"), nil) res := httptest.NewRecorder() tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline) - httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil)(nil).ServeHTTP(res, req) + httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil, false)(nil).ServeHTTP(res, req) location := res.Header().Get("Location") if !assert.NotEmpty(t, location) { return @@ -82,7 +82,7 @@ func TestOAuth2(t *testing.T) { req := httptest.NewRequest("GET", "/?redirect="+url.QueryEscape(uri.String()), nil) res := httptest.NewRecorder() tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline) - httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil)(nil).ServeHTTP(res, req) + httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil, false)(nil).ServeHTTP(res, req) location := res.Header().Get("Location") if !assert.NotEmpty(t, location) { return @@ -97,7 +97,7 @@ func TestOAuth2(t *testing.T) { req := httptest.NewRequest("GET", "/?code=something", nil) res := httptest.NewRecorder() tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline) - httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil)(nil).ServeHTTP(res, req) + httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil, false)(nil).ServeHTTP(res, req) require.Equal(t, http.StatusBadRequest, res.Result().StatusCode) }) t.Run("NoStateCookie", func(t *testing.T) { @@ -105,7 +105,7 @@ func TestOAuth2(t *testing.T) { req := httptest.NewRequest("GET", "/?code=something&state=test", nil) res := httptest.NewRecorder() tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline) - httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil)(nil).ServeHTTP(res, req) + httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil, false)(nil).ServeHTTP(res, req) require.Equal(t, http.StatusUnauthorized, res.Result().StatusCode) }) t.Run("MismatchedState", func(t *testing.T) { @@ -117,7 +117,7 @@ func TestOAuth2(t *testing.T) { }) res := httptest.NewRecorder() tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline) - httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil)(nil).ServeHTTP(res, req) + httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil, false)(nil).ServeHTTP(res, req) require.Equal(t, http.StatusUnauthorized, res.Result().StatusCode) }) t.Run("ExchangeCodeAndState", func(t *testing.T) { @@ -133,7 +133,7 @@ func TestOAuth2(t *testing.T) { }) res := httptest.NewRecorder() tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline) - httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil)(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil, false)(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { state := httpmw.OAuth2(r) require.Equal(t, "/dashboard", state.Redirect) })).ServeHTTP(res, req) @@ -144,7 +144,7 @@ func TestOAuth2(t *testing.T) { res := httptest.NewRecorder() tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline, oauth2.SetAuthURLParam("foo", "bar")) authOpts := map[string]string{"foo": "bar"} - httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, authOpts)(nil).ServeHTTP(res, req) + httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, authOpts, false)(nil).ServeHTTP(res, req) location := res.Header().Get("Location") // Ideally we would also assert that the location contains the query params // we set in the auth URL but this would essentially be testing the oauth2 package. @@ -160,7 +160,7 @@ func TestOAuth2(t *testing.T) { httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{ Secure: true, SameSite: "none", - }, nil)(nil).ServeHTTP(res, req) + }, nil, false)(nil).ServeHTTP(res, req) found := false for _, cookie := range res.Result().Cookies() { diff --git a/codersdk/client.go b/codersdk/client.go index 72dd7ac4b64f4..6c42d7456b136 100644 --- a/codersdk/client.go +++ b/codersdk/client.go @@ -40,6 +40,8 @@ const ( OAuth2StateCookie = "oauth_state" // OAuth2RedirectCookie is the name of the cookie that stores the oauth2 redirect. OAuth2RedirectCookie = "oauth_redirect" + // OAuth2PKCECookie is the name of the cookie that stores the PKCE code verifier. + OAuth2PKCECookie = "oauth_pkce" // PathAppSessionTokenCookie is the name of the cookie that stores an // application-scoped API token on workspace proxy path app domains. diff --git a/codersdk/deployment.go b/codersdk/deployment.go index 0dd082ab5eebc..b2f12d7674d7f 100644 --- a/codersdk/deployment.go +++ b/codersdk/deployment.go @@ -680,6 +680,9 @@ type OIDCConfig struct { IconURL serpent.URL `json:"icon_url" typescript:",notnull"` SignupsDisabledText serpent.String `json:"signups_disabled_text" typescript:",notnull"` SkipIssuerChecks serpent.Bool `json:"skip_issuer_checks" typescript:",notnull"` + // PKCEEnabled enables PKCE (Proof Key for Code Exchange) for the OIDC flow. + // This is recommended for enhanced security, especially for public clients. + PKCEEnabled serpent.Bool `json:"pkce_enabled" typescript:",notnull"` } type TelemetryConfig struct { @@ -2186,6 +2189,16 @@ func (c *DeploymentValues) Options() serpent.OptionSet { Group: &deploymentGroupOIDC, YAML: "dangerousSkipIssuerChecks", }, + { + Name: "OIDC PKCE", + Description: "Enable PKCE (Proof Key for Code Exchange) for OIDC authentication. This adds an extra layer of security to the authorization code flow by preventing authorization code interception attacks.", + Flag: "oidc-pkce", + Env: "CODER_OIDC_PKCE", + Default: "false", + Value: &c.OIDC.PKCEEnabled, + Group: &deploymentGroupOIDC, + YAML: "pkce", + }, // Telemetry settings telemetryEnable, {