crypto: Better docs for cases where peer's public key is invalid #16849

j0t3x · 2017-11-06T22:41:13Z

changes in c++ are in the computeSecret function, but the thrown
exception that was moved to JS land was in BufferToPoint
function, here i let the allocation error be thrown so the only value
returned is the nullptr that i use later to catch the error in
computeSecret, to then construct the exception in JS land.

an ERR_CRYPTO_ECDH_INVALID_PUBLIC_KEY error was added to errors.js
and with that, subsequent changes to docs and tests were made.

Fixes: #16625
Refs: https://www.iacr.org/archive/pkc2003/25670211/25670211.pdf

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

mscdex · 2017-11-06T22:51:40Z

doc/api/crypto.md

@@ -522,6 +522,12 @@ provided, `otherPublicKey` is expected to be a [`Buffer`][],
 If `outputEncoding` is given a string is returned; otherwise, a
 [`Buffer`][] is returned.

+*Note*:
+
+* The `server.computeSecret` will throw an 


Shouldn't this be diffieHellman.computeSecret()?

Also we can drop 'The' at the beginning.

my mistake :)
already corrected 👍 thanks!

jasnell · 2017-11-08T10:28:00Z

Marking major due to the change in the error message and use of internal/errors.

bnoordhuis

Thanks, LGTM modulo nits.

bnoordhuis · 2017-11-08T12:12:17Z

doc/api/crypto.md

+
+* `diffieHellman.computeSecret` will throw an 
+`ERR_CRYPTO_ECDH_INVALID_PUBLIC_KEY` error when the provided 
+public key lies out of the elliptic curve.


s/out of/outside/?

bnoordhuis · 2017-11-08T12:12:29Z

doc/api/errors.md

+
+Used when an invalid value for the `key` argument has been passed to the
+`crypto.ECDH()` class `computeSecret()` method. It means that the public 
+key lies out of the elliptic curve.


xD damn!
corrected the 2 occurrences 👍

bnoordhuis · 2017-11-08T12:14:50Z

src/node_crypto.cc

+  if (pub == nullptr) {
+    args.GetReturnValue().Set(
+      String::NewFromUtf8(env->isolate(),
+      "ERR_CRYPTO_ECDH_KEY_FAIL_TRANSLATE"));


Style nit: can you indent line continuations by four spaces?

Since you're passing a static string, you can use the slightly more efficientFIXED_ONE_BYTE_STRING(env->isolate(), "...") here.

thanks!
this also helped me to correct the error code in c++(which was one of the first names i thought but then dropped for ERR_CRYPTO_ECDH_INVALID_PUBLIC_KEY) 👍

Emill · 2017-11-08T17:47:41Z

Nice!
A few points:

The documentation update is at the wrong place. It's the ECDH class's computeSecret that should be updates; not the DiffieHellman's.
I'd like to see the sample updated at https://github.com/j0t3x/node/blob/3663e21bc0cefb159b6842f75647bf66f048b866/doc/api/crypto.md#class-ecdh. As mentioned earlier, I don't see any cases where you would not want to catch the error since the input is usually supplied from a remote user over an insecure network.

j0t3x · 2017-11-08T18:45:12Z

right @Emill !
corrected 😃

Trott · 2017-11-13T23:30:23Z

doc/api/crypto.md

@@ -668,6 +668,14 @@ provided, `otherPublicKey` is expected to be a [`Buffer`][], `TypedArray`, or
 If `outputEncoding` is given a string will be returned; otherwise a
 [`Buffer`][] is returned.

+*Note*:


Nit: I'd remove *Note*: and let the paragraph stand on its own.

Done!
Updating the pr soon.
Thanks!

Trott · 2017-11-13T23:31:17Z

doc/api/errors.md

+<a id="ERR_CRYPTO_ECDH_INVALID_PUBLIC_KEY"></a>
+### ERR_CRYPTO_ECDH_INVALID_PUBLIC_KEY
+
+Used when an invalid value for the `key` argument has been passed to the


Nit: There's a PR in to remove Used when from all these messages. I'd prefer this go in as something like this:

An invalid value for the key argument was passed to the...

Done :)
But I want to understand the rationale behind the rule. It will help me a lot.
Thanks in advance.

It's to reduce wordiness and make the document more scannable. There were some pretty egregious examples, like:

The 'ERR_INVALID_CURSOR_POS' is thrown specifically when a cursor on
a given stream is attempted to move to a specified row without a specified
column.

That will be shortened to:

A cursor on a given stream cannot be moved to a specified row without
a specified column.

PR is #16954

xD nice example.
Understood.

Emill · 2017-11-14T22:43:19Z

Not sure but should the documentation include something like "since: node version x"?

j0t3x · 2017-11-29T22:04:55Z

can be: "added: v0.0.0"?
In case of a yes: what version should be?

addaleax · 2017-11-30T01:07:24Z

doc/api/crypto.md

@@ -668,6 +668,12 @@ provided, `otherPublicKey` is expected to be a [`Buffer`][], `TypedArray`, or
 If `outputEncoding` is given a string will be returned; otherwise a
 [`Buffer`][] is returned.

+* `ecdh.computeSecret` will throw an 


added: v0.0.0

