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

Verify algorithm before evaluating keyfinder #343

Closed
zinosama opened this issue Dec 26, 2019 · 4 comments
Closed

Verify algorithm before evaluating keyfinder #343

zinosama opened this issue Dec 26, 2019 · 4 comments

Comments

@zinosama
Copy link

@zinosama zinosama commented Dec 26, 2019

Summary

As of 2.2.1, Decode#verify_signature evaluates &@keyfinder before validating algorithm. This is quite inefficient and results in misleading failure messages (see reasoning below). I propose we move algorithm validation before finding the key so that we don't waste our time evaluating key finder when we know the algorithm is wrong.

Reasoning

  1. Key finder evaluation is usually used for supporting features like JWKS, where a network call needs to be made to get a public key. Making network calls can often be expensive for Ruby applications. Therefore it makes sense to do the cheaper checks such as alg validation first. The performance impact of this can be especially significant in applications where multiple types of token are accepted.
  2. A token with the wrong algorithm is most likely never gonna find the right key. In this case, the exception should clearly indicating the issue with the incorrect algorithm and raise JWT::IncorrectAlgorithm instead of JWT::DecodeError, 'No verification key available.

Please let me know if you are willing to accept a PR on this.

@zinosama
Copy link
Author

@zinosama zinosama commented Dec 31, 2019

@ab320012
Copy link
Contributor

@ab320012 ab320012 commented Feb 21, 2020

@zinosama it sounds good to me

@zinosama
Copy link
Author

@zinosama zinosama commented Feb 21, 2020

@zinosama it sounds good to me

Cool. I'll look into opening a PR. Thank you.

@anakinj
Copy link
Member

@anakinj anakinj commented Oct 23, 2020

Closing this issue. The order of validating algorithm and evaluating keyfinder was changed in #346

@anakinj anakinj closed this Oct 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants