crypto: add better scrypt option aliases #21525

addaleax · 2018-06-25T15:47:52Z

Make parameter names available in a human-readable way, for
more accessible/self-documenting usage of the scrypt functions.

This implements a review comment from the original PR that has
not been addressed.

Refs: #20816 (comment)

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

nodejs-github-bot · 2018-06-25T15:47:54Z

@addaleax build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/152/pipeline

cjihrig · 2018-06-25T19:01:16Z

lib/internal/crypto/scrypt.js

@@ -82,10 +82,16 @@ function check(password, salt, keylen, options, callback) {
  if (options && options !== defaults) {
    if (options.hasOwnProperty('N'))
      N = validateInt32(options.N, 'N', 0, INT_MAX);
+    if (options.hasOwnProperty('cost'))


If we do go the route of adding these aliases, I'd prefer if these were else ifs, and the priority were documented and tested. Otherwise, the priority becomes an implementation detail that could subtly break in the future. Throwing is another option if someone were to provide N and cost, but that might be overkill.

Perhaps process.emitWarning() if both are provided would be a softer nudge?

I agree that a test for the priority would be a good addition here. Will sign off with that added.

@jasnell @cjihrig I’ve went with the throwing option – it’s not as “soft”, but I don’t think using both options something that would intentionally happen in normal userland code.

vsemozhetbyt · 2018-06-25T21:42:42Z

doc/api/crypto.md

-  - `r` {number} Block size parameter. **Default:** `8`.
-  - `p` {number} Parallelization parameter. **Default:** `1`.
+  - `cost` {number} CPU/memory cost parameter. Must be a power of two greater
+                    than one. **Default:** `16384`.


Such indentation produces false code blocks in HTML version.

maxmem case below will be fixed in #21500, cost case seems to need fixing here.

@vsemozhetbyt Thanks for pointing that out, done!

vsemozhetbyt · 2018-06-25T21:42:59Z

doc/api/crypto.md

-  - `r` {number} Block size parameter. **Default:** `8`.
-  - `p` {number} Parallelization parameter. **Default:** `1`.
+  - `cost` {number} CPU/memory cost parameter. Must be a power of two greater
+                    than one. **Default:** `16384`.


jasnell · 2018-06-29T18:54:02Z

lib/internal/crypto/scrypt.js

@@ -82,10 +82,16 @@ function check(password, salt, keylen, options, callback) {
  if (options && options !== defaults) {
    if (options.hasOwnProperty('N'))
      N = validateInt32(options.N, 'N', 0, INT_MAX);
+    if (options.hasOwnProperty('cost'))


I agree that a test for the priority would be a good addition here. Will sign off with that added.

demurgos · 2018-07-09T11:27:44Z

Thanks for the PR: it would help me. I always had trouble with these non-readable option names (already with node-scrypt).

addaleax · 2018-07-09T11:28:46Z

ping @jasnell :)

TimothyGu

Have you considered something like this instead:

const { kCost, kBlockSize, kParallelization } = crypto.constants;

// …

scrypt(..., {
  [kCost]: 16,
  [kParallelization]: 1,
  [kBlockSize]: 1
});

This is similarly readable but avoids the maneuver we have to do with regards to option handling.

TimothyGu · 2018-07-09T13:38:43Z

lib/internal/crypto/scrypt.js

@@ -80,12 +80,25 @@ function check(password, salt, keylen, options, callback) {

  let { N, r, p, maxmem } = defaults;
  if (options && options !== defaults) {
-    if (options.hasOwnProperty('N'))
+    let has_N, has_r, has_p;
+    if (has_N = options.hasOwnProperty('N'))


I know the hasOwnProperty has already been there, but we should really use undefined as a measure of if an option was set, to be consistent with the rest of the API surface and to prevent Object.create(null) from breaking this.

@TimothyGu Done!

addaleax · 2018-07-09T15:23:44Z

This is similarly readable but avoids the maneuver we have to do with regards to option handling.

Can you explain how your suggestion would work?

jasnell

sorry for the delay, was on vacation :-)

Make parameter names available in a human-readable way, for more accessible/self-documenting usage of the `scrypt` functions. This implements a review comment from the original PR that has not been addressed. Refs: nodejs#20816 (comment)

addaleax · 2018-07-16T18:37:40Z

Rebased, CI: https://ci.nodejs.org/job/node-test-pull-request/15898/

Trott · 2018-07-16T21:45:34Z

AIX benchmark failures seem inexplicable. Maybe something's up on the host such that it can't launch subprocesses at the moment? Anyway, here's a Resume Build: https://ci.nodejs.org/job/node-test-pull-request/15899/

UPDATE: It's green.

Trott · 2018-07-16T22:15:34Z

We have a green CI and one approval. This can technically land, but would be great if someone from @nodejs/crypto could give it a quick look.

TimothyGu · 2018-07-16T22:28:53Z

@addaleax Sorry I missed the notification on this thread. My proposal was to export three string-valued constants from crypto.constants, such that kCost evaluates to 'N' et cetera.

addaleax · 2018-07-16T22:32:25Z

@TimothyGu How strongly do you feel about that? It seems like that would feel a bit less natural here…

TimothyGu · 2018-07-17T14:17:20Z

@addaleax Not very, just trying to offer an alternative here.

TimothyGu

LGTM :)

addaleax · 2018-07-18T12:20:17Z

Landed in e9b22e9

Make parameter names available in a human-readable way, for more accessible/self-documenting usage of the `scrypt` functions. This implements a review comment from the original PR that has not been addressed. Refs: #20816 (comment) PR-URL: #21525 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

targos · 2018-07-19T07:32:16Z

Depends on #21782 to land on v10.x-staging

Make parameter names available in a human-readable way, for more accessible/self-documenting usage of the `scrypt` functions. This implements a review comment from the original PR that has not been addressed. Refs: #20816 (comment) PR-URL: #21525 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

addaleax added 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. labels Jun 25, 2018

nodejs-github-bot added the crypto Issues and PRs related to the crypto subsystem. label Jun 25, 2018

cjihrig reviewed Jun 25, 2018

View reviewed changes

vsemozhetbyt reviewed Jun 25, 2018

View reviewed changes

jasnell requested changes Jun 29, 2018

View reviewed changes

addaleax force-pushed the crypto-scrypt-aliases branch from d11c385 to f4f7d6b Compare June 29, 2018 22:01

TimothyGu reviewed Jul 9, 2018

View reviewed changes

addaleax force-pushed the crypto-scrypt-aliases branch from f4f7d6b to fad284e Compare July 9, 2018 14:51

jasnell approved these changes Jul 10, 2018

View reviewed changes

addaleax added 2 commits July 16, 2018 20:36

crypto: add better scrypt option aliases

d38b2dd

Make parameter names available in a human-readable way, for more accessible/self-documenting usage of the `scrypt` functions. This implements a review comment from the original PR that has not been addressed. Refs: nodejs#20816 (comment)

[squash] get rid of options.hasOwnProperty

83800fa

addaleax force-pushed the crypto-scrypt-aliases branch from fad284e to 83800fa Compare July 16, 2018 18:37

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

tniessen approved these changes Jul 17, 2018

View reviewed changes

TimothyGu approved these changes Jul 17, 2018

View reviewed changes

addaleax closed this Jul 18, 2018

addaleax deleted the crypto-scrypt-aliases branch July 18, 2018 12:20

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

targos removed the backport-requested-v10.x label Aug 7, 2018

rvagg mentioned this pull request Aug 13, 2018

Release proposal: v10.9.0 #22295

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

crypto: add better scrypt option aliases #21525

crypto: add better scrypt option aliases #21525

addaleax commented Jun 25, 2018

nodejs-github-bot commented Jun 25, 2018

cjihrig Jun 25, 2018

jasnell Jun 29, 2018

jasnell Jun 29, 2018

addaleax Jun 29, 2018 •

edited

vsemozhetbyt Jun 25, 2018

addaleax Jun 29, 2018

vsemozhetbyt Jun 25, 2018

jasnell Jun 29, 2018

demurgos commented Jul 9, 2018

addaleax commented Jul 9, 2018

TimothyGu left a comment

TimothyGu Jul 9, 2018 •

edited

addaleax Jul 9, 2018

addaleax commented Jul 9, 2018

jasnell left a comment

addaleax commented Jul 16, 2018

Trott commented Jul 16, 2018 •

edited

Trott commented Jul 16, 2018

TimothyGu commented Jul 16, 2018

addaleax commented Jul 16, 2018

TimothyGu commented Jul 17, 2018

TimothyGu left a comment

addaleax commented Jul 18, 2018

targos commented Jul 19, 2018

crypto: add better scrypt option aliases #21525

crypto: add better scrypt option aliases #21525

Conversation

addaleax commented Jun 25, 2018

Checklist

nodejs-github-bot commented Jun 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax Jun 29, 2018 • edited

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

demurgos commented Jul 9, 2018

addaleax commented Jul 9, 2018

TimothyGu left a comment

Choose a reason for hiding this comment

TimothyGu Jul 9, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Jul 9, 2018

jasnell left a comment

Choose a reason for hiding this comment

addaleax commented Jul 16, 2018

Trott commented Jul 16, 2018 • edited

Trott commented Jul 16, 2018

TimothyGu commented Jul 16, 2018

addaleax commented Jul 16, 2018

TimothyGu commented Jul 17, 2018

TimothyGu left a comment

Choose a reason for hiding this comment

addaleax commented Jul 18, 2018

targos commented Jul 19, 2018

addaleax Jun 29, 2018 •

edited

TimothyGu Jul 9, 2018 •

edited

Trott commented Jul 16, 2018 •

edited