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

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

Closed
svvac opened this issue Feb 21, 2020 · 13 comments · Fixed by #234 · May be fixed by godaddy/kubernetes-client#612
Closed

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

svvac opened this issue Feb 21, 2020 · 13 comments · Fixed by #234 · May be fixed by godaddy/kubernetes-client#612

Comments

@svvac
Copy link
Contributor

svvac commented Feb 21, 2020

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
Copy link
Owner

panva commented Feb 21, 2020

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
Copy link
Contributor Author

svvac commented Feb 21, 2020

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
Copy link
Contributor Author

svvac commented Feb 22, 2020

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

svvac added a commit to svvac/node-openid-client that referenced this issue Feb 22, 2020
@panva
Copy link
Owner

panva commented Feb 22, 2020

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
Copy link
Contributor Author

svvac commented Feb 22, 2020

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

@panva
Copy link
Owner

panva commented Feb 22, 2020

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

@panva
Copy link
Owner

panva commented Feb 25, 2020

@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

  • constructor for new issuer.Client(metadata[, jwks, [other]])
  • fromUri for issuer.fromUri(uri, token[, jwks, [other]])

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

  • register for issuer.Client.register(metadata[, other]) we should also rename "properties" to "metadata" in the code

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

@svvac
Copy link
Contributor Author

svvac commented Feb 27, 2020

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

@panva
Copy link
Owner

panva commented Feb 27, 2020

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

@panva
Copy link
Owner

panva commented Feb 27, 2020

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

@svvac
Copy link
Contributor Author

svvac commented Feb 27, 2020

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
Copy link
Owner

panva commented Feb 27, 2020

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
Copy link
Contributor Author

svvac commented Feb 27, 2020

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)

svvac added a commit to svvac/node-openid-client that referenced this issue Feb 27, 2020
svvac added a commit to svvac/node-openid-client that referenced this issue Feb 27, 2020
svvac added a commit to svvac/node-openid-client that referenced this issue Feb 27, 2020
svvac added a commit to svvac/node-openid-client that referenced this issue Feb 27, 2020
svvac added a commit to svvac/node-openid-client that referenced this issue Feb 27, 2020
svvac added a commit to svvac/node-openid-client that referenced this issue Feb 27, 2020
svvac added a commit to svvac/node-openid-client that referenced this issue Feb 27, 2020
svvac added a commit to svvac/node-openid-client that referenced this issue Feb 28, 2020
panva pushed a commit to svvac/node-openid-client that referenced this issue Feb 28, 2020
panva pushed a commit that referenced this issue Feb 28, 2020
@github-actions github-actions bot locked and limited conversation to collaborators May 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants