feat: support custom issuer verifier - #928
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for a custom iss (issuer) claim verifier so consumers can validate tokens from providers with dynamic issuers (e.g., Azure AD tenant-specific issuers) while keeping the existing default issuer validation behavior.
Changes:
- Introduces
ISSVerifier,DefaultISSVerifier, andCheckIssuerWithISSVerifierinpkg/oidc. - Updates OP and RP token verification flows to use
CheckIssuerWithISSVerifier(...)(falling back to default behavior when no custom verifier is provided). - Extends unit tests to cover the new issuer-verifier path and override behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/op/verifier_id_token_hint.go | Switches ID token hint verification to the new issuer-check function with optional custom verifier. |
| pkg/op/verifier_access_token.go | Switches access token verification to the new issuer-check function with optional custom verifier. |
| pkg/oidc/verifier.go | Adds ISSVerifier/DefaultISSVerifier and the new issuer-check helper; deprecates CheckIssuer. |
| pkg/oidc/verifier_test.go | Adds tests to ensure default and overridden issuer-verification behavior works as intended. |
| pkg/client/rp/verifier.go | Uses the new issuer-check function in RP verification and adds WithISSVerifier option. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| // Deprecated: Use CheckIssuerWithISSVerifier instead | ||
| func CheckIssuer(claims Claims, issuer string) error { |
There was a problem hiding this comment.
I don't actually know whether we should deprecate this. I think the implementation can call CheckIssuerWithISSVerifier with as verifier nil or the DefaultISSVerifier. Most people won't require a dynamic resolving of the issuer verifier, I think.
There was a problem hiding this comment.
um... I think maybe we should, since we already have CheckIssuerWithISSVerifier which cover all the cases. So the old function no longer needed anymore. (we might could change the CheckIssuerWithISSVerifier to CheckIssuer or somthing more general name. )
with less code might cause less confuse and clean repo.
feel free let me know if we want keep the Deprecated mark or remove it. I will do the change.
Thanks for the quick review!
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (2)
pkg/oidc/verifier.go:147
CheckIssuerWithISSVerifiercurrently changes the default error message formatting compared to the previousCheckIssuerpath (it now comes fromDefaultISSVerifier, using "expected:" vs "Expected:"). This contradicts the PR description that default usage/behavior is unchanged and can break consumers that compare error strings. Consider delegating toCheckIssuerwhenissVerifieris nil to preserve the existing default behavior (while still allowing overrides).
if issVerifier == nil {
return DefaultISSVerifier(issuer)(claims.GetIssuer())
}
return issVerifier(claims.GetIssuer())
}
pkg/oidc/verifier.go:71
- The new
Verifier.ISSfield name is hard to understand and reads like an exported identifier, which is inconsistent with the rest ofVerifierfields (e.g.AZP,ACR). Since this is part of the public verifier configuration, consider renaming the field to something self-descriptive likeISSVerifier(orIssuerVerifier) and updating call sites/options accordingly.
type Verifier struct {
Issuer string
ISS ISSVerifier
MaxAgeIAT time.Duration
|
🎉 This PR is included in version 3.49.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Which Problems Are Solved
Azure split issuer by tenent id
https://login.microsoftonline.com/{tenantid}/v2.0when we want support more general users, like both company and personal users, the issuer is dynamically different by the tenent. the discovery endpoint https://login.microsoftonline.com/common/v2.0/.well-known/openid-configuration shows the
{tenantid}pattern as well.This PR support customized issuer verifier to support dynamic issuer.
reference:
https://learn.microsoft.com/en-us/entra/identity-platform/v2-protocols-oidc#find-your-apps-openid-configuration-document-uri
How the Problems Are Solved
This PR support customized issuer verifier to support dynamic issuer.
Additional Changes
change all the usage to use the new function
oidc.CheckIssuerWithISSVerifier(claims, v.Issuer, v.ISS)it does not change current or default usage, just add option to support cusomized issuer verifier.
Additional Context