Add support for x5t and x5t#S256 header #669
Conversation
hieuk09
force-pushed
the
feature/add-x5t
branch
3 times, most recently
from
f6e2067 to
28a5e72
Compare
|
@hieuk09 Did you have some reference for how this should work? Could take a look at that to educate myself a bit. |
Now when looking closer at the changes I found some links... |
|
Thank you for the detailed review. I'll take a look and fix all the comments tomorrow. |
|
@anakinj could you have another review? |
| jwk = @jwks.find { |key| key_matcher.call(key) } | ||
|
|
||
| return jwk if jwk | ||
|
|
||
| # Second try, invalidate for backwards compatibility | ||
| @jwks = JWT::JWK::Set.new(@jwks_loader.call(invalidate: true, kid_not_found: true, kid: kid)) | ||
| @jwks = JWT::JWK::Set.new(@jwks_loader.call(invalidate: true, kid_not_found: true, key_field => kid)) |
There was a problem hiding this comment.
This could cause some problems as the keyloader could always be expecting the kid. Also do we will start invalidating 3 times if nothing is found.
There was a problem hiding this comment.
Also the line above where we do call with the key_field could cause backwards compatibility issues.
There was a problem hiding this comment.
This could cause some problems as the keyloader could always be expecting the kid
I think it's fine because we only call the JWKS loader with other key fields if token header does not contain kid. I think most of the JWKS endpoints support kid by default.
Also do we will start invalidating 3 times if nothing is found.
No, we only invalidating once. Basically, if kid field exists in token header, we only match using kid. Otherwise, if x5t exists and there is no kid, we'll match using x5t.
Also the line above where we do call with the key_field could cause backwards compatibility issues.
Could you clarify on what could be the compatibility issues? I change key_for signature but I added key_field as optional parameter so it should be backward-compatible. I may miss other scenarios, so please let me know.
There was a problem hiding this comment.
You're right! It happens only once. Now the gem is dictating the lookup order, so if the key contains both kid and x5 headers we'll never try the x5 lookups.
I feel this key lookup is already so complicated, cant wrap my head around this.
There was a problem hiding this comment.
That's right, the gem is dictating the lookup order. Currently I couldn't think of a better way to do this. Do you have any suggestions?
There was a problem hiding this comment.
For use via the JWT.decode method the predefined order is fine i think.
What about having the order as a option when initializing the keyfinder (with the current order as the default).
Then the new token object approach could be easily customized.
key_finder = JWT::JWK::KeyFinder.new(jwks: JWT::JWK::Set.new(jwk), key_fields: [:kid, :x5t])
encoded_token = JWT::EncodedToken.new(token.jwt)
encoded_token.verify!(signature: { algorithm: 'RS256', key_finder: key_finder})
There was a problem hiding this comment.
Thanks, that's a good idea. Let me work on that.
|
|
||
| # First try without invalidation to facilitate application caching | ||
| @jwks ||= JWT::JWK::Set.new(@jwks_loader.call(kid: kid)) | ||
| @jwks ||= JWT::JWK::Set.new(@jwks_loader.call(key_field => kid)) |
There was a problem hiding this comment.
key_field should probably be set to :kid at before this is called if @allow_nil_kid is true
There was a problem hiding this comment.
I wonder if it's too late to drop the @allow_nil_kid option for the 3.0 version... 🤔
There was a problem hiding this comment.
Would simplify this complicated piece of machinery....
There was a problem hiding this comment.
I think the approach in #669 (comment) could address the above problem, too 🤔
Description
Close #562
Checklist
Before the PR can be merged be sure the following are checked: