search

joken-elixir / joken_jwks

A Joken 2 hook for fetching the signer from a public JWKS url
Apache License 2.0
29 stars 24 forks source link

Add support for kid-less jwks signature validation #37

Open gordalina opened 1 year ago

gordalina commented 1 year ago

The documentation states that JWKS mandates a kid claim, but it seems that this is not the case in both the JWS and JWK RFCs, even though there's a statement that says that kid is used to match a JWS, it does not indicate that is a requirement.

I'm implementing application-based access using Teleport and they do not add a kid claim in the header or in JWKS. I'm able to correctly validate JWKS with a stripped-down version of this library.

What do you think about allowing this use case?

bcksl commented 1 year ago

Hey folks, I have to jump in here and add support for this. Having a validation of the kid claim is totally reasonable, but needs to be optional due to many auth providers not adding this claim even if they make their keys available as JWKS. How about making this an option on the hook?

Would be totally happy to hear if there's an alternate solution proposed to remove the kid claim validation that would work with the library as it is currently written, but for now I'm blocked using joken_jwks on this.

victorolinasc commented 1 year ago

I am sorry I am taking a long time to look into this issue more deeply.

I understand that unfortunately some providers don't provide a kid header claim. Reading again the specification it says OPTIONAL with strong "wording" for the method to match a JWK in a set to the one in a token. There could be a different strategy that tries to decode with ALL keys in a set (although I wouldn't recommend it). Since in Elixir verification is fast, this might be a different strategy.

Also, I've noticed that we are actually not 100% compliant with the specification... there could be duplicated kids in a set. In that case, we should ALSO match the kty parameter as per the specification.

Wdyt?

I am pending approving a major rewrite of the process structure in this library that is in the #39 . I will try to take a look at this in a not so distant future so that we "unblock" other work here.

gordalina commented 1 year ago

Also, I've noticed that we are actually not 100% compliant with the specification... there could be duplicated kids in a set. In that case, we should ALSO match the kty parameter as per the specification.

That makes sense.

bcksl commented 1 year ago

For better or worse, the providers who don't provide kid claims mostly seem to be using JWKS as a way to distribute single keys, so adding a match_first_key? option would handle the majority of cases and I think not require a major rewrite. I guess match_any_key? would take care of the rest but would entail a bit more code change.

victorolinasc commented 1 year ago

I am gathering feedbacks here and in other issues about possible breaking changes so that we can think about a 2.0 version here. I will open an issue for that so that it gives a bit more clarity of what I am currently thinking.

In any case, I can't promise a schedule for this kind of fix. I want to address this properly now that I have more experience using all kinds of different implementations (thanks Open Finance Brasil... ).

hjalet commented 1 year ago

In our code where we use joken_jwks we have also run into problems with an OIDC provider not adding the kid to their id token. This is because it is optional in the OIDC specs and they only have one key.

So we are interested in a fix for this as well.

One possible solution could be to allow not providing a kid claim only when exactly one key is present in the keys list.