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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix traversing not matching header algos #545

Merged
merged 6 commits into from
Jan 31, 2023

Conversation

mpospelov
Copy link
Contributor

@mpospelov mpospelov commented Jan 27, 2023

Hello ruby-jwt maintainers! Thank you for these amazing tool. 馃檹

Recently in our company we started to thinking of how we could rotate keys with 0 downtime.

I found that keyfinder can actually return array of keys and ruby-jwt will iterate through these keys until find a matching one

return if Array(@key).any? { |key| verify_signature_for?(key) }

BUT the problem is that for each key we traverse all allowed algoes in decoder.
Our provider code supports multiple consumers which encodes payload with different algoes, so the current code can lead to the case when we have matching public key returned as second element in keyfinder but we even don't try to decode the payload with it as we fail with attempt to decode with first key but with incorrect algo.

You can find these case in specs and implementation to fix this issue in this PR.

raise(JWT::IncorrectAlgorithm, 'An algorithm must be specified') if allowed_algorithms.empty?
raise(JWT::IncorrectAlgorithm, 'Token is missing alg header') unless alg_in_header
raise(JWT::IncorrectAlgorithm, 'Expected a different algorithm') unless valid_alg_in_header?

@algos = find_valid_algos_in_header
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting the @algos in the middle of everything feels a little wrong.

We are interested in the intersection between the allowed_algorithms and what is specified in the token header, right?

Could there be a method (maybe a memoizing one) that would be used in the verify_algos and verify_signature_for??
for example

def allowed_and_valid_algorithms
  @allowed_and_valid_algorithms ||= allowed_algorithms.select { |alg| alg.valid_alg?(alg_in_header) }
end

@anakinj
Copy link
Member

anakinj commented Jan 29, 2023

Hi, I think I get the problem and I don't see a problem filtering the list based on the user provided algorithms. I added a comment on the way the algorithms to be used are resolved.

Could you also be so kind and add a changelog entry to CHANGELOG.md?

@mpospelov
Copy link
Contributor Author

Hey @anakinj thank you for your kind review! I addressed rubocop fails and implemented the #allowed_and_valid_algorithms as you suggested

@mpospelov
Copy link
Contributor Author

also CHANGELOG.md entry is added 馃槉

CHANGELOG.md Show resolved Hide resolved
Co-authored-by: Joakim Antman <antmanj@gmail.com>
@anakinj anakinj merged commit 001d9c4 into jwt:main Jan 31, 2023
@anakinj
Copy link
Member

anakinj commented Jan 31, 2023

Great improvement. Thank you for the effort.

@mpospelov
Copy link
Contributor Author

Hey @anakinj! Thank you for all the support and for reviewing my PR 馃槉 One last thing is don't you mind releasing a new version of gem? It would help us a lot 馃檹

@anakinj
Copy link
Member

anakinj commented Feb 1, 2023

New version out. Have fun with it :)

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

2 participants