Support exporting RSA private keys #373

anakinj · 2020-09-25T06:02:04Z

@phlegx @richardlarocque Got inspired by your work so made this thing that would allow the inclusion of the private key in the JWK.

sourcelevel-bot · 2020-09-25T06:02:32Z

lib/jwt/jwk/rsa.rb

        end

-        key
-      end
+        def rsa_pkey(jwk_n, jwk_e, jwk_d, jwk_p, jwk_q, jwk_dp, jwk_dq, jwk_qi)


Perceived complexity for rsa_pkey is too high. [16/7]

sourcelevel-bot · 2020-09-25T06:02:32Z

lib/jwt/jwk/rsa.rb

        end

-        key
-      end
+        def rsa_pkey(jwk_n, jwk_e, jwk_d, jwk_p, jwk_q, jwk_dp, jwk_dq, jwk_qi)


Avoid parameter lists longer than 5 parameters. [8/5]

sourcelevel-bot · 2020-09-25T06:02:33Z

lib/jwt/jwk/rsa.rb

        end

-        key
-      end
+        def rsa_pkey(jwk_n, jwk_e, jwk_d, jwk_p, jwk_q, jwk_dp, jwk_dq, jwk_qi)


Method has too many lines. [17/15]

You might want to extract some of this method lines into new methods inside its current class or isolate it on a class of its own and make it a collaborator instead.

sourcelevel-bot · 2020-09-25T06:02:34Z

lib/jwt/jwk/rsa.rb

@@ -28,45 +28,68 @@ def kid
        OpenSSL::Digest::SHA256.hexdigest(sequence.to_der)
      end

-      def export
-        {
+      def export(options = {})


Assignment Branch Condition size for export is too high. [28.04/20]

sourcelevel-bot · 2020-09-25T06:02:35Z

lib/jwt/jwk/rsa.rb

        end

-        key
-      end
+        def rsa_pkey(jwk_n, jwk_e, jwk_d, jwk_p, jwk_q, jwk_dp, jwk_dq, jwk_qi)


Cyclomatic complexity for rsa_pkey is too high. [15/6]

sourcelevel-bot · 2020-09-25T06:02:36Z

lib/jwt/jwk/rsa.rb

        end

-        key
-      end
+        def rsa_pkey(jwk_n, jwk_e, jwk_d, jwk_p, jwk_q, jwk_dp, jwk_dq, jwk_qi)


JWT::JWK::RSA#rsa_pkey has approx 14 statements

Read more about it here.

sourcelevel-bot · 2020-09-25T06:02:37Z

lib/jwt/jwk/rsa.rb

-      def self.decode_open_ssl_bn(jwk_data)
-        OpenSSL::BN.new(::Base64.urlsafe_decode64(jwk_data), BINARY)
+        def decode_open_ssl_bn(jwk_data)
+          return nil if jwk_data.nil?


JWT::JWK::RSA#decode_open_ssl_bn performs a nil-check

Read more about it here.

sourcelevel-bot · 2020-09-25T06:02:38Z

lib/jwt/jwk/rsa.rb

+
+          key = OpenSSL::PKey::RSA.new
+
+          if key.respond_to?(:set_key)


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

sourcelevel-bot · 2020-09-25T06:02:39Z

SourceLevel has finished reviewing this Pull Request and has found:

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

See more details about this review.

anakinj · 2020-09-25T06:05:33Z

Will look at those robot comments later. The amount of parameters is a little problematic for these complexity metrics

phlegx

Should I also apply this changes (option include_private) to HMAC JWK?

phlegx · 2020-09-25T08:13:19Z

lib/jwt/jwk/rsa.rb

          kty: KTY,
          n: encode_open_ssl_bn(public_key.n),
          e: encode_open_ssl_bn(public_key.e),
          kid: kid
        }
+
+        return ret if options[:include_private] != true


Also check if the keypair has a private key.

return ret unless private? && options[:include_private] == true

Good point. Will get back to this when I have the time.

anakinj · 2020-09-25T08:29:36Z

Should I also apply this changes (option include_private) to HMAC JWK?

If we can agree on the public export method API to be like this it could be already implemented for the new JWKs.

I went with the options hash to be able to maybe add/override something on the export easier in the future, dunno if that was necessary.

phlegx · 2020-09-25T08:34:56Z

Options with a hash for export is the right way! It is better to handle and change. Method export(options = {}) remains intact in the code for all developers when new options come in the future. I agree.

phlegx · 2020-09-25T08:54:18Z

lib/jwt/jwk/rsa.rb

+          dp: encode_open_ssl_bn(keypair.dmp1),
+          dq: encode_open_ssl_bn(keypair.dmq1),
+          qi: encode_open_ssl_bn(keypair.iqmp)
+        )
      end

      def encode_open_ssl_bn(key_part)


Make this method private. Also method kid. So, we can expose the same set of methods in all other JWKs.

phlegx · 2020-09-25T09:04:07Z

I propose to expose only a defined set of methods for all JWKs. All other methods should be private. An abstract class could help.

module JWT
  module JWK
    class [NAME]
      attr_reader :keypair
      attr_reader :kid

      def initialize(keypair, kid = nil)
        # ...
      end

      def private?
        # ...
      end

      def public_key
        # ...
      end

      def export(options = {})
        # ...
      end

      class << self
        def import(jwk_data)
        end
      end
    end
  end
end

phlegx · 2020-09-25T10:01:58Z

@anakinj and @richardlarocque please review my code on PR #372. I have added a factory class that can be used by RSA and EC JWKs.

richardlarocque

This makes sense to me.

I don't pretend to understand the OpenSSL calls, but they seem fine.

I think there's an API question about what you want to do if someone passes export_private on a public key JWK. You can raise an error or silently return the public key. My preference is to raise an error, but you can argue it either way.

