panva / openid-client

OAuth 2 / OpenID Connect Client API for JavaScript Runtimes
MIT License
1.83k stars 392 forks source link

JWT Authorized Presenter (azp) validation is too strict #231

Closed svvac closed 4 years ago

svvac commented 4 years ago

Currently, JWT validation throws if the authorized presenter (azp field) is not equal to the client_id (see here).

However, according to Google's OIDC docs, it is specifically different from the aud (Audience) field if present (emphasis mine) :

azp : 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.

More specifically, it happens in Android mobile app authentication flow :

  1. The app requests an ID token from Google using the app's cert and client ID, and the target server's client_id
  2. Google issues an ID token with aud set to the server's client_id and azp to the app's client ID
  3. The app gets the token and sends it to the server for logging in
  4. The server receives the id token, validates it, and issues credentials based on the claims of the id token

AFAICT, this auth flow is currently not possible with node-openid-client

panva commented 4 years ago

These validations aren’t strict, thats how the spec is written, what google says in their docs is kinda irrelevant. What would you propose gets loosened?

svvac commented 4 years ago

After reading relevant parts of the spec, it appears that the check in question is indeed mandated :

2 — ID token : azp : OPTIONAL. Authorized party - the party to which the ID Token was issued. If present, it MUST contain the OAuth 2.0 Client ID of this party. This Claim is only needed when the ID Token has a single audience value and that audience is different than the authorized party. It MAY be included even when the authorized party is the same as the sole audience. The azp value is a case sensitive string containing a StringOrURI value.

3.1.3.7 — ID token validation : 5. If an azp (authorized party) Claim is present, the Client SHOULD verify that its client_id is the Claim Value.

However, this raises the question : if multiple audiences can be provided, how are the others (the ones that are not azp) supposed to validate the ID token if the assertion aud === azp === client_id must hold?

The spec indeed considers cases where this is not the case, for instance:

3.1.3.7 ­— ID token validation : 8. If the JWT alg Header Parameter uses a MAC based algorithm such as HS256, HS384, or HS512, the octets of the UTF-8 representation of the client_secret corresponding to the client_id contained in the aud (audience) Claim are used as the key to validate the signature. For MAC based algorithms, the behavior is unspecified if the aud is multi-valued or if an azp value is present that is different than the aud value.

This suggests that it should be possible to handle such tokens and consider them valid.


What would you propose gets loosened?

I was thinking of providing a way to tell Client a list of authorized 3rd-party client_id that should be accepted for azp for the cases where the authorized party is not the audience, or disable the check altogether. Of course, the default should still be to enforce client_id as it is the most common case by far.

svvac commented 4 years ago

the idea : svvac/node-openid-client@7c781d2

panva commented 4 years ago

That won’t fly. That metadata object is only for registered iana parameters and i don’t intend to mix em up with proprietary configuration.

I get the gist tho, when i have the time i’ll take a look at solving this.

svvac commented 4 years ago

I'd like to help, if you have pointers for your preferred way of passing this kind of configuration.

panva commented 4 years ago

I’ll be happy to give you pointers for this when i’m done with my move next week.

panva commented 4 years ago

@svvac

we're looking to add an additional parameter (an object so that we can add more of these in the future) to these methods

The following already has a last argument for these "options"

It boils down to extending the constructor and then just passing the extra from the other methods.

svvac commented 4 years ago

Was the authorized_thrid_parties option name alright? Or maybe you'd prefer camelCase?

panva commented 4 years ago

camelCase for these "extras" is better, the only snake_case present is for IANA registered parameters.

panva commented 4 years ago

And is the boolean value option necessary? To accept all? Why?

svvac commented 4 years ago

It helps validating ID tokens in the case where you can't really know in advance all 3rd-party client IDs that might initiate the OIDC auth flow for logging into your service.

In the case of Google's android app login flow, it can be useful if you wish to be able to roll out new apps without having to redeploy the server component. The responsibility of validating 3rd-party client IDs is thus delegated to the issuer. For instance, Google only allows this if the authorized presenter is in the same « project » as the target audience.

panva commented 4 years ago

I'd prefer to leave the "accept all" option out. All claims need to be validated to be expected values. "anything" isn't really validating.

svvac commented 4 years ago

I see your point, though I'd like to point out that it forces unnecessary coupling between the client configuration and the issuer policy (i.e. is this client allowed to request login for that client)