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

verify_hash/3 expects JWK to have "alg" field #6

Open
YuriiZdyrko opened this issue Sep 21, 2021 · 6 comments
Open

verify_hash/3 expects JWK to have "alg" field #6

YuriiZdyrko opened this issue Sep 21, 2021 · 6 comments

Comments

@YuriiZdyrko
Copy link
Contributor

I tried to use verify_hash/3, but can't because my public JWK doesn't have "alg" field.

According to JWK specification, "alg" is optional, so it seems like OIDC library shouldn't depend on it.

I unsuccessfully tried to find a way to generate key with "alg".
JOSE.JWK is used for generation:

 JOSE.JWK.generate_key({:rsa, 512}) |> JOSE.JWK.to_public |> JOSE.JWK.to_map |> elem(1)

%{
  "e" => "AQAB",
  "kty" => "RSA",
  "n" => "yvJY7MXPn2S6yJRARaXyLgTfXeu-AGMRe0KB5c51vsjypJP-aB-tZKUq0HMkJeZFgyx3aQ_RoSPIrsyhIYGtvw"
}
@tanguilp
Copy link
Owner

"alg" is indeed optional for JWKs, but not for Id tokens since they are JWSes.

If you're using ID tokens that don't have this parameter, then you have a problem with respect to the standard:

ID Tokens MUST be signed using JWS [JWS] and optionally both signed and then encrypted using JWS [JWS] and JWE [JWE] respectively, thereby providing authentication, integrity, non-repudiation, and optionally, confidentiality, per Section 16.14. If the ID Token is encrypted, it MUST be signed then encrypted, with the result being a Nested JWT, as defined in [JWT]. ID Tokens MUST NOT use none as the alg value unless the Response Type used returns no ID Token from the Authorization Endpoint (such as when using the Authorization Code Flow) and the Client explicitly requested the use of none at Registration time.

That said this function should not take into parameter a JWT, but a JOSE header or an ID token (it's not clear which alg to use when the ID token is encrypted - more research his needed). Will take a look at it when possible.

Worth noting that JOSEUtils.JWK.sig_alg_digest/1, which is used by the aforementioned function, makes therefore little sense. It should probably be moved to something like JOSEUtils.JWA.sig_alg_digest/1.

Last note: the "none" signing algorithm should be properly handled too.

Thanks for the bug report.

@YuriiZdyrko
Copy link
Contributor Author

Yes, I can extract "alg" from my ID token.
JOSE.JWS.peek_protected(id_token) |> Jason.decode!() => %{"alg" => "RS512"}

And docs clearly say, that "alg" from ID token header should be used for token/code validation. here

Thanks for help!

@tanguilp
Copy link
Owner

You could also used https://hexdocs.pm/jose_utils/JOSEUtils.JWS.html#peek_header/1 from the jose_utils library used by this package.

I'm leaving this issue opened as rework is needed.

@tanguilp
Copy link
Owner

One side note: if you're doing some OIDC, have you considered using the https://github.com/tanguilp/plugoid library?

@YuriiZdyrko
Copy link
Contributor Author

We didn't notice this library during investigation phase.

It seems like amount of customisation required would be too much.
Like:

  • allow setting client_id at runtime
  • custom redirection logic, because we redirect with js, not phoenix. Our own oidc wrapper doesn't even have Phoenix as dependency.

I'll do some research to understand what exactly plugoid does.
Thanks for pointing to it.

@tanguilp
Copy link
Owner

client-id is dynamic but if you're using js, then this library is not suited for your need, as it's Plug/Phoenix based.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants