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

Improvement/encode hmac without key #312

Merged
merged 1 commit into from Jul 7, 2020

Conversation

JotaSe
Copy link
Contributor

@JotaSe JotaSe commented May 17, 2019

Description

For hmac strategy, will raise an exception if the secret key is nil but no when secret key is blank, for both cases should be the same.

Analysis

in lib/jwt/algos/hmac.rbthe key value is used for

SecurityUtils.rbnacl_fixup(algorithm, key.to_s)

and

OpenSSL::HMAC.digest(OpenSSL::Digest.new(algorithm.sub('HS', 'sha')), key.to_s, msg)

in both cases, the key should be a string for https://ruby-doc.org/stdlib-2.4.0/libdoc/openssl/rdoc/OpenSSL/HMAC.html and

# lib/jwt/security_utils.rb:49
# lib/jwt/security_utils.rb:53
key.bytesize

Solution:

in JWT::Algos::Hmac#sign set key ||= '' to convert nil to String

QA

  • Run tests
  • bin/console.rb =>
# Without secret key
token = JWT.encode payload, nil, 'HS256'

# eyJhbGciOiJIUzI1NiJ9.eyJkYXRhIjoidGVzdCJ9.pVzcY2dX8JNM3LzIYeP2B1e1Wcpt1K3TWVvIYSF4x-o
puts token

decoded_token = JWT.decode token, nil, true, { algorithm: 'HS256' }

# Array
# [
#   {"data"=>"test"}, # payload
#   {"alg"=>"HS256"} # header
# ]
puts decoded_token

@sourcelevel-bot
Copy link

Hello, @JotaSe! This is your first Pull Request that will be reviewed by Ebert, 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.

@rabajaj0509
Copy link
Contributor

@JotaSe good catch! mind squashing the commits to one?

@JotaSe
Copy link
Contributor Author

JotaSe commented Oct 3, 2019

Sure @rahulbajaj0509

@JotaSe JotaSe force-pushed the improvement/encode_hmac_without_key branch from 22f6430 to 6fa95b9 Compare October 3, 2019 17:05
@JotaSe
Copy link
Contributor Author

JotaSe commented Oct 3, 2019

Done @rahulbajaj0509

@sourcelevel-bot
Copy link

SourceLevel has finished reviewing this Pull Request and has found:

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

But beware that this branch is 14 commits behind the jwt:master branch, and a review of an up to date branch would produce more accurate results.

You can see more details about this review at https://app.sourcelevel.io/github/jwt/ruby-jwt/pulls/312.

Copy link
Contributor

@rabajaj0509 rabajaj0509 left a comment

Choose a reason for hiding this comment

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

LGTM!

@excpt excpt added this to the Version 2.2.2 milestone Jul 7, 2020
@excpt excpt self-requested a review July 7, 2020 00:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants