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

Ignore casing of algorithm #405

Merged
merged 13 commits into from Feb 9, 2021
Merged

Conversation

johnnyshields
Copy link

@johnnyshields johnnyshields commented Feb 4, 2021

(Updated 2/6/2021)

I am have to connect to a third party source for JWTs which transmits the algorithm as "Ed25519". This does not match the expected value in Ruby "ED25519". To remedy this:

  • Ignore casing of algorithm on input
  • Output correct casing of algorithm when encoding
  • Consider "Ed25519" as the correct casing, as per RFC#8037 as well as the Go, Erlang, Haskell, etc. JWT implementations.

In order to achieve this in a clean manner, I've extracted out a JWT::Algos module which uses a more efficient lookup (pre-indexed) to find the algo. I've also introduced JWT::Algos::None which is distinct from JWT::Algos::Unsupported

@sourcelevel-bot
Copy link

Hello, @johnnyshields! This is your first Pull Request that will be reviewed by SourceLevel, an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its review page. Please check our documentation for more information.

lib/jwt/decode.rb Outdated Show resolved Hide resolved
lib/jwt/decode.rb Outdated Show resolved Hide resolved
lib/jwt/decode.rb Outdated Show resolved Hide resolved
lib/jwt/decode.rb Outdated Show resolved Hide resolved
lib/jwt/decode.rb Outdated Show resolved Hide resolved
lib/jwt/decode.rb Outdated Show resolved Hide resolved
@johnnyshields johnnyshields changed the title Ignore casing of algorithm. … Ignore casing of algorithm Feb 4, 2021
@anakinj
Copy link
Member

anakinj commented Feb 5, 2021

There might be something wrong with how the gem handles the Ed25519 cases (#334).

In general i think that making the algorithms case insensitive is maybe not a good idea. The RFCs clearly states what the values are expected to be (https://tools.ietf.org/html/rfc7518#page-9).

@johnnyshields
Copy link
Author

johnnyshields commented Feb 5, 2021

The RFCs clearly states what the values are expected to be (https://tools.ietf.org/html/rfc7518#page-9).

It's reasonable to expect that there will case insensitive JWT implementations in the wild. Perhaps we can always encode to correct casing (uppercase), but decode irrespective of case?

@johnnyshields
Copy link
Author

@anakinj I've raised PR #406 which is specific to "Ed25519". That PR includes examples of how other libs are using "Ed25519" (lowercase "d"). Please kindly merge either this PR or #406 .

@anakinj
Copy link
Member

anakinj commented Feb 5, 2021

I agree that that the parameter given in the decode and encode methods could be case insensitive.

Just to compare the OpenSSL API is case insensitive and allows both uppercase and lowercase:

OpenSSL::Digest.new('SHA512') == OpenSSL::Digest.new('sha512')
=> true

OpenSSL::Digest.new('sha512').name == 'SHA512'
=> true

@anakinj
Copy link
Member

anakinj commented Feb 5, 2021

Im fine with either just doing the change in #406 or then make some adjustments to this that the alg value in the token do not change but the encode and decode methods understands the algorithm in any given case

@johnnyshields
Copy link
Author

OK, I've updated this PR to show what it would look like to add casing coercion (e.g. "hs256" becomes "HS256" when encoding). There are still some failing specs related to unsupported case. If you like this implementation I will fix the failing specs over the weekend.

lib/jwt/signature.rb Outdated Show resolved Hide resolved
lib/jwt/algos.rb Outdated Show resolved Hide resolved
lib/jwt/algos.rb Outdated Show resolved Hide resolved
lib/jwt/algos.rb Outdated Show resolved Hide resolved
lib/jwt/algos/none.rb Outdated Show resolved Hide resolved
lib/jwt/algos/none.rb Outdated Show resolved Hide resolved
lib/jwt/algos/none.rb Outdated Show resolved Hide resolved
lib/jwt/decode.rb Outdated Show resolved Hide resolved
Copy link
Member

@anakinj anakinj left a comment

Choose a reason for hiding this comment

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

Looks really great. I added a few questions

lib/jwt/algos.rb Outdated Show resolved Hide resolved
lib/jwt/algos/eddsa.rb Outdated Show resolved Hide resolved
spec/jwt_spec.rb Outdated Show resolved Hide resolved
@anakinj anakinj merged commit 9ba070e into jwt:master Feb 9, 2021
@anakinj
Copy link
Member

anakinj commented Feb 9, 2021

Thank you for your contribution.

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