From 8e086fb482caae46724b9511f9d223d74d258b16 Mon Sep 17 00:00:00 2001 From: camohob Date: Fri, 29 May 2026 11:51:58 +0300 Subject: [PATCH 1/2] feat: add PKCE (S256) support to OAuth2 authorization code flow --- server/server/auth/oidc.go | 7 +- server/server/auth/oidc_test.go | 285 +++++++++++++++++++++++++++++++ server/server/route/auth.go | 27 +++ server/server/route/pkce_test.go | 274 +++++++++++++++++++++++++++++ 4 files changed, 592 insertions(+), 1 deletion(-) create mode 100644 server/server/auth/oidc_test.go create mode 100644 server/server/route/pkce_test.go diff --git a/server/server/auth/oidc.go b/server/server/auth/oidc.go index d863dc0d71..7e71dc0a45 100644 --- a/server/server/auth/oidc.go +++ b/server/server/auth/oidc.go @@ -67,7 +67,12 @@ func ExchangeCode(ctx context.Context, r *http.Request, config *oauth2.Config, p return nil, echo.NewHTTPError(http.StatusBadRequest, "State cookie did not match") } - oauth2Token, err := config.Exchange(ctx, r.URL.Query().Get("code")) + codeVerifier, err := r.Cookie("code_verifier") + if err != nil { + return nil, echo.NewHTTPError(http.StatusBadRequest, "Code verifier is not set in request") + } + + oauth2Token, err := config.Exchange(ctx, r.URL.Query().Get("code"), oauth2.SetAuthURLParam("code_verifier", codeVerifier.Value)) if err != nil { return nil, echo.NewHTTPError(http.StatusInternalServerError, "Unable to exchange token: "+err.Error()) } diff --git a/server/server/auth/oidc_test.go b/server/server/auth/oidc_test.go new file mode 100644 index 0000000000..f103a1aaea --- /dev/null +++ b/server/server/auth/oidc_test.go @@ -0,0 +1,285 @@ +package auth_test + +import ( + "context" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/sha256" + "encoding/base64" + "encoding/json" + "net/http" + "net/http/httptest" + "net/url" + "testing" + "time" + + "github.com/coreos/go-oidc/v3/oidc" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/temporalio/ui-server/v2/server/auth" + "golang.org/x/oauth2" +) + +type oidcTestHarness struct { + server *httptest.Server + privateKey *ecdsa.PrivateKey + keyID string + lastTokenParams url.Values + issuerURL string + nonce string +} + +func newOIDCTestHarness(t *testing.T) *oidcTestHarness { + t.Helper() + + key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err) + + h := &oidcTestHarness{ + privateKey: key, + keyID: "test-key-id", + } + + mux := http.NewServeMux() + mux.HandleFunc("/.well-known/openid-configuration", h.handleDiscovery) + mux.HandleFunc("/token", h.handleToken) + mux.HandleFunc("/keys", h.handleJWKS) + + h.server = httptest.NewServer(mux) + h.issuerURL = h.server.URL + + return h +} + +func (h *oidcTestHarness) Close() { + h.server.Close() +} + +func (h *oidcTestHarness) Provider(ctx context.Context) (*oidc.Provider, error) { + return oidc.NewProvider( + oidc.InsecureIssuerURLContext(ctx, h.issuerURL), + h.issuerURL, + ) +} + +func (h *oidcTestHarness) OAuth2Config() *oauth2.Config { + return &oauth2.Config{ + ClientID: "test-client", + Endpoint: oauth2.Endpoint{ + AuthURL: h.issuerURL + "/auth", + TokenURL: h.issuerURL + "/token", + }, + Scopes: []string{"openid", "email"}, + } +} + +type oidcDiscovery struct { + Issuer string `json:"issuer"` + AuthorizationEndpoint string `json:"authorization_endpoint"` + TokenEndpoint string `json:"token_endpoint"` + JWKSUri string `json:"jwks_uri"` + ResponseTypesSupported []string `json:"response_types_supported"` + SubjectTypesSupported []string `json:"subject_types_supported"` + IDTokenSigningAlgValuesSupported []string `json:"id_token_signing_alg_values_supported"` +} + +func (h *oidcTestHarness) handleDiscovery(w http.ResponseWriter, r *http.Request) { + disc := oidcDiscovery{ + Issuer: h.issuerURL, + AuthorizationEndpoint: h.issuerURL + "/auth", + TokenEndpoint: h.issuerURL + "/token", + JWKSUri: h.issuerURL + "/keys", + ResponseTypesSupported: []string{"code"}, + SubjectTypesSupported: []string{"public"}, + IDTokenSigningAlgValuesSupported: []string{"ES256"}, + } + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(disc) +} + +type jwkKey struct { + KTY string `json:"kty"` + CRV string `json:"crv"` + X string `json:"x"` + Y string `json:"y"` + KID string `json:"kid"` + ALG string `json:"alg"` + USE string `json:"use"` +} + +type jwks struct { + Keys []jwkKey `json:"keys"` +} + +func (h *oidcTestHarness) handleJWKS(w http.ResponseWriter, r *http.Request) { + pub := &h.privateKey.PublicKey + x := base64.RawURLEncoding.EncodeToString(pub.X.Bytes()) + y := base64.RawURLEncoding.EncodeToString(pub.Y.Bytes()) + + keys := jwks{Keys: []jwkKey{{ + KTY: "EC", + CRV: "P-256", + X: x, + Y: y, + KID: h.keyID, + ALG: "ES256", + USE: "sig", + }}} + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(keys) +} + +func (h *oidcTestHarness) handleToken(w http.ResponseWriter, r *http.Request) { + r.ParseForm() + h.lastTokenParams = r.PostForm + + now := time.Now() + claims := map[string]interface{}{ + "iss": h.issuerURL, + "sub": "test-user-id", + "aud": []string{"test-client"}, + "exp": now.Add(time.Hour).Unix(), + "iat": now.Unix(), + "nonce": h.nonce, + "email": "test@example.com", + "name": "Test User", + } + idToken, err := signJWT( + map[string]string{"alg": "ES256", "typ": "JWT", "kid": h.keyID}, + claims, + h.privateKey, + ) + if err != nil { + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + + resp := map[string]interface{}{ + "access_token": "mock-access-token", + "token_type": "Bearer", + "expires_in": 3600, + "id_token": idToken, + "refresh_token": "mock-refresh-token", + } + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(resp) +} + +func signJWT(header, payload interface{}, key *ecdsa.PrivateKey) (string, error) { + hdr, err := json.Marshal(header) + if err != nil { + return "", err + } + pld, err := json.Marshal(payload) + if err != nil { + return "", err + } + + hdrB64 := base64.RawURLEncoding.EncodeToString(hdr) + pldB64 := base64.RawURLEncoding.EncodeToString(pld) + + msg := hdrB64 + "." + pldB64 + hash := sha256.Sum256([]byte(msg)) + + r, s, err := ecdsa.Sign(rand.Reader, key, hash[:]) + if err != nil { + return "", err + } + + rBytes := make([]byte, 32) + sBytes := make([]byte, 32) + r.FillBytes(rBytes) + s.FillBytes(sBytes) + + sig := append(rBytes, sBytes...) + sigB64 := base64.RawURLEncoding.EncodeToString(sig) + + return msg + "." + sigB64, nil +} + +func TestExchangeCode_MissingCodeVerifier(t *testing.T) { + harness := newOIDCTestHarness(t) + defer harness.Close() + + provider, err := harness.Provider(context.Background()) + require.NoError(t, err) + + req := httptest.NewRequest(http.MethodGet, "/auth/sso/callback?code=test-code&state=test-state", nil) + req.AddCookie(&http.Cookie{Name: "state", Value: "test-state"}) + + cfg := harness.OAuth2Config() + _, err = auth.ExchangeCode(context.Background(), req, cfg, provider) + require.Error(t, err) + assert.Contains(t, err.Error(), "Code verifier is not set") +} + +func TestExchangeCode_WithPKCE(t *testing.T) { + harness := newOIDCTestHarness(t) + defer harness.Close() + + tokenNonce := "test-nonce-value" + harness.nonce = tokenNonce + + provider, err := harness.Provider(context.Background()) + require.NoError(t, err) + + req := httptest.NewRequest(http.MethodGet, "/auth/sso/callback?code=test-code&state=test-state", nil) + req.AddCookie(&http.Cookie{Name: "state", Value: "test-state"}) + req.AddCookie(&http.Cookie{Name: "code_verifier", Value: "dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk"}) + req.AddCookie(&http.Cookie{Name: "nonce", Value: tokenNonce}) + + cfg := harness.OAuth2Config() + user, err := auth.ExchangeCode(context.Background(), req, cfg, provider) + require.NoError(t, err) + require.NotNil(t, user) + require.NotNil(t, user.OAuth2Token) + assert.Equal(t, "mock-access-token", user.OAuth2Token.AccessToken) + assert.Equal(t, "mock-refresh-token", user.OAuth2Token.RefreshToken) + + require.NotNil(t, user.IDToken) + require.NotNil(t, user.IDToken.Claims) + assert.Equal(t, "test@example.com", user.IDToken.Claims.Email) + assert.Equal(t, "Test User", user.IDToken.Claims.Name) + + require.NotNil(t, harness.lastTokenParams) + assert.Equal(t, "dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk", + harness.lastTokenParams.Get("code_verifier"), + "code_verifier must be sent to token endpoint") +} + +func TestExchangeCode_StateMismatch(t *testing.T) { + harness := newOIDCTestHarness(t) + defer harness.Close() + + provider, err := harness.Provider(context.Background()) + require.NoError(t, err) + + req := httptest.NewRequest(http.MethodGet, "/auth/sso/callback?code=test-code&state=wrong-state", nil) + req.AddCookie(&http.Cookie{Name: "state", Value: "expected-state"}) + req.AddCookie(&http.Cookie{Name: "code_verifier", Value: "test-verifier"}) + req.AddCookie(&http.Cookie{Name: "nonce", Value: "test-nonce"}) + + cfg := harness.OAuth2Config() + _, err = auth.ExchangeCode(context.Background(), req, cfg, provider) + require.Error(t, err) + assert.Contains(t, err.Error(), "State cookie did not match") +} + +func TestExchangeCode_NonceMismatch(t *testing.T) { + harness := newOIDCTestHarness(t) + defer harness.Close() + + provider, err := harness.Provider(context.Background()) + require.NoError(t, err) + + req := httptest.NewRequest(http.MethodGet, "/auth/sso/callback?code=test-code&state=test-state", nil) + req.AddCookie(&http.Cookie{Name: "state", Value: "test-state"}) + req.AddCookie(&http.Cookie{Name: "code_verifier", Value: "dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk"}) + req.AddCookie(&http.Cookie{Name: "nonce", Value: "expected-nonce"}) + + cfg := harness.OAuth2Config() + _, err = auth.ExchangeCode(context.Background(), req, cfg, provider) + require.Error(t, err) + assert.Contains(t, err.Error(), "Nonce did not match") +} diff --git a/server/server/route/auth.go b/server/server/route/auth.go index fb63e6d16a..cb8257fc21 100644 --- a/server/server/route/auth.go +++ b/server/server/route/auth.go @@ -23,6 +23,7 @@ package route import ( + "crypto/sha256" "encoding/base64" "encoding/json" "errors" @@ -129,8 +130,16 @@ func authenticate(config *oauth2.Config, options map[string]interface{}, allowed setCallbackCookie(c, "state", state) setCallbackCookie(c, "nonce", nonce) + codeVerifier, err := randCodeVerifier() + if err != nil { + return echo.NewHTTPError(http.StatusInternalServerError, err) + } + setCallbackCookie(c, "code_verifier", codeVerifier) + opts := []oauth2.AuthCodeOption{ oidc.Nonce(nonce), + oauth2.SetAuthURLParam("code_challenge_method", "S256"), + oauth2.SetAuthURLParam("code_challenge", sha256CodeChallenge(codeVerifier)), } for k, v := range options { var value string @@ -161,6 +170,8 @@ func authenticateCb(ctx context.Context, oauthCfg *oauth2.Config, provider *oidc return err } + clearCookie(c, "code_verifier") + err = auth.SetUser(c, user) if err != nil { return echo.NewHTTPError(http.StatusInternalServerError, "unable to set user: "+err.Error()) @@ -258,6 +269,9 @@ func logout() func(echo.Context) error { return func(c echo.Context) error { log.Printf("[Auth] User logout initiated from %s", c.RealIP()) + // Clear PKCE code verifier + clearCookie(c, "code_verifier") + // Clear refresh token cookie clearCookie(c, "refresh") log.Printf("[Auth] Cleared refresh token cookie") @@ -364,6 +378,19 @@ func nonceFromString(nonce string) (*Nonce, error) { return &n, nil } +func randCodeVerifier() (string, error) { + b := securecookie.GenerateRandomKey(32) + if b == nil { + return "", errors.New("unable to generate PKCE code verifier") + } + return base64.RawURLEncoding.EncodeToString(b), nil +} + +func sha256CodeChallenge(verifier string) string { + h := sha256.Sum256([]byte(verifier)) + return base64.RawURLEncoding.EncodeToString(h[:]) +} + type Nonce struct { Nonce string `json:"nonce"` ReturnURL string `json:"return_url"` diff --git a/server/server/route/pkce_test.go b/server/server/route/pkce_test.go new file mode 100644 index 0000000000..779d9524b5 --- /dev/null +++ b/server/server/route/pkce_test.go @@ -0,0 +1,274 @@ +package route + +import ( + "encoding/base64" + "net/http" + "net/http/httptest" + "net/url" + "testing" + + "github.com/labstack/echo/v4" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/oauth2" +) + +func TestRandCodeVerifier_GeneratesValidVerifier(t *testing.T) { + v, err := randCodeVerifier() + require.NoError(t, err) + require.NotEmpty(t, v) + + // PKCE verifier must be 43-128 characters (RFC 7636) + assert.Len(t, v, 43) + + // Must be valid base64url (no padding) + decoded, err := base64.RawURLEncoding.DecodeString(v) + require.NoError(t, err) + assert.Len(t, decoded, 32) +} + +func TestRandCodeVerifier_GeneratesUniqueValues(t *testing.T) { + v1, err := randCodeVerifier() + require.NoError(t, err) + + v2, err := randCodeVerifier() + require.NoError(t, err) + + assert.NotEqual(t, v1, v2) +} + +func TestSHA256CodeChallenge_RFC7636TestVector(t *testing.T) { + // RFC 7636 Appendix B test vector + verifier := "dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk" + expected := "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM" + + challenge := sha256CodeChallenge(verifier) + assert.Equal(t, expected, challenge) +} + +func TestSHA256CodeChallenge_Deterministic(t *testing.T) { + verifier := "test-verifier-12345" + c1 := sha256CodeChallenge(verifier) + c2 := sha256CodeChallenge(verifier) + assert.Equal(t, c1, c2) +} + +func TestAuthenticate_AddsPKCEParamsToAuthURL(t *testing.T) { + e := echo.New() + req := httptest.NewRequest(http.MethodGet, "/auth/sso?returnUrl=/dashboard", nil) + rec := httptest.NewRecorder() + c := e.NewContext(req, rec) + + oauthCfg := &oauth2.Config{ + ClientID: "test-client", + Endpoint: oauth2.Endpoint{ + AuthURL: "https://idp.example.com/auth", + TokenURL: "https://idp.example.com/token", + }, + RedirectURL: "https://app.example.com/auth/sso/callback", + Scopes: []string{"openid", "profile"}, + } + + handler := authenticate(oauthCfg, nil, []string{}) + err := handler(c) + require.NoError(t, err) + assert.Equal(t, http.StatusFound, rec.Code) + + location := rec.Header().Get("Location") + require.NotEmpty(t, location) + + parsed, err := url.Parse(location) + require.NoError(t, err) + + query := parsed.Query() + assert.Equal(t, "S256", query.Get("code_challenge_method"), "auth URL must include code_challenge_method=S256") + assert.NotEmpty(t, query.Get("code_challenge"), "auth URL must include code_challenge") + assert.NotEmpty(t, query.Get("state"), "auth URL must include state") + assert.NotEmpty(t, query.Get("nonce"), "auth URL must include nonce") +} + +func TestAuthenticate_SetsCodeVerifierCookie(t *testing.T) { + e := echo.New() + req := httptest.NewRequest(http.MethodGet, "/auth/sso", nil) + rec := httptest.NewRecorder() + c := e.NewContext(req, rec) + + oauthCfg := &oauth2.Config{ + ClientID: "test-client", + Endpoint: oauth2.Endpoint{ + AuthURL: "https://idp.example.com/auth", + TokenURL: "https://idp.example.com/token", + }, + RedirectURL: "https://app.example.com/auth/sso/callback", + Scopes: []string{"openid"}, + } + + handler := authenticate(oauthCfg, nil, []string{}) + err := handler(c) + require.NoError(t, err) + + setCookieHeaders := rec.Header().Values("Set-Cookie") + var codeVerifierCookie string + for _, h := range setCookieHeaders { + if containsCookieName(h, "code_verifier") && containsPathRoot(h) { + codeVerifierCookie = h + break + } + } + require.NotEmpty(t, codeVerifierCookie, "must set code_verifier cookie with Path=/") + + // The cookie should be HttpOnly and have a MaxAge + assert.Contains(t, codeVerifierCookie, "HttpOnly") + assert.Contains(t, codeVerifierCookie, "Max-Age=") + assert.Contains(t, codeVerifierCookie, "Path=/") +} + +func TestAuthenticate_SetsCallbackCookieForPreV280(t *testing.T) { + e := echo.New() + req := httptest.NewRequest(http.MethodGet, "/auth/sso", nil) + rec := httptest.NewRecorder() + c := e.NewContext(req, rec) + + oauthCfg := &oauth2.Config{ + ClientID: "test-client", + Endpoint: oauth2.Endpoint{ + AuthURL: "https://idp.example.com/auth", + TokenURL: "https://idp.example.com/token", + }, + RedirectURL: "https://app.example.com/auth/sso/callback", + Scopes: []string{"openid"}, + } + + handler := authenticate(oauthCfg, nil, []string{}) + err := handler(c) + require.NoError(t, err) + + setCookieHeaders := rec.Header().Values("Set-Cookie") + + // Should have an expired code_verifier cookie with empty path (pre-v2.8.0 cleanup) + var expiredCookie string + for _, h := range setCookieHeaders { + if containsCookieName(h, "code_verifier") && containsMaxAgeZeroOrNegative(h) { + expiredCookie = h + break + } + } + assert.NotEmpty(t, expiredCookie, "must set expired code_verifier cookie for pre-v2.8.0 cleanup") +} + +func TestLogout_ClearsCodeVerifier(t *testing.T) { + e := echo.New() + req := httptest.NewRequest(http.MethodGet, "/auth/logout", nil) + rec := httptest.NewRecorder() + c := e.NewContext(req, rec) + + handler := logout() + err := handler(c) + require.NoError(t, err) + + setCookieHeaders := rec.Header().Values("Set-Cookie") + var cleared bool + for _, h := range setCookieHeaders { + if containsCookieName(h, "code_verifier") { + cleared = containsMaxAgeZeroOrNegative(h) + } + } + assert.True(t, cleared, "logout should clear code_verifier cookie") +} + +func TestAuthenticate_WithOptionsIncludesPKCE(t *testing.T) { + e := echo.New() + req := httptest.NewRequest(http.MethodGet, "/auth/sso", nil) + rec := httptest.NewRecorder() + c := e.NewContext(req, rec) + + oauthCfg := &oauth2.Config{ + ClientID: "test-client", + Endpoint: oauth2.Endpoint{ + AuthURL: "https://idp.example.com/auth", + TokenURL: "https://idp.example.com/token", + }, + RedirectURL: "https://app.example.com/auth/sso/callback", + Scopes: []string{"openid"}, + } + + options := map[string]interface{}{ + "audience": "test-audience", + } + + handler := authenticate(oauthCfg, options, []string{}) + err := handler(c) + require.NoError(t, err) + + location := rec.Header().Get("Location") + parsed, err := url.Parse(location) + require.NoError(t, err) + + query := parsed.Query() + assert.Equal(t, "S256", query.Get("code_challenge_method")) + assert.NotEmpty(t, query.Get("code_challenge")) + assert.Equal(t, "test-audience", query.Get("audience")) +} + +// Test helpers + +func containsCookieName(header, name string) bool { + prefix := name + "=" + for _, part := range splitCookieHeader(header) { + part = trimSpace(part) + if len(part) >= len(prefix) && part[:len(prefix)] == prefix { + return true + } + } + return false +} + +func containsPathRoot(header string) bool { + for _, part := range splitCookieHeader(header) { + part = trimSpace(part) + if part == "Path=/" { + return true + } + } + return false +} + +func containsMaxAgeZeroOrNegative(header string) bool { + for _, part := range splitCookieHeader(header) { + part = trimSpace(part) + if len(part) > 8 && part[:8] == "Max-Age=" { + val := part[8:] + return val == "0" || len(val) > 0 && val[0] == '-' + } + } + return false +} + +func splitCookieHeader(header string) []string { + var parts []string + start := 0 + for i := 0; i < len(header); i++ { + if header[i] == ';' { + parts = append(parts, trimSpace(header[start:i])) + start = i + 1 + } + } + if start < len(header) { + parts = append(parts, trimSpace(header[start:])) + } + return parts +} + +func trimSpace(s string) string { + start, end := 0, len(s) + for start < end && (s[start] == ' ' || s[start] == '\t') { + start++ + } + for end > start && (s[end-1] == ' ' || s[end-1] == '\t') { + end-- + } + if start > 0 || end < len(s) { + return s[start:end] + } + return s +} From fe7e5ffbf6879ec0475e80962a62728001ecfc1b Mon Sep 17 00:00:00 2001 From: camohob Date: Wed, 3 Jun 2026 11:19:44 +0300 Subject: [PATCH 2/2] PKCE env variable --- AUTHENTICATION.md | 39 ++++++++++++++++++ server/config/docker.yaml | 1 + server/server/auth/oidc.go | 25 +++++++----- server/server/auth/oidc_test.go | 38 ++++++++++++++++-- server/server/config/config.go | 4 ++ server/server/route/auth.go | 33 +++++++++------- server/server/route/pkce_test.go | 68 ++++++++++++++++++++++++++++++-- 7 files changed, 176 insertions(+), 32 deletions(-) diff --git a/AUTHENTICATION.md b/AUTHENTICATION.md index ad503bb58b..f79f9a78d2 100644 --- a/AUTHENTICATION.md +++ b/AUTHENTICATION.md @@ -39,6 +39,7 @@ auth: | -------------------- | -------- | ------------------------------------------------------------------------------------------------------------------------------ | | `enabled` | boolean | Enable or disable authentication | | `maxSessionDuration` | duration | Maximum session duration before forced re-login (e.g., `8h`, `24h`, `168h`). Set to `0` or omit for unlimited session duration | +| `pkceEnabled` | boolean | Enable PKCE (Proof Key for Code Exchange) for the OAuth2 authorization code flow. Default: `false` | | `providers` | array | List of auth providers (currently only the first is used) | #### Provider Settings @@ -56,6 +57,44 @@ auth: | `options` | object | Additional URL parameters for the auth redirect | | `useIdTokenAsBearer` | boolean | Use ID token instead of access token in Authorization header | +## PKCE Support + +Temporal UI supports PKCE (Proof Key for Code Exchange, RFC 7636) for the OAuth2 authorization code flow. PKCE provides protection against authorization code interception attacks and is recommended for all OAuth2 applications, especially those that cannot securely store a client secret (e.g., public clients). + +### Configuration + +```yaml +auth: + enabled: true + pkceEnabled: true # Enable PKCE + providers: + # ... provider config +``` + +PKCE can also be enabled via environment variable: + +| Environment Variable | Default | Description | +| ---------------------------- | ------- | ---------------------------- | +| `TEMPORAL_AUTH_PKCE_ENABLED` | `false` | Set to `true` to enable PKCE | + +### How It Works + +When PKCE is enabled: + +1. The UI server generates a cryptographically random `code_verifier` and stores it in a secure, HttpOnly cookie +2. A `code_challenge` (SHA-256 hash of the verifier) is included in the authorization request as `code_challenge_method=S256` +3. Upon receiving the authorization code in the callback, the server sends the original `code_verifier` to the token endpoint +4. The identity provider verifies that the challenge matches the verifier, ensuring the party exchanging the code is the same one that initiated the request + +### When to Enable + +Enabling PKCE is recommended: + +- When using public clients or single-page applications +- To mitigate authorization code interception attacks +- As a defense-in-depth measure even for confidential clients with a client secret +- When required by your identity provider (many modern IdPs strongly recommend or require PKCE) + ## Session Duration Management ### Token Refresh vs Session Duration diff --git a/server/config/docker.yaml b/server/config/docker.yaml index 203a4d6c71..bbba397e84 100644 --- a/server/config/docker.yaml +++ b/server/config/docker.yaml @@ -49,6 +49,7 @@ uiServerTLS: keyFile: {{ env "TEMPORAL_UI_SERVER_TLS_KEY" | default "" }} auth: enabled: {{ env "TEMPORAL_AUTH_ENABLED" | default "false" }} + pkceEnabled: {{ env "TEMPORAL_AUTH_PKCE_ENABLED" | default "false" }} providers: - label: {{ env "TEMPORAL_AUTH_LABEL" | default "sso" }} type: {{ env "TEMPORAL_AUTH_TYPE" | default "oidc" }} diff --git a/server/server/auth/oidc.go b/server/server/auth/oidc.go index 7e71dc0a45..688878544c 100644 --- a/server/server/auth/oidc.go +++ b/server/server/auth/oidc.go @@ -58,7 +58,7 @@ type Claims struct { Picture string `json:"picture"` } -func ExchangeCode(ctx context.Context, r *http.Request, config *oauth2.Config, provider *oidc.Provider) (*User, error) { +func ExchangeCode(ctx context.Context, r *http.Request, config *oauth2.Config, provider *oidc.Provider, pkceEnabled bool) (*User, error) { state, err := r.Cookie("state") if err != nil { return nil, echo.NewHTTPError(http.StatusBadRequest, "State cookie is not set in request") @@ -67,14 +67,21 @@ func ExchangeCode(ctx context.Context, r *http.Request, config *oauth2.Config, p return nil, echo.NewHTTPError(http.StatusBadRequest, "State cookie did not match") } - codeVerifier, err := r.Cookie("code_verifier") - if err != nil { - return nil, echo.NewHTTPError(http.StatusBadRequest, "Code verifier is not set in request") - } - - oauth2Token, err := config.Exchange(ctx, r.URL.Query().Get("code"), oauth2.SetAuthURLParam("code_verifier", codeVerifier.Value)) - if err != nil { - return nil, echo.NewHTTPError(http.StatusInternalServerError, "Unable to exchange token: "+err.Error()) + var oauth2Token *oauth2.Token + if pkceEnabled { + codeVerifier, err := r.Cookie("code_verifier") + if err != nil { + return nil, echo.NewHTTPError(http.StatusBadRequest, "Code verifier is not set in request") + } + oauth2Token, err = config.Exchange(ctx, r.URL.Query().Get("code"), oauth2.SetAuthURLParam("code_verifier", codeVerifier.Value)) + if err != nil { + return nil, echo.NewHTTPError(http.StatusInternalServerError, "Unable to exchange token: "+err.Error()) + } + } else { + oauth2Token, err = config.Exchange(ctx, r.URL.Query().Get("code")) + if err != nil { + return nil, echo.NewHTTPError(http.StatusInternalServerError, "Unable to exchange token: "+err.Error()) + } } rawIDToken, ok := oauth2Token.Extra("id_token").(string) diff --git a/server/server/auth/oidc_test.go b/server/server/auth/oidc_test.go index f103a1aaea..581ada0d9b 100644 --- a/server/server/auth/oidc_test.go +++ b/server/server/auth/oidc_test.go @@ -209,7 +209,7 @@ func TestExchangeCode_MissingCodeVerifier(t *testing.T) { req.AddCookie(&http.Cookie{Name: "state", Value: "test-state"}) cfg := harness.OAuth2Config() - _, err = auth.ExchangeCode(context.Background(), req, cfg, provider) + _, err = auth.ExchangeCode(context.Background(), req, cfg, provider, true) require.Error(t, err) assert.Contains(t, err.Error(), "Code verifier is not set") } @@ -230,7 +230,7 @@ func TestExchangeCode_WithPKCE(t *testing.T) { req.AddCookie(&http.Cookie{Name: "nonce", Value: tokenNonce}) cfg := harness.OAuth2Config() - user, err := auth.ExchangeCode(context.Background(), req, cfg, provider) + user, err := auth.ExchangeCode(context.Background(), req, cfg, provider, true) require.NoError(t, err) require.NotNil(t, user) require.NotNil(t, user.OAuth2Token) @@ -248,6 +248,36 @@ func TestExchangeCode_WithPKCE(t *testing.T) { "code_verifier must be sent to token endpoint") } +func TestExchangeCode_WithoutPKCE(t *testing.T) { + harness := newOIDCTestHarness(t) + defer harness.Close() + + tokenNonce := "non-pkce-nonce" + harness.nonce = tokenNonce + + provider, err := harness.Provider(context.Background()) + require.NoError(t, err) + + req := httptest.NewRequest(http.MethodGet, "/auth/sso/callback?code=test-code&state=test-state", nil) + req.AddCookie(&http.Cookie{Name: "state", Value: "test-state"}) + req.AddCookie(&http.Cookie{Name: "nonce", Value: tokenNonce}) + + cfg := harness.OAuth2Config() + user, err := auth.ExchangeCode(context.Background(), req, cfg, provider, false) + require.NoError(t, err) + require.NotNil(t, user) + require.NotNil(t, user.OAuth2Token) + assert.Equal(t, "mock-access-token", user.OAuth2Token.AccessToken) + + require.NotNil(t, user.IDToken) + require.NotNil(t, user.IDToken.Claims) + assert.Equal(t, "test@example.com", user.IDToken.Claims.Email) + + require.NotNil(t, harness.lastTokenParams) + assert.Empty(t, harness.lastTokenParams.Get("code_verifier"), + "code_verifier must NOT be sent to token endpoint when PKCE disabled") +} + func TestExchangeCode_StateMismatch(t *testing.T) { harness := newOIDCTestHarness(t) defer harness.Close() @@ -261,7 +291,7 @@ func TestExchangeCode_StateMismatch(t *testing.T) { req.AddCookie(&http.Cookie{Name: "nonce", Value: "test-nonce"}) cfg := harness.OAuth2Config() - _, err = auth.ExchangeCode(context.Background(), req, cfg, provider) + _, err = auth.ExchangeCode(context.Background(), req, cfg, provider, true) require.Error(t, err) assert.Contains(t, err.Error(), "State cookie did not match") } @@ -279,7 +309,7 @@ func TestExchangeCode_NonceMismatch(t *testing.T) { req.AddCookie(&http.Cookie{Name: "nonce", Value: "expected-nonce"}) cfg := harness.OAuth2Config() - _, err = auth.ExchangeCode(context.Background(), req, cfg, provider) + _, err = auth.ExchangeCode(context.Background(), req, cfg, provider, true) require.Error(t, err) assert.Contains(t, err.Error(), "Nonce did not match") } diff --git a/server/server/config/config.go b/server/server/config/config.go index a569e689e9..9f263fd1c1 100644 --- a/server/server/config/config.go +++ b/server/server/config/config.go @@ -108,6 +108,10 @@ type ( // forced to re-login after this duration regardless of token validity. // Example values: "8h", "24h", "168h" (1 week). If zero, no max duration is enforced. MaxSessionDuration time.Duration `yaml:"maxSessionDuration"` + // PKCEEnabled - enable PKCE (Proof Key for Code Exchange) for OAuth2 authorization code flow. + // When enabled, the server generates a code_verifier and code_challenge (S256) for each auth request. + // Default is false (PKCE disabled). + PKCEEnabled bool `yaml:"pkceEnabled"` } AuthProvider struct { diff --git a/server/server/route/auth.go b/server/server/route/auth.go index cb8257fc21..1a2d9e3e6e 100644 --- a/server/server/route/auth.go +++ b/server/server/route/auth.go @@ -110,14 +110,14 @@ func SetAuthRoutes(e *echo.Echo, cfgProvider *config.ConfigProviderWithRefresh) } api := e.Group("/auth") - api.GET("/sso", authenticate(&oauthCfg, providerCfg.Options, serverCfg.CORS.AllowOrigins)) - api.GET("/sso/callback", authenticateCb(ctx, &oauthCfg, provider, serverCfg.Auth.MaxSessionDuration)) - api.GET("/sso_callback", authenticateCb(ctx, &oauthCfg, provider, serverCfg.Auth.MaxSessionDuration)) // compatibility with UI v1 + api.GET("/sso", authenticate(&oauthCfg, providerCfg.Options, serverCfg.CORS.AllowOrigins, serverCfg.Auth.PKCEEnabled)) + api.GET("/sso/callback", authenticateCb(ctx, &oauthCfg, provider, serverCfg.Auth.MaxSessionDuration, serverCfg.Auth.PKCEEnabled)) + api.GET("/sso_callback", authenticateCb(ctx, &oauthCfg, provider, serverCfg.Auth.MaxSessionDuration, serverCfg.Auth.PKCEEnabled)) // compatibility with UI v1 api.GET("/refresh", refreshTokens(ctx, &oauthCfg, provider, serverCfg.Auth.MaxSessionDuration)) api.GET("/logout", logout()) } -func authenticate(config *oauth2.Config, options map[string]interface{}, allowedOrigins []string) func(echo.Context) error { +func authenticate(config *oauth2.Config, options map[string]interface{}, allowedOrigins []string, pkceEnabled bool) func(echo.Context) error { return func(c echo.Context) error { state, err := randString() if err != nil { @@ -130,17 +130,20 @@ func authenticate(config *oauth2.Config, options map[string]interface{}, allowed setCallbackCookie(c, "state", state) setCallbackCookie(c, "nonce", nonce) - codeVerifier, err := randCodeVerifier() - if err != nil { - return echo.NewHTTPError(http.StatusInternalServerError, err) - } - setCallbackCookie(c, "code_verifier", codeVerifier) + opts := []oauth2.AuthCodeOption{oidc.Nonce(nonce)} - opts := []oauth2.AuthCodeOption{ - oidc.Nonce(nonce), - oauth2.SetAuthURLParam("code_challenge_method", "S256"), - oauth2.SetAuthURLParam("code_challenge", sha256CodeChallenge(codeVerifier)), + if pkceEnabled { + codeVerifier, err := randCodeVerifier() + if err != nil { + return echo.NewHTTPError(http.StatusInternalServerError, err) + } + setCallbackCookie(c, "code_verifier", codeVerifier) + opts = append(opts, + oauth2.SetAuthURLParam("code_challenge_method", "S256"), + oauth2.SetAuthURLParam("code_challenge", sha256CodeChallenge(codeVerifier)), + ) } + for k, v := range options { var value string if vStr, ok := v.(string); ok { @@ -163,9 +166,9 @@ func authenticate(config *oauth2.Config, options map[string]interface{}, allowed } } -func authenticateCb(ctx context.Context, oauthCfg *oauth2.Config, provider *oidc.Provider, maxSessionDuration time.Duration) func(echo.Context) error { +func authenticateCb(ctx context.Context, oauthCfg *oauth2.Config, provider *oidc.Provider, maxSessionDuration time.Duration, pkceEnabled bool) func(echo.Context) error { return func(c echo.Context) error { - user, err := auth.ExchangeCode(ctx, c.Request(), oauthCfg, provider) + user, err := auth.ExchangeCode(ctx, c.Request(), oauthCfg, provider, pkceEnabled) if err != nil { return err } diff --git a/server/server/route/pkce_test.go b/server/server/route/pkce_test.go index 779d9524b5..1cd88a306f 100644 --- a/server/server/route/pkce_test.go +++ b/server/server/route/pkce_test.go @@ -69,7 +69,7 @@ func TestAuthenticate_AddsPKCEParamsToAuthURL(t *testing.T) { Scopes: []string{"openid", "profile"}, } - handler := authenticate(oauthCfg, nil, []string{}) + handler := authenticate(oauthCfg, nil, []string{}, true) err := handler(c) require.NoError(t, err) assert.Equal(t, http.StatusFound, rec.Code) @@ -103,7 +103,7 @@ func TestAuthenticate_SetsCodeVerifierCookie(t *testing.T) { Scopes: []string{"openid"}, } - handler := authenticate(oauthCfg, nil, []string{}) + handler := authenticate(oauthCfg, nil, []string{}, true) err := handler(c) require.NoError(t, err) @@ -139,7 +139,7 @@ func TestAuthenticate_SetsCallbackCookieForPreV280(t *testing.T) { Scopes: []string{"openid"}, } - handler := authenticate(oauthCfg, nil, []string{}) + handler := authenticate(oauthCfg, nil, []string{}, true) err := handler(c) require.NoError(t, err) @@ -176,6 +176,66 @@ func TestLogout_ClearsCodeVerifier(t *testing.T) { assert.True(t, cleared, "logout should clear code_verifier cookie") } +func TestAuthenticate_PKCEDisabled_NoChallengeParams(t *testing.T) { + e := echo.New() + req := httptest.NewRequest(http.MethodGet, "/auth/sso", nil) + rec := httptest.NewRecorder() + c := e.NewContext(req, rec) + + oauthCfg := &oauth2.Config{ + ClientID: "test-client", + Endpoint: oauth2.Endpoint{ + AuthURL: "https://idp.example.com/auth", + TokenURL: "https://idp.example.com/token", + }, + RedirectURL: "https://app.example.com/auth/sso/callback", + Scopes: []string{"openid"}, + } + + handler := authenticate(oauthCfg, nil, []string{}, false) + err := handler(c) + require.NoError(t, err) + + location := rec.Header().Get("Location") + require.NotEmpty(t, location) + + parsed, err := url.Parse(location) + require.NoError(t, err) + + query := parsed.Query() + assert.Empty(t, query.Get("code_challenge_method"), "must NOT include code_challenge_method when PKCE disabled") + assert.Empty(t, query.Get("code_challenge"), "must NOT include code_challenge when PKCE disabled") + assert.NotEmpty(t, query.Get("state"), "auth URL must include state") + assert.NotEmpty(t, query.Get("nonce"), "auth URL must include nonce") +} + +func TestAuthenticate_PKCEDisabled_NoVerifierCookie(t *testing.T) { + e := echo.New() + req := httptest.NewRequest(http.MethodGet, "/auth/sso", nil) + rec := httptest.NewRecorder() + c := e.NewContext(req, rec) + + oauthCfg := &oauth2.Config{ + ClientID: "test-client", + Endpoint: oauth2.Endpoint{ + AuthURL: "https://idp.example.com/auth", + TokenURL: "https://idp.example.com/token", + }, + RedirectURL: "https://app.example.com/auth/sso/callback", + Scopes: []string{"openid"}, + } + + handler := authenticate(oauthCfg, nil, []string{}, false) + err := handler(c) + require.NoError(t, err) + + setCookieHeaders := rec.Header().Values("Set-Cookie") + for _, h := range setCookieHeaders { + assert.False(t, containsCookieName(h, "code_verifier"), + "must NOT set code_verifier cookie when PKCE disabled") + } +} + func TestAuthenticate_WithOptionsIncludesPKCE(t *testing.T) { e := echo.New() req := httptest.NewRequest(http.MethodGet, "/auth/sso", nil) @@ -196,7 +256,7 @@ func TestAuthenticate_WithOptionsIncludesPKCE(t *testing.T) { "audience": "test-audience", } - handler := authenticate(oauthCfg, options, []string{}) + handler := authenticate(oauthCfg, options, []string{}, true) err := handler(c) require.NoError(t, err)