Add PKCE support for OIDC (#20094)

gitpod-io · Aug 8, 2024 · 5ebe1cc · 5ebe1cc
1 parent c4b53f9
commit 5ebe1cc
Show file tree

Hide file tree

Showing 14 changed files with 1,256 additions and 1,031 deletions.
diff --git a/components/dashboard/src/dedicated-setup/SSOSetupStep.tsx b/components/dashboard/src/dedicated-setup/SSOSetupStep.tsx
@@ -28,6 +28,7 @@ export const SSOSetupStep: FC<Props> = ({ config, onComplete, progressCurrent, p
         clientId: config?.oauth2Config?.clientId ?? "",
         clientSecret: config?.oauth2Config?.clientSecret ?? "",
         celExpression: config?.oauth2Config?.celExpression ?? "",
+        usePKCE: config?.oauth2Config?.usePkce ?? false,
     });
     const configIsValid = isValid(ssoConfig);
 

diff --git a/components/dashboard/src/teams/sso/OIDCClientConfigModal.tsx b/components/dashboard/src/teams/sso/OIDCClientConfigModal.tsx
@@ -27,6 +27,7 @@ export const OIDCClientConfigModal: FC<Props> = ({ clientConfig, onSaved, onClos
         clientId: clientConfig?.oauth2Config?.clientId ?? "",
         clientSecret: clientConfig?.oauth2Config?.clientSecret ?? "",
         celExpression: clientConfig?.oauth2Config?.celExpression ?? "",
+        usePKCE: clientConfig?.oauth2Config?.usePkce ?? false,
     });
     const configIsValid = isValid(ssoConfig);
 

diff --git a/components/dashboard/src/teams/sso/SSOConfigForm.tsx b/components/dashboard/src/teams/sso/SSOConfigForm.tsx
@@ -14,6 +14,7 @@ import isURL from "validator/lib/isURL";
 import { useCurrentOrg } from "../../data/organizations/orgs-query";
 import { useUpsertOIDCClientMutation } from "../../data/oidc-clients/upsert-oidc-client-mutation";
 import { Subheading } from "../../components/typography/headings";
+import { CheckboxInputField } from "../../components/forms/CheckboxInputField";
 
 type Props = {
     config: SSOConfig;
@@ -72,6 +73,13 @@ export const SSOConfigForm: FC<Props> = ({ config, readOnly = false, onChange })
                 onChange={(val) => onChange({ clientSecret: val })}
             />
 
+            <CheckboxInputField
+                label="Use PKCE"
+                checked={config.usePKCE}
+                disabled={readOnly}
+                onChange={(val) => onChange({ usePKCE: val })}
+            />
+
             <Subheading className="mt-8">
                 <strong>3.</strong> Restrict available accounts in your Identity Providers.
                 <a
@@ -103,6 +111,7 @@ export type SSOConfig = {
     clientId: string;
     clientSecret: string;
     celExpression?: string;
+    usePKCE: boolean;
 };
 
 export const ssoConfigReducer = (state: SSOConfig, action: Partial<SSOConfig>) => {
@@ -155,6 +164,7 @@ export const useSaveSSOConfig = () => {
                               clientId: trimmedClientId,
                               clientSecret: trimmedClientSecret,
                               celExpression: trimmedCelExpression,
+                              usePkce: ssoConfig.usePKCE,
                           },
                           oidcConfig: {
                               issuer: trimmedIssuer,
@@ -168,6 +178,7 @@ export const useSaveSSOConfig = () => {
                               // TODO: determine how we should handle when user doesn't change their secret
                               clientSecret: trimmedClientSecret.toLowerCase() === "redacted" ? "" : trimmedClientSecret,
                               celExpression: trimmedCelExpression,
+                              usePkce: ssoConfig.usePKCE,
                           },
                           oidcConfig: {
                               issuer: trimmedIssuer,

diff --git a/components/gitpod-db/go/oidc_client_config.go b/components/gitpod-db/go/oidc_client_config.go
@@ -60,6 +60,9 @@ type OIDCSpec struct {
 
 	// CelExpression is an optional expression that can be used to determine if the client should be allowed to authenticate.
 	CelExpression string `json:"celExpression"`
+
+	// UsePKCE specifies if the client should use PKCE for the OAuth flow.
+	UsePKCE bool `json:"usePKCE"`
 }
 
 func CreateOIDCClientConfig(ctx context.Context, conn *gorm.DB, cfg OIDCClientConfig) (OIDCClientConfig, error) {
@@ -352,6 +355,7 @@ func partialUpdateOIDCSpec(old, new OIDCSpec) OIDCSpec {
 	}
 
 	old.CelExpression = new.CelExpression
+	old.UsePKCE = new.UsePKCE
 
 	if !oidcScopesEqual(old.Scopes, new.Scopes) {
 		old.Scopes = new.Scopes

diff --git a/components/public-api-server/go.mod b/components/public-api-server/go.mod
@@ -29,7 +29,7 @@ require (
 	github.com/stretchr/testify v1.8.4
 	github.com/stripe/stripe-go/v72 v72.122.0
 	github.com/zitadel/oidc v1.13.0
-	golang.org/x/oauth2 v0.7.0
+	golang.org/x/oauth2 v0.22.0
 	google.golang.org/grpc v1.57.0
 	google.golang.org/protobuf v1.33.0
 	gopkg.in/square/go-jose.v2 v2.6.0

diff --git a/components/public-api-server/go.sum b/components/public-api-server/go.sum
diff --git a/components/public-api-server/pkg/apiv1/oidc.go b/components/public-api-server/pkg/apiv1/oidc.go
@@ -443,6 +443,7 @@ func dbOIDCClientConfigToAPI(config db.OIDCClientConfig, decryptor db.Decryptor)
 			AuthorizationEndpoint: decrypted.RedirectURL,
 			Scopes:                decrypted.Scopes,
 			CelExpression:         decrypted.CelExpression,
+			UsePkce:               decrypted.UsePKCE,
 		},
 		OidcConfig: &v1.OIDCConfig{
 			Issuer: config.Issuer,
@@ -472,6 +473,7 @@ func toDbOIDCSpec(oauth2Config *v1.OAuth2Config) db.OIDCSpec {
 		ClientID:      oauth2Config.GetClientId(),
 		ClientSecret:  oauth2Config.GetClientSecret(),
 		CelExpression: oauth2Config.GetCelExpression(),
+		UsePKCE:       oauth2Config.GetUsePkce(),
 		RedirectURL:   oauth2Config.GetAuthorizationEndpoint(),
 		Scopes:        append([]string{goidc.ScopeOpenID, "profile", "email"}, oauth2Config.GetScopes()...),
 	}

diff --git a/components/public-api-server/pkg/oidc/oauth2.go b/components/public-api-server/pkg/oidc/oauth2.go
@@ -84,8 +84,18 @@ func (s *Service) OAuth2Middleware(next http.Handler) http.Handler {
 			return
 		}
 
+		opts := []oauth2.AuthCodeOption{}
+		if config.UsePKCE {
+			codeVerifier, err := r.Cookie(verifierCookieName)
+			if err != nil {
+				http.Error(rw, "code_verifier cookie not found", http.StatusBadRequest)
+				return
+			}
+			opts = append(opts, oauth2.VerifierOption(codeVerifier.Value))
+		}
+
 		config.OAuth2Config.RedirectURL = getCallbackURL(r.Host)
-		oauth2Token, err := config.OAuth2Config.Exchange(r.Context(), code)
+		oauth2Token, err := config.OAuth2Config.Exchange(r.Context(), code, opts...)
 		if err != nil {
 			log.WithError(err).Warn("Failed to exchange OAuth2 token.")
 			respondeWithError(rw, r, "Failed to exchange OAuth2 token: "+err.Error(), http.StatusInternalServerError, useHttpErrors)

diff --git a/components/public-api-server/pkg/oidc/router.go b/components/public-api-server/pkg/oidc/router.go
@@ -32,8 +32,9 @@ func Router(s *Service) *chi.Mux {
 }
 
 const (
-	stateCookieName = "state"
-	nonceCookieName = "nonce"
+	stateCookieName    = "state"
+	nonceCookieName    = "nonce"
+	verifierCookieName = "verifier"
 )
 
 func (s *Service) getStartHandler() http.HandlerFunc {
@@ -83,6 +84,7 @@ func (s *Service) getStartHandler() http.HandlerFunc {
 
 		http.SetCookie(rw, newCallbackCookie(r, nonceCookieName, startParams.Nonce))
 		http.SetCookie(rw, newCallbackCookie(r, stateCookieName, startParams.State))
+		http.SetCookie(rw, newCallbackCookie(r, verifierCookieName, startParams.CodeVerifier))
 
 		http.Redirect(rw, r, startParams.AuthCodeURL, http.StatusTemporaryRedirect)
 	}

diff --git a/components/public-api-server/pkg/oidc/service.go b/components/public-api-server/pkg/oidc/service.go
@@ -52,12 +52,14 @@ type ClientConfig struct {
 	OAuth2Config   *oauth2.Config
 	VerifierConfig *goidc.Config
 	CelExpression  string
+	UsePKCE        bool
 }
 
 type StartParams struct {
-	State       string
-	Nonce       string
-	AuthCodeURL string
+	State        string
+	Nonce        string
+	CodeVerifier string
+	AuthCodeURL  string
 }
 
 type AuthFlowResult struct {
@@ -92,12 +94,21 @@ func (s *Service) getStartParams(config *ClientConfig, redirectURL string, state
 
 	// Configuring `AuthCodeOption`s, e.g. nonce
 	config.OAuth2Config.RedirectURL = redirectURL
-	authCodeURL := config.OAuth2Config.AuthCodeURL(state, goidc.Nonce(nonce))
+
+	opts := []oauth2.AuthCodeOption{goidc.Nonce(nonce)}
+	var verifier string
+	if config.UsePKCE {
+		verifier = oauth2.GenerateVerifier()
+		opts = append(opts, oauth2.S256ChallengeOption(verifier))
+	}
+
+	authCodeURL := config.OAuth2Config.AuthCodeURL(state, opts...)
 
 	return &StartParams{
-		AuthCodeURL: authCodeURL,
-		State:       state,
-		Nonce:       nonce,
+		AuthCodeURL:  authCodeURL,
+		State:        state,
+		Nonce:        nonce,
+		CodeVerifier: verifier,
 	}, nil
 }
 
@@ -252,6 +263,7 @@ func (s *Service) convertClientConfig(ctx context.Context, dbEntry db.OIDCClient
 			Scopes:       spec.Scopes,
 		},
 		CelExpression: spec.CelExpression,
+		UsePKCE:       spec.UsePKCE,
 		VerifierConfig: &goidc.Config{
 			ClientID: spec.ClientID,
 		},

diff --git a/components/public-api/gitpod/experimental/v1/oidc.proto b/components/public-api/gitpod/experimental/v1/oidc.proto
@@ -121,6 +121,9 @@ message OAuth2Config {
   // CEL expression to verify a profile.
   // Optional.
   string cel_expression = 9;
+
+  // Use PKCE for the OAuth2 flow.
+  bool use_pkce = 10;
 }
 
 // Description of keys of a userinfo result.