Skip to content

Commit b24bb99

Browse files
blink-so[bot]blink-so[bot]
committed
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
1 parent 3641404 commit b24bb99

File tree

5 files changed

+60
-15
lines changed

5 files changed

+60
-15
lines changed

‎coderd/coderd.go‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -940,7 +940,8 @@ func New(options *Options) *API {
940940
r.Route(fmt.Sprintf("/%s/callback", externalAuthConfig.ID), func(r chi.Router) {
941941
r.Use(
942942
apiKeyMiddlewareRedirect,
943-
httpmw.ExtractOAuth2(externalAuthConfig, options.HTTPClient, options.DeploymentValues.HTTPCookies, nil),
943+
// External auth providers don't use PKCE currently
944+
httpmw.ExtractOAuth2(externalAuthConfig, options.HTTPClient, options.DeploymentValues.HTTPCookies, nil, false),
944945
)
945946
r.Get("/", api.externalAuthCallback(externalAuthConfig))
946947
})
@@ -1289,14 +1290,16 @@ func New(options *Options) *API {
12891290
r.Get("/github/device", api.userOAuth2GithubDevice)
12901291
r.Route("/github", func(r chi.Router) {
12911292
r.Use(
1292-
httpmw.ExtractOAuth2(options.GithubOAuth2Config, options.HTTPClient, options.DeploymentValues.HTTPCookies, nil),
1293+
// GitHub OAuth2 does not use PKCE
1294+
httpmw.ExtractOAuth2(options.GithubOAuth2Config, options.HTTPClient, options.DeploymentValues.HTTPCookies, nil, false),
12931295
)
12941296
r.Get("/callback", api.userOAuth2Github)
12951297
})
12961298
})
12971299
r.Route("/oidc/callback", func(r chi.Router) {
12981300
r.Use(
1299-
httpmw.ExtractOAuth2(options.OIDCConfig, options.HTTPClient, options.DeploymentValues.HTTPCookies, oidcAuthURLParams),
1301+
// PKCE is configurable via CODER_OIDC_PKCE
1302+
httpmw.ExtractOAuth2(options.OIDCConfig, options.HTTPClient, options.DeploymentValues.HTTPCookies, oidcAuthURLParams, options.DeploymentValues.OIDC.PKCEEnabled.Value()),
13001303
)
13011304
r.Get("/", api.userOIDC)
13021305
})

‎coderd/httpmw/oauth2.go‎

Lines changed: 30 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,8 @@ func OAuth2(r *http.Request) OAuth2State {
4040
// a "code" URL parameter will be redirected.
4141
// AuthURLOpts are passed to the AuthCodeURL function. If this is nil,
4242
// the default option oauth2.AccessTypeOffline will be used.
43-
func ExtractOAuth2(config promoauth.OAuth2Config, client *http.Client, cookieCfg codersdk.HTTPCookieConfig, authURLOpts map[string]string) func(http.Handler) http.Handler {
43+
// pkceEnabled enables PKCE (Proof Key for Code Exchange) for enhanced security.
44+
func ExtractOAuth2(config promoauth.OAuth2Config, client *http.Client, cookieCfg codersdk.HTTPCookieConfig, authURLOpts map[string]string, pkceEnabled bool) func(http.Handler) http.Handler {
4445
opts := make([]oauth2.AuthCodeOption, 0, len(authURLOpts)+1)
4546
opts = append(opts, oauth2.AccessTypeOffline)
4647
for k, v := range authURLOpts {
@@ -133,7 +134,20 @@ func ExtractOAuth2(config promoauth.OAuth2Config, client *http.Client, cookieCfg
133134
HttpOnly: true,
134135
}))
135136

136-
http.Redirect(rw, r, config.AuthCodeURL(state, opts...), http.StatusTemporaryRedirect)
137+
// Build auth URL options, adding PKCE if enabled
138+
authOpts := opts
139+
if pkceEnabled {
140+
verifier := oauth2.GenerateVerifier()
141+
http.SetCookie(rw, cookieCfg.Apply(&http.Cookie{
142+
Name: codersdk.OAuth2PKCECookie,
143+
Value: verifier,
144+
Path: "/",
145+
HttpOnly: true,
146+
}))
147+
authOpts = append(authOpts, oauth2.S256ChallengeOption(verifier))
148+
}
149+
150+
http.Redirect(rw, r, config.AuthCodeURL(state, authOpts...), http.StatusTemporaryRedirect)
137151
return
138152
}
139153

@@ -163,7 +177,20 @@ func ExtractOAuth2(config promoauth.OAuth2Config, client *http.Client, cookieCfg
163177
redirect = stateRedirect.Value
164178
}
165179

166-
oauthToken, err := config.Exchange(ctx, code)
180+
// Build exchange options, including PKCE verifier if present
181+
var exchangeOpts []oauth2.AuthCodeOption
182+
if pkceEnabled {
183+
pkceCookie, err := r.Cookie(codersdk.OAuth2PKCECookie)
184+
if err != nil {
185+
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
186+
Message: "PKCE is enabled but verifier cookie is missing.",
187+
})
188+
return
189+
}
190+
exchangeOpts = append(exchangeOpts, oauth2.VerifierOption(pkceCookie.Value))
191+
}
192+
193+
oauthToken, err := config.Exchange(ctx, code, exchangeOpts...)
167194
if err != nil {
168195
errorCode := http.StatusInternalServerError
169196
detail := err.Error()

‎coderd/httpmw/oauth2_test.go‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -50,15 +50,15 @@ func TestOAuth2(t *testing.T) {
5050
t.Parallel()
5151
req := httptest.NewRequest("GET", "/", nil)
5252
res := httptest.NewRecorder()
53-
httpmw.ExtractOAuth2(nil, nil, codersdk.HTTPCookieConfig{}, nil)(nil).ServeHTTP(res, req)
53+
httpmw.ExtractOAuth2(nil, nil, codersdk.HTTPCookieConfig{}, nil, false)(nil).ServeHTTP(res, req)
5454
require.Equal(t, http.StatusBadRequest, res.Result().StatusCode)
5555
})
5656
t.Run("RedirectWithoutCode", func(t *testing.T) {
5757
t.Parallel()
5858
req := httptest.NewRequest("GET", "/?redirect="+url.QueryEscape("/dashboard"), nil)
5959
res := httptest.NewRecorder()
6060
tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline)
61-
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil)(nil).ServeHTTP(res, req)
61+
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil, false)(nil).ServeHTTP(res, req)
6262
location := res.Header().Get("Location")
6363
if !assert.NotEmpty(t, location) {
6464
return
@@ -82,7 +82,7 @@ func TestOAuth2(t *testing.T) {
8282
req := httptest.NewRequest("GET", "/?redirect="+url.QueryEscape(uri.String()), nil)
8383
res := httptest.NewRecorder()
8484
tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline)
85-
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil)(nil).ServeHTTP(res, req)
85+
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil, false)(nil).ServeHTTP(res, req)
8686
location := res.Header().Get("Location")
8787
if !assert.NotEmpty(t, location) {
8888
return
@@ -97,15 +97,15 @@ func TestOAuth2(t *testing.T) {
9797
req := httptest.NewRequest("GET", "/?code=something", nil)
9898
res := httptest.NewRecorder()
9999
tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline)
100-
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil)(nil).ServeHTTP(res, req)
100+
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil, false)(nil).ServeHTTP(res, req)
101101
require.Equal(t, http.StatusBadRequest, res.Result().StatusCode)
102102
})
103103
t.Run("NoStateCookie", func(t *testing.T) {
104104
t.Parallel()
105105
req := httptest.NewRequest("GET", "/?code=something&state=test", nil)
106106
res := httptest.NewRecorder()
107107
tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline)
108-
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil)(nil).ServeHTTP(res, req)
108+
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil, false)(nil).ServeHTTP(res, req)
109109
require.Equal(t, http.StatusUnauthorized, res.Result().StatusCode)
110110
})
111111
t.Run("MismatchedState", func(t *testing.T) {
@@ -117,7 +117,7 @@ func TestOAuth2(t *testing.T) {
117117
})
118118
res := httptest.NewRecorder()
119119
tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline)
120-
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil)(nil).ServeHTTP(res, req)
120+
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil, false)(nil).ServeHTTP(res, req)
121121
require.Equal(t, http.StatusUnauthorized, res.Result().StatusCode)
122122
})
123123
t.Run("ExchangeCodeAndState", func(t *testing.T) {
@@ -133,7 +133,7 @@ func TestOAuth2(t *testing.T) {
133133
})
134134
res := httptest.NewRecorder()
135135
tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline)
136-
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil)(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) {
136+
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil, false)(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) {
137137
state := httpmw.OAuth2(r)
138138
require.Equal(t, "/dashboard", state.Redirect)
139139
})).ServeHTTP(res, req)
@@ -144,7 +144,7 @@ func TestOAuth2(t *testing.T) {
144144
res := httptest.NewRecorder()
145145
tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline, oauth2.SetAuthURLParam("foo", "bar"))
146146
authOpts := map[string]string{"foo": "bar"}
147-
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, authOpts)(nil).ServeHTTP(res, req)
147+
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, authOpts, false)(nil).ServeHTTP(res, req)
148148
location := res.Header().Get("Location")
149149
// Ideally we would also assert that the location contains the query params
150150
// we set in the auth URL but this would essentially be testing the oauth2 package.
@@ -160,7 +160,7 @@ func TestOAuth2(t *testing.T) {
160160
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{
161161
Secure: true,
162162
SameSite: "none",
163-
}, nil)(nil).ServeHTTP(res, req)
163+
}, nil, false)(nil).ServeHTTP(res, req)
164164

165165
found := false
166166
for _, cookie := range res.Result().Cookies() {

‎codersdk/client.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,8 @@ const (
4040
OAuth2StateCookie = "oauth_state"
4141
// OAuth2RedirectCookie is the name of the cookie that stores the oauth2 redirect.
4242
OAuth2RedirectCookie = "oauth_redirect"
43+
// OAuth2PKCECookie is the name of the cookie that stores the PKCE code verifier.
44+
OAuth2PKCECookie = "oauth_pkce"
4345

4446
// PathAppSessionTokenCookie is the name of the cookie that stores an
4547
// application-scoped API token on workspace proxy path app domains.

‎codersdk/deployment.go‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -680,6 +680,9 @@ type OIDCConfig struct {
680680
IconURL serpent.URL `json:"icon_url" typescript:",notnull"`
681681
SignupsDisabledText serpent.String `json:"signups_disabled_text" typescript:",notnull"`
682682
SkipIssuerChecks serpent.Bool `json:"skip_issuer_checks" typescript:",notnull"`
683+
// PKCEEnabled enables PKCE (Proof Key for Code Exchange) for the OIDC flow.
684+
// This is recommended for enhanced security, especially for public clients.
685+
PKCEEnabled serpent.Bool `json:"pkce_enabled" typescript:",notnull"`
683686
}
684687

685688
type TelemetryConfig struct {
@@ -2186,6 +2189,16 @@ func (c *DeploymentValues) Options() serpent.OptionSet {
21862189
Group: &deploymentGroupOIDC,
21872190
YAML: "dangerousSkipIssuerChecks",
21882191
},
2192+
{
2193+
Name: "OIDC PKCE",
2194+
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.",
2195+
Flag: "oidc-pkce",
2196+
Env: "CODER_OIDC_PKCE",
2197+
Default: "false",
2198+
Value: &c.OIDC.PKCEEnabled,
2199+
Group: &deploymentGroupOIDC,
2200+
YAML: "pkce",
2201+
},
21892202
// Telemetry settings
21902203
telemetryEnable,
21912204
{

0 commit comments

Comments
 (0)