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

Support exporting RSA JWK private keys #375

Merged
merged 3 commits into from Oct 15, 2020
Merged

Conversation

@anakinj
Copy link
Member

@anakinj anakinj commented Oct 1, 2020

This replaces and improves #373

Created a new branch and combined the following:

  • The fix from #320 (merged the branch @rkmetzl had made into this)
  • The proposal I made in #373

Still not totally sure if the test matrix is able to test both versions of the populate_key method. The original issue was fixed in #333 but before that there were no failing tests so I don't think the tests cover the older versions. Maybe we don't need to support the outdated rubies anyway soon.

FYI @richardlarocque, @phlegx This includes fixes for the great comments you provided.

lib/jwt/jwk/rsa.rb Outdated Show resolved Hide resolved
@anakinj anakinj force-pushed the anakinj:rsa-jwk-improvements branch from 63f1a4a to 7055538 Oct 2, 2020
Copy link
Contributor

@phlegx phlegx left a comment

Please see my request changes. See my PR #377. I have adapted my code to your code style (export variable name and generate_kid without param).

@@ -2,16 +2,18 @@

module JWT
module JWK
class RSA
class RSA < KeyAbstract
attr_reader :keypair

This comment has been minimized.

@phlegx

phlegx Oct 2, 2020
Contributor

attr_reader :keypair can be removed, because inherited from KeyAbstract.

sequence = OpenSSL::ASN1::Sequence([OpenSSL::ASN1::Integer.new(public_key.n),
OpenSSL::ASN1::Integer.new(public_key.e)])
OpenSSL::Digest::SHA256.hexdigest(sequence.to_der)
@kid ||= generate_kid

This comment has been minimized.

@phlegx

phlegx Oct 2, 2020
Contributor

You can remove this method def kid and move @kid ||= generate_kid to initialize method like in https://github.com/jwt/ruby-jwt/blob/master/lib/jwt/jwk/hmac.rb

This comment has been minimized.

@anakinj

anakinj Oct 2, 2020
Author Member

This is a little problematic. I would like to do it so the @kid would only be set once. Doing it after super would potentially set the kid twice in the initializer.

I would like to do it like this but the keypair that generate_kid needs is not initialized yet.

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

A litte ugly solution but would work is to have the kid generation as a static method that gets the public_key as a parameter. I think I'll go with that.

This comment has been minimized.

@anakinj

anakinj Oct 2, 2020
Author Member

Did something in ec9a4c7

This comment has been minimized.

@phlegx

phlegx Oct 2, 2020
Contributor

"Doing it after super would potentially set the kid twice in the initializer.", at the moment I don't understand why, but you have a better overview of the JWK RSA code flow.

My opinion: a developer can set the kid manually on JWK creation or leave this parameter blank and it gets generated automatically. And import uses the kid to have the same kid from export. I cannot see the problem with set kid twice.

This comment has been minimized.

@anakinj

anakinj Oct 2, 2020
Author Member

I meant in cases where the kid is not given. Then the super will set the @kid instance variable to nil and then the derived class will set @kid to the generated kid.

What i changed this to give the generated kid to the parent if no kid is provided to the derived class, then @kid is only set once in the super.

This comment has been minimized.

@phlegx

phlegx Oct 2, 2020
Contributor

Is it a problem, if kid is set to nil first? I see no problem with that and the code looks much simpler to understand.

This comment has been minimized.

@anakinj

anakinj Oct 2, 2020
Author Member

I feel that im starting to over analyse this, sorry for that :) But when the kid is set from outside the parent it could to be set via attribute writer. Maybe @kid should not be used from the derived classes at all but the kid accessors instead.

This comment has been minimized.

@phlegx

phlegx Oct 2, 2020
Contributor

Yep! That's true. Then we remove @kid = kid from initialize method in KeyAbstract class after PR are merged? Or can you remove it with this PR please?

E.g.:

      def initialize(keypair, _kid = nil)
        @keypair = keypair
      end

This comment has been minimized.

@anakinj

anakinj Oct 2, 2020
Author Member

I was maybe more referring to not setting @kid directly from the RSA, EC or HMAC classes but via some accessor.

What about something like this 6754a58

This comment has been minimized.

@phlegx

phlegx Oct 2, 2020
Contributor

Very nice! I agree.


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

@keypair = keypair

This comment has been minimized.

@phlegx

phlegx Oct 2, 2020
Contributor

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

lib/jwt/jwk/rsa.rb Outdated Show resolved Hide resolved
@anakinj anakinj force-pushed the anakinj:rsa-jwk-improvements branch from a3843e3 to ec9a4c7 Oct 2, 2020
@excpt
Copy link
Member

@excpt excpt commented Oct 2, 2020

lib/jwt/jwk/rsa.rb Outdated Show resolved Hide resolved
lib/jwt/jwk/key_abstract.rb Outdated Show resolved Hide resolved
@anakinj anakinj force-pushed the anakinj:rsa-jwk-improvements branch from 840a1e8 to 7edbec0 Oct 2, 2020
@sourcelevel-bot
Copy link

@sourcelevel-bot sourcelevel-bot bot commented Oct 2, 2020

SourceLevel has finished reviewing this Pull Request and has found:

  • 1 fixed issue! 🎉

See more details about this review.

@anakinj anakinj force-pushed the anakinj:rsa-jwk-improvements branch from 7edbec0 to 6754a58 Oct 2, 2020
@excpt excpt self-requested a review Oct 5, 2020
@excpt
excpt approved these changes Oct 5, 2020
Copy link
Member

@excpt excpt left a comment

Bonus points for the documentation! 👍

@phlegx
phlegx approved these changes Oct 5, 2020
@phlegx
Copy link
Contributor

@phlegx phlegx commented Oct 6, 2020

Where is authorized to merge this PR?

@anakinj anakinj force-pushed the anakinj:rsa-jwk-improvements branch from 6754a58 to f44445b Oct 9, 2020
@anakinj
Copy link
Member Author

@anakinj anakinj commented Oct 13, 2020

Could you @excpt retrigger the CI for this.

@excpt
Copy link
Member

@excpt excpt commented Oct 13, 2020

Build restarted. :-)

@phlegx
Copy link
Contributor

@phlegx phlegx commented Oct 14, 2020

@anakinj do you need help to get this merged?

@anakinj anakinj merged commit e6c1487 into jwt:master Oct 15, 2020
2 of 3 checks passed
2 of 3 checks passed
codeclimate 1 issue to fix
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sourcelevel-bot
sourcelevel SourceLevel has found 1 fixed issue.
Details
@tt
Copy link

@tt tt commented Mar 3, 2021

@anakinj, are there any plans to cut a new release with these changes or will it only happen when the milestone is complete?

I'm specifically interested in the kid change included in 876f6cd (from #320) so if there are no immediate plans to push a new version, I can pin to a recent commit instead.

@anakinj
Copy link
Member Author

@anakinj anakinj commented Mar 3, 2021

I think now would be a good time to cut a release. There are no known blockers or regressions anymore. Just merged a fix for a missing require fixing #392.

@excpt what do you think?

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

Successfully merging this pull request may close these issues.

None yet

5 participants