crypto: ecdh - generate public key when setting private key and more #3511
Conversation
|
Added ECDH.validKeyPair (and tests) since it's useful to be able to validate a key pair that was stored previously and used again. I added this as part of the public API, instead of just validating the key pair at the end of ECDH.setPrivateKey, since setPublicKey is still part of the public API and since having the ability to validate a key pair is also tangentially useful for allowing the creation and validation of EC key pairs in general using only the crypto module (a nice little side feature). Also, marked ECDH.setPublicKey as deprecated in the docs since it's inclusion as part of the API is not useful. Either a previously stored private key should be set, which automatically generates the associated public key, or generateKeys should be called. The main drawback of ECDH.setPublicKey is that it can be used to put the key pair into an inconsistent state. Updated docs and tests accordingly and squashed everything into one commit.. |
jasnell
added
the
semver-minor
|
Sorry, looks like this fell by the wayside. Can you rebase against master? I'll do a quick review. /cc @nodejs/crypto |
| @@ -4624,6 +4620,26 @@ void ECDH::SetPrivateKey(const FunctionCallbackInfo<Value>& args) { | |||
| if (!result) { | |||
| return env->ThrowError("Failed to convert BN to a private key"); | |||
| } | |||
|
|
|||
| const BIGNUM* privKey = EC_KEY_get0_private_key(ecdh->key_); | |||
There was a problem hiding this comment.
Style issue: priv_key (but maybe prefer private_key.)
|
@bnoordhuis Thanks for the review. I've rebased, incorporated changes for all of your feedback, and squashed it all into a new commit. About the one naming style issue, I went with "priv_key" to avoid a long line lint caused by using the name "private_key". About the EC_KEY_check_key error stack side-effect possibility, thanks for catching that! That caught me by surprise. That wasn't mentioned in the library doc that I read. But, looking that the OpenSSL source, you're totally correct. Let me know if you want the error stack handled differently than I did. I could have used ClearErrorOnReturn or ERR_clear_error, but went with ERR_get_error to try to do as little as possible. Yes, that hex pub key passed the linter and it still does :) |
|
Also, I just did a quick dig through some more OpenSSL and noticed that the error stack can also be changed by the following call chains. I'll likely have to make more updates to ECDH::SetPrivateKey because of this. I'll work on that later, just noting as something todo for this PR.
|
|
Just added a commit to add ClearErrorOnReturn to handle any errors being put on the openssl error stack within ECDH::SetPrivateKey. Handling this way seemed to fit the pattern for how other code in the file was already handling similar situations. Note: ECDH::BufferToPoint could use a similar change to handle anything that it leaves on the openssl error stack too. I did not touch that code since I didn't want to grow my commit with tangential changes. |
|
@shigeki @indutny @bnoordhuis ... ping |
| This allows you to only store and provide the private part of the EC key pair. | ||
| To support verifying that the private key used is valid for the curve, the | ||
| method `ECDH.validKeyPair` was added. `ECDH.setPublicKey` was marked as | ||
| deprecated since it's inclusion as part of the API is not useful. Either a |
|
@mruddy Can you rebase against master? I'd like at least one other member of @nodejs/crypto to take a look at this. |
|
@bnoordhuis Thanks for the useful feedback! I rebased again and made the updates that were mentioned. Thanks for the tip on ERR_set_mark + ERR_pop_to_mark. Those capture more what I wanted to do. I created MarkPopErrorOnReturn (using those two funcs) and use it from the two places where I needed to cleanup the OpenSSL error stack now. |
|
Sorry for being late. Ben deeply takes care of reviewing code so no need to change it but I have three comments for this API change.
|
|
@jasnell Can we run the CI again? I looked through the failure logs and they were only for the ARM builds and only logged "TIMEOUT". I'm thinking maybe it was a build system problem, but can't be sure. 2 of the 3 failures name a script that I didn't touch for this PR and that doesn't appear to use my changes (directly at least). Here are the log messages that I found:
|
|
I filed #3881 for the failing tests, I agree they're unrelated flakes. |
|
To answer @shigeki:
One other thing I was aiming to fix with these changes is that right now a secret can be computed with a private key value of zero, which is invalid and not good. |
|
|
Personally, I'm fine with the "soft" deprecation but printing the warning would likely be the best option. Would ask how other @nodejs/ctc members feel tho. |
|
@shigeki No, I'm not planning to submit a PR for the non-EC DH API. Also, I just added another commit that adds the ECDH::IsKeyValidForCurve check into ECDH::SetPrivateKey. I also updated the unit tests accordingly. I left it as a separate commit for easy diff. |
|
@mruddy Thanks. Checking a private kye by @bnoordhuis One more commit is added after your review. PTAL. @jasnell I agree it is better to add warning. The api is shared with DH api so that I will work it with DH api change later. |
|
@shigeki @bnoordhuis I'm just working on the latest updates based on your feedback (and a couple minor things I noticed too). Looking at my changes, now that we check the validity of the private key and generate the public key in ECDH::SetPrivateKey, I'm leaning towards removing my addition of ECDH.isValid. Before I put together a commit with that removed, do you agree? I'm thinking that adding ECDH.isValid really will only help people check that ECDH.setPublicKey was not used to put the pair into an inconsistent state. ECDH.setPublicKey is being deprecated and the docs explain why. Plus the public key doesn't matter for the local secret computation. So, after ECDH.setPublicKey finally goes away, I don't see any reason for having added ECDH.isValid to the API. Plus it'll make this change set smaller. |
|
Last CI build was some Jenkins failure soon after it was kicked it off. |
|
Build system failed again. |
|
FWIW CI is all green except some strange issue with |
|
CI, once more with feeling: https://ci.nodejs.org/job/node-test-pull-request/929/ |
|
Only red failure on Windows is known-flaky ARM failure is a CI failure. Everything else is green. TL;DR: CI looks good. |
|
Thanks for the extra eyes on the CI failures. I agree that the failures are not caused by this PR's changes. |
These changes simplify using ECDH with private keys that are not dynamically generated with ECDH.generateKeys. Support for computing the public key corresponding to the given private key was added. Validity checks to reduce the possibility of computing a weak or invalid shared secret were also added. Finally, ECDH.setPublicKey was softly deprecated. PR-URL: #3511 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
|
Thanks Michael, landed in 322b36c. |
These changes simplify using ECDH with private keys that are not dynamically generated with ECDH.generateKeys. Support for computing the public key corresponding to the given private key was added. Validity checks to reduce the possibility of computing a weak or invalid shared secret were also added. Finally, ECDH.setPublicKey was softly deprecated. PR-URL: #3511 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
Notable changes:
* build:
- Add support for Intel's VTune JIT profiling when compiled with
--enable-vtune-profiling. For more information about VTune, see
https://software.intel.com/en-us/node/544211. (Chunyang Dai) #3785.
- Properly enable V8 snapshots by default. Due to a configuration
error, snapshots have been kept off by default when the intention
is for the feature to be enabled. (Fedor Indutny) #3962.
* crypto:
- Simplify use of ECDH (Elliptic Curve Diffie-Hellman) objects
(created via crypto.createECDH(curve_name)) with private keys that
are not dynamically generated via generateKeys(). The public key
is now computed when explicitly setting a private key. Added
validity checks to reduce the possibility of computing weak or
invalid shared secrets. Also, deprecated the setPublicKey() method
for ECDH objects as its usage is unnecessary and can lead to
inconsistent state. (Michael Ruddy) #3511.
- Update root certificates from the current list stored maintained
by Mozilla NSS. (Ben Noordhuis) #3951.
- Multiple CA certificates can now be passed with the ca option to
TLS methods as an array of strings or in a single new-line
separated string. (Ben Noordhuis) #4099
* tools: Include a tick processor in core, exposed via the
--prof-process command-line argument which can be used to process V8
profiling output files generated when using the --prof command-line
argument. (Matt Loring) #4021.
PR-URL: #4181
Notable changes:
* build:
- Add support for Intel's VTune JIT profiling when compiled with
--enable-vtune-profiling. For more information about VTune, see
https://software.intel.com/en-us/node/544211. (Chunyang Dai) #3785.
- Properly enable V8 snapshots by default. Due to a configuration
error, snapshots have been kept off by default when the intention
is for the feature to be enabled. (Fedor Indutny) #3962.
* crypto:
- Simplify use of ECDH (Elliptic Curve Diffie-Hellman) objects
(created via crypto.createECDH(curve_name)) with private keys that
are not dynamically generated via generateKeys(). The public key
is now computed when explicitly setting a private key. Added
validity checks to reduce the possibility of computing weak or
invalid shared secrets. Also, deprecated the setPublicKey() method
for ECDH objects as its usage is unnecessary and can lead to
inconsistent state. (Michael Ruddy) #3511.
- Update root certificates from the current list stored maintained
by Mozilla NSS. (Ben Noordhuis) #3951.
- Multiple CA certificates can now be passed with the ca option to
TLS methods as an array of strings or in a single new-line
separated string. (Ben Noordhuis) #4099
* tools: Include a tick processor in core, exposed via the
--prof-process command-line argument which can be used to process V8
profiling output files generated when using the --prof command-line
argument. (Matt Loring) #4021.
PR-URL: #4181
These changes simplify using ECDH with private keys that are not dynamically generated with ECDH.generateKeys. Support for computing the public key corresponding to the given private key was added. Validity checks to reduce the possibility of computing a weak or invalid shared secret were also added. Finally, ECDH.setPublicKey was softly deprecated. PR-URL: nodejs#3511 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
Notable changes:
* build:
- Add support for Intel's VTune JIT profiling when compiled with
--enable-vtune-profiling. For more information about VTune, see
https://software.intel.com/en-us/node/544211. (Chunyang Dai) nodejs#3785.
- Properly enable V8 snapshots by default. Due to a configuration
error, snapshots have been kept off by default when the intention
is for the feature to be enabled. (Fedor Indutny) nodejs#3962.
* crypto:
- Simplify use of ECDH (Elliptic Curve Diffie-Hellman) objects
(created via crypto.createECDH(curve_name)) with private keys that
are not dynamically generated via generateKeys(). The public key
is now computed when explicitly setting a private key. Added
validity checks to reduce the possibility of computing weak or
invalid shared secrets. Also, deprecated the setPublicKey() method
for ECDH objects as its usage is unnecessary and can lead to
inconsistent state. (Michael Ruddy) nodejs#3511.
- Update root certificates from the current list stored maintained
by Mozilla NSS. (Ben Noordhuis) nodejs#3951.
- Multiple CA certificates can now be passed with the ca option to
TLS methods as an array of strings or in a single new-line
separated string. (Ben Noordhuis) nodejs#4099
* tools: Include a tick processor in core, exposed via the
--prof-process command-line argument which can be used to process V8
profiling output files generated when using the --prof command-line
argument. (Matt Loring) nodejs#4021.
PR-URL: nodejs#4181
The ECDH API changes were made more than six years ago and this section is not helpful for new applications. The behavior of the ECDH APIs should be explained in the relevant sections, not in a note. Refs: nodejs#3511
The ECDH API changes were made more than six years ago and this section is not helpful for new applications. The behavior of the ECDH APIs should be explained in the relevant sections, not in a note. Refs: #3511 PR-URL: #41773 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
The ECDH API changes were made more than six years ago and this section is not helpful for new applications. The behavior of the ECDH APIs should be explained in the relevant sections, not in a note. Refs: #3511 PR-URL: #41773 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
The ECDH API changes were made more than six years ago and this section is not helpful for new applications. The behavior of the ECDH APIs should be explained in the relevant sections, not in a note. Refs: #3511 PR-URL: #41773 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
The ECDH API changes were made more than six years ago and this section is not helpful for new applications. The behavior of the ECDH APIs should be explained in the relevant sections, not in a note. Refs: #3511 PR-URL: #41773 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
The ECDH API changes were made more than six years ago and this section is not helpful for new applications. The behavior of the ECDH APIs should be explained in the relevant sections, not in a note. Refs: #3511 PR-URL: #41773 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
crypto: ecdh updates to support using previously known private keys
This pull request shares many similarities with #1020.
I decided not to add the public key generation as a separate method like was proposed in #1020.
Rather I made it part of setting the private key.
I liked this way more because then it leaves two main ways to use the class. Either generate the keys, or set the private key then compute the shared secret.
Originally I started looking at this code because the following error was annoying and unnecessary (and is why I removed the generated_ member from ECDH):