diff --git a/internal/command/idp_intent_test.go b/internal/command/idp_intent_test.go index 9503548ed..3e01048d3 100644 --- a/internal/command/idp_intent_test.go +++ b/internal/command/idp_intent_test.go @@ -439,8 +439,9 @@ func TestCommands_AuthFromProvider(t *testing.T) { samlRootURL string } type res struct { - auth idp.Auth - err error + auth idp.Auth + expectPKCE bool + err error } tests := []struct { name string @@ -585,7 +586,8 @@ func TestCommands_AuthFromProvider(t *testing.T) { callbackURL: "url", }, res{ - auth: &idp.RedirectAuth{RedirectURL: "auth?client_id=clientID&prompt=select_account&redirect_uri=url&response_type=code&state=id"}, + auth: &idp.RedirectAuth{RedirectURL: "auth?client_id=clientID&prompt=select_account&redirect_uri=url&response_type=code&state=id"}, + expectPKCE: true, }, }, { @@ -647,7 +649,8 @@ func TestCommands_AuthFromProvider(t *testing.T) { loginHint: "user@example.com", }, res{ - auth: &idp.RedirectAuth{RedirectURL: "auth?client_id=clientID&login_hint=user%40example.com&redirect_uri=url&response_type=code&state=id"}, + auth: &idp.RedirectAuth{RedirectURL: "auth?client_id=clientID&login_hint=user%40example.com&redirect_uri=url&response_type=code&state=id"}, + expectPKCE: true, }, }, { @@ -755,13 +758,44 @@ func TestCommands_AuthFromProvider(t *testing.T) { var got idp.Auth if err == nil { got, err = session.GetAuth(tt.args.ctx) - assert.Equal(t, tt.res.auth, got) assert.NoError(t, err) + if tt.res.expectPKCE { + assertRedirectAuthWithPKCE(t, tt.res.auth, got) + assert.NotEmpty(t, session.PersistentParameters()[oauth.CodeVerifier]) + } else { + assert.Equal(t, tt.res.auth, got) + } } }) } } +func assertRedirectAuthWithPKCE(t *testing.T, expected, actual idp.Auth) { + t.Helper() + + expectedRedirect, ok := expected.(*idp.RedirectAuth) + require.True(t, ok) + actualRedirect, ok := actual.(*idp.RedirectAuth) + require.True(t, ok) + + expectedURL, err := url.Parse(expectedRedirect.RedirectURL) + require.NoError(t, err) + actualURL, err := url.Parse(actualRedirect.RedirectURL) + require.NoError(t, err) + + actualQuery := actualURL.Query() + assert.NotEmpty(t, actualQuery.Get("code_challenge")) + assert.Equal(t, "S256", actualQuery.Get("code_challenge_method")) + + actualQuery.Del("code_challenge") + actualQuery.Del("code_challenge_method") + + assert.Equal(t, expectedURL.Scheme, actualURL.Scheme) + assert.Equal(t, expectedURL.Host, actualURL.Host) + assert.Equal(t, expectedURL.Path, actualURL.Path) + assert.Equal(t, expectedURL.Query(), actualQuery) +} + func TestCommands_AuthFromProvider_SAML(t *testing.T) { type fields struct { eventstore func(t *testing.T) *eventstore.Eventstore diff --git a/internal/command/idp_model.go b/internal/command/idp_model.go index fdfd9e21c..c8f73defe 100644 --- a/internal/command/idp_model.go +++ b/internal/command/idp_model.go @@ -179,7 +179,10 @@ func (wm *OAuthIDPWriteModel) ToProvider(callbackURL string, idpAlg crypto.Encry RedirectURL: callbackURL, Scopes: wm.Scopes, } - opts := make([]oauth.ProviderOpts, 0, 4) + opts := make([]oauth.ProviderOpts, 0, 5) + if wm.UsePKCE { + opts = append(opts, oauth.WithRelyingPartyOption(rp.WithPKCE(nil))) + } if wm.IsCreationAllowed { opts = append(opts, oauth.WithCreationAllowed()) } @@ -382,11 +385,14 @@ func (wm *OIDCIDPWriteModel) ToProvider(callbackURL string, idpAlg crypto.Encryp if err != nil { return nil, err } - opts := make([]oidc.ProviderOpts, 1, 6) + opts := make([]oidc.ProviderOpts, 1, 7) opts[0] = oidc.WithSelectAccount() if wm.IsIDTokenMapping { opts = append(opts, oidc.WithIDTokenMapping()) } + if wm.UsePKCE { + opts = append(opts, oidc.WithRelyingPartyOption(rp.WithPKCE(nil))) + } if wm.IsCreationAllowed { opts = append(opts, oidc.WithCreationAllowed()) } diff --git a/internal/command/idp_model_test.go b/internal/command/idp_model_test.go index 423bff39b..d4deba371 100644 --- a/internal/command/idp_model_test.go +++ b/internal/command/idp_model_test.go @@ -1,12 +1,22 @@ package command import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "net/url" + "sync/atomic" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + oidc_pkg "github.com/zitadel/oidc/v3/pkg/oidc" + "github.com/zitadel/zitadel/internal/crypto" "github.com/zitadel/zitadel/internal/domain" + providers "github.com/zitadel/zitadel/internal/idp" + "github.com/zitadel/zitadel/internal/idp/providers/oauth" "github.com/zitadel/zitadel/internal/zerrors" ) @@ -353,3 +363,92 @@ func TestCommands_AllIDPWriteModel(t *testing.T) { }) } } + +func TestOAuthIDPWriteModel_ToProvider_WithPKCE(t *testing.T) { + wm := &OAuthIDPWriteModel{ + Name: "OAuth", + ClientID: "clientID", + ClientSecret: &crypto.CryptoValue{Algorithm: "plain", Crypted: []byte("clientSecret"), KeyID: "keyID"}, + AuthorizationEndpoint: "https://idp.example.com/authorize", + TokenEndpoint: "https://idp.example.com/token", + UserEndpoint: "https://idp.example.com/user", + Scopes: []string{"user"}, + IDAttribute: "id", + UsePKCE: true, + } + + provider, err := wm.ToProvider("https://zitadel.example.com/idps/callback", plainTextEncryption{}) + require.NoError(t, err) + + assertProviderUsesPKCE(t, provider) +} + +func TestOIDCIDPWriteModel_ToProvider_WithPKCE(t *testing.T) { + var ( + issuer string + discoveryRequested atomic.Bool + ) + handler := func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != oidc_pkg.DiscoveryEndpoint { + http.NotFound(w, r) + return + } + discoveryRequested.Store(true) + w.Header().Set("Content-Type", "application/json") + // assert, not require: FailNow must not be called from the server's handler goroutine + assert.NoError(t, json.NewEncoder(w).Encode(&oidc_pkg.DiscoveryConfiguration{ + Issuer: issuer, + AuthorizationEndpoint: issuer + "/authorize", + TokenEndpoint: issuer + "/token", + UserinfoEndpoint: issuer + "/userinfo", + })) + } + + server := httptest.NewServer(http.HandlerFunc(handler)) + t.Cleanup(server.Close) + issuer = server.URL + + wm := &OIDCIDPWriteModel{ + Name: "OIDC", + Issuer: issuer, + ClientID: "clientID", + ClientSecret: &crypto.CryptoValue{Algorithm: "plain", Crypted: []byte("clientSecret"), KeyID: "keyID"}, + Scopes: []string{"openid"}, + UsePKCE: true, + } + + provider, err := wm.ToProvider("https://zitadel.example.com/idps/callback", plainTextEncryption{}) + require.NoError(t, err) + require.True(t, discoveryRequested.Load(), "expected OIDC discovery to be requested") + + assertProviderUsesPKCE(t, provider) +} + +func assertProviderUsesPKCE(t *testing.T, provider providers.Provider) { + t.Helper() + + session, err := provider.BeginAuth(context.Background(), "state") + require.NoError(t, err) + auth, err := session.GetAuth(context.Background()) + require.NoError(t, err) + redirect, ok := auth.(*providers.RedirectAuth) + require.True(t, ok) + authURL, err := url.Parse(redirect.RedirectURL) + require.NoError(t, err) + + query := authURL.Query() + assert.NotEmpty(t, query.Get("code_challenge")) + assert.Equal(t, "S256", query.Get("code_challenge_method")) + assert.NotEmpty(t, session.PersistentParameters()[oauth.CodeVerifier]) +} + +type plainTextEncryption struct{} + +func (plainTextEncryption) Algorithm() string { return "plain" } +func (plainTextEncryption) EncryptionKeyID() string { return "keyID" } +func (plainTextEncryption) DecryptionKeyIDs() []string { return []string{"keyID"} } +func (plainTextEncryption) Encrypt(value []byte) ([]byte, error) { return value, nil } +func (plainTextEncryption) Decrypt(hashed []byte, _ string) ([]byte, error) { return hashed, nil } +func (plainTextEncryption) DecryptString(hashed []byte, _ string) (string, error) { + return string(hashed), nil +}