crypto: add better scrypt option aliases by addaleax · Pull Request #21525 · nodejs/node

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

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

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

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

lib/internal/crypto/scrypt.js

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

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>

It is confusing to have both ERR_CRYPTO_SCRYPT_INVALID_PARAMETER and ERR_CRYPTO_INVALID_SCRYPT_PARAMS. The former was the original error code, added in 371103d, but parameter validation gradually changed and now produces ERR_CRYPTO_INVALID_SCRYPT_PARAMS for all parameter validation errors coming from OpenSSL, as well as different error codes for validation errors coming from JavaScript. The only remaining use of ERR_CRYPTO_SCRYPT_INVALID_PARAMETER is in the validation logic that ensures that no two synonymous options were passed. We already have an error code for that particular case, ERR_INCOMPATIBLE_OPTION_PAIR, so replace these last instances of ERR_CRYPTO_SCRYPT_INVALID_PARAMETER with that error code and remove ERR_CRYPTO_SCRYPT_INVALID_PARAMETER. If there ever is need again for such an error code, we can just use ERR_CRYPTO_INVALID_SCRYPT_PARAMS. Refs: nodejs#35093 Refs: nodejs#21525 Refs: nodejs#20816

It is confusing to have both ERR_CRYPTO_SCRYPT_INVALID_PARAMETER and ERR_CRYPTO_INVALID_SCRYPT_PARAMS. The former was the original error code, added in 371103d, but parameter validation gradually changed and now produces ERR_CRYPTO_INVALID_SCRYPT_PARAMS for all parameter validation errors coming from OpenSSL, as well as different error codes for validation errors coming from JavaScript. The only remaining use of ERR_CRYPTO_SCRYPT_INVALID_PARAMETER is in the validation logic that ensures that no two synonymous options were passed. We already have an error code for that particular case, ERR_INCOMPATIBLE_OPTION_PAIR, so replace these last instances of ERR_CRYPTO_SCRYPT_INVALID_PARAMETER with that error code and remove ERR_CRYPTO_SCRYPT_INVALID_PARAMETER. If there ever is need again for such an error code, we can just use ERR_CRYPTO_INVALID_SCRYPT_PARAMS. Refs: #35093 Refs: #21525 Refs: #20816 PR-URL: #53305 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

It is confusing to have both ERR_CRYPTO_SCRYPT_INVALID_PARAMETER and ERR_CRYPTO_INVALID_SCRYPT_PARAMS. The former was the original error code, added in 371103d, but parameter validation gradually changed and now produces ERR_CRYPTO_INVALID_SCRYPT_PARAMS for all parameter validation errors coming from OpenSSL, as well as different error codes for validation errors coming from JavaScript. The only remaining use of ERR_CRYPTO_SCRYPT_INVALID_PARAMETER is in the validation logic that ensures that no two synonymous options were passed. We already have an error code for that particular case, ERR_INCOMPATIBLE_OPTION_PAIR, so replace these last instances of ERR_CRYPTO_SCRYPT_INVALID_PARAMETER with that error code and remove ERR_CRYPTO_SCRYPT_INVALID_PARAMETER. If there ever is need again for such an error code, we can just use ERR_CRYPTO_INVALID_SCRYPT_PARAMS. Refs: nodejs#35093 Refs: nodejs#21525 Refs: nodejs#20816 PR-URL: nodejs#53305 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

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/node#20816 (comment) PR-URL: nodejs/node#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

doc/api/crypto.md Outdated

Copy link

Contributor

vsemozhetbyt Jun 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

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

tniessen mentioned this pull request Jun 3, 2024

crypto: remove ERR_CRYPTO_SCRYPT_INVALID_PARAMETER #53305

Merged

Uh oh!

Conversation

addaleax commented Jun 25, 2018

Checklist

Uh oh!

nodejs-github-bot commented Jun 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax Jun 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

demurgos commented Jul 9, 2018

Uh oh!

addaleax commented Jul 9, 2018

Uh oh!

TimothyGu left a comment

Choose a reason for hiding this comment

Uh oh!

TimothyGu Jul 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax commented Jul 9, 2018

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Jul 16, 2018

Uh oh!

Trott commented Jul 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Jul 16, 2018

Uh oh!

TimothyGu commented Jul 16, 2018

Uh oh!

addaleax commented Jul 16, 2018

Uh oh!

TimothyGu commented Jul 17, 2018

Uh oh!

TimothyGu left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Jul 18, 2018

Uh oh!

targos commented Jul 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

addaleax Jun 29, 2018 •

edited

Loading

TimothyGu Jul 9, 2018 •

edited

Loading

Trott commented Jul 16, 2018 •

edited

Loading