Add support for JWKs with EC key type #371

richardlarocque · 2020-09-18T23:34:52Z

Adds support for JWKs with "kty" value "EC". This commit includes
support for curves ("crv") "P-256", "P-384", "P-521".

For additional details on these JWKs and their contents, see
https://tools.ietf.org/html/rfc7518#section-6.2.1.

This implementation adheres closely to the pattern set by
JWT::JWK::RSA. It keeps the same coding style and method names.
It also inherits a few quirks:

It ignores any private key ("d") values when importing JWKs.
It never emits private key ("d") values when exporting JWKs.
An import followed by an export does not preserve the "kid" value.

These behaviors seem strange to me, but I worry that changing the
convention would be surprising and potentially dangerous to existing
users of this library.

lib/jwt/jwk/ec.rb

anakinj · 2020-09-22T05:16:47Z

Looks great, thanks for your contribution.

Wondering about this 'An import followed by an export does not preserve the "kid" value.', this behaviour was not intended and is fixed in #320 for the RSA keys. Maybe this feature could be perfect from the beginning in regards to that :)

About not exporting private keys, I recall me thinking that it could be dangerous to export the private key by default. If we would like to support this we could add a parameter to the export method that would allow inclusion of the private parts.

lib/jwt/jwk/ec.rb

sourcelevel-bot · 2020-09-23T18:27:52Z

lib/jwt/jwk/ec.rb

+          jwk_crv, jwk_x, jwk_y, jwk_kid = jwk_attrs(jwk_data, %i[crv x y kid])
+          raise Jwt::JWKError, 'Key format is invalid for EC' unless jwk_crv && jwk_x && jwk_y
+
+          new(ec_pkey(jwk_crv, jwk_x, jwk_y), kid=jwk_kid)


Useless assignment to variable - kid.

sourcelevel-bot · 2020-09-23T18:27:53Z

lib/jwt/jwk/ec.rb

+
+      def export
+        crv, x_octets, y_octets = keypair_components(keypair)
+        sequence = OpenSSL::ASN1::Sequence([OpenSSL::ASN1::Integer.new(OpenSSL::BN.new(x_octets, BINARY)),


Useless assignment to variable - sequence.

sourcelevel-bot · 2020-09-23T18:27:54Z

lib/jwt/jwk/ec.rb

+          jwk_crv, jwk_x, jwk_y, jwk_kid = jwk_attrs(jwk_data, %i[crv x y kid])
+          raise Jwt::JWKError, 'Key format is invalid for EC' unless jwk_crv && jwk_x && jwk_y
+
+          new(ec_pkey(jwk_crv, jwk_x, jwk_y), kid=jwk_kid)


Surrounding space missing for operator =.

anakinj · 2020-09-24T04:38:41Z

lib/jwt/jwk/ec.rb

+          crv = 'P-521'
+          x_octets, y_octets = encoded_point.unpack('xa66a66')
+        else
+          raise "Unsupported curve '#{ec_keypair.group.curve_name}'"


Missed this the first time around. The JWT::JWKError could be used here also

My bad. Should be fixed now.

phlegx · 2020-09-24T22:51:11Z

Hi! Why JWT::JWK::RSA and JWT::JWK::EC don't export private keys? It is defined in RFC7517 that also private keys can be exported. Also other libraries do that.

anakinj · 2020-09-25T04:42:40Z

Hi @phlegx

Think that it would be a nice feature.

As mentioned before the main reason for it was to avoid accidental leaking of the private key, so still think that including the private key in the export should not be the default behaviour. In other words the private key should only be included when asked for.

anakinj · 2020-09-25T04:44:16Z

@richardlarocque Looks good.

phlegx · 2020-09-25T08:03:31Z

@anakinj the RFC describes JWK with:

A JSON Web Key (JWK) is a JavaScript Object Notation (JSON) data structure that represents a cryptographic key.

So, JWK is only a data structure that represents a cryptographic key. How a user expose a JWK to a web endpoint is not part of the RFC7517 and should not affect the behavior or structure of a JWK.

anakinj · 2020-09-25T08:25:15Z

The need until now has been focused on the public parts of the keys (signature verification and exports in tests to simulate public JWK endpoints). So therefore the private parts have been totally left out.

Think that if we would support exporting of the private keys we should protect the uneducated user from leaking sensitive information and require the user to actually ask for the private parts to be included.

Made a quick proposal on how the private parts of the key could be included in the export #373

phlegx · 2020-09-25T08:31:09Z

@anakinj it is clear for me thx! It is a good compromise for me to add an option if private key should be exported or not. We should focus to add this feature to RSA, EC and HMAC and if possible release a new version.

phlegx · 2020-09-28T12:39:31Z

@richardlarocque I suggest to merge master branch to your PR, that includes the abstract class. Then you can adapt your PR to the abstract structure and the include_private option.

Merges in upstream changes to JWK code that occurred while jwt#371 was under review.

richardlarocque · 2020-09-30T17:43:54Z

Merged in upstream changes to fix conflicts. Please take another look and merge if desired. (I don't have permission to hit the "merge" button, even if there is an approving review.)

phlegx · 2020-09-30T20:35:11Z

lib/jwt/jwk/ec.rb

+
+module JWT
+  module JWK
+    class EC


You have to inherit from abstract class here:

class EC < KeyAbstract

phlegx · 2020-09-30T20:35:54Z

lib/jwt/jwk/ec.rb

+      attr_reader :keypair
+      attr_reader :kid


This can be removed, because already included from abstract class.

# remove this two lines. attr_reader :keypair attr_reader :kid

phlegx · 2020-09-30T20:36:52Z

lib/jwt/jwk/ec.rb

+      def initialize(keypair, kid = nil)
+        raise ArgumentError, 'keypair must be of type OpenSSL::PKey::EC' unless keypair.is_a?(OpenSSL::PKey::EC)
+
+        @keypair = keypair


Remove the line @keypair = keypair and add super instead.

phlegx · 2020-09-30T20:38:11Z

lib/jwt/jwk/ec.rb

+        @kid = kid || generate_kid(@keypair)
+      end
+
+      def export


Add parameter options like: def export(options = {})

phlegx

Can you please implement methods private? and public_key like JWK HMAC and RSA (PR #373) does?
Or see how RSA JWK implements this methods:

ruby-jwt/lib/jwt/jwk/rsa.rb

Line 17 in 9b5d19f

def private?

phlegx · 2020-09-30T20:39:27Z

lib/jwt/jwk/ec.rb

+
+      def export
+        crv, x_octets, y_octets = keypair_components(keypair)
+        {


Export private key if option include_private is set to true like HMAC or RSA JWK does.

richardlarocque · 2020-10-06T00:54:27Z

Sorry, I got distracted with other things and ended up not paying much attention to this PR.

I find that abstract classes are not very common in Ruby. There's no need to define stub methods that throw exceptions when they're not overridden; that's what NoMethodError is for. Ruby provides no compile-time enforcement of interfaces, so the best way to prove that two classes implement the same interface is with generic unit tests. If you need to share functionality between classes, that's usually done with mixins. In other words, the usual arguments for an abstract class apply as well in Ruby. (At least, that's my interpretation of https://www.poodr.com/, which is my preferred reference on Ruby style.)

But I'm not the one who has to live with this codebase. I'll defer to the maintainers on this one.

@excpt, @anakinj, do you have opinions on @phlegx's suggestions? I'm happy to implement it however you prefer.

anakinj · 2020-10-06T04:50:27Z

I somewhat agree about the empty stubs. The naming is always hard, maybe in Ruby base classes are more common than abstracts.

But then again something that would bind these different types of keys together would be of benefit. Had a thought when this was discussed was that the list of supported types could be generated using something shared between these classes. Is that shared thing a base class or mixin i have no strong opinion on.

anakinj · 2020-10-09T07:23:17Z

Added something small to the thing I'm currently working on in regards to the Base class for the different key types.
89247c4

And as @richardlarocque suggested I also removed the empty method stubs.

phlegx · 2020-10-14T19:53:16Z

@richardlarocque can you please adapt your code to @anakinj base class and solve review issues?

richardlarocque · 2020-10-16T22:28:01Z

I have reservations about rebasing on top of an uncommitted branch.

My goal in opening this PR was to get this functionality into the ruby-jwt upstream. I built this PR so that it fit the style of the project as it existed at the time, so that it would be easy to merge. My preference is that we merge this more or less as it is, in conformance with the old style.

If this project is in the middle of a refactor, and as a result we can't accept the PR as-is right now, that's OK. I just had bad timing, I guess. But I'd rather not get involved with the refactoring myself. I'd prefer to wait until that refactoring lands in master and then rebase my change against that.

I don't want this feature's fate to be tied to that of some refactoring work. I'd like to keep the two things separate.

I am willing to add support for the include_private flag on export and adding inheritance from KeyAbstract, though. That stuff is in master, so I'd be OK with making this PR conform to those standards, if that is sufficient to get this PR merged.

d11wtq · 2020-10-27T06:13:47Z

Just a comment to say thank you so much for doing this. We need this functionality too and exploring the alternative JWT gems, I didn't like what I saw in some of the solutions. Would love to see this one get across the line. I'm happy to help in any way I can do too ❤️

phlegx · 2020-10-27T07:19:29Z

@richardlarocque all other PR have been merged and are in master. Can you please adapt your code style and options to RSA and HMAC? Thank you so much.

phlegx · 2020-11-06T11:30:57Z

@anakinj @excpt should I open a new PR with this code changes, so that we can continue?

anakinj · 2020-11-07T15:34:40Z

@richardlarocque do you mind someone continue with this to get it into the next release or are you planning on continuing on this on your own?

Would be really great to get this into master.

richardlarocque · 2020-11-18T19:31:42Z

Sorry I'm late to the follow-up! I can try to rebase in the next few days, but if one of you wants to beat me to it, feel free.

Adds support for JWKs with "kty" value "EC". This commit includes support for curves ("crv") "P-256", "P-384", "P-521". For additional details on these JWKs and their contents, see https://tools.ietf.org/html/rfc7518#section-6.2.1. This implementation adheres closely to the pattern set by `JWT::JWK::RSA`. It keeps the same coding style and method names. It also inherits a few quirks: - It ignores any private key ("d") values when importing JWKs. - It never emits private key ("d") values when exporting JWKs. - An import followed by an export does not preserve the "kid" value. These behaviors seem strange to me, but I worry that changing the convention would be surprising and potentially dangerous to existing users of this library.

Updates the implementation of JWT:JWK::EC so it will import and export custom "kid" values. See also jwt#320, which proposes doing the same for JWT::JWK::RSA.

sourcelevel-bot · 2020-11-30T17:00:07Z

lib/jwt/jwk/ec.rb

+      def initialize(keypair, kid = nil)
+        raise ArgumentError, 'keypair must be of type OpenSSL::PKey::EC' unless keypair.is_a?(OpenSSL::PKey::EC)
+
+        kid = kid || generate_kid(keypair)


Use self-assignment shorthand ||=.

Updates `JWT::JWK::EC` to extend `KeyBase` and support other conventions like the `JWT::JWK::KTYS` array. Adds support for `#export(include_private: true)` to `JWT::JWK::EC` and updates tests to match the new behavior.

phlegx · 2020-12-01T08:33:28Z

Nice work @richardlarocque! Thx

anakinj · 2020-12-01T09:41:25Z

Thanks @richardlarocque for the contribution and patience.

phlegx · 2020-12-01T21:28:22Z

@anakinj is a release planned?

anakinj · 2020-12-04T06:28:46Z

@phlegx I would like to get the version in a shape that it would work with Ruby 2.1 and 2.2 as the .gemspec claims. Then the version could be the last version on the 2.x branch.

There is a few PRs open that addresses this issue.

@excpt hope you agree :)

sourcelevel-bot bot reviewed Sep 18, 2020

View reviewed changes

lib/jwt/jwk/ec.rb Outdated Show resolved Hide resolved

richardlarocque force-pushed the richardlarocque/ec_jwk_upstreamable branch from 6346031 to 64bc94d Compare September 19, 2020 00:35

anakinj reviewed Sep 22, 2020

View reviewed changes

lib/jwt/jwk/ec.rb Outdated Show resolved Hide resolved

sourcelevel-bot bot reviewed Sep 23, 2020

View reviewed changes

richardlarocque force-pushed the richardlarocque/ec_jwk_upstreamable branch from 6b6895e to 7562cdf Compare September 23, 2020 18:29

anakinj reviewed Sep 24, 2020

View reviewed changes

phlegx mentioned this pull request Sep 24, 2020

Add support for JWKs with HMAC key type. #372

Merged

anakinj approved these changes Sep 25, 2020

View reviewed changes

richardlarocque added a commit to getoutreach/ruby-jwt that referenced this pull request Sep 30, 2020

Merge upstream changes into EC JWK branch

5161ef1

Merges in upstream changes to JWK code that occurred while jwt#371 was under review.

phlegx reviewed Sep 30, 2020

View reviewed changes

excpt self-requested a review October 5, 2020 12:42

excpt approved these changes Oct 5, 2020

View reviewed changes

richardlarocque and others added 4 commits November 30, 2020 07:08

Raise JWT::JWKError on invalid 'crv'

6672386

Add support for kid in JWT::JWK::EC

5a629a6

Updates the implementation of JWT:JWK::EC so it will import and export custom "kid" values. See also jwt#320, which proposes doing the same for JWT::JWK::RSA.

Raise JWT::JWKError on unsupported curve

fce0cd7

richardlarocque force-pushed the richardlarocque/ec_jwk_upstreamable branch 2 times, most recently from 50a1500 to 17967f1 Compare November 30, 2020 16:59

sourcelevel-bot bot reviewed Nov 30, 2020

View reviewed changes

Rebase JWT::JWK::EC and add private key export

162f256

Updates `JWT::JWK::EC` to extend `KeyBase` and support other conventions like the `JWT::JWK::KTYS` array. Adds support for `#export(include_private: true)` to `JWT::JWK::EC` and updates tests to match the new behavior.

richardlarocque force-pushed the richardlarocque/ec_jwk_upstreamable branch from 17967f1 to 162f256 Compare November 30, 2020 17:05

anakinj merged commit 752ee22 into jwt:master Dec 1, 2020

Add support for JWKs with EC key type #371

Add support for JWKs with EC key type #371

Conversation

richardlarocque commented Sep 18, 2020

anakinj commented Sep 22, 2020

sourcelevel-bot bot Sep 23, 2020

Choose a reason for hiding this comment

sourcelevel-bot bot Sep 23, 2020

Choose a reason for hiding this comment

sourcelevel-bot bot Sep 23, 2020

Choose a reason for hiding this comment

anakinj Sep 24, 2020

Choose a reason for hiding this comment

richardlarocque Sep 24, 2020

Choose a reason for hiding this comment

phlegx commented Sep 24, 2020

anakinj commented Sep 25, 2020

anakinj commented Sep 25, 2020

phlegx commented Sep 25, 2020

anakinj commented Sep 25, 2020

phlegx commented Sep 25, 2020

phlegx commented Sep 28, 2020

richardlarocque commented Sep 30, 2020

phlegx Sep 30, 2020

Choose a reason for hiding this comment

phlegx Sep 30, 2020 • edited

Choose a reason for hiding this comment

phlegx Sep 30, 2020 • edited

Choose a reason for hiding this comment

phlegx Sep 30, 2020

Choose a reason for hiding this comment

phlegx left a comment • edited

Choose a reason for hiding this comment

phlegx Sep 30, 2020 • edited

Choose a reason for hiding this comment

richardlarocque commented Oct 6, 2020

anakinj commented Oct 6, 2020

anakinj commented Oct 9, 2020

phlegx commented Oct 14, 2020

richardlarocque commented Oct 16, 2020

d11wtq commented Oct 27, 2020

phlegx commented Oct 27, 2020 • edited

phlegx commented Nov 6, 2020

anakinj commented Nov 7, 2020

richardlarocque commented Nov 18, 2020

sourcelevel-bot bot Nov 30, 2020

Choose a reason for hiding this comment

phlegx commented Dec 1, 2020

anakinj commented Dec 1, 2020

phlegx commented Dec 1, 2020

anakinj commented Dec 4, 2020

phlegx Sep 30, 2020 •

edited

phlegx Sep 30, 2020 •

edited

phlegx left a comment •

edited

phlegx Sep 30, 2020 •

edited

phlegx commented Oct 27, 2020 •

edited