From 8fefd91e4a70990dcb30f2ac81b72a168ee48458 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 15 Dec 2025 11:41:47 -0600 Subject: [PATCH] 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 ``` --- cli/server.go | 11 ++ coderd/apidoc/docs.go | 7 + coderd/apidoc/swagger.json | 7 + coderd/coderd.go | 7 +- coderd/coderdtest/oidctest/idp.go | 71 ++++++++- coderd/externalauth.go | 18 ++- coderd/externalauth/externalauth.go | 110 ++++++++------ .../externalauth_internal_test.go | 141 ++++++++++++------ coderd/externalauth/externalauth_test.go | 16 +- coderd/externalauth_test.go | 19 +++ coderd/httpmw/oauth2.go | 38 ++++- coderd/httpmw/oauth2_test.go | 18 +-- coderd/promoauth/oauth2.go | 7 + coderd/userauth.go | 13 ++ coderd/userauth_test.go | 8 + codersdk/client.go | 4 + codersdk/deployment.go | 6 +- codersdk/deployment_test.go | 43 +++--- codersdk/externalauth.go | 17 ++- codersdk/testdata/githubcfg.yaml | 2 + docs/admin/external-auth/index.md | 17 +++ docs/reference/api/general.md | 3 + docs/reference/api/schemas.md | 43 ++++-- site/src/api/typesGenerated.ts | 14 ++ .../ExternalAuthSettingsPageView.stories.tsx | 1 + site/src/testHelpers/entities.ts | 1 + 26 files changed, 473 insertions(+), 169 deletions(-) diff --git a/cli/server.go b/cli/server.go index e8e2d24de1..dbc35141e8 100644 --- a/cli/server.go +++ b/cli/server.go @@ -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, @@ -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 } @@ -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 } diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index 91f0f0d98c..0bb555de96 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -14640,6 +14640,13 @@ const docTemplate = `{ "client_id": { "type": "string" }, + "code_challenge_methods_supported": { + "description": "CodeChallengeMethodsSupported lists the PKCE code challenge methods\nThe only one supported by Coder is \"S256\".", + "type": "array", + "items": { + "type": "string" + } + }, "device_code_url": { "type": "string" }, diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index b1803bd143..aa4af943bf 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -13217,6 +13217,13 @@ "client_id": { "type": "string" }, + "code_challenge_methods_supported": { + "description": "CodeChallengeMethodsSupported lists the PKCE code challenge methods\nThe only one supported by Coder is \"S256\".", + "type": "array", + "items": { + "type": "string" + } + }, "device_code_url": { "type": "string" }, diff --git a/coderd/coderd.go b/coderd/coderd.go index e08a2a3036..720f1236a6 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -941,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)) }) @@ -1290,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 + httpmw.ExtractOAuth2(options.GithubOAuth2Config, options.HTTPClient, options.DeploymentValues.HTTPCookies, nil, options.GithubOAuth2Config.PKCESupported()), ) 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) }) diff --git a/coderd/coderdtest/oidctest/idp.go b/coderd/coderdtest/oidctest/idp.go index d5215b9964..8d9276d18a 100644 --- a/coderd/coderdtest/oidctest/idp.go +++ b/coderd/coderdtest/oidctest/idp.go @@ -169,6 +169,7 @@ type FakeIDP struct { // clientID to be used by coderd clientID string clientSecret string + pkce bool // TODO(Emyrk): Implement for refresh token flow as well // externalProviderID is optional to match the provider in coderd for // redirectURLs. externalProviderID string @@ -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 @@ -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 @@ -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](), @@ -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". @@ -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) @@ -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 } @@ -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")) @@ -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) @@ -1009,8 +1050,28 @@ 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) + if !ok { + httpError(rw, http.StatusBadRequest, xerrors.New("pkce: challenge not found for code")) + return + } + codeVerifier := values.Get("code_verifier") + if codeVerifier == "" { + httpError(rw, http.StatusBadRequest, xerrors.New("pkce: missing code_verifier")) + return + } + expecter := oauth2.S256ChallengeFromVerifier(codeVerifier) + if challenge != expecter { + httpError(rw, http.StatusBadRequest, xerrors.New("pkce: invalid code verifier")) + return + } + } + // Always invalidate the code after it is used. f.codeToStateMap.Delete(code) + f.codeToChallengeMap.Delete(code) idTokenClaims, ok := f.getClaims(f.stateToIDTokenClaims, stateStr) if !ok { diff --git a/coderd/externalauth.go b/coderd/externalauth.go index 23ae7e9fe2..95978a5ac8 100644 --- a/coderd/externalauth.go +++ b/coderd/externalauth.go @@ -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" ) @@ -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), } } diff --git a/coderd/externalauth/externalauth.go b/coderd/externalauth/externalauth.go index f33a9d3670..2957efd698 100644 --- a/coderd/externalauth/externalauth.go +++ b/coderd/externalauth/externalauth.go @@ -25,6 +25,7 @@ import ( "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/dbtime" "github.com/coder/coder/v2/coderd/promoauth" + "github.com/coder/coder/v2/coderd/util/slice" "github.com/coder/coder/v2/codersdk" "github.com/coder/retry" ) @@ -102,7 +103,8 @@ type Config struct { // injected into Coder AI Bridge upstream requests. // In the case of conflicts, items evaluated by this list override [MCPToolAllowRegex]. // This field can be nil if unspecified in the config. - MCPToolDenyRegex *regexp.Regexp + MCPToolDenyRegex *regexp.Regexp + CodeChallengeMethodsSupported []promoauth.Oauth2PKCEChallengeMethod } // GenerateTokenExtra generates the extra token data to store in the database. @@ -723,24 +725,25 @@ func ConvertConfig(instrument *promoauth.Factory, entries []codersdk.ExternalAut } cfg := &Config{ - InstrumentedOAuth2Config: instrumented, - ID: entry.ID, - ClientID: entry.ClientID, - ClientSecret: entry.ClientSecret, - Regex: regex, - Type: entry.Type, - NoRefresh: entry.NoRefresh, - ValidateURL: entry.ValidateURL, - RevokeURL: entry.RevokeURL, - RevokeTimeout: tokenRevocationTimeout, - AppInstallationsURL: entry.AppInstallationsURL, - AppInstallURL: entry.AppInstallURL, - DisplayName: entry.DisplayName, - DisplayIcon: entry.DisplayIcon, - ExtraTokenKeys: entry.ExtraTokenKeys, - MCPURL: entry.MCPURL, - MCPToolAllowRegex: mcpToolAllow, - MCPToolDenyRegex: mcpToolDeny, + InstrumentedOAuth2Config: instrumented, + ID: entry.ID, + ClientID: entry.ClientID, + ClientSecret: entry.ClientSecret, + Regex: regex, + Type: entry.Type, + NoRefresh: entry.NoRefresh, + ValidateURL: entry.ValidateURL, + RevokeURL: entry.RevokeURL, + RevokeTimeout: tokenRevocationTimeout, + AppInstallationsURL: entry.AppInstallationsURL, + AppInstallURL: entry.AppInstallURL, + DisplayName: entry.DisplayName, + DisplayIcon: entry.DisplayIcon, + ExtraTokenKeys: entry.ExtraTokenKeys, + MCPURL: entry.MCPURL, + MCPToolAllowRegex: mcpToolAllow, + MCPToolDenyRegex: mcpToolDeny, + CodeChallengeMethodsSupported: slice.StringEnums[promoauth.Oauth2PKCEChallengeMethod](entry.CodeChallengeMethodsSupported), } if entry.DeviceFlow { @@ -800,8 +803,7 @@ func applyDefaultsToConfig(config *codersdk.ExternalAuthConfig) { copyDefaultSettings(config, azureDevopsEntraDefaults(config)) return default: - // No defaults for this type. We still want to run this apply with - // an empty set of defaults. + // Global defaults are specified at the end of the `copyDefaultSettings` function. copyDefaultSettings(config, codersdk.ExternalAuthConfig{}) return } @@ -844,6 +846,9 @@ func copyDefaultSettings(config *codersdk.ExternalAuthConfig, defaults codersdk. if len(config.ExtraTokenKeys) == 0 { config.ExtraTokenKeys = defaults.ExtraTokenKeys } + if config.CodeChallengeMethodsSupported == nil { + config.CodeChallengeMethodsSupported = defaults.CodeChallengeMethodsSupported + } // Apply defaults if it's still empty... if config.ID == "" { @@ -856,6 +861,9 @@ func copyDefaultSettings(config *codersdk.ExternalAuthConfig, defaults codersdk. // This is a key emoji. config.DisplayIcon = "/emojis/1f511.png" } + if config.CodeChallengeMethodsSupported == nil { + config.CodeChallengeMethodsSupported = []string{string(promoauth.PKCEChallengeMethodSha256)} + } } // gitHubDefaults returns default config values for GitHub. @@ -869,9 +877,10 @@ func gitHubDefaults(config *codersdk.ExternalAuthConfig) codersdk.ExternalAuthCo DisplayIcon: "/icon/github.svg", Regex: `^(https?://)?github\.com(/.*)?$`, // "workflow" is required for managing GitHub Actions in a repository. - Scopes: []string{"repo", "workflow"}, - DeviceCodeURL: "https://github.com/login/device/code", - AppInstallationsURL: "https://api.github.com/user/installations", + Scopes: []string{"repo", "workflow"}, + DeviceCodeURL: "https://github.com/login/device/code", + AppInstallationsURL: "https://api.github.com/user/installations", + CodeChallengeMethodsSupported: []string{string(promoauth.PKCEChallengeMethodSha256)}, } if config.RevokeURL == "" && config.ClientID != "" { @@ -886,6 +895,8 @@ func bitbucketServerDefaults(config *codersdk.ExternalAuthConfig) codersdk.Exter DisplayName: "Bitbucket Server", Scopes: []string{"PUBLIC_REPOS", "REPO_READ", "REPO_WRITE"}, DisplayIcon: "/icon/bitbucket.svg", + // TODO: Investigate if 'S256' is accepted and PKCE is supported + CodeChallengeMethodsSupported: []string{string(promoauth.PKCEChallengeMethodNone)}, } // Bitbucket servers will have some base url, e.g. https://bitbucket.coder.com. // We will grab this from the Auth URL. This choice is a bit arbitrary, @@ -923,14 +934,15 @@ func bitbucketServerDefaults(config *codersdk.ExternalAuthConfig) codersdk.Exter // Any user specific fields will override this if provided. func gitlabDefaults(config *codersdk.ExternalAuthConfig) codersdk.ExternalAuthConfig { cloud := codersdk.ExternalAuthConfig{ - AuthURL: "https://gitlab.com/oauth/authorize", - TokenURL: "https://gitlab.com/oauth/token", - ValidateURL: "https://gitlab.com/oauth/token/info", - RevokeURL: "https://gitlab.com/oauth/revoke", - DisplayName: "GitLab", - DisplayIcon: "/icon/gitlab.svg", - Regex: `^(https?://)?gitlab\.com(/.*)?$`, - Scopes: []string{"write_repository"}, + AuthURL: "https://gitlab.com/oauth/authorize", + TokenURL: "https://gitlab.com/oauth/token", + ValidateURL: "https://gitlab.com/oauth/token/info", + RevokeURL: "https://gitlab.com/oauth/revoke", + DisplayName: "GitLab", + DisplayIcon: "/icon/gitlab.svg", + Regex: `^(https?://)?gitlab\.com(/.*)?$`, + Scopes: []string{"write_repository"}, + CodeChallengeMethodsSupported: []string{string(promoauth.PKCEChallengeMethodSha256)}, } if config.AuthURL == "" || config.AuthURL == cloud.AuthURL { @@ -946,14 +958,15 @@ func gitlabDefaults(config *codersdk.ExternalAuthConfig) codersdk.ExternalAuthCo // At this point, assume it is self-hosted and use the AuthURL return codersdk.ExternalAuthConfig{ - DisplayName: cloud.DisplayName, - Scopes: cloud.Scopes, - DisplayIcon: cloud.DisplayIcon, - AuthURL: au.ResolveReference(&url.URL{Path: "/oauth/authorize"}).String(), - TokenURL: au.ResolveReference(&url.URL{Path: "/oauth/token"}).String(), - ValidateURL: au.ResolveReference(&url.URL{Path: "/oauth/token/info"}).String(), - RevokeURL: au.ResolveReference(&url.URL{Path: "/oauth/revoke"}).String(), - Regex: fmt.Sprintf(`^(https?://)?%s(/.*)?$`, strings.ReplaceAll(au.Host, ".", `\.`)), + DisplayName: cloud.DisplayName, + Scopes: cloud.Scopes, + DisplayIcon: cloud.DisplayIcon, + AuthURL: au.ResolveReference(&url.URL{Path: "/oauth/authorize"}).String(), + TokenURL: au.ResolveReference(&url.URL{Path: "/oauth/token"}).String(), + ValidateURL: au.ResolveReference(&url.URL{Path: "/oauth/token/info"}).String(), + RevokeURL: au.ResolveReference(&url.URL{Path: "/oauth/revoke"}).String(), + Regex: fmt.Sprintf(`^(https?://)?%s(/.*)?$`, strings.ReplaceAll(au.Host, ".", `\.`)), + CodeChallengeMethodsSupported: []string{string(promoauth.PKCEChallengeMethodSha256)}, } } @@ -962,6 +975,8 @@ func jfrogArtifactoryDefaults(config *codersdk.ExternalAuthConfig) codersdk.Exte DisplayName: "JFrog Artifactory", Scopes: []string{"applied-permissions/user"}, DisplayIcon: "/icon/jfrog.svg", + // TODO: Investigate if 'S256' is accepted and PKCE is supported + CodeChallengeMethodsSupported: []string{string(promoauth.PKCEChallengeMethodNone)}, } // Artifactory servers will have some base url, e.g. https://jfrog.coder.com. // We will grab this from the Auth URL. This choice is not arbitrary. It is a @@ -997,9 +1012,10 @@ func jfrogArtifactoryDefaults(config *codersdk.ExternalAuthConfig) codersdk.Exte func giteaDefaults(config *codersdk.ExternalAuthConfig) codersdk.ExternalAuthConfig { defaults := codersdk.ExternalAuthConfig{ - DisplayName: "Gitea", - Scopes: []string{"read:repository", " write:repository", "read:user"}, - DisplayIcon: "/icon/gitea.svg", + DisplayName: "Gitea", + Scopes: []string{"read:repository", " write:repository", "read:user"}, + DisplayIcon: "/icon/gitea.svg", + CodeChallengeMethodsSupported: []string{string(promoauth.PKCEChallengeMethodSha256)}, } // Gitea's servers will have some base url, e.g: https://gitea.coder.com. // If an auth url is not set, we will assume they are using the default @@ -1031,6 +1047,8 @@ func azureDevopsEntraDefaults(config *codersdk.ExternalAuthConfig) codersdk.Exte DisplayName: "Azure DevOps (Entra)", DisplayIcon: "/icon/azure-devops.svg", Regex: `^(https?://)?dev\.azure\.com(/.*)?$`, + // TODO: Investigate if 'S256' is accepted and PKCE is supported + CodeChallengeMethodsSupported: []string{string(promoauth.PKCEChallengeMethodNone)}, } // The tenant ID is required for urls and is in the auth url. if config.AuthURL == "" { @@ -1069,6 +1087,8 @@ var staticDefaults = map[codersdk.EnhancedExternalAuthProvider]codersdk.External DisplayIcon: "/icon/azure-devops.svg", Regex: `^(https?://)?dev\.azure\.com(/.*)?$`, Scopes: []string{"vso.code_write"}, + // TODO: Investigate if 'S256' is accepted and PKCE is supported + CodeChallengeMethodsSupported: []string{string(promoauth.PKCEChallengeMethodNone)}, }, codersdk.EnhancedExternalAuthProviderBitBucketCloud: { AuthURL: "https://bitbucket.org/site/oauth2/authorize", @@ -1078,6 +1098,8 @@ var staticDefaults = map[codersdk.EnhancedExternalAuthProvider]codersdk.External DisplayIcon: "/icon/bitbucket.svg", Regex: `^(https?://)?bitbucket\.org(/.*)?$`, Scopes: []string{"account", "repository:write"}, + // TODO: Investigate if 'S256' is accepted and PKCE is supported + CodeChallengeMethodsSupported: []string{string(promoauth.PKCEChallengeMethodNone)}, }, codersdk.EnhancedExternalAuthProviderSlack: { AuthURL: "https://slack.com/oauth/v2/authorize", @@ -1087,6 +1109,8 @@ var staticDefaults = map[codersdk.EnhancedExternalAuthProvider]codersdk.External DisplayIcon: "/icon/slack.svg", // See: https://api.slack.com/authentication/oauth-v2#exchanging ExtraTokenKeys: []string{"authed_user"}, + // TODO: Investigate if 'S256' is accepted and PKCE is supported + CodeChallengeMethodsSupported: []string{string(promoauth.PKCEChallengeMethodNone)}, }, } diff --git a/coderd/externalauth/externalauth_internal_test.go b/coderd/externalauth/externalauth_internal_test.go index 65bb5ee7de..f88299412e 100644 --- a/coderd/externalauth/externalauth_internal_test.go +++ b/coderd/externalauth/externalauth_internal_test.go @@ -5,6 +5,7 @@ import ( "github.com/stretchr/testify/require" + "github.com/coder/coder/v2/coderd/promoauth" "github.com/coder/coder/v2/codersdk" ) @@ -13,17 +14,20 @@ func TestGitlabDefaults(t *testing.T) { // The default cloud setup. Copying this here as hard coded // values. - cloud := codersdk.ExternalAuthConfig{ - Type: string(codersdk.EnhancedExternalAuthProviderGitLab), - ID: string(codersdk.EnhancedExternalAuthProviderGitLab), - AuthURL: "https://gitlab.com/oauth/authorize", - TokenURL: "https://gitlab.com/oauth/token", - ValidateURL: "https://gitlab.com/oauth/token/info", - RevokeURL: "https://gitlab.com/oauth/revoke", - DisplayName: "GitLab", - DisplayIcon: "/icon/gitlab.svg", - Regex: `^(https?://)?gitlab\.com(/.*)?$`, - Scopes: []string{"write_repository"}, + cloud := func() codersdk.ExternalAuthConfig { + return codersdk.ExternalAuthConfig{ + Type: string(codersdk.EnhancedExternalAuthProviderGitLab), + ID: string(codersdk.EnhancedExternalAuthProviderGitLab), + AuthURL: "https://gitlab.com/oauth/authorize", + TokenURL: "https://gitlab.com/oauth/token", + ValidateURL: "https://gitlab.com/oauth/token/info", + RevokeURL: "https://gitlab.com/oauth/revoke", + DisplayName: "GitLab", + DisplayIcon: "/icon/gitlab.svg", + Regex: `^(https?://)?gitlab\.com(/.*)?$`, + Scopes: []string{"write_repository"}, + CodeChallengeMethodsSupported: []string{string(promoauth.PKCEChallengeMethodSha256)}, + } } tests := []struct { @@ -38,7 +42,7 @@ func TestGitlabDefaults(t *testing.T) { input: codersdk.ExternalAuthConfig{ Type: string(codersdk.EnhancedExternalAuthProviderGitLab), }, - expected: cloud, + expected: cloud(), }, { // If someone was to manually configure the gitlab cli. @@ -47,7 +51,7 @@ func TestGitlabDefaults(t *testing.T) { Type: string(codersdk.EnhancedExternalAuthProviderGitLab), AuthURL: "https://gitlab.com/oauth/authorize", }, - expected: cloud, + expected: cloud(), }, { // Changing some of the defaults of the cloud option @@ -60,7 +64,7 @@ func TestGitlabDefaults(t *testing.T) { DisplayName: "custom", Regex: ".*", }, - expected: cloud, + expected: cloud(), mutateExpected: func(config *codersdk.ExternalAuthConfig) { config.AuthURL = "https://gitlab.com/oauth/authorize?foo=bar" config.DisplayName = "custom" @@ -75,7 +79,7 @@ func TestGitlabDefaults(t *testing.T) { Type: string(codersdk.EnhancedExternalAuthProviderGitLab), AuthURL: "https://gitlab.company.org/oauth/authorize?foo=bar", }, - expected: cloud, + expected: cloud(), mutateExpected: func(config *codersdk.ExternalAuthConfig) { config.AuthURL = "https://gitlab.company.org/oauth/authorize?foo=bar" config.ValidateURL = "https://gitlab.company.org/oauth/token/info" @@ -88,20 +92,22 @@ func TestGitlabDefaults(t *testing.T) { // Strange values name: "RandomValues", input: codersdk.ExternalAuthConfig{ - Type: string(codersdk.EnhancedExternalAuthProviderGitLab), - AuthURL: "https://auth.com/auth", - ValidateURL: "https://validate.com/validate", - TokenURL: "https://token.com/token", - RevokeURL: "https://token.com/revoke", - Regex: "random", + Type: string(codersdk.EnhancedExternalAuthProviderGitLab), + AuthURL: "https://auth.com/auth", + ValidateURL: "https://validate.com/validate", + TokenURL: "https://token.com/token", + RevokeURL: "https://token.com/revoke", + Regex: "random", + CodeChallengeMethodsSupported: []string{"random"}, }, - expected: cloud, + expected: cloud(), mutateExpected: func(config *codersdk.ExternalAuthConfig) { config.AuthURL = "https://auth.com/auth" config.ValidateURL = "https://validate.com/validate" config.TokenURL = "https://token.com/token" config.RevokeURL = "https://token.com/revoke" config.Regex = `random` + config.CodeChallengeMethodsSupported = []string{"random"} }, }, } @@ -133,11 +139,12 @@ func Test_bitbucketServerConfigDefaults(t *testing.T) { Type: bbType, }, expected: codersdk.ExternalAuthConfig{ - Type: bbType, - ID: bbType, - DisplayName: "Bitbucket Server", - Scopes: []string{"PUBLIC_REPOS", "REPO_READ", "REPO_WRITE"}, - DisplayIcon: "/icon/bitbucket.svg", + Type: bbType, + ID: bbType, + DisplayName: "Bitbucket Server", + Scopes: []string{"PUBLIC_REPOS", "REPO_READ", "REPO_WRITE"}, + DisplayIcon: "/icon/bitbucket.svg", + CodeChallengeMethodsSupported: []string{string(promoauth.PKCEChallengeMethodNone)}, }, }, { @@ -148,15 +155,16 @@ func Test_bitbucketServerConfigDefaults(t *testing.T) { AuthURL: "https://bitbucket.example.com/login/oauth/authorize", }, expected: codersdk.ExternalAuthConfig{ - Type: bbType, - ID: bbType, - AuthURL: "https://bitbucket.example.com/login/oauth/authorize", - TokenURL: "https://bitbucket.example.com/rest/oauth2/latest/token", - ValidateURL: "https://bitbucket.example.com/rest/api/latest/inbox/pull-requests/count", - Scopes: []string{"PUBLIC_REPOS", "REPO_READ", "REPO_WRITE"}, - Regex: `^(https?://)?bitbucket\.example\.com(/.*)?$`, - DisplayName: "Bitbucket Server", - DisplayIcon: "/icon/bitbucket.svg", + Type: bbType, + ID: bbType, + AuthURL: "https://bitbucket.example.com/login/oauth/authorize", + TokenURL: "https://bitbucket.example.com/rest/oauth2/latest/token", + ValidateURL: "https://bitbucket.example.com/rest/api/latest/inbox/pull-requests/count", + Scopes: []string{"PUBLIC_REPOS", "REPO_READ", "REPO_WRITE"}, + Regex: `^(https?://)?bitbucket\.example\.com(/.*)?$`, + DisplayName: "Bitbucket Server", + DisplayIcon: "/icon/bitbucket.svg", + CodeChallengeMethodsSupported: []string{string(promoauth.PKCEChallengeMethodNone)}, }, }, { @@ -167,15 +175,16 @@ func Test_bitbucketServerConfigDefaults(t *testing.T) { Type: "bitbucket", }, expected: codersdk.ExternalAuthConfig{ - Type: string(codersdk.EnhancedExternalAuthProviderBitBucketCloud), - ID: "bitbucket", // Legacy ID remains unchanged - AuthURL: "https://bitbucket.org/site/oauth2/authorize", - TokenURL: "https://bitbucket.org/site/oauth2/access_token", - ValidateURL: "https://api.bitbucket.org/2.0/user", - DisplayName: "BitBucket", - DisplayIcon: "/icon/bitbucket.svg", - Regex: `^(https?://)?bitbucket\.org(/.*)?$`, - Scopes: []string{"account", "repository:write"}, + Type: string(codersdk.EnhancedExternalAuthProviderBitBucketCloud), + ID: "bitbucket", // Legacy ID remains unchanged + AuthURL: "https://bitbucket.org/site/oauth2/authorize", + TokenURL: "https://bitbucket.org/site/oauth2/access_token", + ValidateURL: "https://api.bitbucket.org/2.0/user", + DisplayName: "BitBucket", + DisplayIcon: "/icon/bitbucket.svg", + Regex: `^(https?://)?bitbucket\.org(/.*)?$`, + Scopes: []string{"account", "repository:write"}, + CodeChallengeMethodsSupported: []string{string(promoauth.PKCEChallengeMethodNone)}, }, }, } @@ -187,3 +196,45 @@ func Test_bitbucketServerConfigDefaults(t *testing.T) { }) } } + +func TestUntyped(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + input codersdk.ExternalAuthConfig + expected codersdk.ExternalAuthConfig + }{ + { + // Unknown Type uses S256 by default. + name: "RandomValues", + input: codersdk.ExternalAuthConfig{ + Type: "unknown", + AuthURL: "https://auth.com/auth", + ValidateURL: "https://validate.com/validate", + TokenURL: "https://token.com/token", + RevokeURL: "https://token.com/revoke", + Regex: "random", + }, + expected: codersdk.ExternalAuthConfig{ + ID: "unknown", + Type: "unknown", + DisplayName: "unknown", + DisplayIcon: "/emojis/1f511.png", + AuthURL: "https://auth.com/auth", + ValidateURL: "https://validate.com/validate", + TokenURL: "https://token.com/token", + RevokeURL: "https://token.com/revoke", + Regex: `random`, + CodeChallengeMethodsSupported: []string{string(promoauth.PKCEChallengeMethodSha256)}, + }, + }, + } + for _, c := range tests { + t.Run(c.name, func(t *testing.T) { + t.Parallel() + applyDefaultsToConfig(&c.input) + require.Equal(t, c.input, c.expected) + }) + } +} diff --git a/coderd/externalauth/externalauth_test.go b/coderd/externalauth/externalauth_test.go index 670d1cbf11..61fdbb2de5 100644 --- a/coderd/externalauth/externalauth_test.go +++ b/coderd/externalauth/externalauth_test.go @@ -729,6 +729,7 @@ func TestConstantQueryParams(t *testing.T) { authURL.RawQuery = url.Values{constantQueryParamKey: []string{constantQueryParamValue}}.Encode() cfg.OAuth2Config.(*oauth2.Config).Endpoint.AuthURL = authURL.String() require.Contains(t, cfg.OAuth2Config.(*oauth2.Config).Endpoint.AuthURL, constantQueryParam) + cfg.PKCEMethods = []promoauth.Oauth2PKCEChallengeMethod{promoauth.PKCEChallengeMethodSha256} }, }, }) @@ -792,7 +793,7 @@ func setupOauth2Test(t *testing.T, settings testConfig) (*oidctest.FakeIDP, *ext const providerID = "test-idp" fake := oidctest.NewFakeIDP(t, - append([]oidctest.FakeIDPOpt{}, settings.FakeIDPOpts...)..., + append([]oidctest.FakeIDPOpt{oidctest.WithPKCE()}, settings.FakeIDPOpts...)..., ) f := promoauth.NewFactory(prometheus.NewRegistry()) @@ -800,12 +801,13 @@ func setupOauth2Test(t *testing.T, settings testConfig) (*oidctest.FakeIDP, *ext config := &externalauth.Config{ InstrumentedOAuth2Config: f.New("test-oauth2", fake.OIDCConfig(t, nil, settings.CoderOIDCConfigOpts...)), - ID: providerID, - ClientID: cid, - ClientSecret: cs, - ValidateURL: fake.WellknownConfig().UserInfoURL, - RevokeURL: fake.WellknownConfig().RevokeURL, - RevokeTimeout: 1 * time.Second, + ID: providerID, + ClientID: cid, + ClientSecret: cs, + ValidateURL: fake.WellknownConfig().UserInfoURL, + RevokeURL: fake.WellknownConfig().RevokeURL, + RevokeTimeout: 1 * time.Second, + CodeChallengeMethodsSupported: []promoauth.Oauth2PKCEChallengeMethod{promoauth.PKCEChallengeMethodSha256}, } settings.ExternalAuthOpt(config) diff --git a/coderd/externalauth_test.go b/coderd/externalauth_test.go index cf85a7867c..3eb7c2a002 100644 --- a/coderd/externalauth_test.go +++ b/coderd/externalauth_test.go @@ -26,6 +26,7 @@ import ( "github.com/coder/coder/v2/coderd/database/dbtime" "github.com/coder/coder/v2/coderd/externalauth" "github.com/coder/coder/v2/coderd/httpapi" + "github.com/coder/coder/v2/coderd/promoauth" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/codersdk/agentsdk" "github.com/coder/coder/v2/provisioner/echo" @@ -34,6 +35,24 @@ import ( func TestExternalAuthByID(t *testing.T) { t.Parallel() + t.Run("PKCEMissing", func(t *testing.T) { + t.Parallel() + const providerID = "fake-github" + fake := oidctest.NewFakeIDP(t, oidctest.WithServing()) + + client := coderdtest.New(t, &coderdtest.Options{ + ExternalAuthConfigs: []*externalauth.Config{ + fake.ExternalAuthConfig(t, providerID, nil, func(cfg *externalauth.Config) { + cfg.Type = codersdk.EnhancedExternalAuthProviderGitHub.String() + cfg.CodeChallengeMethodsSupported = []promoauth.Oauth2PKCEChallengeMethod{} + }), + }, + }) + coderdtest.CreateFirstUser(t, client) + auth, err := client.ExternalAuthByID(context.Background(), providerID) + require.NoError(t, err) + require.False(t, auth.Authenticated) + }) t.Run("Unauthenticated", func(t *testing.T) { t.Parallel() const providerID = "fake-github" diff --git a/coderd/httpmw/oauth2.go b/coderd/httpmw/oauth2.go index 28e6400c8a..0e2b99e907 100644 --- a/coderd/httpmw/oauth2.go +++ b/coderd/httpmw/oauth2.go @@ -6,6 +6,7 @@ import ( "net/http" "net/url" "reflect" + "slices" "github.com/go-chi/chi/v5" "github.com/google/uuid" @@ -40,13 +41,19 @@ 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 { +// +// pkceMethods should be a list like ['S256', 'plain'] indicating +// which PKCE methods are supported by the OAuth2 provider. If empty, +// PKCE will not be used. +func ExtractOAuth2(config promoauth.OAuth2Config, client *http.Client, cookieCfg codersdk.HTTPCookieConfig, authURLOpts map[string]string, pkceMethods []promoauth.Oauth2PKCEChallengeMethod) func(http.Handler) http.Handler { opts := make([]oauth2.AuthCodeOption, 0, len(authURLOpts)+1) opts = append(opts, oauth2.AccessTypeOffline) for k, v := range authURLOpts { opts = append(opts, oauth2.SetAuthURLParam(k, v)) } + // Only S256 PKCE is currently supported. + sha256PKCESupported := slices.Contains(pkceMethods, promoauth.PKCEChallengeMethodSha256) return func(next http.Handler) http.Handler { return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() @@ -133,7 +140,20 @@ func ExtractOAuth2(config promoauth.OAuth2Config, client *http.Client, cookieCfg HttpOnly: true, })) - http.Redirect(rw, r, config.AuthCodeURL(state, opts...), http.StatusTemporaryRedirect) + authOpts := slices.Clone(opts) + if sha256PKCESupported { + verifier := oauth2.GenerateVerifier() + authOpts = append(authOpts, oauth2.S256ChallengeOption(verifier)) + + http.SetCookie(rw, cookieCfg.Apply(&http.Cookie{ + Name: codersdk.OAuth2PKCEVerifier, + Value: verifier, + Path: "/", + HttpOnly: true, + })) + } + + http.Redirect(rw, r, config.AuthCodeURL(state, authOpts...), http.StatusTemporaryRedirect) return } @@ -163,7 +183,19 @@ func ExtractOAuth2(config promoauth.OAuth2Config, client *http.Client, cookieCfg redirect = stateRedirect.Value } - oauthToken, err := config.Exchange(ctx, code) + exchangeOpts := make([]oauth2.AuthCodeOption, 0) + if sha256PKCESupported { + pkceVerifier, err := r.Cookie(codersdk.OAuth2PKCEVerifier) + if err != nil { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "PKCE challenge must be provided.", + }) + return + } + exchangeOpts = append(exchangeOpts, oauth2.VerifierOption(pkceVerifier.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 9739735f3e..baedd2cc2f 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, nil)(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, nil)(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, nil)(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, nil)(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, nil)(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, nil)(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, nil)(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, nil)(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, nil)(nil).ServeHTTP(res, req) found := false for _, cookie := range res.Result().Cookies() { diff --git a/coderd/promoauth/oauth2.go b/coderd/promoauth/oauth2.go index a89875cb75..6ab54b604f 100644 --- a/coderd/promoauth/oauth2.go +++ b/coderd/promoauth/oauth2.go @@ -11,6 +11,13 @@ import ( "golang.org/x/oauth2" ) +type Oauth2PKCEChallengeMethod string + +const ( + PKCEChallengeMethodSha256 Oauth2PKCEChallengeMethod = "S256" + PKCEChallengeMethodNone Oauth2PKCEChallengeMethod = "" +) + type Oauth2Source string const ( diff --git a/coderd/userauth.go b/coderd/userauth.go index 9147299673..adf150461e 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -770,6 +770,10 @@ type GithubOAuth2Config struct { DefaultProviderConfigured bool } +func (*GithubOAuth2Config) PKCESupported() []promoauth.Oauth2PKCEChallengeMethod { + return []promoauth.Oauth2PKCEChallengeMethod{promoauth.PKCEChallengeMethodSha256} +} + func (c *GithubOAuth2Config) Exchange(ctx context.Context, code string, opts ...oauth2.AuthCodeOption) (*oauth2.Token, error) { if !c.DeviceFlowEnabled { return c.OAuth2Config.Exchange(ctx, code, opts...) @@ -1172,6 +1176,15 @@ type OIDCConfig struct { IconURL string // SignupsDisabledText is the text do display on the static error page. SignupsDisabledText string + PKCEMethods []promoauth.Oauth2PKCEChallengeMethod +} + +// PKCESupported is to prevent nil pointer dereference. +func (o *OIDCConfig) PKCESupported() []promoauth.Oauth2PKCEChallengeMethod { + if o == nil { + return nil + } + return o.PKCEMethods } // @Summary OpenID Connect Callback diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index 86fe30bf3c..53db08aa11 100644 --- a/coderd/userauth_test.go +++ b/coderd/userauth_test.go @@ -1017,6 +1017,10 @@ func TestUserOAuth2Github(t *testing.T) { Name: "oauth_state", Value: "somestate", }) + req.AddCookie(&http.Cookie{ + Name: codersdk.OAuth2PKCEVerifier, + Value: oauth2.GenerateVerifier(), + }) require.NoError(t, err) res, err = client.HTTPClient.Do(req) require.NoError(t, err) @@ -2460,6 +2464,10 @@ func oauth2Callback(t *testing.T, client *codersdk.Client, opts ...func(*http.Re Name: codersdk.OAuth2StateCookie, Value: state, }) + req.AddCookie(&http.Cookie{ + Name: codersdk.OAuth2PKCEVerifier, + Value: oauth2.GenerateVerifier(), + }) res, err := client.HTTPClient.Do(req) require.NoError(t, err) t.Cleanup(func() { diff --git a/codersdk/client.go b/codersdk/client.go index 72dd7ac4b6..e1591f90fa 100644 --- a/codersdk/client.go +++ b/codersdk/client.go @@ -38,6 +38,10 @@ const ( SessionTokenHeader = "Coder-Session-Token" // OAuth2StateCookie is the name of the cookie that stores the oauth2 state. OAuth2StateCookie = "oauth_state" + // OAuth2PKCEVerifier is the name of the cookie that stores the oauth2 PKCE + // verifier. This is the raw verifier that when hashed, will match the challenge + // sent in the initial oauth2 request. + OAuth2PKCEVerifier = "oauth_pkce_verifier" // OAuth2RedirectCookie is the name of the cookie that stores the oauth2 redirect. OAuth2RedirectCookie = "oauth_redirect" diff --git a/codersdk/deployment.go b/codersdk/deployment.go index 665433bbe6..58725eea84 100644 --- a/codersdk/deployment.go +++ b/codersdk/deployment.go @@ -772,6 +772,9 @@ type ExternalAuthConfig struct { DisplayName string `json:"display_name" yaml:"display_name"` // DisplayIcon is a URL to an icon to display in the UI. DisplayIcon string `json:"display_icon" yaml:"display_icon"` + // CodeChallengeMethodsSupported lists the PKCE code challenge methods + // The only one supported by Coder is "S256". + CodeChallengeMethodsSupported []string `json:"code_challenge_methods_supported" yaml:"code_challenge_methods_supported"` } type ProvisionerConfig struct { @@ -1681,8 +1684,7 @@ func (c *DeploymentValues) Options() serpent.OptionSet { Env: "CODER_BLOCK_DIRECT", Value: &c.DERP.Config.BlockDirect, Group: &deploymentGroupNetworkingDERP, - YAML: "blockDirect", Annotations: serpent.Annotations{}. - Mark(annotationExternalProxies, "true"), + YAML: "blockDirect", Annotations: serpent.Annotations{}.Mark(annotationExternalProxies, "true"), }, { Name: "DERP Force WebSockets", diff --git a/codersdk/deployment_test.go b/codersdk/deployment_test.go index 8b811480d5..7407bf40b7 100644 --- a/codersdk/deployment_test.go +++ b/codersdk/deployment_test.go @@ -396,27 +396,28 @@ func TestExternalAuthYAMLConfig(t *testing.T) { return string(data) } githubCfg := codersdk.ExternalAuthConfig{ - Type: "github", - ClientID: "client_id", - ClientSecret: "client_secret", - ID: "id", - AuthURL: "https://example.com/auth", - TokenURL: "https://example.com/token", - ValidateURL: "https://example.com/validate", - RevokeURL: "https://example.com/revoke", - AppInstallURL: "https://example.com/install", - AppInstallationsURL: "https://example.com/installations", - NoRefresh: true, - Scopes: []string{"user:email", "read:org"}, - ExtraTokenKeys: []string{"extra", "token"}, - DeviceFlow: true, - DeviceCodeURL: "https://example.com/device", - Regex: "^https://example.com/.*$", - DisplayName: "GitHub", - DisplayIcon: "/static/icons/github.svg", - MCPURL: "https://api.githubcopilot.com/mcp/", - MCPToolAllowRegex: ".*", - MCPToolDenyRegex: "create_gist", + Type: "github", + ClientID: "client_id", + ClientSecret: "client_secret", + ID: "id", + AuthURL: "https://example.com/auth", + TokenURL: "https://example.com/token", + ValidateURL: "https://example.com/validate", + RevokeURL: "https://example.com/revoke", + AppInstallURL: "https://example.com/install", + AppInstallationsURL: "https://example.com/installations", + NoRefresh: true, + Scopes: []string{"user:email", "read:org"}, + ExtraTokenKeys: []string{"extra", "token"}, + DeviceFlow: true, + DeviceCodeURL: "https://example.com/device", + Regex: "^https://example.com/.*$", + DisplayName: "GitHub", + DisplayIcon: "/static/icons/github.svg", + MCPURL: "https://api.githubcopilot.com/mcp/", + MCPToolAllowRegex: ".*", + MCPToolDenyRegex: "create_gist", + CodeChallengeMethodsSupported: []string{"S256"}, } // Input the github section twice for testing a slice of configs. diff --git a/codersdk/externalauth.go b/codersdk/externalauth.go index 48c4781605..70c0060a73 100644 --- a/codersdk/externalauth.go +++ b/codersdk/externalauth.go @@ -94,14 +94,15 @@ type ExternalAuthLink struct { // ExternalAuthLinkProvider are the static details of a provider. type ExternalAuthLinkProvider struct { - ID string `json:"id"` - Type string `json:"type"` - Device bool `json:"device"` - DisplayName string `json:"display_name"` - DisplayIcon string `json:"display_icon"` - AllowRefresh bool `json:"allow_refresh"` - AllowValidate bool `json:"allow_validate"` - SupportsRevocation bool `json:"supports_revocation"` + ID string `json:"id"` + Type string `json:"type"` + Device bool `json:"device"` + DisplayName string `json:"display_name"` + DisplayIcon string `json:"display_icon"` + AllowRefresh bool `json:"allow_refresh"` + AllowValidate bool `json:"allow_validate"` + SupportsRevocation bool `json:"supports_revocation"` + CodeChallengeMethodsSupported []string `json:"code_challenge_methods_supported"` } type ExternalAuthAppInstallation struct { diff --git a/codersdk/testdata/githubcfg.yaml b/codersdk/testdata/githubcfg.yaml index c5e61baa03..838d8f0c2e 100644 --- a/codersdk/testdata/githubcfg.yaml +++ b/codersdk/testdata/githubcfg.yaml @@ -24,3 +24,5 @@ externalAuthProviders: regex: ^https://example.com/.*$ display_name: GitHub display_icon: /static/icons/github.svg + code_challenge_methods_supported: + - S256 diff --git a/docs/admin/external-auth/index.md b/docs/admin/external-auth/index.md index 634a22b23c..61d1b5816b 100644 --- a/docs/admin/external-auth/index.md +++ b/docs/admin/external-auth/index.md @@ -133,6 +133,23 @@ You must add the SSH key to your Git provider. - [GitHub](https://docs.github.com/en/authentication/connecting-to-github-with-ssh/adding-a-new-ssh-key-to-your-github-account#adding-a-new-ssh-key-to-your-account) - [GitLab](https://docs.gitlab.com/user/ssh/#add-an-ssh-key-to-your-gitlab-account) +## PKCE Support + +[PKCE (Proof Key for Code Exchange)](https://datatracker.ietf.org/doc/html/rfc7636) is an OAuth 2.0 +security extension that prevents authorization code interception attacks. Coder supports PKCE when +acting as an OAuth client to external identity providers. + +Coder will usually assume PKCE support is available with "S256" as the code challenge method. Manual +configuration is available to override any default behavior. + +```env +# Enable PKCE with S256 (recommended when supported) +CODER_EXTERNAL_AUTH_0_PKCE_METHODS="S256" + +# Disable PKCE entirely +CODER_EXTERNAL_AUTH_0_PKCE_METHODS="none" +``` + ## Git-provider specific env variables ### Azure DevOps diff --git a/docs/reference/api/general.md b/docs/reference/api/general.md index 129d43293e..e9a12a196f 100644 --- a/docs/reference/api/general.md +++ b/docs/reference/api/general.md @@ -262,6 +262,9 @@ curl -X GET http://coder-server:8080/api/v2/deployment/config \ "app_installations_url": "string", "auth_url": "string", "client_id": "string", + "code_challenge_methods_supported": [ + "string" + ], "device_code_url": "string", "device_flow": true, "display_icon": "string", diff --git a/docs/reference/api/schemas.md b/docs/reference/api/schemas.md index c260b5e729..ca609d0ac4 100644 --- a/docs/reference/api/schemas.md +++ b/docs/reference/api/schemas.md @@ -2952,6 +2952,9 @@ CreateWorkspaceRequest provides options for creating a new workspace. Only one o "app_installations_url": "string", "auth_url": "string", "client_id": "string", + "code_challenge_methods_supported": [ + "string" + ], "device_code_url": "string", "device_flow": true, "display_icon": "string", @@ -3480,6 +3483,9 @@ CreateWorkspaceRequest provides options for creating a new workspace. Only one o "app_installations_url": "string", "auth_url": "string", "client_id": "string", + "code_challenge_methods_supported": [ + "string" + ], "device_code_url": "string", "device_flow": true, "display_icon": "string", @@ -4219,6 +4225,9 @@ CreateWorkspaceRequest provides options for creating a new workspace. Only one o "app_installations_url": "string", "auth_url": "string", "client_id": "string", + "code_challenge_methods_supported": [ + "string" + ], "device_code_url": "string", "device_flow": true, "display_icon": "string", @@ -4241,21 +4250,22 @@ CreateWorkspaceRequest provides options for creating a new workspace. Only one o ### Properties -| Name | Type | Required | Restrictions | Description | -|-------------------------|---------|----------|--------------|-----------------------------------------------------------------------------------------| -| `app_install_url` | string | false | | | -| `app_installations_url` | string | false | | | -| `auth_url` | string | false | | | -| `client_id` | string | false | | | -| `device_code_url` | string | false | | | -| `device_flow` | boolean | false | | | -| `display_icon` | string | false | | Display icon is a URL to an icon to display in the UI. | -| `display_name` | string | false | | Display name is shown in the UI to identify the auth config. | -| `id` | string | false | | ID is a unique identifier for the auth config. It defaults to `type` when not provided. | -| `mcp_tool_allow_regex` | string | false | | | -| `mcp_tool_deny_regex` | string | false | | | -| `mcp_url` | string | false | | | -| `no_refresh` | boolean | false | | | +| Name | Type | Required | Restrictions | Description | +|------------------------------------|-----------------|----------|--------------|-------------------------------------------------------------------------------------------------------------------| +| `app_install_url` | string | false | | | +| `app_installations_url` | string | false | | | +| `auth_url` | string | false | | | +| `client_id` | string | false | | | +| `code_challenge_methods_supported` | array of string | false | | Code challenge methods supported lists the PKCE code challenge methods The only one supported by Coder is "S256". | +| `device_code_url` | string | false | | | +| `device_flow` | boolean | false | | | +| `display_icon` | string | false | | Display icon is a URL to an icon to display in the UI. | +| `display_name` | string | false | | Display name is shown in the UI to identify the auth config. | +| `id` | string | false | | ID is a unique identifier for the auth config. It defaults to `type` when not provided. | +| `mcp_tool_allow_regex` | string | false | | | +| `mcp_tool_deny_regex` | string | false | | | +| `mcp_url` | string | false | | | +| `no_refresh` | boolean | false | | | |`regex`|string|false||Regex allows API requesters to match an auth config by a string (e.g. coder.com) instead of by it's type. Git clone makes use of this by parsing the URL from: 'Username for "https://github.com":' And sending it to the Coder server to match against the Regex.| |`revoke_url`|string|false||| @@ -14031,6 +14041,9 @@ None "app_installations_url": "string", "auth_url": "string", "client_id": "string", + "code_challenge_methods_supported": [ + "string" + ], "device_code_url": "string", "device_flow": true, "display_icon": "string", diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index f46e379d50..47da5bb36d 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -2004,6 +2004,11 @@ export interface ExternalAuthConfig { * DisplayIcon is a URL to an icon to display in the UI. */ readonly display_icon: string; + /** + * CodeChallengeMethodsSupported lists the PKCE code challenge methods + * The only one supported by Coder is "S256". + */ + readonly code_challenge_methods_supported: readonly string[]; } // From codersdk/externalauth.go @@ -2053,6 +2058,7 @@ export interface ExternalAuthLinkProvider { readonly allow_refresh: boolean; readonly allow_validate: boolean; readonly supports_revocation: boolean; + readonly code_challenge_methods_supported: readonly string[]; } // From codersdk/externalauth.go @@ -3050,6 +3056,14 @@ export interface OAuth2GithubConfig { readonly enterprise_base_url: string; } +// From codersdk/client.go +/** + * OAuth2PKCEVerifier is the name of the cookie that stores the oauth2 PKCE + * verifier. This is the raw verifier that when hashed, will match the challenge + * sent in the initial oauth2 request. + */ +export const OAuth2PKCEVerifier = "oauth_pkce_verifier"; + // From codersdk/oauth2.go /** * OAuth2ProtectedResourceMetadata represents RFC 9728 OAuth 2.0 Protected Resource Metadata diff --git a/site/src/pages/DeploymentSettingsPage/ExternalAuthSettingsPage/ExternalAuthSettingsPageView.stories.tsx b/site/src/pages/DeploymentSettingsPage/ExternalAuthSettingsPage/ExternalAuthSettingsPageView.stories.tsx index a558918e71..a37c490fc0 100644 --- a/site/src/pages/DeploymentSettingsPage/ExternalAuthSettingsPage/ExternalAuthSettingsPageView.stories.tsx +++ b/site/src/pages/DeploymentSettingsPage/ExternalAuthSettingsPage/ExternalAuthSettingsPageView.stories.tsx @@ -27,6 +27,7 @@ const meta: Meta = { mcp_url: "", mcp_tool_allow_regex: "", mcp_tool_deny_regex: "", + code_challenge_methods_supported: ["S256"], }, ], }, diff --git a/site/src/testHelpers/entities.ts b/site/src/testHelpers/entities.ts index 06096f1ef4..98a2653b9b 100644 --- a/site/src/testHelpers/entities.ts +++ b/site/src/testHelpers/entities.ts @@ -4503,6 +4503,7 @@ export const MockGithubExternalProvider: TypesGen.ExternalAuthLinkProvider = { allow_refresh: true, allow_validate: true, supports_revocation: false, + code_challenge_methods_supported: ["S256"], }; export const MockGithubAuthLink: TypesGen.ExternalAuthLink = {