★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/21525Nouvelle recherche | Portail wanayoo
Skip to content

/node

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

Reviewers

@cjihrig cjihrig

@vsemozhetbyt vsemozhetbyt

@jasnell jasnell

@TimothyGu TimothyGu

@tniessen tniessen

Assignees
No one assigned
Labels
crypto semver-minor
Projects
None yet
Milestone
No milestone
10 participants
@addaleax @nodejs-github-bot @demurgos @Trott @TimothyGu @targos @jasnell @cjihrig @tniessen @vsemozhetbyt
@addaleax
Copy link
Member

addaleax commented Jun 25, 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)

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 semver-minor labels Jun 25, 2018

@nodejs-github-bot

@nodejs-github-bot nodejs-github-bot added the crypto label Jun 25, 2018

@cjihrig

cjihrig reviewed Jun 25, 2018
View changes

lib/internal/crypto/scrypt.js Outdated
@@ -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'))

This comment has been minimized.

Sign in to view
Copy link
@cjihrig

cjihrig Jun 25, 2018

Contributor

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.

This comment has been minimized.

Sign in to view
Copy link
@jasnell

jasnell Jun 29, 2018

Member

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

This comment has been minimized.

Sign in to view
Copy link
@jasnell

jasnell Jun 29, 2018

Member

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

This comment has been minimized.

Sign in to view
Copy link
@addaleax

addaleax Jun 29, 2018 •
edited

Author Member

@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

vsemozhetbyt reviewed Jun 25, 2018
View changes

doc/api/crypto.md Outdated
- `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`.

This comment has been minimized.

Sign in to view
Copy link
@vsemozhetbyt

vsemozhetbyt Jun 25, 2018

Member

Such indentation produces false code blocks in HTML version.

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

This comment has been minimized.

Sign in to view
Copy link
@addaleax

addaleax Jun 29, 2018

Author Member

@vsemozhetbyt Thanks for pointing that out, done!
@vsemozhetbyt

vsemozhetbyt reviewed Jun 25, 2018
View changes

doc/api/crypto.md Outdated
- `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`.

This comment has been minimized.

Sign in to view
Copy link
@vsemozhetbyt

vsemozhetbyt Jun 25, 2018

Member

Ditto.
@jasnell

jasnell requested changes Jun 29, 2018
View changes

lib/internal/crypto/scrypt.js Outdated
@@ -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'))

This comment has been minimized.

Sign in to view
Copy link
@jasnell

jasnell Jun 29, 2018

Member

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

@addaleax addaleax force-pushed the addaleax:crypto-scrypt-aliases branch from d11c385 to f4f7d6b Jun 29, 2018

@demurgos

This comment has been minimized.

Sign in to view
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

This comment has been minimized.

Sign in to view
Copy link
Member Author

addaleax commented Jul 9, 2018

ping @jasnell :)
@TimothyGu

TimothyGu reviewed Jul 9, 2018
View changes

Copy link
Member

TimothyGu left a comment

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.
lib/internal/crypto/scrypt.js Outdated
@@ -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'))

This comment has been minimized.

Sign in to view
Copy link
@TimothyGu

TimothyGu Jul 9, 2018 •
edited

Member

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.

This comment has been minimized.

Sign in to view
Copy link
@addaleax

addaleax Jul 9, 2018

Author Member

@TimothyGu Done!

@addaleax addaleax force-pushed the addaleax:crypto-scrypt-aliases branch from f4f7d6b to fad284e Jul 9, 2018

@addaleax

This comment has been minimized.

Sign in to view
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?
@jasnell

jasnell approved these changes Jul 10, 2018
View changes

Copy link
Member

jasnell left a comment

sorry for the delay, was on vacation :-)

addaleax added some commits Jun 25, 2018

@addaleax
crypto: add better scrypt option aliases 
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)
Verified
This commit was signed with a verified signature.
@addaleax addaleax Anna Henningsen
GPG key ID: 9C63F3A6CD2AD8F9 Learn about signing commits
d38b2dd
@addaleax
[squash] get rid of options.hasOwnProperty
Verified
This commit was signed with a verified signature.
@addaleax addaleax Anna Henningsen
GPG key ID: 9C63F3A6CD2AD8F9 Learn about signing commits
Loading status checks…
83800fa

@addaleax addaleax force-pushed the addaleax:crypto-scrypt-aliases branch from fad284e to 83800fa Jul 16, 2018

@addaleax

This comment has been minimized.

Sign in to view
Copy link
Member Author

addaleax commented Jul 16, 2018

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

@addaleax addaleax added the author ready label Jul 16, 2018

@Trott

This comment has been minimized.

Sign in to view
Copy link
Member

Trott commented Jul 16, 2018 •
edited

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

This comment has been minimized.

Sign in to view
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

This comment has been minimized.

Sign in to view
Copy link
Member

TimothyGu commented Jul 16, 2018

@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

This comment has been minimized.

Sign in to view
Copy link
Member Author

addaleax commented Jul 16, 2018

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

tniessen approved these changes Jul 17, 2018
View changes

@TimothyGu

This comment has been minimized.

Sign in to view
Copy link
Member

TimothyGu commented Jul 17, 2018

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

TimothyGu approved these changes Jul 17, 2018
View changes

Copy link
Member

TimothyGu left a comment

LGTM :)
@addaleax

This comment has been minimized.

Sign in to view
Copy link
Member Author

addaleax commented Jul 18, 2018

Landed in e9b22e9

@addaleax addaleax closed this Jul 18, 2018

@addaleax addaleax deleted the addaleax:crypto-scrypt-aliases branch Jul 18, 2018

addaleax added a commit that referenced this pull request Jul 18, 2018

@addaleax
crypto: add better scrypt option aliases 
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>
Verified
This commit was signed with a verified signature.
@addaleax addaleax Anna Henningsen
GPG key ID: 9C63F3A6CD2AD8F9 Learn about signing commits
Loading status checks…
e9b22e9
@targos

This comment has been minimized.

Sign in to view
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 labels Jul 19, 2018

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

targos added a commit that referenced this pull request Aug 7, 2018

@addaleax @targos
crypto: add better scrypt option aliases 
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>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
d005ae0

rvagg added a commit that referenced this pull request Aug 13, 2018

@addaleax @rvagg
crypto: add better scrypt option aliases 
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>
58580e9

@rvagg rvagg referenced this pull request Aug 13, 2018

Merged

Release proposal: v10.9.0 #22295

This was referenced Aug 16, 2018

Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.