crypto: add generatePublicKey to ECDH, fix corner cases #1020
Conversation
ECDH.generatePublicKey can get the public key using the curve and private key. This allows usage of the crypto module where a developer imports a private key, and then generates a public key without needing it stored. Removed the generated_ boolean from the ECDH class, and stopped checking for it with getPrivateKey() and getPublicKey(). This allows you to import public and private keys without having to generate first, which would just rewrite the generated keys regardless. An error message was changed to accurately reflect what the error was.
| @@ -535,6 +535,12 @@ ECDH.prototype.generateKeys = function generateKeys(encoding, format) { | |||
| return this.getPublicKey(encoding, format); | |||
| }; | |||
|
|
|||
| ECDH.prototype.generatePublicKey = function generateKeys(encoding, format) { | |||
There was a problem hiding this comment.
I think the function name here needs to be changed from generateKeys to generatePublicKey
brendanashworth
added
the
semver-minor
Change the function name of generatePublicKey to be generatePublicKey instead of generateKeys.
|
cc'ing @iojs/crypto |
| if (pub == nullptr) | ||
| return env->ThrowError("Failed to allocate EC_POINT for a public key"); | ||
|
|
||
| if(EC_POINT_mul(ecdh->group_, pub, priv, nullptr, nullptr, nullptr) <= 0) { |
There was a problem hiding this comment.
Style: space after 'if' (here and below.)
|
LGTM sans nits. |
| @@ -4062,10 +4063,33 @@ void ECDH::GenerateKeys(const FunctionCallbackInfo<Value>& args) { | |||
|
|
|||
| if (!EC_KEY_generate_key(ecdh->key_)) | |||
| return env->ThrowError("Failed to generate EC_KEY"); | |||
|
|
|||
| ecdh->generated_ = true; | |||
There was a problem hiding this comment.
I mean, perhaps it should be set from GeneratePublicKey too? Or be a flag field with two possible flags: GeneratedPrivate and GeneratedPublic?
There was a problem hiding this comment.
If I have something like the following code sample, getPrivateKey() will return an error if you haven't generated the keys, which is a pain for imported keys. I'd have to generate a keypair, then overwrite the keys (wasting a keypair just to toggle the bool).
var ecdh = crypto.createECDH(curve);
ecdh.setPrivateKey(privateKey);
ecdh.setPublicKey(publicKey);
ecdh.getPrivateKey(); // error
I could also import only the private key then use generatePublicKey to toggle the bool, but the getPrivateKey/getPublicKey functionality with the bool initially can be unexpected behavior.
Perhaps computeSecret could just check for the private key and throw an error based on that?
There was a problem hiding this comment.
Ok, if setPrivateKey will set both Private and Public flag - will that work?
|
Reiterating, @mbullington if you could make the requested changes, I could try and get this merged in soon! |
|
This PR is getting stale. @mbullington Can you update the PR based on the feedback provided? |
|
I'll update the PR based on master and the feedback provided. Thank you for being patient, I'm very sorry if having this PR open for this long caused any inconvenience. |
|
No worries. I just want to make sure you get your contributions in and that they aren't forgotten. :) |
|
@nodejs/crypto Anyone want to update and retarget this? |
|
@mbullington could you make a new PR against |
|
Please see #3511. It's a little different, but solves many of the same things. |
|
Closing, obsolete now that #3511 has landed. |
ECDH.generatePublicKey can get the public key using the curve
and private key. This allows usage of the crypto module where
a developer imports a private key, and then generates a public
key without needing it stored.
Removed the generated_ boolean from the ECDH class, and stopped
checking for it with getPrivateKey() and getPublicKey(). This
allows you to import public and private keys without having to
generate first, which would just rewrite the generated keys
regardless.
An error message was changed to accurately reflect what the error
was.