-
Notifications
You must be signed in to change notification settings - Fork 375
Add support for x5t and x5t#S256 header #669
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
base: main
Are you sure you want to change the base?
Changes from all commits
8af6dbe
e1645c5
323ffc1
431b524
2ef7768
5008010
0212e3d
08d3bdd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,9 @@ class KeyFinder | |
# @param [Hash] options the options to create a KeyFinder with | ||
# @option options [Proc, JWT::JWK::Set] :jwks the jwks or a loader proc | ||
# @option options [Boolean] :allow_nil_kid whether to allow nil kid | ||
# @option options [Array] :key_fields the fields to use for key matching, | ||
# the order of the fields are used to determine | ||
# the priority of the keys. | ||
def initialize(options) | ||
@allow_nil_kid = options[:allow_nil_kid] | ||
jwks_or_loader = options[:jwks] | ||
|
@@ -18,15 +21,16 @@ def initialize(options) | |
else | ||
->(_options) { jwks_or_loader } | ||
end | ||
|
||
@key_fields = options[:key_fields] || %i[kid] | ||
end | ||
|
||
# Returns the verification key for the given kid | ||
# @param [String] kid the key id | ||
def key_for(kid) | ||
raise ::JWT::DecodeError, 'No key id (kid) found from token headers' unless kid || @allow_nil_kid | ||
raise ::JWT::DecodeError, 'Invalid type for kid header parameter' unless kid.nil? || kid.is_a?(String) | ||
def key_for(kid, key_field = :kid) | ||
raise ::JWT::DecodeError, "Invalid type for #{key_field} header parameter" unless kid.nil? || kid.is_a?(String) | ||
|
||
jwk = resolve_key(kid) | ||
jwk = resolve_key(kid, key_field) | ||
|
||
raise ::JWT::DecodeError, 'No keys found in jwks' unless @jwks.any? | ||
raise ::JWT::DecodeError, "Could not find public key for kid #{kid}" unless jwk | ||
|
@@ -37,22 +41,31 @@ def key_for(kid) | |
# Returns the key for the given token | ||
# @param [JWT::EncodedToken] token the token | ||
def call(token) | ||
key_for(token.header['kid']) | ||
@key_fields.each do |key_field| | ||
field_value = token.header[key_field.to_s] | ||
|
||
return key_for(field_value, key_field) if field_value | ||
end | ||
|
||
raise ::JWT::DecodeError, 'No key id (kid) or x5t found from token headers' unless @allow_nil_kid | ||
|
||
kid = token.header['kid'] | ||
key_for(kid) | ||
end | ||
|
||
private | ||
|
||
def resolve_key(kid) | ||
key_matcher = ->(key) { (kid.nil? && @allow_nil_kid) || key[:kid] == kid } | ||
def resolve_key(kid, key_field) | ||
key_matcher = ->(key) { (kid.nil? && @allow_nil_kid) || key[key_field] == kid } | ||
|
||
# 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)) | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also the line above where we do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think it's fine because we only call the JWKS loader with other key fields if token header does not contain
No, we only invalidating once. Basically, if
Could you clarify on what could be the compatibility issues? I change There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For use via the 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.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, that's a good idea. Let me work on that. |
||
@jwks.find { |key| key_matcher.call(key) } | ||
end | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
key_field should probably be set to
:kid
at before this is called if@allow_nil_kid
is trueThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would simplify this complicated piece of machinery....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the approach in #669 (comment) could address the above problem, too 🤔