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

Instantiate both the RSA private and public keys when setting parameters #82

Merged
merged 1 commit into from
Mar 10, 2016

Conversation

philr
Copy link
Contributor

@philr philr commented Mar 8, 2016

The RSA e (exponent) and n (modulus) parameters are initially backed by the rsa_e and rsa_n fields. These fields are then used to instantiate the internal privateKey and publicKey objects in generatePrivateKeyIfParams() and generatePublicKeyIfParams(). However, both rsa_e and rsa_n are set to null after use. This means that once either of the private or public keys has been created, the other cannot be. Consequently, the RSA object can only be used for private or public functions, but not for both.

For example:

orig = OpenSSL::PKey::RSA.generate(2048)
copy = OpenSSL::PKey::RSA.new
[:e, :n, :d, :p, :q, :iqmp, :dmp1, :dmq1].each do |param|
  copy.send("#{param}=", orig.send(param))
end

# e and n were set first, copy will now act only as a public key

# public_encrypt will succeed
copy.public_encrypt('Test')

# private_encrypt will fail with an 'OpenSSL::PKey::RSAError: private key needed' exception
copy.private_encrypt('Test')

When the above code is run using the MRI standard library, both the public_encrypt and private_encrypt calls succeed.

This pull request modifies generatePrivateKeyIfParams() and generatePublicKeyIfParams() so that they will obtain the e and n parameters from either privateKey or publicKey if rsa_e and rsa_n have already been consumed.

@philr
Copy link
Contributor Author

philr commented Mar 9, 2016

The build failure seems to be unrelated to this change (the same job was failing with the same error before).

@kares
Copy link
Member

kares commented Mar 9, 2016

Thanks Phil, this is looking good. we probably do not need those private methods to be synchronized.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
The rsa_n and rsa_e parameter fields are used to instantiate both the
private and public keys, but are nulled after use in
generatePrivateKeyIfParams() and generatePublicKeyIfParams(). This meant
that once one of the keys had been created, the other could not be. The
RSA object could not then be used for both private and public functions.

If a private or public key has already been instantiated, use the key as
the source for the n and e parameters in generatePrivateKeyIfParams()
and generatePublicKeyIfParams().
@philr philr force-pushed the rsa_priv_and_pub_from_params branch from aa15924 to c106648 Compare March 9, 2016 22:16
@philr
Copy link
Contributor Author

philr commented Mar 9, 2016

@kares I have amended my commit to remove synchronized from the getPublicExponent() and getModulus() private methods.

Travis CI has done a new build. However, this time the jobs have all failed with an unrelated issue initializing the build environment.

@kares
Copy link
Member

kares commented Mar 10, 2016

yep, restarted the build and all seems well, thank you for your effort.

kares added a commit that referenced this pull request Mar 10, 2016
Instantiate both the RSA private and public keys when setting parameters
@kares kares merged commit b0cffd6 into jruby:master Mar 10, 2016
@philr philr deleted the rsa_priv_and_pub_from_params branch March 12, 2016 20:37
philr added a commit to philr/putty-key that referenced this pull request Mar 28, 2016
DSA/DSS keys will be supported once
jruby/jruby-openssl#82 is released.

RSA keys will be instantiated correctly once
jruby/jruby-openssl#83 is released.
@kares kares added this to the 0.9.17 milestone Jun 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants