Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crypto: add better scrypt option aliases #21525

Closed
wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

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

@addaleax 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 nodejs-github-bot added the crypto Issues and PRs related to the crypto subsystem. label Jun 25, 2018
@@ -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'))
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@addaleax addaleax Jun 29, 2018

Choose a reason for hiding this comment

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

@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.

- `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`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Such indentation produces false code blocks in HTML version.

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@vsemozhetbyt Thanks for pointing that out, done!

- `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`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

@@ -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'))
Copy link
Member

Choose a reason for hiding this comment

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

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

@demurgos
Copy link
Contributor

demurgos commented Jul 9, 2018

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

@addaleax
Copy link
Member Author

addaleax commented Jul 9, 2018

ping @jasnell :)

Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

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.

@@ -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'))
Copy link
Member

@TimothyGu TimothyGu Jul 9, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

@TimothyGu Done!

@addaleax
Copy link
Member Author

addaleax commented Jul 9, 2018

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?

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member Author

@addaleax 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
@Trott
Copy link
Member

Trott commented Jul 16, 2018

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
Copy link
Member

Trott commented Jul 16, 2018

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
Copy link
Member

@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
Copy link
Member Author

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

@TimothyGu
Copy link
Member

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

Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

LGTM :)

@addaleax
Copy link
Member Author

Landed in e9b22e9

@addaleax addaleax closed this Jul 18, 2018
@addaleax addaleax deleted the crypto-scrypt-aliases branch July 18, 2018 12:20
addaleax added a commit that referenced this pull request Jul 18, 2018
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 <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
@targos
Copy link
Member

targos commented Jul 19, 2018

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

@targos 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 pushed a commit that referenced this pull request Aug 7, 2018
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 <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
rvagg pushed a commit that referenced this pull request Aug 13, 2018
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 <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
tniessen added a commit to tniessen/node that referenced this pull request Jun 3, 2024
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
nodejs-github-bot pushed a commit that referenced this pull request Jul 10, 2024
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 <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
ehsankhfr pushed a commit to ehsankhfr/node that referenced this pull request Jul 18, 2024
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 <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants