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

Add support for kid-less jwks signature validation #37

Open
gordalina opened this issue Aug 11, 2022 · 6 comments
Open

Add support for kid-less jwks signature validation #37

gordalina opened this issue Aug 11, 2022 · 6 comments

Comments

@gordalina
Copy link

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

bcksl commented Mar 2, 2023

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

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

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

bcksl commented Mar 13, 2023

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

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

hjalet commented Mar 31, 2023

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.

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

4 participants