richardlarocque · 2020-09-25T21:12:13Z

spec/jwk/rsa_spec.rb

@@ -34,7 +34,7 @@
      it 'returns a hash with the public parts of the key' do
        expect(subject).to be_a Hash
        expect(subject).to include(:kty, :n, :e, :kid)
-        expect(subject).not_to include(:d)
+        expect(subject).not_to include(:d, :p, :dp, :dq,:qi)


nit: whitespace between :dq and :qi.

richardlarocque · 2020-09-25T21:12:23Z

spec/jwk/rsa_spec.rb

@@ -43,7 +43,7 @@
      it 'returns a hash with the public parts of the key' do
        expect(subject).to be_a Hash
        expect(subject).to include(:kty, :n, :e, :kid)
-        expect(subject).not_to include(:d)
+        expect(subject).not_to include(:d, :p, :dp, :dq,:qi)


nit: whitespace again.

richardlarocque · 2020-09-25T21:12:33Z

spec/jwk/rsa_spec.rb

+      let(:keypair) { rsa_key }
+      it 'returns a hash with the public AND private parts of the key' do
+        expect(subject).to be_a Hash
+        expect(subject).to include(:kty, :n, :e, :kid, :d, :p, :q, :dp, :dq,:qi)


nit: also whitespace.

anakinj · 2020-09-28T08:30:50Z

Thanks for the comments @richardlarocque. I hope to get back to this in the end of this week.

phlegx · 2020-09-28T08:43:47Z

@anakinj @richardlarocque @excpt I suggest to first merge PR #372 that includes the abstract class. Then JWK RSA and EC can merge and adapt own PR to the abstract structure.

excpt · 2020-09-28T09:39:48Z

#372 merged.

@anakinj I'd recommend a rebase for this pr.

anakinj · 2020-10-01T20:03:03Z

Replaced by #375

Support exporting RSA private keys

9b5d19f

sourcelevel-bot bot reviewed Sep 25, 2020

View reviewed changes

phlegx suggested changes Sep 25, 2020

View reviewed changes

anakinj mentioned this pull request Sep 25, 2020

Add support for JWKs with EC key type #371

Merged

phlegx reviewed Sep 25, 2020

View reviewed changes

richardlarocque reviewed Sep 25, 2020

View reviewed changes

excpt self-requested a review September 28, 2020 09:39

anakinj mentioned this pull request Oct 1, 2020

Support exporting RSA JWK private keys #375

Merged

anakinj closed this Oct 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support exporting RSA private keys #373

Support exporting RSA private keys #373

anakinj commented Sep 25, 2020

sourcelevel-bot bot Sep 25, 2020

sourcelevel-bot bot Sep 25, 2020

sourcelevel-bot bot Sep 25, 2020

sourcelevel-bot bot Sep 25, 2020

sourcelevel-bot bot Sep 25, 2020

sourcelevel-bot bot Sep 25, 2020

sourcelevel-bot bot Sep 25, 2020

sourcelevel-bot bot Sep 25, 2020

sourcelevel-bot bot commented Sep 25, 2020

anakinj commented Sep 25, 2020

phlegx left a comment

phlegx Sep 25, 2020

anakinj Sep 25, 2020

anakinj commented Sep 25, 2020

phlegx commented Sep 25, 2020 •

edited

phlegx Sep 25, 2020 •

edited

phlegx commented Sep 25, 2020

phlegx commented Sep 25, 2020 •

edited

richardlarocque left a comment

richardlarocque Sep 25, 2020

richardlarocque Sep 25, 2020

richardlarocque Sep 25, 2020

anakinj commented Sep 28, 2020

phlegx commented Sep 28, 2020

excpt commented Sep 28, 2020

anakinj commented Oct 1, 2020

Support exporting RSA private keys #373

Support exporting RSA private keys #373

Conversation

anakinj commented Sep 25, 2020

sourcelevel-bot bot Sep 25, 2020

Choose a reason for hiding this comment

sourcelevel-bot bot Sep 25, 2020

Choose a reason for hiding this comment

sourcelevel-bot bot Sep 25, 2020

Choose a reason for hiding this comment

sourcelevel-bot bot Sep 25, 2020

Choose a reason for hiding this comment

sourcelevel-bot bot Sep 25, 2020

Choose a reason for hiding this comment

sourcelevel-bot bot Sep 25, 2020

Choose a reason for hiding this comment

sourcelevel-bot bot Sep 25, 2020

Choose a reason for hiding this comment

sourcelevel-bot bot Sep 25, 2020

Choose a reason for hiding this comment

sourcelevel-bot bot commented Sep 25, 2020

anakinj commented Sep 25, 2020

phlegx left a comment

Choose a reason for hiding this comment

phlegx Sep 25, 2020

Choose a reason for hiding this comment

anakinj Sep 25, 2020

Choose a reason for hiding this comment

anakinj commented Sep 25, 2020

phlegx commented Sep 25, 2020 • edited

phlegx Sep 25, 2020 • edited

Choose a reason for hiding this comment

phlegx commented Sep 25, 2020

phlegx commented Sep 25, 2020 • edited

richardlarocque left a comment

Choose a reason for hiding this comment

richardlarocque Sep 25, 2020

Choose a reason for hiding this comment

richardlarocque Sep 25, 2020

Choose a reason for hiding this comment

richardlarocque Sep 25, 2020

Choose a reason for hiding this comment

anakinj commented Sep 28, 2020

phlegx commented Sep 28, 2020

excpt commented Sep 28, 2020

anakinj commented Oct 1, 2020

phlegx commented Sep 25, 2020 •

edited

phlegx Sep 25, 2020 •

edited

phlegx commented Sep 25, 2020 •

edited