We only use added for new functions/classes/modules/etc.

What you want is something like this, added to the YAML block for this function:

changes: - version: REPLACEME pr-url: https://github.com/nodejs/node/pull/16849 description: The default `computeSecret` option is supported now-

Oh, wait. I think I misunderstood; can you not make this a list item (not let this start with a *)?

And I don't think you need a changelog entry if all that changed is the error format.

Looks good with this changed though.

what version should be there instead of replace me? Maybe 9?

forget it, i saw its 10 :)

addaleax · 2017-11-30T01:09:24Z

doc/api/crypto.md

@@ -668,6 +668,12 @@ provided, `otherPublicKey` is expected to be a [`Buffer`][], `TypedArray`, or
 If `outputEncoding` is given a string will be returned; otherwise a
 [`Buffer`][] is returned.

+* `ecdh.computeSecret` will throw an 


Oh, wait. I think I misunderstood; can you not make this a list item (not let this start with a *)?

And I don't think you need a changelog entry if all that changed is the error format.

Looks good with this changed though.

can probably be fixed while landing

addaleax · 2017-11-30T01:13:17Z

CI: https://ci.nodejs.org/job/node-test-commit/14469/

changes in c++ are in the computeSecret function, but the thrown exception that was moved to JS land was in BufferToPoint function, here i let the allocation error be thrown so the only value returned is the nullptr that i use later to catch the error in computeSecret, to then construct the exception in JS land. an ERR_CRYPTO_ECDH_INVALID_PUBLIC_KEY error was added to errors.js and with that, subsequent changes to docs and tests were made. Fixes: #16625 Refs: https://www.iacr.org/archive/pkc2003/25670211/25670211.pdf

addaleax · 2017-12-01T20:19:44Z

Landed in 845633a, thanks for the PR! 🎉

changes in c++ are in the computeSecret function, but the thrown exception that was moved to JS land was in BufferToPoint function, here i let the allocation error be thrown so the only value returned is the nullptr that i use later to catch the error in computeSecret, to then construct the exception in JS land. an ERR_CRYPTO_ECDH_INVALID_PUBLIC_KEY error was added to errors.js and with that, subsequent changes to docs and tests were made. PR-URL: #16849 Refs: https://www.iacr.org/archive/pkc2003/25670211/25670211.pdf Fixes: #16625 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

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. errors Issues and PRs related to JavaScript errors originated in Node.js core. labels Nov 6, 2017

j0t3x mentioned this pull request Nov 6, 2017

crypto: Better documentation for cases where peer's public key is invalid #16625

Closed

mscdex reviewed Nov 6, 2017

View reviewed changes

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Nov 8, 2017

jasnell approved these changes Nov 8, 2017

View reviewed changes

bnoordhuis approved these changes Nov 8, 2017

View reviewed changes

Trott reviewed Nov 13, 2017

View reviewed changes

addaleax approved these changes Nov 30, 2017

View reviewed changes

addaleax previously requested changes Nov 30, 2017

View reviewed changes

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 30, 2017

addaleax closed this Dec 1, 2017

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

crypto: Better docs for cases where peer's public key is invalid #16849

crypto: Better docs for cases where peer's public key is invalid #16849

j0t3x commented Nov 6, 2017

mscdex Nov 6, 2017

j0t3x Nov 6, 2017

jasnell commented Nov 8, 2017

bnoordhuis left a comment

bnoordhuis Nov 8, 2017

bnoordhuis Nov 8, 2017

j0t3x Nov 8, 2017

bnoordhuis Nov 8, 2017

j0t3x Nov 8, 2017

Emill commented Nov 8, 2017

j0t3x commented Nov 8, 2017

Trott Nov 13, 2017

j0t3x Nov 14, 2017

Trott Nov 13, 2017

j0t3x Nov 14, 2017 •

edited

Loading

Trott Nov 14, 2017

j0t3x Nov 14, 2017

Emill commented Nov 14, 2017

j0t3x commented Nov 29, 2017

addaleax Nov 30, 2017

addaleax Nov 30, 2017 •

edited

Loading

j0t3x Nov 30, 2017 •

edited

Loading

j0t3x Nov 30, 2017

addaleax Nov 30, 2017 •

edited

Loading

addaleax commented Nov 30, 2017

addaleax commented Dec 1, 2017

crypto: Better docs for cases where peer's public key is invalid #16849

crypto: Better docs for cases where peer's public key is invalid #16849

Conversation

j0t3x commented Nov 6, 2017

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Nov 8, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Emill commented Nov 8, 2017

j0t3x commented Nov 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j0t3x Nov 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Emill commented Nov 14, 2017

j0t3x commented Nov 29, 2017

Choose a reason for hiding this comment

addaleax Nov 30, 2017 • edited Loading

Choose a reason for hiding this comment

j0t3x Nov 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax Nov 30, 2017 • edited Loading

Choose a reason for hiding this comment

addaleax commented Nov 30, 2017

addaleax commented Dec 1, 2017

j0t3x Nov 14, 2017 •

edited

Loading

addaleax Nov 30, 2017 •

edited

Loading

j0t3x Nov 30, 2017 •

edited

Loading

addaleax Nov 30, 2017 •

edited

Loading