Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The current rp.VerifyIDToken's azp check is too strict and doesn't works with OPs like google #405

Open
wzy9607 opened this issue Jun 6, 2023 · 2 comments
Labels
auth enhancement New feature or request

Comments

@wzy9607
Copy link

wzy9607 commented Jun 6, 2023

Is your feature request related to a problem? Please describe.

if err = oidc.CheckAudience(claims, v.ClientID); err != nil {
return nilClaims, err
}
if err = oidc.CheckAuthorizedParty(claims, v.ClientID); err != nil {

Currently, VerifyIDToken checks aud and azp against the same ClientID. However, id token issued by some OP, e.g. Google, can have two different client_id in aud and azp claims.

When using Google identity on Android, the azp claim is the Android app's client_id, while the aud claim is the web application's client_id, as Google's document and this discussion and this oidc spec issue. This means that id token issued by google can't pass VerifyIDToken.

Describe the solution you'd like

Make azp claim check configurable and allow user pass-in a function in NewIDTokenVerifier to specify how they want to check the claim.
e.g. an option func WithAuthorizedPartyChecker(func (azp string, aud []string) error) VerifierOption

Describe alternatives you've considered

Add an option to pass-in a list of client_id when NewIDTokenVerifier, and check azp against that list.

Additional context

The OIDC work group seems to have changed the azp validation in an errata, see https://bitbucket.org/openid/connect/src/b84078b1aeb694a79823f3de5a22315df700b22f/openid-connect-core-1_0.xml#lines-1933:1944, making it optional. But for some reason they haven't updated their website to include the errata yet.

@wzy9607 wzy9607 added the enhancement New feature or request label Jun 6, 2023
@muhlemmer
Copy link
Collaborator

Thanks for pointing it out. We will discuss. CC @livio-a

@muhlemmer
Copy link
Collaborator

Checked the standards and azp has indeed become an extension outside the scope of OIDC. We can remove the check and allow an option to be passed that enables a custom check, as @wzy9607 proposed.

Putting this to the backlog, but a PR is also welcome.

On a side note, reading the google docs you linked:

The client_id of the authorized presenter. This claim is only needed when the party requesting the ID token is not the same as the audience of the ID token. This may be the case at Google for hybrid apps where a web application and Android app have a different OAuth 2.0 client_id but share the same Google APIs project.

It seems to be a side-effect of the hybrid flow. We don't support that and perhaps your issue does not exist if you use code flow instead. (We use codeflow in zitadel without issues).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auth enhancement New feature or request
Projects
Status: 📨 Product Backlog
Development

No branches or pull requests

3 participants