crypto: ECDH convertKey to convert public keys between different formats #19080

wuweiweiwu · 2018-03-02T01:24:39Z

ECDH#convertKey is used to convert public keys between different formats, as per #18977

ECDH#convertKey(key, curve[, inputEncoding[, outputEncoding[, format]]])

key <string> | <Buffer> | <TypedArray> | <DataView>
curve <string>
inputEncoding <string>
outputEncoding <string>
format <string> can be 'uncompressed', 'compressed', 'hybrid'

Returns the input key converted to the specified encoding and format.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

crypto, doc, test

bnoordhuis · 2018-03-02T16:05:07Z

lib/internal/crypto/diffiehellman.js

+  if (outEnc && outEnc !== 'buffer')
+    convertedKey = convertedKey.toString(outEnc);
+  return convertedKey;
+};


Don't duplicate this logic, factor it out so it can be shared with ECDH#getPublicKey(). Likewise the C++ code.

wuweiweiwu · 2018-03-02T21:11:32Z

lib/internal/crypto/diffiehellman.js

+  return encode(convertedKey, outEnc);
+};
+
+function encode(buffer, encoding) {


@bnoordhuis I also added the encode function since I noticed that the test for 'buffer' encoding happens a lot in this file. Should I also change that in this PR or should I submit another PR for that?

Thanks, doing it in this PR is fine.

bnoordhuis

Mostly LGTM apart from some comments. Thanks for doing this.

bnoordhuis · 2018-03-03T12:49:45Z

lib/internal/crypto/diffiehellman.js

+  return encode(convertedKey, outEnc);
+};
+
+function encode(buffer, encoding) {


Thanks, doing it in this PR is fine.

bnoordhuis · 2018-03-03T12:53:05Z

src/node_crypto.cc

    return nullptr;
  }

  r = EC_POINT_oct2point(
-      group_,
+      group,
      pub,
      reinterpret_cast<unsigned char*>(data),
      len,


Since you're making changes here, you might want to get rid of the goto by moving the fatal: block inside the if statement.

bnoordhuis · 2018-03-03T12:53:44Z

src/node_crypto.cc

@@ -5532,6 +5539,63 @@ void ExportChallenge(const FunctionCallbackInfo<Value>& args) {
  args.GetReturnValue().Set(outString);
 }

+
+// convert public key to compressed, uncompressed, hybrid format


Can you capitalize and punctuate the comments?

bnoordhuis · 2018-03-03T12:58:39Z

src/node_crypto.cc

+                                      Buffer::Data(args[0]),
+                                      len);
+  if (pub == nullptr)
+    return env->ThrowError("Failed to convert Buffer to EC_POINT");


~~Leaks group. group and pub are leaked on lines 5580 and 5587.~~

~~Suggestion: use a std::unique_ptr with a custom deleter or use the std::shared_ptr trick from 8ccd320.~~

bnoordhuis · 2018-03-03T12:59:32Z

src/node_crypto.cc

+  point_conversion_form_t form =
+      static_cast<point_conversion_form_t>(args[2]->Uint32Value());
+
+  size = EC_POINT_point2oct(group, pub, form, nullptr, 0, nullptr);


Minor thing but can you write this as int size = ...?

bnoordhuis · 2018-03-03T13:00:32Z

test/parallel/test-crypto-ecdh-convert-key.js

+const cafebabeKey = 'cafebabe'.repeat(8);
+// Associated compressed and uncompressed public keys (points).
+const cafebabePubPtComp =
+'03672a31bfc59d3f04548ec9b7daeeba2f61814e8ccc40448045007f5479f693a3';


Can you indent these by four spaces? Four because line continuation.

wuweiweiwu · 2018-03-04T02:19:16Z

src/node_crypto.cc

+                                      Buffer::Data(args[0]),
+                                      len);
+
+  std::shared_ptr<void> cleanup(nullptr, [group, pub] (...) {


@bnoordhuis I am not sure if I am using shared_ptr correctly. I created it before the pub === nullptr check so that returns. the delete function will be called

@wuweiweiwu we also have OnScopeLeave in util.h, you could use that if you prefer? It might be a bit more self-documenting?

@addaleax I will look at that! Is there a preference using onScopeLeave v. shared_ptr?

@wuweiweiwu The former is pretty new as an utility -- but basically, use whichever makes sense to you, or lets the code seem more readable. :)

(Also, probably doesn't matter much now that this has landed, unless you want to send another PR right behind this. ¯\_(ツ)_/¯)

Sounds good! I'll go over both so I am more familiar

bnoordhuis

Thanks, LGTM. CI: https://ci.nodejs.org/job/node-test-pull-request/13488/

This PR should probably get one or two more sign-offs because it adds a new API. Also, cc @nodejs/crypto.

wuweiweiwu · 2018-03-04T21:17:31Z

node-test-commit failure looks unrelated

console output

not ok 1331 parallel/test-regress-GH-4948
  ---
  duration_ms: 0.169
  severity: fail
  stack: |-
  ...

tniessen

Well done!

tniessen · 2018-03-04T21:47:05Z

doc/api/crypto.md

@@ -656,6 +656,54 @@ assert.strictEqual(aliceSecret.toString('hex'), bobSecret.toString('hex'));
 // OK
 ```

+### ECDH.convertKey(key, curve[, inputEncoding][, outputEncoding][, format])
+<!-- YAML
+added: v10.0.0


This should be added: REPLACEME (it might be a different version on other release branches, not necessarily v10.0.0).

tniessen · 2018-03-04T21:50:26Z

lib/internal/crypto/diffiehellman.js

@@ -192,6 +181,28 @@ ECDH.prototype.generateKeys = function generateKeys(encoding, format) {
 };

 ECDH.prototype.getPublicKey = function getPublicKey(encoding, format) {
+  var f = getFormat(format);
+  var key = this._handle.getPublicKey(f);


Is there a reason not to use const here? In general, let is preferred over var and const over let if possible.

tniessen · 2018-03-04T21:52:15Z

lib/internal/crypto/diffiehellman.js

+  inEnc = inEnc || encoding;
+  outEnc = outEnc || encoding;
+  var f = getFormat(format);
+  var convertedKey = _ECDHConvertKey(toBuf(key, inEnc), curve, f);


tniessen · 2018-03-04T21:55:05Z

test/parallel/test-crypto-ecdh-convert-key.js

+const assert = require('assert');
+const crypto = require('crypto');
+
+const ECDH = crypto.ECDH;


Nit: I'd prefer destructuring here: const { ECDH } = crypto;, maybe even const { ECDH, getCurves } = require('crypto');.

tniessen · 2018-03-04T21:58:30Z

doc/api/crypto.md

@@ -656,6 +656,54 @@ assert.strictEqual(aliceSecret.toString('hex'), bobSecret.toString('hex'));
 // OK
 ```

+### ECDH.convertKey(key, curve[, inputEncoding][, outputEncoding][, format])


Is this signature correct? According to this signature, all of the following calls would be valid:

ECDH.convertKey(key, curve[, inputEncoding][, outputEncoding][, format]) ECDH.convertKey(key, curve[, inputEncoding][, outputEncoding]) ECDH.convertKey(key, curve[, inputEncoding]) ECDH.convertKey(key, curve) ECDH.convertKey(key, curve[, inputEncoding][, format]) ECDH.convertKey(key, curve[, outputEncoding][, format]) ECDH.convertKey(key, curve[, format])

@tniessen How would I structure the signature?

Function signature?

key and curve are required but the rest are optional. These are the available calls:

ECDH.convertKey(key, curve, inputEncoding, outputEncoding, format) ECDH.convertKey(key, curve, inputEncoding, outputEncoding) ECDH.convertKey(key, curve, inputEncoding) ECDH.convertKey(key, curve)

Would it be: ECDH.convertKey(key, curve[, inputEncoding, outputEncoding, format])?

or just ECDH.convertKey(key, curve, inputEncoding, outputEncoding, format)

I figured it out!

ECDH.convertKey(key, curve[, inputEncoding[, outputEncoding[, format]]])

tniessen · 2018-03-04T21:59:30Z

doc/api/crypto.md

+Example (uncompressing a key):
+
+```js
+const ECDH = require('crypto').ECDH;


Nit: Use destructuring: const { ECDH } = require('crypto');.

tniessen · 2018-03-05T10:35:30Z

test/parallel/test-crypto-ecdh-convert-key.js

+
+const availableCurves = new Set(getCurves());
+
+if (availableCurves.has('secp256k1')) {


Tiny nit: No need for a set, just use if (getCurves().includes('secp256k1')) {.

tniessen · 2018-03-05T10:35:36Z

test/parallel/test-crypto-ecdh-convert-key.js

+const assert = require('assert');
+const crypto = require('crypto');
+
+const { ECDH, getCurves } = crypto;


Tiny nit: This can be combined with the call to require:

const assert = require('assert'); const { ECDH, getCurves } = require('crypto');

jasnell · 2018-03-05T15:45:53Z

src/node_crypto.cc

+
+  CHECK_EQ(args.Length(), 3);
+
+  THROW_AND_RETURN_IF_NOT_BUFFER(args[0], "Public key");


Since this is adding a new API, please handle the input type checking in javascript and use the internal/errors mechanism.

wuweiweiwu · 2018-03-09T18:14:08Z

@jasnell @tniessen @Trott I had to resolve some merge conflicts in diffiehelman.js. Is there anything else that I should do on this PR?

tniessen · 2018-03-10T12:58:25Z

@wuweiweiwu Please rebase your changes on top of master instead of adding a merge commit if there are conflicts. In order to undo the merge commit and to properly rebase your changes, follow these steps.

# This is only necessary if you did not add the upstream remote before.
git remote add upstream https://github.com/nodejs/node.git

# Checkout your branch.
git checkout crypto-convert-key
# Undo the merge commit.
git reset --hard 7a921c56bd19cb2e7fa68862bcfa023fedc817b8
# Make sure your copy of our master branch is up-to-date.
git fetch upstream master
# Rebase on top of our changes.
git rebase upstream/master

git rebase will stop as soon as it encounters a conflict, at which point you will need to follow the instructions on the screen. It is usually a cycle of

using git status to determine which files the conflict occurred in,
manually resolving the conflicts,
telling git that you are done with git add path/to/file/with/conflicts, and
resuming the rebase with git rebase --continue.

ECDH.convertKey is used to convert public keys between different formats. Fixes: nodejs#18977

refactored ECDH::BufferToPoint and key formatting

Adding tests to verify that convertkey correctly converts ECDH keys to different formats

Adding docs and usage examples.

Using std::shared_ptr to clean up pointers. Formatting changes. Refactoring encode function.

Using const over var. Destructuring notation. Updated function signature.

Handle input typechecking in JavaScript using errors. Minor fixes and updated tests

Using the new error throwing mechanism.

wuweiweiwu · 2018-03-10T21:07:51Z

@tniessen Thanks for the tips! I have rebased and also updated my error throwing using the new errors.

tniessen · 2018-03-11T13:07:30Z

Thank you! CI: https://ci.nodejs.org/job/node-test-pull-request/13624/
(By the way, you don't need to follow the commit message guidelines for subsequent commits as they will usually be squashed into a single commit anyway.)

wuweiweiwu · 2018-03-11T13:33:57Z

Not sure why test-process-env-deprecation fails for those test cases

09:16:20 not ok 1280 parallel/test-process-env-deprecation
09:16:20   ---
09:16:20   duration_ms: 0.297
09:16:20   severity: fail
09:16:20   stack: |-
09:16:20     Mismatched <anonymous> function calls. Expected exactly 1, actual 0.
09:16:20         at Object.exports.mustCall (/home/iojs/build/workspace/node-test-commit-linux-containered/nodes/ubuntu1604_sharedlibs_withoutintl_x64/test/common/index.js:427:10)
09:16:20         at expectWarning (/home/iojs/build/workspace/node-test-commit-linux-containered/nodes/ubuntu1604_sharedlibs_withoutintl_x64/test/common/index.js:615:18)
09:16:20         at expectWarningByName (/home/iojs/build/workspace/node-test-commit-linux-containered/nodes/ubuntu1604_sharedlibs_withoutintl_x64/test/common/index.js:629:25)
09:16:20         at Object.exports.expectWarning (/home/iojs/build/workspace/node-test-commit-linux-containered/nodes/ubuntu1604_sharedlibs_withoutintl_x64/test/common/index.js:649:5)
09:16:20         at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux-containered/nodes/ubuntu1604_sharedlibs_withoutintl_x64/test/parallel/test-process-env-deprecation.js:7:8)
09:16:20         at Module._compile (module.js:671:30)
09:16:20         at Object.Module._extensions..js (module.js:682:10)
09:16:20         at Module.load (module.js:582:32)
09:16:20         at tryModuleLoad (module.js:522:12)
09:16:20   ...

It passes locally.

@tniessen Is it something I need to fix?

Trott · 2018-03-13T13:20:36Z

I think the CI problems were on master and not in this PR and that they've been fixed. Let's find out...

CI: https://ci.nodejs.org/job/node-test-pull-request/13656/

wuweiweiwu · 2018-03-13T17:36:37Z

Offending test: sequential/test-inspector-stop-profile-after-done

not ok 537 sequential/test-inspector-stop-profile-after-done
  ---
  duration_ms: 1.201
  severity: fail
  stack: |-
    [test] Connecting to a child Node process
    [test] Testing /json/list
    [err] Debugger listening on ws://127.0.0.1:63629/5d4b460e-4dec-4c63-a4f3-b9675efce5a3
    [err] For help see https://nodejs.org/en/docs/inspector
    [err] 
    [out] {}
    [out] 
    [out] {}
    [out] 
    [out] {}
    [out] 
    [out] {}
    [out] 
    [out] {}
    [out] 
    [out] {}
    [out] 
    [out] {}
    [out] 
    [out] {}
    [out] 
    [out] {}
    [out] 
    [out] {}
    [out] 
    [out] {}
    [out] 
    [err] Debugger attached.
    [err] Waiting for the debugger to disconnect...
    [err] 
    { AssertionError [ERR_ASSERTION]: 3221225477 strictEqual 0
        at runTests (c:\workspace\node-test-binary-windows\test\sequential\test-inspector-stop-profile-after-done.js:28:10)
        at process._tickCallback (internal/process/next_tick.js:114:7)
      generatedMessage: true,
      name: 'AssertionError [ERR_ASSERTION]',
      code: 'ERR_ASSERTION',
      actual: 3221225477,
      expected: 0,
      operator: 'strictEqual' }
    1
  ...

@Trott seems unrelated. But this test was passing for me locally. What should I do?

Trott · 2018-03-13T17:48:37Z

@wuweiweiwu That test failing on Windows in CI is a known issue: #19263

I think this can be treated as "CI has passed" at this point.

Also: "Node.js project needs to get CI back to green, this is ridiculous." But that's not your problem to solve (unless you want to). :-D

Trott · 2018-03-13T17:49:48Z

/ping @nodejs/crypto

wuweiweiwu · 2018-03-22T19:57:07Z

@Trott just checking up on the status of this PR!

tniessen · 2018-03-22T21:07:48Z

@wuweiweiwu I didn't have time to review this thoroughly, but this should be ready to land nevertheless. I will try to review and land on Saturday if no one beats me to it.

One more CI: https://ci.nodejs.org/job/node-test-pull-request/13826/

Trott · 2018-03-22T21:59:03Z

Re-running FreeBSD: https://ci.nodejs.org/job/node-test-commit-freebsd/16379/

Re-running arm-fanned: https://ci.nodejs.org/job/node-test-commit-arm-fanned/15339/

Trott · 2018-03-23T18:29:25Z

Both re-runs of CI were green. This can land.

tniessen · 2018-03-23T18:58:23Z

Landed in f2e0288, thank you @wuweiweiwu! 🎉

ECDH.convertKey is used to convert public keys between different formats. PR-URL: #19080 Fixes: #18977 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

wuweiweiwu · 2018-03-23T19:01:09Z

Awesome! Can't wait to contribute more :)

ECDH.convertKey is used to convert public keys between different formats. PR-URL: nodejs#19080 Fixes: nodejs#18977 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

ECDH.convertKey is used to convert public keys between different formats. PR-URL: #19080 Fixes: #18977 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Backport-PR-URL: #19745 Reviewed-By: Tobias Nießen <tniessen@tnie.de>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. labels Mar 2, 2018

wuweiweiwu force-pushed the crypto-convert-key branch from 9060702 to 7869581 Compare March 2, 2018 01:27

Trott added the wip Issues and PRs that are still a work in progress. label Mar 2, 2018

bnoordhuis reviewed Mar 2, 2018

View reviewed changes

wuweiweiwu commented Mar 2, 2018

View reviewed changes

wuweiweiwu changed the title ~~[WIP] crypto: ECDH convertKey to convert public keys between different formats~~ crypto: ECDH convertKey to convert public keys between different formats Mar 3, 2018

wuweiweiwu force-pushed the crypto-convert-key branch from 61fe99b to bd78e2c Compare March 3, 2018 00:50

bnoordhuis reviewed Mar 3, 2018

View reviewed changes

wuweiweiwu commented Mar 4, 2018

View reviewed changes

bnoordhuis approved these changes Mar 4, 2018

View reviewed changes

tniessen reviewed Mar 4, 2018

View reviewed changes

tniessen reviewed Mar 5, 2018

View reviewed changes

jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 5, 2018

jasnell reviewed Mar 5, 2018

View reviewed changes

jasnell approved these changes Mar 5, 2018

View reviewed changes

Trott removed the wip Issues and PRs that are still a work in progress. label Mar 9, 2018

wuweiweiwu added 7 commits March 10, 2018 12:19

crypto: ECDH function to convert public keys

41a17db

ECDH.convertKey is used to convert public keys between different formats. Fixes: nodejs#18977

crypto: refactoring shared code

9ded80c

refactored ECDH::BufferToPoint and key formatting

test: adding tests for ECDH#convertKey

e876f36

Adding tests to verify that convertkey correctly converts ECDH keys to different formats

doc: adding docs for ECDH#convertKey

810cb37

Adding docs and usage examples.

crypto: clean up pointers on failure

17f1703

Using std::shared_ptr to clean up pointers. Formatting changes. Refactoring encode function.

crypto: minor fixes

d5f07d9

Using const over var. Destructuring notation. Updated function signature.

crypto: input typechecking

c201c26

Handle input typechecking in JavaScript using errors. Minor fixes and updated tests

wuweiweiwu force-pushed the crypto-convert-key branch from 9f2490f to c201c26 Compare March 10, 2018 20:35

crypto: using the updated errors

541a651

Using the new error throwing mechanism.

Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 22, 2018

tniessen closed this Mar 23, 2018

targos mentioned this pull request Apr 2, 2018

[v9.x backport] crypto: add ECDH.convertKey to convert public keys #19745

Closed

targos added backport-requested-v9.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Apr 2, 2018

targos added backported-to-v9.x and removed backport-requested-v9.x labels Apr 12, 2018

MylesBorins added the dont-land-on-v8.x label Aug 7, 2018


		const availableCurves = new Set(getCurves());

		if (availableCurves.has('secp256k1')) {


		CHECK_EQ(args.Length(), 3);

		THROW_AND_RETURN_IF_NOT_BUFFER(args[0], "Public key");

crypto: ECDH convertKey to convert public keys between different formats #19080

crypto: ECDH convertKey to convert public keys between different formats #19080

Conversation

wuweiweiwu commented Mar 2, 2018 • edited Loading

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wuweiweiwu Mar 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wuweiweiwu Mar 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis Mar 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wuweiweiwu Mar 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis left a comment

Choose a reason for hiding this comment

wuweiweiwu commented Mar 4, 2018

tniessen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wuweiweiwu Mar 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wuweiweiwu Mar 5, 2018 • edited Loading

Choose a reason for hiding this comment

wuweiweiwu commented Mar 9, 2018 • edited Loading

tniessen commented Mar 10, 2018

wuweiweiwu commented Mar 10, 2018 • edited Loading

tniessen commented Mar 11, 2018 • edited Loading

wuweiweiwu commented Mar 11, 2018 • edited Loading

Trott commented Mar 13, 2018

wuweiweiwu commented Mar 13, 2018 • edited Loading

Trott commented Mar 13, 2018

Trott commented Mar 13, 2018

wuweiweiwu commented Mar 22, 2018

tniessen commented Mar 22, 2018

Trott commented Mar 22, 2018

Trott commented Mar 23, 2018

tniessen commented Mar 23, 2018

wuweiweiwu commented Mar 23, 2018

wuweiweiwu commented Mar 2, 2018 •

edited

Loading

wuweiweiwu Mar 2, 2018 •

edited

Loading

wuweiweiwu Mar 4, 2018 •

edited

Loading

bnoordhuis Mar 3, 2018 •

edited

Loading

wuweiweiwu Mar 4, 2018 •

edited

Loading

wuweiweiwu Mar 5, 2018 •

edited

Loading

wuweiweiwu Mar 5, 2018 •

edited

Loading

wuweiweiwu commented Mar 9, 2018 •

edited

Loading

wuweiweiwu commented Mar 10, 2018 •

edited

Loading

tniessen commented Mar 11, 2018 •

edited

Loading

wuweiweiwu commented Mar 11, 2018 •

edited

Loading

wuweiweiwu commented Mar 13, 2018 •

edited

Loading