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

Handle parsed JSON JWKS input with string keys #348

Merged
merged 5 commits into from Sep 1, 2020

Conversation

@martinemde
Copy link
Contributor

@martinemde martinemde commented Mar 25, 2020

When decoded JSON is passed to the JWK.import, there are difficult to diagnose failures that occur.

Since the goal of JWKS is to support loading of json files, we should cleanly support string keys as that is the natural input to these methods.

This PR creates support for directly parsed JSON with string keys.

  • No realistic performance decrease.
  • Backwards compatibility is maintained by searching for symbol keys and then string keys.
  • Improved error messaging that would have helped me find this issue more quickly.

I have copied below the spec output without the fixes I added. You can see from the failures that the errors are very difficult to understand when all that has happened is the keys were missed.

For instance, "could not find public key" is confusing because debugging shows a perfect match between your JWK and the JWT kid. It's simply the fact that the key is a string and this is not expected.

  1. JWT.decode for JWK usecase when jwk keys are loaded from JSON with string keys decodes the token
    Failure/Error: raise ::JWT::DecodeError, "Could not find public key for kid #{kid}" unless jwk
JWT::DecodeError:
  Could not find public key for kid 3240eb23aa4f72d26b1b1b534fee4c58c68a51af73b18acedd1e697a775a67fe
# ./lib/jwt/jwk/key_finder.rb:17:in `key_for'
# ./lib/jwt/decode.rb:37:in `verify_signature'
# ./lib/jwt/decode.rb:26:in `decode_segments'
# ./lib/jwt.rb:28:in `decode'
# ./spec/jwk/decode_with_jwk_spec.rb:71:in `block (4 levels) in <top (required)>'
  1. JWT::JWK::RSA.import when jwk_data is given without e and/or n raises an error
    Failure/Error: expect { subject }.to raise_error(JWT::JWKError, "Key format is invalid for RSA")
expected JWT::JWKError with "Key format is invalid for RSA", got #<NoMethodError: undefined method `tr' for nil:NilClass> with backtrace:
# ./lib/jwt/jwk/rsa.rb:43:in `import'
# ./spec/jwk/rsa_spec.rb:59:in `block (3 levels) in <top (required)>'
# ./spec/jwk/rsa_spec.rb:83:in `block (5 levels) in <top (required)>'
# ./spec/jwk/rsa_spec.rb:83:in `block (4 levels) in <top (required)>'
# ./spec/jwk/rsa_spec.rb:83:in `block (4 levels) in <top (required)>'
  1. JWT::JWK::RSA.import when keypair is imported with string keys from JSON returns a hash with the public parts of the key
Failure/Error:
  imported_key.set_key(OpenSSL::BN.new(::Base64.urlsafe_decode64(jwk_data[:n]), BINARY),
    OpenSSL::BN.new(::Base64.urlsafe_decode64(jwk_data[:e]), BINARY),
    nil)

NoMethodError:
  undefined method `tr' for nil:NilClass
# ./lib/jwt/jwk/rsa.rb:43:in `import'
# ./spec/jwk/rsa_spec.rb:59:in `block (3 levels) in <top (required)>'
# ./spec/jwk/rsa_spec.rb:74:in `block (4 levels) in <top (required)>'

Linking to #95 also because it is somewhat related to the string/symbol issue here.

EDIT:

The source of this problem could be tied back to the README example which uses .export instead of parsing an actual parsed JSON Web Key as the input to the key finder. We could update export to return string keys to make it more clear.

@sourcelevel-bot
Copy link

@sourcelevel-bot sourcelevel-bot bot commented Mar 25, 2020

Hello, @martinemde! 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.

@martinemde martinemde changed the title Add specs to show that parsed JSON doesn't work Handle parsed JSON JWKS input with string keys Mar 25, 2020
lib/jwt/jwk/rsa.rb Outdated Show resolved Hide resolved
lib/jwt/jwk/key_finder.rb Outdated Show resolved Hide resolved
@martinemde martinemde force-pushed the martinemde:allow-json-parsed-jwk branch from a4d8272 to 7a42483 Mar 25, 2020
key_n = decode_open_ssl_bn(jwk_n)
key_e = decode_open_ssl_bn(jwk_e)

if key.respond_to?(:set_key)

This comment has been minimized.

@sourcelevel-bot

sourcelevel-bot bot Mar 25, 2020

JWT::JWK::RSA#self.rsa_pkey manually dispatches method call

@sourcelevel-bot
Copy link

@sourcelevel-bot sourcelevel-bot bot commented Mar 25, 2020

SourceLevel has finished reviewing this Pull Request and has found:

  • 1 possible new issue (including those that may have been commented here).
  • 1 fixed issue! 🎉

See more details about this review.

@anakinj
Copy link
Member

@anakinj anakinj commented Jul 2, 2020

Hi, this feature is great. It's always a hassle to ensure the keys are the correct type.

Im wondering if it would be a good idea to transform the keys into either strings or symbols before using the hash, that would remove the need to always check for both key types whenever the values are accessed.

@excpt excpt added this to the Version 2.3.0 milestone Jul 7, 2020
@excpt excpt self-assigned this Jul 7, 2020
@martinemde
Copy link
Contributor Author

@martinemde martinemde commented Jul 7, 2020

Im wondering if it would be a good idea to transform the keys into either strings or symbols before using the hash, that would remove the need to always check for both key types whenever the values are accessed.

I think most of the hash lookups are during import so it wouldn't buy us much.

The example in the readme for loading JWK indicates that caching should be performed inside the loader (before this import step). Unless I'm mistaken, after importing, a single key is grabbed from the key set and used to check the signature, then the entire thing is discarded.

Given just how very fast and lightweight 2 hash lookups is compared to deep copying a parsed json object of unknown size, I'd recommend we don't convert the input in any way.

martinemde added 5 commits Mar 24, 2020
This fixes an issue where JSON.parse results of an actual .jwks file
fail to import into the key finder because they have string keys.

Errors also improved in a way that helps debugging issues with JWKS loading.
@martinemde martinemde force-pushed the martinemde:allow-json-parsed-jwk branch from 13d1b74 to 549df84 Jul 7, 2020
@anakinj
anakinj approved these changes Jul 7, 2020
@excpt excpt merged commit ba2244f into jwt:master Sep 1, 2020
3 checks passed
3 checks passed
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sourcelevel-bot
sourcelevel SourceLevel has found 1 possible new issue and 1 fixed issue.
Details
@martinemde martinemde deleted the martinemde:allow-json-parsed-jwk branch Sep 1, 2020
@martinemde
Copy link
Contributor Author

@martinemde martinemde commented Sep 1, 2020

Yay, thank you!

@excpt
Copy link
Member

@excpt excpt commented Sep 1, 2020

Yay, thank you!

I thank you for the contribution! Thanks! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants