lib: remove useless input parameters and make 'digest' clear in the doc #22858

ghost · 2018-09-14T09:05:23Z

Remove 'callback' in 'check' function, because we don't check or use
that directly.
Adjust the orders of codes in 'pbkdf2' to have a quick check whether
the callback is a function or not instead of using it into the 'check'
method.
Add explainations to 'digest' in the doc.

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]

tniessen · 2018-09-14T10:09:41Z

I think this changes the behavior if digest and callback are both functions?

ghost · 2018-09-14T10:50:27Z

Since in the doc, we didn't make 'digest' so clear (allowing which types it takes and what error it will throw out...ect). I'd also take this :)

tniessen · 2018-09-14T11:49:50Z

That is technically correct... Weird how null is still a valid value even though it should have been deprecated years ago. Fixed in #22861. I think I'd prefer not to document deprecated behavior,

For the future: The first word of the commit message title should not be capitalized, see our guidelines.

mscdex · 2018-09-14T18:23:35Z

doc/api/crypto.md

@@ -1791,6 +1791,11 @@ but will take a longer amount of time to complete.
 The `salt` should be as unique as possible. It is recommended that a salt is
 random and at least 16 bytes long. See [NIST SP 800-132][] for details.

+The `digest` should either be a string value or null:
+- If it's null, `sha1` will be the default one


String literals should be enclosed in single quotes: 'sha1'. Also the two instances of null above should be styled as null.

I'd prefer not to document this at all since the behavior should have been EOL years ago, see #22861.

mscdex · 2018-09-14T18:24:50Z

doc/api/crypto.md

@@ -1791,6 +1791,11 @@ but will take a longer amount of time to complete.
 The `salt` should be as unique as possible. It is recommended that a salt is
 random and at least 16 bytes long. See [NIST SP 800-132][] for details.

+The `digest` should either be a string value or null:
+- If it's null, `sha1` will be the default one
+- For other type values, An error `ERR_INVALID_ARG_TYPE`


I don't think we need to document this explicitly.

mscdex · 2018-09-14T18:29:14Z

doc/api/crypto.md

@@ -1791,6 +1791,11 @@ but will take a longer amount of time to complete.
 The `salt` should be as unique as possible. It is recommended that a salt is
 random and at least 16 bytes long. See [NIST SP 800-132][] for details.

+The `digest` should either be a string value or null:


I think we can just shorten this to just mention that if digest is null, then 'sha1' is used, or perhaps include that explanation earlier in the function description where digest is first mentioned.

Take this adive! :)
On the one hand, it tells the truth, though we've missed that by accident.
On the other hand, this has given a good explaination to tell users that this value of null is deprecated since v11.

What I was mainly getting at about this line is that the part about needing to be a string value is unnecessary as earlier in the description it's noted that digest should be an HMAC digest algorithm, which implicitly means a string value.

ghost · 2018-09-14T23:06:44Z

@mscdex & @tniessen：Having reviewed all of your suggestions. It seems we'd better include this usage of digest (though it's puzzling) in our doc, BUT just mention that this will be deprecated in v11.x. I've put the declaration to the top of the function as the explainations

We've got such a sample before, see : https://nodejs.org/dist/latest-v8.x/docs/api/events.html#events_error_events (Note, however, that the domain module has been deprecated.)

tniessen · 2018-09-15T01:19:52Z

It seems we'd better include this usage of digest (though it's puzzling) in our doc

I would prefer to keep that very concise, maybe something like "If the digest is null, SHA1 will be used."

mention that this will be deprecated in v11.x

We usually don't explicitely reference future Node.js versions. You can write something like "will be deprecated in a future Node.js version" though, but please keep it short.

Review corresponds to older revision

mscdex · 2018-09-15T02:34:11Z

doc/api/crypto.md

@@ -1784,6 +1784,12 @@ otherwise `err` will be `null`. By default, the successfully generated
 `derivedKey` will be passed to the callback as a [`Buffer`][]. An error will be
 thrown if any of the input arguments specify invalid values or types.

+The `digest` should either be a string value or null:
+- if it's null, `sha1` will be the default one


String literals should be enclosed in single quotes: 'sha1'. Also the two instances of null above should be styled as null.

mscdex · 2018-09-15T02:35:08Z

doc/api/crypto.md

+The `digest` should either be a string value or null:
+- if it's null, `sha1` will be the default one
+(this will be deprecated since v11.x)
+- for other type values, An error `ERR_INVALID_ARG_TYPE`


I don't think we need to document this behavior explicitly.

mscdex · 2018-09-15T02:36:39Z

doc/api/crypto.md

@@ -1784,6 +1784,12 @@ otherwise `err` will be `null`. By default, the successfully generated
 `derivedKey` will be passed to the callback as a [`Buffer`][]. An error will be
 thrown if any of the input arguments specify invalid values or types.

+The `digest` should either be a string value or null:


I still believe that mentioning 'string value' here is redundant since it is stated earlier in the function description that digest is an HMAC digest algorithm, which implicitly means a string value.

ghost · 2018-09-15T06:49:49Z

OK. I'll just keep that very concise, according to what @tniessen's suggestion's.

doc/api/crypto.md

Trott · 2018-09-15T16:13:45Z

(Any reason not to immediately introduce a Documentation-only deprecation? /cc @tniessen Is that already happening in another PR? If so, presumably the docs are updated there already and so maybe this isn't necessary, except perhaps for backporting to older versions or something?)

tniessen · 2018-09-15T16:25:52Z

@Trott #22861 makes that a runtime deprecation. (When I approved this PR it was much simpler and only touched lib/.)

tniessen · 2018-09-17T11:20:36Z

CI: https://ci.nodejs.org/job/node-test-pull-request/17232/

lib/internal/crypto/pbkdf2.js

1) Remove 'callback' in 'check' function, because we don't check or use that directly. 2) Adjust the orders of codes in 'pbkdf2' to have a quick check whether the callback is a function or not instead of using it into the 'check' method. 3) Make 'digest' more clear in doc.

tniessen

Still LGTM.

tniessen · 2018-09-17T13:00:22Z

Thanks for updating your PR so often! Pre-land CI: https://ci.nodejs.org/job/node-test-pull-request/17238/
New CI: https://ci.nodejs.org/job/node-test-pull-request/17276/
New CI: https://ci.nodejs.org/job/node-test-pull-request/17279/

tniessen · 2018-09-18T14:07:38Z

Only failure seems to be an OS / HW problem, see nodejs/build#1499.

Landed in ba0b4e4, thank you @Maledong! 🎉

1) Remove 'callback' in 'check' function, because we don't check or use that directly. 2) Make 'digest' clearer in the documentation. PR-URL: #22858 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

ghost · 2018-09-19T02:24:33Z

Thanks all!

1) Remove 'callback' in 'check' function, because we don't check or use that directly. 2) Make 'digest' clearer in the documentation. PR-URL: #22858 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

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

ghost changed the title ~~lib: Fix and Remove useless input parameters~~ lib: Remove useless input parameters Sep 14, 2018

tniessen previously approved these changes Sep 14, 2018

View reviewed changes

ghost changed the title ~~lib: Remove useless input parameters~~ lib: Remove useless input parameters and make 'digest' clear in the doc Sep 14, 2018

mscdex reviewed Sep 14, 2018

View reviewed changes

ghost changed the title ~~lib: Remove useless input parameters and make 'digest' clear in the doc~~ lib: remove useless input parameters and make 'digest' clear in the doc Sep 14, 2018

mscdex reviewed Sep 15, 2018

View reviewed changes

Trott reviewed Sep 15, 2018

View reviewed changes

doc/api/crypto.md Outdated Show resolved Hide resolved

Trott reviewed Sep 15, 2018

View reviewed changes

doc/api/crypto.md Outdated Show resolved Hide resolved

tniessen approved these changes Sep 16, 2018

View reviewed changes

lpinca approved these changes Sep 16, 2018

View reviewed changes

tniessen reviewed Sep 17, 2018

View reviewed changes

lib/internal/crypto/pbkdf2.js Outdated Show resolved Hide resolved

tniessen approved these changes Sep 17, 2018

View reviewed changes

tniessen closed this Sep 18, 2018

tniessen mentioned this pull request Sep 18, 2018

crypto: deprecate digest == null in PBKDF2 #22861

Closed

4 tasks

joyeecheung mentioned this pull request Sep 18, 2018

CI failures: 20180918 nodejs/reliability#16

Closed

ghost deleted the FixAndRemove branch September 19, 2018 02:24

targos mentioned this pull request Sep 19, 2018

Release proposal: v10.11.0 #22932

Merged

tniessen mentioned this pull request Sep 20, 2018

deps: Use '===' instead of '==' #22970

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: remove useless input parameters and make 'digest' clear in the doc #22858

lib: remove useless input parameters and make 'digest' clear in the doc #22858

ghost commented Sep 14, 2018 •

edited by ghost

Loading

tniessen commented Sep 14, 2018

ghost commented Sep 14, 2018 •

edited by ghost

Loading

tniessen commented Sep 14, 2018 •

edited

Loading

mscdex Sep 14, 2018

tniessen Sep 14, 2018

mscdex Sep 14, 2018

mscdex Sep 14, 2018

ghost Sep 14, 2018

mscdex Sep 15, 2018

ghost commented Sep 14, 2018 •

edited by ghost

Loading

tniessen commented Sep 15, 2018 •

edited

Loading

mscdex Sep 15, 2018

mscdex Sep 15, 2018

mscdex Sep 15, 2018

ghost commented Sep 15, 2018

Trott commented Sep 15, 2018

tniessen commented Sep 15, 2018

tniessen commented Sep 17, 2018

tniessen left a comment

tniessen commented Sep 17, 2018 •

edited

Loading

tniessen commented Sep 18, 2018

ghost commented Sep 19, 2018

lib: remove useless input parameters and make 'digest' clear in the doc #22858

lib: remove useless input parameters and make 'digest' clear in the doc #22858

Conversation

ghost commented Sep 14, 2018 • edited by ghost Loading

tniessen commented Sep 14, 2018

ghost commented Sep 14, 2018 • edited by ghost Loading

tniessen commented Sep 14, 2018 • edited Loading

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

ghost commented Sep 14, 2018 • edited by ghost Loading

tniessen commented Sep 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Sep 15, 2018

Trott commented Sep 15, 2018

tniessen commented Sep 15, 2018

tniessen commented Sep 17, 2018

tniessen left a comment

Choose a reason for hiding this comment

tniessen commented Sep 17, 2018 • edited Loading

tniessen commented Sep 18, 2018

ghost commented Sep 19, 2018

ghost commented Sep 14, 2018 •

edited by ghost

Loading

ghost commented Sep 14, 2018 •

edited by ghost

Loading

tniessen commented Sep 14, 2018 •

edited

Loading

ghost commented Sep 14, 2018 •

edited by ghost

Loading

tniessen commented Sep 15, 2018 •

edited

Loading

tniessen commented Sep 17, 2018 •

edited

Loading