Support for ED25519 #229

ab320012 · 2017-09-06T12:23:34Z

Added support + tests, readme section for eddsa algo ed25519
#217

sourcelevel-bot · 2017-09-06T12:23:36Z

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

sourcelevel-bot · 2017-09-06T12:23:57Z

lib/jwt/signature.rb

+    def sign_eddsa(algorithm, msg, private_key)
+      raise EncodeError, "Key given is a #{private_key.class} but has to be an RbNaCl::Signatures::Ed25519::SigningKey" if private_key.class != RbNaCl::Signatures::Ed25519::SigningKey
+      raise IncorrectAlgorithm, "payload algorithm is #{algorithm} but #{private_key.primitive} signing key was provided"  if algorithm.downcase.to_sym != private_key.primitive
+      private_key.sign(msg) 


Trailing whitespace detected.

sourcelevel-bot · 2017-09-06T12:23:57Z

lib/jwt/signature.rb

@@ -17,7 +17,8 @@ module Signature
    HMAC_ALGORITHMS = %w[HS256 HS512256 HS384 HS512].freeze
    RSA_ALGORITHMS = %w[RS256 RS384 RS512].freeze
    ECDSA_ALGORITHMS = %w[ES256 ES384 ES512].freeze
-
+    EDDSA_ALGORITHMS = %w[ED25519].freeze
+


Trailing whitespace detected.

sourcelevel-bot · 2017-09-06T12:23:57Z

lib/jwt/signature.rb

@@ -79,13 +88,16 @@ def sign_hmac(algorithm, msg, key)
        OpenSSL::HMAC.digest(OpenSSL::Digest.new(algorithm.sub('HS', 'sha')), key, msg)
      end
    end
-
+    def verify_eddsa(algorithm, public_key,signing_input, signature)


Space missing after comma.

sourcelevel-bot · 2017-09-06T12:23:57Z

lib/jwt/signature.rb

@@ -17,7 +17,8 @@ module Signature
    HMAC_ALGORITHMS = %w[HS256 HS512256 HS384 HS512].freeze
    RSA_ALGORITHMS = %w[RS256 RS384 RS512].freeze
    ECDSA_ALGORITHMS = %w[ES256 ES384 ES512].freeze
-
+    EDDSA_ALGORITHMS = %w[ED25519].freeze


%w-literals should be delimited by ( and ).

sourcelevel-bot · 2017-09-06T12:23:57Z

lib/jwt/signature.rb

@@ -79,13 +88,16 @@ def sign_hmac(algorithm, msg, key)
        OpenSSL::HMAC.digest(OpenSSL::Digest.new(algorithm.sub('HS', 'sha')), key, msg)
      end
    end
-
+    def verify_eddsa(algorithm, public_key,signing_input, signature)


Use empty lines between method definitions.

sourcelevel-bot · 2017-09-06T12:23:58Z

lib/jwt/signature.rb

@@ -70,7 +75,11 @@ def sign_ecdsa(algorithm, msg, private_key)
      digest = OpenSSL::Digest.new(algorithm.sub('ES', 'sha'))
      SecurityUtils.asn1_to_raw(private_key.dsa_sign_asn1(digest.digest(msg)), private_key)
    end
-
+    def sign_eddsa(algorithm, msg, private_key)


Use empty lines between method definitions.

sourcelevel-bot · 2017-09-06T12:23:58Z

lib/jwt/signature.rb

@@ -43,6 +46,8 @@ def verify(algo, key, signing_input, signature)
        SecurityUtils.verify_rsa(algo, key, signing_input, signature)
      elsif ECDSA_ALGORITHMS.include?(algo)
        verify_ecdsa(algo, key, signing_input, signature)
+      elsif EDDSA_ALGORITHMS.include?(algo)


Align elsif with if.

sourcelevel-bot · 2017-09-06T12:23:58Z

lib/jwt/signature.rb

@@ -79,13 +88,16 @@ def sign_hmac(algorithm, msg, key)
        OpenSSL::HMAC.digest(OpenSSL::Digest.new(algorithm.sub('HS', 'sha')), key, msg)
      end
    end
-
+    def verify_eddsa(algorithm, public_key,signing_input, signature)


JWT::Signature takes parameters ['algorithm', 'public_key', 'signature', 'signing_input'] to 3 methods

Read more about it here.

…ure.rb

sourcelevel-bot · 2017-09-06T12:33:10Z

lib/jwt/signature.rb

@@ -17,6 +17,7 @@ module Signature
    HMAC_ALGORITHMS = %w[HS256 HS512256 HS384 HS512].freeze
    RSA_ALGORITHMS = %w[RS256 RS384 RS512].freeze
    ECDSA_ALGORITHMS = %w[ES256 ES384 ES512].freeze
+    EDDSA_ALGORITHMS = %w[ED25519].freeze


%w-literals should be delimited by ( and ).

sourcelevel-bot · 2017-09-06T12:33:10Z

lib/jwt/signature.rb

@@ -80,12 +91,17 @@ def sign_hmac(algorithm, msg, key)
      end
    end

+    def verify_eddsa(algorithm, public_key, signing_input, signature)


JWT::Signature takes parameters ['algorithm', 'public_key', 'signature', 'signing_input'] to 3 methods

Read more about it here.

sourcelevel-bot · 2017-09-06T12:33:12Z

Ebert has finished reviewing this Pull Request and has found:

6 possible new issues (including those that may have been commented here).

You can see more details about this review at https://ebertapp.io/github/jwt/ruby-jwt/pulls/229.

ab320012 · 2017-09-06T12:41:43Z

method length + complexity for verify and arity of sign + verify eddsa methods follow the surrounding code convention. I can probably fix these but would have to modify existing code, let me know if this is ok

Here is a snippet to remove longevity of if statements that doesn't modify the code base much at all.

    ALGOS = [
      {name: :hmac, types: %(HS256 HS512256 HS384 HS512).freeze },
      {name: :rsa, types: %w(RS256 RS384 RS512).freeze }, 
      {name: :ecdsa, types: %w(ES256 ES384 ES512).freeze },
      {name: :eddsa, types: %w(ED25519).freeze }
    ]
    def sign(algorithm, msg, key)
      algo= ALGOS.find{|algo| 
        algo[:types].include? algorithm
      }
      raise NotImplementedError, 'Unsupported signing method'  if algo.nil? 
      self.send("sign_#{algo[:name]}", algorithm, msg, key) 
    end

    def verify(algorithm, key, signing_input, signature)
      algo= ALGOS.find{|algo|
        algo[:types].include? algorithm 
      }
      raise JWT::VerificationError, 'Algorithm not supported' if algo.nil?  
      verified = self.send("verify_#{algo[:name]}", algorithm, key, signing_input, signature)
      raise JWT::VerificationError, 'Signature verification raised' unless verified 
    end

excpt · 2017-09-06T19:39:03Z

@ab320012 Thank you for the PR!

Refactoring the code to reduce the arity and other code smells should happen in a different PR. Don't do it in this PR.

Reviewing you code now.

ab320012 · 2017-09-06T19:58:05Z

@excpt Thanks, will add that one after this one closes. Wondering if class based solution will make more sense.

# lib/jwt/algos/hmac.rb
class Hmac
   # other classes same structure: ecdsa, eddsa, rsa
  TYPES= %w(HS256 HS512256 HS384 HS512).freeze 
 def initialize; end 
 def sign (signing_input); end
 def verify(verifying_input); end 
end

# signing_input and verifying_input will be refactored into a struct 

#lib/signature.rb 
Signed = Struct.new(:algo, :key, :msg) 
Verified = Struct.new(:algorithm, :public_key, :signing_input, :signature)

Let me know if something like this is ok with you for next PR

excpt · 2017-09-06T20:03:53Z

@ab320012 That's the way to go. Most important: the two signatures of the main methods JWT.encode and JWT.decode need to stay the same. Everything else is code behind and that should not break anything for the users.

ab320012 added 3 commits September 6, 2017 08:06

generates valid token with RbNaCl

566b2f3

added readme section for ed25519

6ac12c2

fix to readme

02191bd

sourcelevel-bot bot reviewed Sep 6, 2017

View reviewed changes

trailing white spaces removed + spaces between method names in signat…

a4557ce

…ure.rb

sourcelevel-bot bot reviewed Sep 6, 2017

View reviewed changes

excpt self-assigned this Sep 6, 2017

excpt self-requested a review September 6, 2017 19:40

excpt added enhancement feature labels Sep 6, 2017

excpt added this to the Version 2.1.0 milestone Sep 6, 2017

excpt approved these changes Sep 6, 2017

View reviewed changes

excpt merged commit ace22c6 into jwt:master Sep 6, 2017

excpt mentioned this pull request Sep 6, 2017

Ed25519 support planned? #217

Closed

ab320012 mentioned this pull request Sep 7, 2017

added algos/strategy classes + structs for inputs #230

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for ED25519 #229

Support for ED25519 #229

ab320012 commented Sep 6, 2017

sourcelevel-bot bot commented Sep 6, 2017

sourcelevel-bot bot Sep 6, 2017

sourcelevel-bot bot Sep 6, 2017

sourcelevel-bot bot Sep 6, 2017

sourcelevel-bot bot Sep 6, 2017

sourcelevel-bot bot Sep 6, 2017

sourcelevel-bot bot Sep 6, 2017

sourcelevel-bot bot Sep 6, 2017

sourcelevel-bot bot Sep 6, 2017

sourcelevel-bot bot Sep 6, 2017

sourcelevel-bot bot Sep 6, 2017

sourcelevel-bot bot commented Sep 6, 2017

ab320012 commented Sep 6, 2017 •

edited

Loading

excpt commented Sep 6, 2017

ab320012 commented Sep 6, 2017

excpt commented Sep 6, 2017

Support for ED25519 #229

Support for ED25519 #229

Conversation

ab320012 commented Sep 6, 2017

sourcelevel-bot bot commented Sep 6, 2017

sourcelevel-bot bot Sep 6, 2017

Choose a reason for hiding this comment

sourcelevel-bot bot Sep 6, 2017

Choose a reason for hiding this comment

sourcelevel-bot bot Sep 6, 2017

Choose a reason for hiding this comment

sourcelevel-bot bot Sep 6, 2017

Choose a reason for hiding this comment

sourcelevel-bot bot Sep 6, 2017

Choose a reason for hiding this comment

sourcelevel-bot bot Sep 6, 2017

Choose a reason for hiding this comment

sourcelevel-bot bot Sep 6, 2017

Choose a reason for hiding this comment

sourcelevel-bot bot Sep 6, 2017

Choose a reason for hiding this comment

sourcelevel-bot bot Sep 6, 2017

Choose a reason for hiding this comment

sourcelevel-bot bot Sep 6, 2017

Choose a reason for hiding this comment

sourcelevel-bot bot commented Sep 6, 2017

ab320012 commented Sep 6, 2017 • edited Loading

excpt commented Sep 6, 2017

ab320012 commented Sep 6, 2017

excpt commented Sep 6, 2017

ab320012 commented Sep 6, 2017 •

edited

Loading