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

crypto: allow password protected private keys with publicEncrypt #626

Closed

Conversation

calvinmetcalf
Copy link
Contributor

Private keys may be used along with publicEncrypt since the private key includes
the public one. This adds the ability to use encrypted private keys which
previously threw an error.

@vkurchatkin vkurchatkin added the crypto Issues and PRs related to the crypto subsystem. label Jan 27, 2015
@vkurchatkin
Copy link
Contributor

it requires some documentation, doesn't it?

@vkurchatkin vkurchatkin added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 27, 2015
@calvinmetcalf
Copy link
Contributor Author

#625 conflicts with this one as it rewrites these methods

@calvinmetcalf
Copy link
Contributor Author

though this is allowing something slightly different

@calvinmetcalf calvinmetcalf force-pushed the password-publicEncrypt branch from 4d9ad0f to 380e4bf Compare January 28, 2015 14:51
@calvinmetcalf
Copy link
Contributor Author

rebased with updates to the docs and I make sure that the functions that get exposed to the user aren't anonymous.

@calvinmetcalf
Copy link
Contributor Author

and I'm geting a jshint failure one sec

function rsaPublic(method, defaultPadding) {
return function(options, buffer) {
function makeRsaPublic(method, defaultPadding) {
return function rsaPublic(options, buffer) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rename makes the diff noisier than it needs to be.

@calvinmetcalf calvinmetcalf force-pushed the password-publicEncrypt branch from 380e4bf to 058deb7 Compare January 28, 2015 14:59
key: rsaKeyPemEncrypted,
passphrase: 'password'
}, encryptedBuffer);
assert.equal(input, decryptedBufferWithPassword.toString());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test that tests decryption with a bad password? What if password is a buffer, an array, etc.?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup can do

@indutny
Copy link
Member

indutny commented Jan 31, 2015

cc @iojs/crypto


crypto.publicDecrypt({
key: rsaKeyPemEncrypted,
passphrase: [].concat.apply([],new Buffer('password'))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tiny style nit: space after comma.

@bnoordhuis
Copy link
Member

LGTM assuming tests pass. I'm still not quite clear on why you renamed the methods.

@calvinmetcalf
Copy link
Contributor Author

Oh sorry, I renamed them because I gave a name to the inner function and it
made sense to give it the name the outer function used.

On Sat, Jan 31, 2015, 5:12 AM Ben Noordhuis notifications@github.com
wrote:

LGTM assuming tests pass. I'm still not quite clear on why you renamed the
methods.


Reply to this email directly or view it on GitHub
#626 (comment).

Private keys may be used along with publicEncrypt since the private key includes
the public one.  This adds the ability to use encrypted private keys which
previously threw an error.  This commit also makes sure the user exposed
functions have names.
@calvinmetcalf calvinmetcalf force-pushed the password-publicEncrypt branch from 4a671a6 to 48a71db Compare January 31, 2015 22:03
bnoordhuis pushed a commit that referenced this pull request Feb 2, 2015
Private keys may be used along with publicEncrypt since the private key
includes the public one.  This adds the ability to use encrypted private
keys which previously threw an error.  This commit also makes sure the
user exposed functions have names.

PR-URL: #626
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@bnoordhuis
Copy link
Member

Thanks Calvin, landed in 6561274 with minor changes (reworded/reformatted the commit log and backed out the function renaming, it made the functions show up as [Function: rsaPublic] or [Function: rsaPrivate] in the REPL.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants