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

allow_nil_kid option for the JWK KeyFinder #543

Merged
merged 4 commits into from
Jan 31, 2023

Conversation

bellebaum
Copy link
Contributor

When a JWT does not specify a kid explicitly in its header, its key may still have to be fetched from a JWKS.
This may happen e.g. when there is only one key in the JWKS and thus there is no need to specify an ID.

This PR adds a decoding option :allow_nil_kid, which, when used in conjunction with the :jwks option, prevents an error from being raised when the JWT does not specify the kid.
In this case, the first key in the provided JWKS is used as the decryption key.

To prevent application jwks-handlers from being overwhelmed by a sudden nil in the options hash, the functionality is disabled by default. (To be fair though, there are no type checks on kid and this is all before any verification has happened, so applications should have implemented the necessary checks anyways. So there is an argument that one could just make this the default. :D)

@anakinj
Copy link
Member

anakinj commented Jan 27, 2023

Makes sense, gets a little hacky like this when we need to keep backwards compatibility. Would you still be kind to add a changelog entry into the CHANGELOG.md file?

@bellebaum
Copy link
Contributor Author

I went ahead and added a simple type check on the kid value in the keyfinder.

I think there are few applications which handle the case where kid is not a string but fail if it is nil. So making this the default should not break applications that do not need fixing anyway. But there is of course the remote chance that someone may have taken a look at this gem's code, saw the nil-check and decided to take that for granted.

Personally, I think that if such people exist, they will also be able to fix their code easily and would be comfortable taking a chance here, but strictly speaking it is a backwards-incompatible change and you may also simplify this with the next major version.

Your choice :)

@anakinj
Copy link
Member

anakinj commented Jan 29, 2023

I think it's great. Just added very small suggestion:)

@anakinj anakinj merged commit fce8bbc into jwt:main Jan 31, 2023
@anakinj
Copy link
Member

anakinj commented Jan 31, 2023

Thank you for your contribution.

@sandstrom
Copy link

Small suggestion: Keep a list somewhere (markdown list in a meta/tracking issue, for example) with stuff you want to update in the next major. In this case, flipping default behavior.

tatsuyafw added a commit to tatsuyafw/ruby-jwt that referenced this pull request Feb 3, 2023
`allow_nil_kid` option was introduced in jwt#543, but the option name was wrong
in README.

This change fixes the typo.
@tatsuyafw tatsuyafw mentioned this pull request Feb 3, 2023
anakinj pushed a commit that referenced this pull request Feb 3, 2023
`allow_nil_kid` option was introduced in #543, but the option name was wrong
in README.

This change fixes the typo.
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

Successfully merging this pull request may close these issues.

None yet

3 participants