Repository: zitadel/oidc
Pull request #525 (2024-01-25 17:08:50)

feat(op): split the access and ID token hint key sets
In zitadel we require different behaviors wrt public key expiry between access tokens and ID token hints.
This change splits the two verifiers in the OP.
The default is still based on Storage and passed to both verifier fields.

Related to https://github.com/zitadel/zitadel/issues/5232

### Definition of Ready

- [x] I am happy with the code
- [x] Short description of the feature/issue is added in the pr description
- [x] PR is linked to the corresponding user story
- [x] Acceptance criteria are met
- [x] All open todos and follow ups are defined in a new ticket and justified
- [x] Deviations from the acceptance criteria and design are agreed with the PO and documented.
- [x] No debug or dead code
- [x] My code has no repetitions
- [x] Critical parts are tested automatically
- [x] Where possible E2E tests are implemented
- [x] Documentation/examples are up-to-date
- [x] All non-functional requirements are met
- [x] Functionality of the acceptance criteria is checked manually on the dev system.



codecov[bot]: ## [Codecov](https://app.codecov.io/gh/zitadel/oidc/pull/525?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zitadel) Report
Attention: `10 lines` in your changes are missing coverage. Please review.
> Comparison is base [(`437a049`)](https://app.codecov.io/gh/zitadel/oidc/commit/437a0497ab3454b8ce6cdd5310bae2d9c906d906?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zitadel) 60.05% compared to head [(`8402435`)](https://app.codecov.io/gh/zitadel/oidc/pull/525?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zitadel) 59.91%.

| [Files](https://app.codecov.io/gh/zitadel/oidc/pull/525?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zitadel) | Patch % | Lines |
|---|---|---|
| [pkg/op/op.go](https://app.codecov.io/gh/zitadel/oidc/pull/525?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zitadel#diff-cGtnL29wL29wLmdv) | 52.38% | [10 Missing :warning: ](https://app.codecov.io/gh/zitadel/oidc/pull/525?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zitadel) |

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main     #525      +/-   ##
==========================================
- Coverage   60.05%   59.91%   -0.14%     
==========================================
  Files          80       80              
  Lines        6997     6998       +1     
==========================================
- Hits         4202     4193       -9     
- Misses       2498     2508      +10     
  Partials      297      297              
```



</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/zitadel/oidc/pull/525?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zitadel).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zitadel).


Patch:
diff --git a/pkg/op/op.go b/pkg/op/op.go
index fdc073c..14c5356 100644
--- a/pkg/op/op.go
+++ b/pkg/op/op.go
@@ -257,13 +257,16 @@ func NewForwardedOpenIDProvider(path string, config *Config, storage Storage, op
 // op.AuthCallbackURL(provider) which is probably /callback. On the redirect back
 // to the AuthCallbackURL, the request id should be passed as the "id" parameter.
 func NewProvider(config *Config, storage Storage, issuer func(insecure bool) (IssuerFromRequest, error), opOpts ...Option) (_ *Provider, err error) {
+	keySet := &OpenIDKeySet{storage}
 	o := &Provider{
-		config:    config,
-		storage:   storage,
-		endpoints: DefaultEndpoints,
-		timer:     make(<-chan time.Time),
-		corsOpts:  &defaultCORSOptions,
-		logger:    slog.Default(),
+		config:            config,
+		storage:           storage,
+		accessTokenKeySet: keySet,
+		idTokenHinKeySet:  keySet,
+		endpoints:         DefaultEndpoints,
+		timer:             make(<-chan time.Time),
+		corsOpts:          &defaultCORSOptions,
+		logger:            slog.Default(),
 	}
 
 	for _, optFunc := range opOpts {
@@ -276,19 +279,11 @@ func NewProvider(config *Config, storage Storage, issuer func(insecure bool) (Is
 	if err != nil {
 		return nil, err
 	}
-
 	o.Handler = CreateRouter(o, o.interceptors...)
-
 	o.decoder = schema.NewDecoder()
 	o.decoder.IgnoreUnknownKeys(true)
-
 	o.encoder = oidc.NewEncoder()
-
 	o.crypto = NewAESCrypto(config.CryptoKey)
-
-	// Avoid potential race conditions by calling these early
-	_ = o.openIDKeySet() // sets keySet
-
 	return o, nil
 }
 
@@ -299,7 +294,8 @@ type Provider struct {
 	insecure                bool
 	endpoints               *Endpoints
 	storage                 Storage
-	keySet                  *openIDKeySet
+	accessTokenKeySet       oidc.KeySet
+	idTokenHinKeySet        oidc.KeySet
 	crypto                  Crypto
 	decoder                 *schema.Decoder
 	encoder                 *schema.Encoder
@@ -435,7 +431,7 @@ func (o *Provider) Encoder() httphelper.Encoder {
 }
 
 func (o *Provider) IDTokenHintVerifier(ctx context.Context) *IDTokenHintVerifier {
-	return NewIDTokenHintVerifier(IssuerFromContext(ctx), o.openIDKeySet(), o.idTokenHintVerifierOpts...)
+	return NewIDTokenHintVerifier(IssuerFromContext(ctx), o.idTokenHinKeySet, o.idTokenHintVerifierOpts...)
 }
 
 func (o *Provider) JWTProfileVerifier(ctx context.Context) *JWTProfileVerifier {
@@ -443,14 +439,7 @@ func (o *Provider) JWTProfileVerifier(ctx context.Context) *JWTProfileVerifier {
 }
 
 func (o *Provider) AccessTokenVerifier(ctx context.Context) *AccessTokenVerifier {
-	return NewAccessTokenVerifier(IssuerFromContext(ctx), o.openIDKeySet(), o.accessTokenVerifierOpts...)
-}
-
-func (o *Provider) openIDKeySet() oidc.KeySet {
-	if o.keySet == nil {
-		o.keySet = &openIDKeySet{o.Storage()}
-	}
-	return o.keySet
+	return NewAccessTokenVerifier(IssuerFromContext(ctx), o.accessTokenKeySet, o.accessTokenVerifierOpts...)
 }
 
 func (o *Provider) Crypto() Crypto {
@@ -480,13 +469,13 @@ func (o *Provider) HttpHandler() http.Handler {
 	return o
 }
 
-type openIDKeySet struct {
+type OpenIDKeySet struct {
 	Storage
 }
 
 // VerifySignature implements the oidc.KeySet interface
 // providing an implementation for the keys stored in the OP Storage interface
-func (o *openIDKeySet) VerifySignature(ctx context.Context, jws *jose.JSONWebSignature) ([]byte, error) {
+func (o *OpenIDKeySet) VerifySignature(ctx context.Context, jws *jose.JSONWebSignature) ([]byte, error) {
 	keySet, err := o.Storage.KeySet(ctx)
 	if err != nil {
 		return nil, fmt.Errorf("error fetching keys: %w", err)
@@ -617,6 +606,15 @@ func WithHttpInterceptors(interceptors ...HttpInterceptor) Option {
 	}
 }
 
+// WithAccessTokenKeySet allows passing a KeySet with public keys for Access Token verification.
+// The default KeySet uses the [Storage] interface
+func WithAccessTokenKeySet(keySet oidc.KeySet) Option {
+	return func(o *Provider) error {
+		o.accessTokenKeySet = keySet
+		return nil
+	}
+}
+
 func WithAccessTokenVerifierOpts(opts ...AccessTokenVerifierOpt) Option {
 	return func(o *Provider) error {
 		o.accessTokenVerifierOpts = opts
@@ -624,6 +622,15 @@ func WithAccessTokenVerifierOpts(opts ...AccessTokenVerifierOpt) Option {
 	}
 }
 
+// WithIDTokenHintKeySet allows passing a KeySet with public keys for ID Token Hint verification.
+// The default KeySet uses the [Storage] interface.
+func WithIDTokenHintKeySet(keySet oidc.KeySet) Option {
+	return func(o *Provider) error {
+		o.idTokenHinKeySet = keySet
+		return nil
+	}
+}
+
 func WithIDTokenHintVerifierOpts(opts ...IDTokenHintVerifierOpt) Option {
 	return func(o *Provider) error {
 		o.idTokenHintVerifierOpts = opts


Test patch:
diff --git a/pkg/op/op_test.go b/pkg/op/op_test.go
index f97f666..b2a758c 100644
--- a/pkg/op/op_test.go
+++ b/pkg/op/op_test.go
@@ -58,8 +58,12 @@ func init() {
 }
 
 func newTestProvider(config *op.Config) op.OpenIDProvider {
-	provider, err := op.NewOpenIDProvider(testIssuer, config,
-		storage.NewStorage(storage.NewUserStore(testIssuer)), op.WithAllowInsecure(),
+	storage := storage.NewStorage(storage.NewUserStore(testIssuer))
+	keySet := &op.OpenIDKeySet{storage}
+	provider, err := op.NewOpenIDProvider(testIssuer, config, storage,
+		op.WithAllowInsecure(),
+		op.WithAccessTokenKeySet(keySet),
+		op.WithIDTokenHintKeySet(keySet),
 	)
 	if err != nil {
 		panic(err)