From fd0c15dd4f201c9b362eec983d9a01cecef4c45e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20M=C3=B6hlmann?= Date: Fri, 23 Aug 2024 15:43:46 +0300 Subject: [PATCH] feat(oidc): use web keys for token signing and verification (#8449) # Which Problems Are Solved Use web keys, managed by the `resources/v3alpha/web_keys` API, for OIDC token signing and verification, as well as serving the public web keys on the jwks / keys endpoint. Response header on the keys endpoint now allows caching of the response. This is now "safe" to do since keys can be created ahead of time and caches have sufficient time to pickup the change before keys get enabled. # How the Problems Are Solved - The web key format is used in the `getSignerOnce` function in the `api/oidc` package. - The public key cache is changed to get and store web keys. - The jwks / keys endpoint returns the combined set of valid "legacy" public keys and all available web keys. - Cache-Control max-age default to 5 minutes and is configured in `defaults.yaml`. When the web keys feature is enabled, fallback mechanisms are in place to obtain and convert "legacy" `query.PublicKey` as web keys when needed. This allows transitioning to the feature without invalidating existing tokens. A small performance overhead may be noticed on the keys endpoint, because 2 queries need to be run sequentially. This will disappear once the feature is stable and the legacy code gets cleaned up. # Additional Changes - Extend legacy key lifetimes so that tests can be run on an existing database with more than 6 hours apart. - Discovery endpoint returns all supported algorithms when the Web Key feature is enabled. # Additional Context - Closes https://github.com/zitadel/zitadel/issues/8031 - Part of https://github.com/zitadel/zitadel/issues/7809 - After https://github.com/zitadel/oidc/pull/637 - After https://github.com/zitadel/oidc/pull/638 --- cmd/defaults.yaml | 11 +- go.mod | 6 +- go.sum | 12 +- .../api/http/middleware/cache_interceptor.go | 5 + internal/api/oidc/access_token.go | 4 +- internal/api/oidc/key.go | 114 +++++++++-- internal/api/oidc/key_test.go | 178 +++++++++++++++--- internal/api/oidc/keys_integration_test.go | 115 +++++++++++ internal/api/oidc/op.go | 38 +++- internal/api/oidc/server.go | 11 +- internal/api/oidc/server_test.go | 89 +++++++++ internal/api/oidc/token.go | 31 ++- .../api/oidc/userinfo_integration_test.go | 13 ++ internal/integration/config/zitadel.yaml | 11 +- .../webkey/v3alpha/webkey_service.proto | 2 +- 15 files changed, 570 insertions(+), 70 deletions(-) create mode 100644 internal/api/oidc/keys_integration_test.go diff --git a/cmd/defaults.yaml b/cmd/defaults.yaml index 0f56ddb8b..76f2fb7fb 100644 --- a/cmd/defaults.yaml +++ b/cmd/defaults.yaml @@ -339,6 +339,9 @@ OIDC: AuthMethodPrivateKeyJWT: true # ZITADEL_OIDC_AUTHMETHODPRIVATEKEYJWT GrantTypeRefreshToken: true # ZITADEL_OIDC_GRANTTYPEREFRESHTOKEN RequestObjectSupported: true # ZITADEL_OIDC_REQUESTOBJECTSUPPORTED + + # Deprecated: The signing algorithm is determined by the generated keys. + # Use the web keys resource to generate keys with different algorithms. SigningKeyAlgorithm: RS256 # ZITADEL_OIDC_SIGNINGKEYALGORITHM # Sets the default values for lifetime and expiration for OIDC # This default can be overwritten in the default instance configuration and for each instance during runtime @@ -349,10 +352,10 @@ OIDC: DefaultRefreshTokenIdleExpiration: 720h # ZITADEL_OIDC_DEFAULTREFRESHTOKENIDLEEXPIRATION # 2160h are 90 days, three months DefaultRefreshTokenExpiration: 2160h # ZITADEL_OIDC_DEFAULTREFRESHTOKENEXPIRATION - Cache: - MaxAge: 12h # ZITADEL_OIDC_CACHE_MAXAGE - # 168h is 7 days, one week - SharedMaxAge: 168h # ZITADEL_OIDC_CACHE_SHAREDMAXAGE + + # HTTP Cache-Control max-age header value to set on the jwks endpoint. + # Only used when the web keys feature is enabled. 0 sets a no-store value. + JWKSCacheControlMaxAge: 5m # ZITADEL_OIDC_JWKSCACHECONTROLMAXAGE CustomEndpoints: Auth: Path: /oauth/v2/authorize # ZITADEL_OIDC_CUSTOMENDPOINTS_AUTH_PATH diff --git a/go.mod b/go.mod index 70c0b2859..e514c62f7 100644 --- a/go.mod +++ b/go.mod @@ -59,7 +59,7 @@ require ( github.com/superseriousbusiness/exifremove v0.0.0-20210330092427-6acd27eac203 github.com/ttacon/libphonenumber v1.2.1 github.com/zitadel/logging v0.6.0 - github.com/zitadel/oidc/v3 v3.27.0 + github.com/zitadel/oidc/v3 v3.28.1 github.com/zitadel/passwap v0.6.0 github.com/zitadel/saml v0.1.3 github.com/zitadel/schema v1.3.0 @@ -78,8 +78,8 @@ require ( golang.org/x/exp v0.0.0-20240613232115-7f521ea00fb8 golang.org/x/net v0.26.0 golang.org/x/oauth2 v0.22.0 - golang.org/x/sync v0.7.0 - golang.org/x/text v0.16.0 + golang.org/x/sync v0.8.0 + golang.org/x/text v0.17.0 google.golang.org/api v0.187.0 google.golang.org/genproto/googleapis/api v0.0.0-20240701130421-f6361c86f094 google.golang.org/grpc v1.65.0 diff --git a/go.sum b/go.sum index 30eee5954..aef33fe16 100644 --- a/go.sum +++ b/go.sum @@ -723,8 +723,8 @@ github.com/zenazn/goji v1.0.1 h1:4lbD8Mx2h7IvloP7r2C0D6ltZP6Ufip8Hn0wmSK5LR8= github.com/zenazn/goji v1.0.1/go.mod h1:7S9M489iMyHBNxwZnk9/EHS098H4/F6TATF2mIxtB1Q= github.com/zitadel/logging v0.6.0 h1:t5Nnt//r+m2ZhhoTmoPX+c96pbMarqJvW1Vq6xFTank= github.com/zitadel/logging v0.6.0/go.mod h1:Y4CyAXHpl3Mig6JOszcV5Rqqsojj+3n7y2F591Mp/ow= -github.com/zitadel/oidc/v3 v3.27.0 h1:zeYpyRH0UcgdCjVHUYzSsqf1jbSwVMPVxYKOnRXstgU= -github.com/zitadel/oidc/v3 v3.27.0/go.mod h1:ZwBEqSviCpJVZiYashzo53bEGRGXi7amE5Q8PpQg9IM= +github.com/zitadel/oidc/v3 v3.28.1 h1:PsbFm5CzEMQq9HBXUNJ8yvnWmtVYxpwV5Cinj7TTsHo= +github.com/zitadel/oidc/v3 v3.28.1/go.mod h1:WmDFu3dZ9YNKrIoZkmxjGG8QyUR4PbbhsVVSY+rpojM= github.com/zitadel/passwap v0.6.0 h1:m9F3epFC0VkBXu25rihSLGyHvWiNlCzU5kk8RoI+SXQ= github.com/zitadel/passwap v0.6.0/go.mod h1:kqAiJ4I4eZvm3Y6oAk6hlEqlZZOkjMHraGXF90GG7LI= github.com/zitadel/saml v0.1.3 h1:LI4DOCVyyU1qKPkzs3vrGcA5J3H4pH3+CL9zr9ShkpM= @@ -871,8 +871,8 @@ golang.org/x/sync v0.0.0-20190911185100-cd5d95a43a6e/go.mod h1:RxMgew5VJxzue5/jJ golang.org/x/sync v0.0.0-20201020160332-67f06af15bc9/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20220722155255-886fb9371eb4/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.1.0/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= -golang.org/x/sync v0.7.0 h1:YsImfSBoP9QPYL0xyKJPq0gcaJdG3rInoqxTWbfQu9M= -golang.org/x/sync v0.7.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk= +golang.org/x/sync v0.8.0 h1:3NFvSEYkUoMifnESzZl15y791HH1qU2xm6eCJU5ZPXQ= +golang.org/x/sync v0.8.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk= golang.org/x/sys v0.0.0-20180823144017-11551d06cbcc/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20180830151530-49385e6e1522/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20180905080454-ebe1bf3edb33/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= @@ -932,8 +932,8 @@ golang.org/x/text v0.3.7/go.mod h1:u+2+/6zg+i71rQMx5EYifcz6MCKuco9NR6JIITiCfzQ= golang.org/x/text v0.7.0/go.mod h1:mrYo+phRRbMaCq/xk9113O4dZlRixOauAjOtrjsXDZ8= golang.org/x/text v0.9.0/go.mod h1:e1OnstbJyHTd6l/uOt8jFFHp6TRDWZR/bV3emEE/zU8= golang.org/x/text v0.14.0/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU= -golang.org/x/text v0.16.0 h1:a94ExnEXNtEwYLGJSIUxnWoxoRz/ZcCsV63ROupILh4= -golang.org/x/text v0.16.0/go.mod h1:GhwF1Be+LQoKShO3cGOHzqOgRrGaYc9AvblQOmPVHnI= +golang.org/x/text v0.17.0 h1:XtiM5bkSOt+ewxlOE/aE/AKEHibwj/6gvWMl9Rsh0Qc= +golang.org/x/text v0.17.0/go.mod h1:BuEKDfySbSR4drPmRPG/7iBdf8hvFMuRexcpahXilzY= golang.org/x/time v0.0.0-20180412165947-fbb02b2291d2/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ= golang.org/x/time v0.0.0-20191024005414-555d28b269f0/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ= golang.org/x/time v0.5.0 h1:o7cqy6amK/52YcAKIPlM3a+Fpj35zvRj2TP+e1xFSfk= diff --git a/internal/api/http/middleware/cache_interceptor.go b/internal/api/http/middleware/cache_interceptor.go index 04dbf2b6c..de0c0df61 100644 --- a/internal/api/http/middleware/cache_interceptor.go +++ b/internal/api/http/middleware/cache_interceptor.go @@ -108,6 +108,11 @@ func (c *Cache) serializeHeaders(w http.ResponseWriter) { control := make([]string, 0, 6) pragma := false + // Do not overwrite cache-control header if set by business logic. + if w.Header().Get(http_utils.CacheControl) != "" { + return + } + if c.Cacheability != CacheabilityNotSet { control = append(control, string(c.Cacheability)) control = append(control, fmt.Sprintf("max-age=%v", c.MaxAge.Seconds())) diff --git a/internal/api/oidc/access_token.go b/internal/api/oidc/access_token.go index 4ac1646ed..66da6e3cc 100644 --- a/internal/api/oidc/access_token.go +++ b/internal/api/oidc/access_token.go @@ -52,7 +52,9 @@ func (s *Server) verifyAccessToken(ctx context.Context, tkn string) (_ *accessTo } tokenID, subject = split[0], split[1] } else { - verifier := op.NewAccessTokenVerifier(op.IssuerFromContext(ctx), s.accessTokenKeySet) + verifier := op.NewAccessTokenVerifier(op.IssuerFromContext(ctx), s.accessTokenKeySet, + op.WithSupportedAccessTokenSigningAlgorithms(supportedSigningAlgs(ctx)...), + ) claims, err := op.VerifyAccessToken[*oidc.AccessTokenClaims](ctx, tkn, verifier) if err != nil { return nil, zerrors.ThrowPermissionDenied(err, "OIDC-Eib8e", "token is not valid or has expired") diff --git a/internal/api/oidc/key.go b/internal/api/oidc/key.go index 2db5baf83..a7e156fe7 100644 --- a/internal/api/oidc/key.go +++ b/internal/api/oidc/key.go @@ -3,16 +3,19 @@ package oidc import ( "context" "fmt" + "slices" "sync" "sync/atomic" "time" "github.com/go-jose/go-jose/v4" "github.com/jonboulle/clockwork" + "github.com/muhlemmer/gu" "github.com/zitadel/logging" "github.com/zitadel/oidc/v3/pkg/op" "github.com/zitadel/zitadel/internal/api/authz" + http_util "github.com/zitadel/zitadel/internal/api/http" "github.com/zitadel/zitadel/internal/crypto" "github.com/zitadel/zitadel/internal/eventstore" "github.com/zitadel/zitadel/internal/query" @@ -22,14 +25,33 @@ import ( "github.com/zitadel/zitadel/internal/zerrors" ) -type cachedPublicKey struct { - lastUse atomic.Int64 // unix micro time. - query.PublicKey +var supportedWebKeyAlgs = []string{ + string(jose.EdDSA), + string(jose.RS256), + string(jose.RS384), + string(jose.RS512), + string(jose.ES256), + string(jose.ES384), + string(jose.ES512), } -func newCachedPublicKey(key query.PublicKey, now time.Time) *cachedPublicKey { +func supportedSigningAlgs(ctx context.Context) []string { + if authz.GetFeatures(ctx).WebKey { + return supportedWebKeyAlgs + } + return []string{string(jose.RS256)} +} + +type cachedPublicKey struct { + lastUse atomic.Int64 // unix micro time. + expiry *time.Time // expiry may be nil if the key does not expire. + webKey *jose.JSONWebKey +} + +func newCachedPublicKey(key *jose.JSONWebKey, expiry *time.Time, now time.Time) *cachedPublicKey { cachedKey := &cachedPublicKey{ - PublicKey: key, + expiry: expiry, + webKey: key, } cachedKey.setLastUse(now) return cachedKey @@ -53,14 +75,17 @@ func (c *cachedPublicKey) expired(now time.Time, validity time.Duration) bool { type publicKeyCache struct { mtx sync.RWMutex instanceKeys map[string]map[string]*cachedPublicKey - queryKey func(ctx context.Context, keyID string) (query.PublicKey, error) - clock clockwork.Clock + + // queryKey returns a public web key. + // If the key does not have expiry, Time may be nil. + queryKey func(ctx context.Context, keyID string) (*jose.JSONWebKey, *time.Time, error) + clock clockwork.Clock } // newPublicKeyCache initializes a keySetCache starts a purging Go routine. // The purge routine deletes all public keys that are older than maxAge. // When the passed context is done, the purge routine will terminate. -func newPublicKeyCache(background context.Context, maxAge time.Duration, queryKey func(ctx context.Context, keyID string) (query.PublicKey, error)) *publicKeyCache { +func newPublicKeyCache(background context.Context, maxAge time.Duration, queryKey func(ctx context.Context, keyID string) (*jose.JSONWebKey, *time.Time, error)) *publicKeyCache { k := &publicKeyCache{ instanceKeys: make(map[string]map[string]*cachedPublicKey), queryKey: queryKey, @@ -119,11 +144,11 @@ func (k *publicKeyCache) getKey(ctx context.Context, keyID string) (_ *cachedPub if ok { key.setLastUse(k.clock.Now()) } else { - newKey, err := k.queryKey(ctx, keyID) + newKey, expiry, err := k.queryKey(ctx, keyID) if err != nil { return nil, err } - key = newCachedPublicKey(newKey, k.clock.Now()) + key = newCachedPublicKey(newKey, expiry, k.clock.Now()) k.setKey(instanceID, keyID, key) } @@ -144,10 +169,10 @@ func (k *publicKeyCache) verifySignature(ctx context.Context, jws *jose.JSONWebS if err != nil { return nil, err } - if checkKeyExpiry && key.Expiry().Before(k.clock.Now()) { + if checkKeyExpiry && key.expiry != nil && key.expiry.Before(k.clock.Now()) { return nil, zerrors.ThrowInvalidArgument(err, "QUERY-ciF4k", "Errors.Key.ExpireBeforeNow") } - return jws.Verify(jsonWebkey(key)) + return jws.Verify(key.webKey) } type oidcKeySet struct { @@ -423,3 +448,68 @@ func retry(retryable func() error) (err error) { } return err } + +func (s *Server) Keys(ctx context.Context, r *op.Request[struct{}]) (_ *op.Response, err error) { + ctx, span := tracing.NewSpan(ctx) + defer func() { span.EndWithError(err) }() + + if !authz.GetFeatures(ctx).WebKey { + return s.LegacyServer.Keys(ctx, r) + } + + keyset, err := s.query.GetWebKeySet(ctx) + if err != nil { + return nil, err + } + + // Return legacy keys, so we do not invalidate all tokens + // once the feature flag is enabled. + legacyKeys, err := s.query.ActivePublicKeys(ctx, time.Now()) + logging.OnError(err).Error("oidc server: active public keys (legacy)") + appendPublicKeysToWebKeySet(keyset, legacyKeys) + + resp := op.NewResponse(keyset) + if s.jwksCacheControlMaxAge != 0 { + resp.Header.Set(http_util.CacheControl, + fmt.Sprintf("max-age=%d, must-revalidate", int(s.jwksCacheControlMaxAge/time.Second)), + ) + } + + return resp, nil +} + +func appendPublicKeysToWebKeySet(keyset *jose.JSONWebKeySet, pubkeys *query.PublicKeys) { + if pubkeys == nil || len(pubkeys.Keys) == 0 { + return + } + keyset.Keys = slices.Grow(keyset.Keys, len(pubkeys.Keys)) + + for _, key := range pubkeys.Keys { + keyset.Keys = append(keyset.Keys, jose.JSONWebKey{ + Key: key.Key(), + KeyID: key.ID(), + Algorithm: key.Algorithm(), + Use: key.Use().String(), + }) + } +} + +func queryKeyFunc(q *query.Queries) func(ctx context.Context, keyID string) (*jose.JSONWebKey, *time.Time, error) { + return func(ctx context.Context, keyID string) (*jose.JSONWebKey, *time.Time, error) { + if authz.GetFeatures(ctx).WebKey { + webKey, err := q.GetPublicWebKeyByID(ctx, keyID) + if err == nil { + return webKey, nil, nil + } + if !zerrors.IsNotFound(err) { + return nil, nil, err + } + } + + pubKey, err := q.GetPublicKeyByID(ctx, keyID) + if err != nil { + return nil, nil, err + } + return jsonWebkey(pubKey), gu.Ptr(pubKey.Expiry()), nil + } +} diff --git a/internal/api/oidc/key_test.go b/internal/api/oidc/key_test.go index 3f84722a9..b495b4132 100644 --- a/internal/api/oidc/key_test.go +++ b/internal/api/oidc/key_test.go @@ -2,12 +2,14 @@ package oidc import ( "context" + "crypto/rand" "errors" "testing" "time" "github.com/go-jose/go-jose/v4" "github.com/jonboulle/clockwork" + "github.com/muhlemmer/gu" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -51,36 +53,45 @@ func (k *publicKey) Key() any { var ( clock = clockwork.NewFakeClock() - keyDB = map[string]*publicKey{ + keyDB = map[string]struct { + webKey *jose.JSONWebKey + expiry *time.Time + }{ "key1": { - id: "key1", - alg: "alg", - use: crypto.KeyUsageSigning, - seq: 1, - expiry: clock.Now().Add(time.Minute), + webKey: &jose.JSONWebKey{ + Key: "abc", + KeyID: "key1", + Algorithm: "alg", + Use: "sig", + }, + expiry: gu.Ptr(clock.Now().Add(time.Minute)), }, "key2": { - id: "key2", - alg: "alg", - use: crypto.KeyUsageSigning, - seq: 3, - expiry: clock.Now().Add(10 * time.Hour), + webKey: &jose.JSONWebKey{ + Key: "def", + KeyID: "key1", + Algorithm: "alg", + Use: "sig", + }, + expiry: gu.Ptr(clock.Now().Add(10 * time.Hour)), }, "exp1": { - id: "key2", - alg: "alg", - use: crypto.KeyUsageSigning, - seq: 4, - expiry: clock.Now().Add(-time.Hour), + webKey: &jose.JSONWebKey{ + Key: "ghi", + KeyID: "exp1", + Algorithm: "alg", + Use: "sig", + }, + expiry: gu.Ptr(clock.Now().Add(-time.Hour)), }, } ) -func queryKeyDB(_ context.Context, keyID string) (query.PublicKey, error) { +func queryKeyDB(_ context.Context, keyID string) (*jose.JSONWebKey, *time.Time, error) { if key, ok := keyDB[keyID]; ok { - return key, nil + return key.webKey, key.expiry, nil } - return nil, errors.New("not found") + return nil, nil, errors.New("not found") } func Test_publicKeyCache(t *testing.T) { @@ -102,7 +113,7 @@ func Test_publicKeyCache(t *testing.T) { got, err := cache.getKey(ctx, "key1") require.NoError(t, err) require.NotNil(t, got) - assert.Equal(t, keyDB["key1"], got.PublicKey) + assert.Equal(t, keyDB["key1"].webKey, got.webKey) // move time forward clock.Advance(15 * time.Minute) @@ -122,7 +133,7 @@ func Test_publicKeyCache(t *testing.T) { got, err = cache.getKey(ctx, "key2") require.NoError(t, err) require.NotNil(t, got) - assert.Equal(t, keyDB["key2"], got.PublicKey) + assert.Equal(t, keyDB["key2"].webKey, got.webKey) // move time forward clock.Advance(15 * time.Minute) @@ -140,7 +151,7 @@ func Test_publicKeyCache(t *testing.T) { got, err = cache.getKey(ctx, "key2") require.NoError(t, err) require.NotNil(t, got) - assert.Equal(t, keyDB["key2"], got.PublicKey) + assert.Equal(t, keyDB["key2"].webKey, got.webKey) // move time forward clock.Advance(2 * time.Hour) @@ -266,3 +277,126 @@ func Test_keySetMap_VerifySignature(t *testing.T) { }) } } + +func Test_appendPublicKeysToWebKeySet(t *testing.T) { + keys := [...][]byte{ + make([]byte, 32), + make([]byte, 32), + } + for _, key := range keys { + _, err := rand.Read(key) + require.NoError(t, err) + } + + type args struct { + keyset *jose.JSONWebKeySet + pubkeys *query.PublicKeys + } + tests := []struct { + name string + args args + want *jose.JSONWebKeySet + }{ + { + name: "nil pubkeys", + args: args{ + keyset: &jose.JSONWebKeySet{ + Keys: []jose.JSONWebKey{ + { + Key: keys[0], + KeyID: "key0", + Algorithm: "XYZ", + Use: crypto.KeyUsageSigning.String(), + }, + }, + }, + pubkeys: nil, + }, + want: &jose.JSONWebKeySet{ + Keys: []jose.JSONWebKey{ + { + Key: keys[0], + KeyID: "key0", + Algorithm: "XYZ", + Use: crypto.KeyUsageSigning.String(), + }, + }, + }, + }, + { + name: "empty pubkeys", + args: args{ + keyset: &jose.JSONWebKeySet{ + Keys: []jose.JSONWebKey{ + { + Key: keys[0], + KeyID: "key0", + Algorithm: "XYZ", + Use: crypto.KeyUsageSigning.String(), + }, + }, + }, + pubkeys: &query.PublicKeys{ + Keys: []query.PublicKey{}, + }, + }, + want: &jose.JSONWebKeySet{ + Keys: []jose.JSONWebKey{ + { + Key: keys[0], + KeyID: "key0", + Algorithm: "XYZ", + Use: crypto.KeyUsageSigning.String(), + }, + }, + }, + }, + { + name: "append pubkeys", + args: args{ + keyset: &jose.JSONWebKeySet{ + Keys: []jose.JSONWebKey{ + { + Key: keys[0], + KeyID: "key0", + Algorithm: "XYZ", + Use: crypto.KeyUsageSigning.String(), + }, + }, + }, + pubkeys: &query.PublicKeys{ + Keys: []query.PublicKey{ + &publicKey{ + id: "key1", + key: keys[1], + alg: "XYZ", + use: crypto.KeyUsageSigning, + }, + }, + }, + }, + want: &jose.JSONWebKeySet{ + Keys: []jose.JSONWebKey{ + { + Key: keys[0], + KeyID: "key0", + Algorithm: "XYZ", + Use: crypto.KeyUsageSigning.String(), + }, + { + Key: keys[1], + KeyID: "key1", + Algorithm: "XYZ", + Use: crypto.KeyUsageSigning.String(), + }, + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + appendPublicKeysToWebKeySet(tt.args.keyset, tt.args.pubkeys) + assert.Equal(t, tt.want, tt.args.keyset) + }) + } +} diff --git a/internal/api/oidc/keys_integration_test.go b/internal/api/oidc/keys_integration_test.go new file mode 100644 index 000000000..a78e4fc1b --- /dev/null +++ b/internal/api/oidc/keys_integration_test.go @@ -0,0 +1,115 @@ +//go:build integration + +package oidc_test + +import ( + "crypto/rsa" + "encoding/json" + "net/http" + "testing" + "time" + + "github.com/go-jose/go-jose/v4" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/zitadel/oidc/v3/pkg/client" + "github.com/zitadel/oidc/v3/pkg/oidc" + "google.golang.org/protobuf/proto" + + http_util "github.com/zitadel/zitadel/internal/api/http" + "github.com/zitadel/zitadel/internal/crypto" + "github.com/zitadel/zitadel/pkg/grpc/feature/v2" + oidc_pb "github.com/zitadel/zitadel/pkg/grpc/oidc/v2" +) + +func TestServer_Keys(t *testing.T) { + // TODO: isolated instance + + clientID, _ := createClient(t) + authRequestID := createAuthRequest(t, clientID, redirectURI, oidc.ScopeOpenID, oidc.ScopeOfflineAccess, zitadelAudienceScope) + sessionID, sessionToken, _, _ := Tester.CreateVerifiedWebAuthNSession(t, CTXLOGIN, User.GetUserId()) + linkResp, err := Tester.Client.OIDCv2.CreateCallback(CTXLOGIN, &oidc_pb.CreateCallbackRequest{ + AuthRequestId: authRequestID, + CallbackKind: &oidc_pb.CreateCallbackRequest_Session{ + Session: &oidc_pb.Session{ + SessionId: sessionID, + SessionToken: sessionToken, + }, + }, + }) + require.NoError(t, err) + + // code exchange so we are sure there is 1 legacy key pair. + code := assertCodeResponse(t, linkResp.GetCallbackUrl()) + _, err = exchangeTokens(t, clientID, code, redirectURI) + require.NoError(t, err) + + issuer := http_util.BuildHTTP(Tester.Config.ExternalDomain, Tester.Config.Port, Tester.Config.ExternalSecure) + discovery, err := client.Discover(CTX, issuer, http.DefaultClient) + require.NoError(t, err) + + tests := []struct { + name string + webKeyFeature bool + wantLen int + }{ + { + name: "legacy only", + webKeyFeature: false, + wantLen: 1, + }, + { + name: "webkeys with legacy", + webKeyFeature: true, + wantLen: 3, // 1 legacy + 2 created by enabling feature flag + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ensureWebKeyFeature(t, tt.webKeyFeature) + + assert.EventuallyWithT(t, func(ttt *assert.CollectT) { + resp, err := http.Get(discovery.JwksURI) + require.NoError(ttt, err) + require.Equal(ttt, resp.StatusCode, http.StatusOK) + defer resp.Body.Close() + + got := new(jose.JSONWebKeySet) + err = json.NewDecoder(resp.Body).Decode(got) + require.NoError(ttt, err) + + assert.Len(t, got.Keys, tt.wantLen) + for _, key := range got.Keys { + _, ok := key.Key.(*rsa.PublicKey) + require.True(ttt, ok) + require.NotEmpty(ttt, key.KeyID) + require.Equal(ttt, key.Algorithm, string(jose.RS256)) + require.Equal(ttt, key.Use, crypto.KeyUsageSigning.String()) + } + + cacheControl := resp.Header.Get("cache-control") + if tt.webKeyFeature { + require.Equal(ttt, "max-age=300, must-revalidate", cacheControl) + return + } + require.Equal(ttt, "no-store", cacheControl) + + }, time.Minute, time.Second/10) + }) + + } +} + +func ensureWebKeyFeature(t *testing.T, set bool) { + _, err := Tester.Client.FeatureV2.SetInstanceFeatures(CTXIAM, &feature.SetInstanceFeaturesRequest{ + WebKey: proto.Bool(set), + }) + require.NoError(t, err) + + t.Cleanup(func() { + _, err := Tester.Client.FeatureV2.SetInstanceFeatures(CTXIAM, &feature.SetInstanceFeaturesRequest{ + WebKey: proto.Bool(false), + }) + require.NoError(t, err) + }) +} diff --git a/internal/api/oidc/op.go b/internal/api/oidc/op.go index 67bc1765a..c8dafb50f 100644 --- a/internal/api/oidc/op.go +++ b/internal/api/oidc/op.go @@ -36,8 +36,7 @@ type Config struct { DefaultIdTokenLifetime time.Duration DefaultRefreshTokenIdleExpiration time.Duration DefaultRefreshTokenExpiration time.Duration - UserAgentCookieConfig *middleware.UserAgentCookieConfig - Cache *middleware.CacheConfig + JWKSCacheControlMaxAge time.Duration CustomEndpoints *EndpointConfig DeviceAuth *DeviceAuthorizationConfig DefaultLoginURLV2 string @@ -79,6 +78,27 @@ type OPStorage struct { assetAPIPrefix func(ctx context.Context) string } +// Provider is used to overload certain [op.Provider] methods +type Provider struct { + *op.Provider + accessTokenKeySet oidc.KeySet + idTokenHintKeySet oidc.KeySet +} + +// IDTokenHintVerifier configures a Verifier and supported signing algorithms based on the Web Key feature in the context. +func (o *Provider) IDTokenHintVerifier(ctx context.Context) *op.IDTokenHintVerifier { + return op.NewIDTokenHintVerifier(op.IssuerFromContext(ctx), o.idTokenHintKeySet, op.WithSupportedIDTokenHintSigningAlgorithms( + supportedSigningAlgs(ctx)..., + )) +} + +// AccessTokenVerifier configures a Verifier and supported signing algorithms based on the Web Key feature in the context. +func (o *Provider) AccessTokenVerifier(ctx context.Context) *op.AccessTokenVerifier { + return op.NewAccessTokenVerifier(op.IssuerFromContext(ctx), o.accessTokenKeySet, op.WithSupportedAccessTokenSigningAlgorithms( + supportedSigningAlgs(ctx)..., + )) +} + func NewServer( ctx context.Context, config Config, @@ -101,14 +121,11 @@ func NewServer( return nil, zerrors.ThrowInternal(err, "OIDC-EGrqd", "cannot create op config: %w") } storage := newStorage(config, command, query, repo, encryptionAlg, es, projections) - keyCache := newPublicKeyCache(ctx, config.PublicKeyCacheMaxAge, query.GetPublicKeyByID) + keyCache := newPublicKeyCache(ctx, config.PublicKeyCacheMaxAge, queryKeyFunc(query)) accessTokenKeySet := newOidcKeySet(keyCache, withKeyExpiryCheck(true)) idTokenHintKeySet := newOidcKeySet(keyCache) - options := []op.Option{ - op.WithAccessTokenKeySet(accessTokenKeySet), - op.WithIDTokenHintKeySet(idTokenHintKeySet), - } + var options []op.Option if !externalSecure { options = append(options, op.WithAllowInsecure()) } @@ -126,7 +143,11 @@ func NewServer( return nil, zerrors.ThrowInternal(err, "OIDC-Aij4e", "cannot create secret hasher") } server := &Server{ - LegacyServer: op.NewLegacyServer(provider, endpoints(config.CustomEndpoints)), + LegacyServer: op.NewLegacyServer(&Provider{ + Provider: provider, + accessTokenKeySet: accessTokenKeySet, + idTokenHintKeySet: idTokenHintKeySet, + }, endpoints(config.CustomEndpoints)), repo: repo, query: query, command: command, @@ -137,6 +158,7 @@ func NewServer( defaultLogoutURLV2: config.DefaultLogoutURLV2, defaultAccessTokenLifetime: config.DefaultAccessTokenLifetime, defaultIdTokenLifetime: config.DefaultIdTokenLifetime, + jwksCacheControlMaxAge: config.JWKSCacheControlMaxAge, fallbackLogger: fallbackLogger, hasher: hasher, signingKeyAlgorithm: config.SigningKeyAlgorithm, diff --git a/internal/api/oidc/server.go b/internal/api/oidc/server.go index 1152d81b4..15628cad8 100644 --- a/internal/api/oidc/server.go +++ b/internal/api/oidc/server.go @@ -34,6 +34,7 @@ type Server struct { defaultLogoutURLV2 string defaultAccessTokenLifetime time.Duration defaultIdTokenLifetime time.Duration + jwksCacheControlMaxAge time.Duration fallbackLogger *slog.Logger hasher *crypto.Hasher @@ -129,13 +130,6 @@ func (s *Server) Discovery(ctx context.Context, r *op.Request[struct{}]) (_ *op. return op.NewResponse(s.createDiscoveryConfig(ctx, allowedLanguages)), nil } -func (s *Server) Keys(ctx context.Context, r *op.Request[struct{}]) (_ *op.Response, err error) { - ctx, span := tracing.NewSpan(ctx) - defer func() { span.EndWithError(err) }() - - return s.LegacyServer.Keys(ctx, r) -} - func (s *Server) VerifyAuthRequest(ctx context.Context, r *op.Request[oidc.AuthRequest]) (_ *op.ClientRequest[oidc.AuthRequest], err error) { ctx, span := tracing.NewSpan(ctx) defer func() { span.EndWithError(err) }() @@ -173,6 +167,7 @@ func (s *Server) EndSession(ctx context.Context, r *op.Request[oidc.EndSessionRe func (s *Server) createDiscoveryConfig(ctx context.Context, supportedUILocales oidc.Locales) *oidc.DiscoveryConfiguration { issuer := op.IssuerFromContext(ctx) + return &oidc.DiscoveryConfiguration{ Issuer: issuer, AuthorizationEndpoint: s.Endpoints().Authorization.Absolute(issuer), @@ -192,7 +187,7 @@ func (s *Server) createDiscoveryConfig(ctx context.Context, supportedUILocales o }, GrantTypesSupported: op.GrantTypes(s.Provider()), SubjectTypesSupported: op.SubjectTypes(s.Provider()), - IDTokenSigningAlgValuesSupported: []string{s.signingKeyAlgorithm}, + IDTokenSigningAlgValuesSupported: supportedSigningAlgs(ctx), RequestObjectSigningAlgValuesSupported: op.RequestObjectSigAlgorithms(s.Provider()), TokenEndpointAuthMethodsSupported: op.AuthMethodsTokenEndpoint(s.Provider()), TokenEndpointAuthSigningAlgValuesSupported: op.TokenSigAlgorithms(s.Provider()), diff --git a/internal/api/oidc/server_test.go b/internal/api/oidc/server_test.go index 19404933b..47f24fcde 100644 --- a/internal/api/oidc/server_test.go +++ b/internal/api/oidc/server_test.go @@ -7,6 +7,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/zitadel/oidc/v3/pkg/oidc" "github.com/zitadel/oidc/v3/pkg/op" + "github.com/zitadel/zitadel/internal/api/authz" + "github.com/zitadel/zitadel/internal/feature" "golang.org/x/text/language" ) @@ -30,6 +32,7 @@ func TestServer_createDiscoveryConfig(t *testing.T) { fields{ LegacyServer: op.NewLegacyServer( func() *op.Provider { + //nolint:staticcheck provider, _ := op.NewForwardedOpenIDProvider("path", &op.Config{ CodeMethodS256: true, @@ -107,6 +110,92 @@ func TestServer_createDiscoveryConfig(t *testing.T) { OPTermsOfServiceURI: "", }, }, + { + "web keys feature enabled", + fields{ + LegacyServer: op.NewLegacyServer( + func() *op.Provider { + //nolint:staticcheck + provider, _ := op.NewForwardedOpenIDProvider("path", + &op.Config{ + CodeMethodS256: true, + AuthMethodPost: true, + AuthMethodPrivateKeyJWT: true, + GrantTypeRefreshToken: true, + RequestObjectSupported: true, + }, + nil, + ) + return provider + }(), + op.Endpoints{ + Authorization: op.NewEndpoint("auth"), + Token: op.NewEndpoint("token"), + Introspection: op.NewEndpoint("introspect"), + Userinfo: op.NewEndpoint("userinfo"), + Revocation: op.NewEndpoint("revoke"), + EndSession: op.NewEndpoint("logout"), + JwksURI: op.NewEndpoint("keys"), + DeviceAuthorization: op.NewEndpoint("device"), + }, + ), + signingKeyAlgorithm: "RS256", + }, + args{ + ctx: authz.WithFeatures( + op.ContextWithIssuer(context.Background(), "https://issuer.com"), + feature.Features{WebKey: true}, + ), + supportedUILocales: []language.Tag{language.English, language.German}, + }, + &oidc.DiscoveryConfiguration{ + Issuer: "https://issuer.com", + AuthorizationEndpoint: "https://issuer.com/auth", + TokenEndpoint: "https://issuer.com/token", + IntrospectionEndpoint: "https://issuer.com/introspect", + UserinfoEndpoint: "https://issuer.com/userinfo", + RevocationEndpoint: "https://issuer.com/revoke", + EndSessionEndpoint: "https://issuer.com/logout", + DeviceAuthorizationEndpoint: "https://issuer.com/device", + CheckSessionIframe: "", + JwksURI: "https://issuer.com/keys", + RegistrationEndpoint: "", + ScopesSupported: []string{oidc.ScopeOpenID, oidc.ScopeProfile, oidc.ScopeEmail, oidc.ScopePhone, oidc.ScopeAddress, oidc.ScopeOfflineAccess}, + ResponseTypesSupported: []string{string(oidc.ResponseTypeCode), string(oidc.ResponseTypeIDTokenOnly), string(oidc.ResponseTypeIDToken)}, + ResponseModesSupported: []string{string(oidc.ResponseModeQuery), string(oidc.ResponseModeFragment), string(oidc.ResponseModeFormPost)}, + GrantTypesSupported: []oidc.GrantType{oidc.GrantTypeCode, oidc.GrantTypeImplicit, oidc.GrantTypeRefreshToken, oidc.GrantTypeBearer}, + ACRValuesSupported: nil, + SubjectTypesSupported: []string{"public"}, + IDTokenSigningAlgValuesSupported: supportedWebKeyAlgs, + IDTokenEncryptionAlgValuesSupported: nil, + IDTokenEncryptionEncValuesSupported: nil, + UserinfoSigningAlgValuesSupported: nil, + UserinfoEncryptionAlgValuesSupported: nil, + UserinfoEncryptionEncValuesSupported: nil, + RequestObjectSigningAlgValuesSupported: []string{"RS256"}, + RequestObjectEncryptionAlgValuesSupported: nil, + RequestObjectEncryptionEncValuesSupported: nil, + TokenEndpointAuthMethodsSupported: []oidc.AuthMethod{oidc.AuthMethodNone, oidc.AuthMethodBasic, oidc.AuthMethodPost, oidc.AuthMethodPrivateKeyJWT}, + TokenEndpointAuthSigningAlgValuesSupported: []string{"RS256"}, + RevocationEndpointAuthMethodsSupported: []oidc.AuthMethod{oidc.AuthMethodNone, oidc.AuthMethodBasic, oidc.AuthMethodPost, oidc.AuthMethodPrivateKeyJWT}, + RevocationEndpointAuthSigningAlgValuesSupported: []string{"RS256"}, + IntrospectionEndpointAuthMethodsSupported: []oidc.AuthMethod{oidc.AuthMethodBasic, oidc.AuthMethodPrivateKeyJWT}, + IntrospectionEndpointAuthSigningAlgValuesSupported: []string{"RS256"}, + DisplayValuesSupported: nil, + ClaimTypesSupported: nil, + ClaimsSupported: []string{"sub", "aud", "exp", "iat", "iss", "auth_time", "nonce", "acr", "amr", "c_hash", "at_hash", "act", "scopes", "client_id", "azp", "preferred_username", "name", "family_name", "given_name", "locale", "email", "email_verified", "phone_number", "phone_number_verified"}, + ClaimsParameterSupported: false, + CodeChallengeMethodsSupported: []oidc.CodeChallengeMethod{"S256"}, + ServiceDocumentation: "", + ClaimsLocalesSupported: nil, + UILocalesSupported: []language.Tag{language.English, language.German}, + RequestParameterSupported: true, + RequestURIParameterSupported: false, + RequireRequestURIRegistration: false, + OPPolicyURI: "", + OPTermsOfServiceURI: "", + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/internal/api/oidc/token.go b/internal/api/oidc/token.go index be3a30ed7..56ed22590 100644 --- a/internal/api/oidc/token.go +++ b/internal/api/oidc/token.go @@ -12,9 +12,11 @@ import ( "github.com/zitadel/oidc/v3/pkg/oidc" "github.com/zitadel/oidc/v3/pkg/op" + "github.com/zitadel/zitadel/internal/api/authz" "github.com/zitadel/zitadel/internal/command" "github.com/zitadel/zitadel/internal/domain" "github.com/zitadel/zitadel/internal/telemetry/tracing" + "github.com/zitadel/zitadel/internal/zerrors" ) /* @@ -75,22 +77,43 @@ func (s *Server) getSignerOnce() signerFunc { ctx, span := tracing.NewSpan(ctx) defer func() { span.EndWithError(err) }() + if authz.GetFeatures(ctx).WebKey { + var webKey *jose.JSONWebKey + webKey, err = s.query.GetActiveSigningWebKey(ctx) + if err != nil { + return + } + signer, signAlg, err = signerFromWebKey(webKey) + return + } + var signingKey op.SigningKey signingKey, err = s.Provider().Storage().SigningKey(ctx) if err != nil { return } signAlg = signingKey.SignatureAlgorithm() - signer, err = op.SignerFromKey(signingKey) - if err != nil { - return - } }) return signer, signAlg, err } } +func signerFromWebKey(signingKey *jose.JSONWebKey) (jose.Signer, jose.SignatureAlgorithm, error) { + signAlg := jose.SignatureAlgorithm(signingKey.Algorithm) + signer, err := jose.NewSigner( + jose.SigningKey{ + Algorithm: signAlg, + Key: signingKey, + }, + (&jose.SignerOptions{}).WithType("JWT"), + ) + if err != nil { + return nil, "", zerrors.ThrowInternal(err, "OIDC-oaF0s", "Errors.Internal") + } + return signer, signAlg, nil +} + // userInfoFunc is a getter function that allows add-hoc retrieval of a user. type userInfoFunc func(ctx context.Context, roleAssertion bool, triggerType domain.TriggerType) (*oidc.UserInfo, error) diff --git a/internal/api/oidc/userinfo_integration_test.go b/internal/api/oidc/userinfo_integration_test.go index ad95defd6..e93807807 100644 --- a/internal/api/oidc/userinfo_integration_test.go +++ b/internal/api/oidc/userinfo_integration_test.go @@ -36,6 +36,7 @@ func TestServer_UserInfo(t *testing.T) { name string legacy bool trigger bool + webKey bool }{ { name: "legacy enabled", @@ -51,6 +52,17 @@ func TestServer_UserInfo(t *testing.T) { legacy: false, trigger: true, }, + + // This is the only functional test we need to cover web keys. + // - By creating tokens the signer is tested + // - When obtaining the tokens, the RP verifies the ID Token using the key set from the jwks endpoint. + // - By calling userinfo with the access token as JWT, the Token Verifier with the public key cache is tested. + { + name: "web keys", + legacy: false, + trigger: false, + webKey: true, + }, } for _, tt := range tests { @@ -58,6 +70,7 @@ func TestServer_UserInfo(t *testing.T) { _, err := Tester.Client.FeatureV2.SetInstanceFeatures(iamOwnerCTX, &feature.SetInstanceFeaturesRequest{ OidcLegacyIntrospection: &tt.legacy, OidcTriggerIntrospectionProjections: &tt.trigger, + WebKey: &tt.webKey, }) require.NoError(t, err) testServer_UserInfo(t) diff --git a/internal/integration/config/zitadel.yaml b/internal/integration/config/zitadel.yaml index 291099ea1..d58529b7a 100644 --- a/internal/integration/config/zitadel.yaml +++ b/internal/integration/config/zitadel.yaml @@ -53,4 +53,13 @@ SystemAPIUsers: KeyData: "LS0tLS1CRUdJTiBQVUJMSUMgS0VZLS0tLS0KTUlJQklqQU5CZ2txaGtpRzl3MEJBUUVGQUFPQ0FROEFNSUlCQ2dLQ0FRRUF6aStGRlNKTDdmNXl3NEtUd3pnTQpQMzRlUEd5Y20vTStrVDBNN1Y0Q2d4NVYzRWFESXZUUUtUTGZCYUVCNDV6YjlMdGpJWHpEdzByWFJvUzJoTzZ0CmgrQ1lRQ3ozS0N2aDA5QzBJenhaaUIySVMzSC9hVCs1Qng5RUZZK3ZuQWtaamNjYnlHNVlOUnZtdE9sbnZJZUkKSDdxWjB0RXdrUGZGNUdFWk5QSlB0bXkzVUdWN2lvZmRWUVMxeFJqNzMrYU13NXJ2SDREOElkeWlBQzNWZWtJYgpwdDBWajBTVVgzRHdLdG9nMzM3QnpUaVBrM2FYUkYwc2JGaFFvcWRKUkk4TnFnWmpDd2pxOXlmSTV0eXhZc3duCitKR3pIR2RIdlczaWRPRGxtd0V0NUsycGFzaVJJV0syT0dmcSt3MEVjbHRRSGFidXFFUGdabG1oQ2tSZE5maXgKQndJREFRQUIKLS0tLS1FTkQgUFVCTElDIEtFWS0tLS0tCg==" InitProjections: - Enabled: true \ No newline at end of file + Enabled: true + +# Extend key lifetimes so we do not see more legacy keys when +# integration tests are rerun on the same DB with more than 6 hours apart. +# The test counts the amount of keys returned from the JWKS endpoint and fails +# with 2 or more legacy public keys, +SystemDefaults: + KeyConfig: + PrivateKeyLifetime: 7200h + PublicKeyLifetime: 14400h diff --git a/proto/zitadel/resources/webkey/v3alpha/webkey_service.proto b/proto/zitadel/resources/webkey/v3alpha/webkey_service.proto index 8cf6c379d..c9783d3b0 100644 --- a/proto/zitadel/resources/webkey/v3alpha/webkey_service.proto +++ b/proto/zitadel/resources/webkey/v3alpha/webkey_service.proto @@ -147,7 +147,7 @@ service ZITADELWebKeys { option (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_operation) = { summary: "Activate a signing key for the instance"; - description: "Switch the active signing web key. The previously active key will be deactivated." + description: "Switch the active signing web key. The previously active key will be deactivated. Note that the JWKs OIDC endpoint returns a cacheable response. Therefore it is not advised to activate a key that has been created within the cache duration (default is 5min), as the public key may not have been propagated to caches and clients yet." responses: { key: "200" value: {