crypto: pbkdf.js deprecate digest overload. #4047

tomgco · 2015-11-27T10:55:04Z

As per #3292, this PR introduces a deprecation notice about removing
the 'default digest' overload which currently defaults to the soon
to be defunct SHA1 digest.

Instead it should be left up to the documentation and implementor to
suggest a suitable digest function.

I have also updated some of the tests to specifically use the ‘sha1’ digest
to not warn with this commit.

Thanks 👯

rvagg · 2015-11-30T10:27:19Z

/cc @nodejs/crypto

bnoordhuis · 2015-11-30T16:15:50Z

lib/crypto.js

+      callback = digest;
+      digest = undefined;
+    }, 'crypto.pbkdf2 without specifying a digest is deprecated. ' +
+       'Please specify a digest')();


Can you write this as:

if (typeof digest === 'function') { callback = digest; digest = undefined; pbkdf2DeprecationWarning(); }

Where pbkdf2DeprecationWarning is defined outside the function body:

const pbkdf2DeprecationWarning = internalUtil.deprecate(() => {}, 'crypto.pbkdf2 without...');

That avoids a) allocating a closure context for every call and b) printing the deprecation message repeatedly.

That avoids a) allocating a closure context for every call and b) printing the deprecation message repeatedly.

Ok, I have updated the PR to include your suggestions.

bnoordhuis · 2015-12-07T18:05:55Z

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

jasnell · 2015-12-11T19:20:05Z

LGTM

tomgco · 2016-01-08T13:59:59Z

@jasnell @bnoordhuis @rvagg I have updated this PR as it was conflicting, also I have implemented the warning for the crypto.pbkdf2Sync as well as crypto.pbkdf2.

rvagg · 2016-01-12T00:48:34Z

another CI https://ci.nodejs.org/job/node-test-commit/1700/

I'd prefer if someone from @nodejs/crypto took responsibility for landing this.

jasnell · 2016-01-12T20:18:43Z

@shigeki @indutny ... could either of you please take a look at this and sign off :-)

indutny · 2016-01-12T20:45:57Z

test/parallel/test-crypto-binary-default.js

@@ -633,10 +633,10 @@ assert.strictEqual(rsaVerify.verify(rsaPubPem, rsaSignature, 'hex'), true);
 // Test PBKDF2 with RFC 6070 test vectors (except #4)
 //
 function testPBKDF2(password, salt, iterations, keylen, expected) {
-  var actual = crypto.pbkdf2Sync(password, salt, iterations, keylen);
+  var actual = crypto.pbkdf2Sync(password, salt, iterations, keylen, 'sha1');


Let's use sha256 here. I know it is just a test, but people are actually picking the code from here sometimes.

indutny · 2016-01-12T20:47:30Z

Heya, looking great! Sorry for nit-picking, but we don't have any file named pbkdf.js, may I ask you to change it to just pbkdf in commit log? ;)

Otherwise (after fixing comments too), this LGTM! Thank you!

As per nodejs#3292, this PR introduces a deprecation notice about removing the 'default digest' overload which currently defaults to the soon to be defunct SHA1 digest. Instead it should be left up to the documentation and implementor to suggest a suitable digest function.

tomgco · 2016-01-22T14:17:05Z

Heya, looking great! Sorry for nit-picking, but we don't have any file named pbkdf.js, may I ask you to change it to just pbkdf in commit log? ;)

Otherwise (after fixing comments too), this LGTM! Thank you!

Thanks @indutny, I have no idea why I called it pbkdf.js! I have just swapped it for pbkdf2 in the commit log. :D I have also updated the tests mentioned in your comments to assert against sha256 instead of sha1.

shigeki · 2016-01-22T16:52:56Z

LGTM if CI is green . Running in in https://ci.nodejs.org/job/node-test-pull-request/1352.

As per #3292, this PR introduces a deprecation notice about removing the 'default digest' overload which currently defaults to the soon to be defunct SHA1 digest. Instead it should be left up to the documentation and implementor to suggest a suitable digest function. Ref: #3292 PR-URL: #4047 Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Shigeki Ohtsu <[email protected]>

shigeki · 2016-01-22T17:32:43Z

CI is green except Jenkins error on Win10. Landed in a116358. Thank you very much.

As per nodejs#3292, this PR introduces a deprecation notice about removing the 'default digest' overload which currently defaults to the soon to be defunct SHA1 digest. Instead it should be left up to the documentation and implementor to suggest a suitable digest function. Ref: nodejs#3292 PR-URL: nodejs#4047 Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Shigeki Ohtsu <[email protected]>

The following significant (semver-major) changes have been made since the previous Node v5.0.0 release. * Buffer * New Buffer constructors have been added [#4682](#4682) * Previously deprecated Buffer APIs are removed [#5048](#5048), [#4594](#4594) * Improved error handling [#4514](#4514) * Cluster * Worker emitted as first argument in 'message' event [#5361](#5361). * Crypto * Improved error handling [#3100](#3100), [#5611](#5611) * Simplified Certificate class bindings [#5382](#5382) * Improved control over FIPS mode [#5181](#5181) * pbkdf2 digest overloading is deprecated [#4047](#4047) * Dependencies * Reintroduce shared c-ares build support [#5775](#5775). * V8 updated to 5.0.71.31 [#6111](#6111). * DNS * Add resolvePtr API to query plain DNS PTR records [#4921](#4921). * Domains * Clear stack when no error handler [#4659](#4659). * File System * The `fs.realpath()` and `fs.realpathSync()` methods have been updated to use a more efficient libuv implementation. This change includes the removal of the `cache` argument and the method can throw new errors [#3594](#3594) * FS apis can now accept and return paths as Buffers [#5616](#5616). * Error handling and type checking improvements [#5616](#5616), [#5590](#5590), [#4518](#4518), [#3917](#3917). * fs.read's string interface is deprecated [#4525](#4525) * HTTP * 'clientError' can now be used to return custom errors from an HTTP server [#4557](#4557). * Modules * Current directory is now prioritized for local lookups [#5689](#5689) * Symbolic links are preserved when requiring modules [#5950](#5950) * Net * DNS hints no longer implicitly set [#6021](#6021). * Improved error handling and type checking [#5981](#5981), [#5733](#5733), [#2904](#2904) * Path * Improved type checking [#5348](#5348). * Process * Introduce process warnings API [#4782](#4782). * Throw exception when non-function passed to nextTick [#3860](#3860). * Readline * Emit key info unconditionally [#6024](#6024) * REPL * Assignment to `_` will emit a warning. [#5535](#5535) * Timers * Fail early when callback is not a function [#4362](#4362) * TLS * Rename 'clientError' to 'tlsClientError' [#4557](#4557) * SHA1 used for sessionIdContext [#3866](#3866) * TTY * Previously deprecated setRawMode wrapper is removed [#2528](#2528). * Util * Changes to Error object formatting [#4582](#4582). * Windows * Windows XP and Vista are no longer supported [#5167](#5167), [#5167](#5167).

The following significant (semver-major) changes have been made since the previous Node v5.0.0 release. * Buffer * New Buffer constructors have been added [#4682](#4682) * Previously deprecated Buffer APIs are removed [#5048](#5048), [#4594](#4594) * Improved error handling [#4514](#4514) * Cluster * Worker emitted as first argument in 'message' event [#5361](#5361). * Crypto * Improved error handling [#3100](#3100), [#5611](#5611) * Simplified Certificate class bindings [#5382](#5382) * Improved control over FIPS mode [#5181](#5181) * pbkdf2 digest overloading is deprecated [#4047](#4047) * Dependencies * Reintroduce shared c-ares build support [#5775](#5775). * V8 updated to 5.0.71.31 [#6111](#6111). * DNS * Add resolvePtr API to query plain DNS PTR records [#4921](#4921). * Domains * Clear stack when no error handler [#4659](#4659). * File System * The `fs.realpath()` and `fs.realpathSync()` methods have been updated to use a more efficient libuv implementation. This change includes the removal of the `cache` argument and the method can throw new errors [#3594](#3594) * FS apis can now accept and return paths as Buffers [#5616](#5616). * Error handling and type checking improvements [#5616](#5616), [#5590](#5590), [#4518](#4518), [#3917](#3917). * fs.read's string interface is deprecated [#4525](#4525) * HTTP * 'clientError' can now be used to return custom errors from an HTTP server [#4557](#4557). * Modules * Current directory is now prioritized for local lookups [#5689](#5689) * Symbolic links are preserved when requiring modules [#5950](#5950) * Net * DNS hints no longer implicitly set [#6021](#6021). * Improved error handling and type checking [#5981](#5981), [#5733](#5733), [#2904](#2904) * OS X * MACOSX_DEPLOYMENT_TARGET has been bumped up to 10.7 [#6402](#6402). * Path * Improved type checking [#5348](#5348). * Process * Introduce process warnings API [#4782](#4782). * Throw exception when non-function passed to nextTick [#3860](#3860). * Readline * Emit key info unconditionally [#6024](#6024) * REPL * Assignment to `_` will emit a warning. [#5535](#5535) * Timers * Fail early when callback is not a function [#4362](#4362) * TLS * Rename 'clientError' to 'tlsClientError' [#4557](#4557) * SHA1 used for sessionIdContext [#3866](#3866) * TTY * Previously deprecated setRawMode wrapper is removed [#2528](#2528). * Util * Changes to Error object formatting [#4582](#4582). * Windows * Windows XP and Vista are no longer supported [#5167](#5167), [#5167](#5167).

As of Node 6, crypto.pbkdf emits a deprecation warning when the digest isn't explicily set. Under certain conditions we are seeing this warning from Hapi's dependency Iron. Iron resolved this issue as of 4.0.4, which was introduced into Hapi as of 14.0.0. Since the level of effort appeared to be similar, I decided to bump us to the current version of Hapi. Node deprecation: nodejs/node#4047 Iron's resolution: hapijs/iron#43 As of Hapi v9, they have removed three build-in plugins from the core which we rely on inert (files and directories), vision (view templates), and h2o2 (proxy). hapijs/hapi#2682 Signed-off-by: Tyler Smalley <[email protected]>

Passing no digest to pbkdf2() has been deprecated and throws an error with the newest node version. Default value for the digest was "sha1" see: nodejs/node#4047

mscdex added the crypto Issues and PRs related to the crypto subsystem. label Nov 27, 2015

bnoordhuis reviewed Nov 30, 2015
View reviewed changes

tomgco force-pushed the pbkdf2-deprecate-override branch 2 times, most recently from 9582c02 to e1fff22 Compare December 4, 2015 10:53

bnoordhuis added the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 7, 2015

Trott force-pushed the master branch from 1e896a6 to 082cc8d Compare December 27, 2015 02:01

tomgco force-pushed the pbkdf2-deprecate-override branch 2 times, most recently from 0d769ed to 84beae3 Compare January 8, 2016 13:55

indutny reviewed Jan 12, 2016
View reviewed changes

tomgco force-pushed the pbkdf2-deprecate-override branch from 84beae3 to 8e8959d Compare January 22, 2016 14:13

shigeki closed this Jan 22, 2016

shigeki mentioned this pull request Jan 22, 2016

Proposal: crypto: Make the digest parameter required for pbkdf2. #3292

Closed

jasnell mentioned this pull request Mar 17, 2016

Planning for v6 #5766

Closed

jasnell mentioned this pull request Apr 19, 2016

What is new in v6? #6264

Closed

jasnell mentioned this pull request Apr 26, 2016

2016-04-26, Version 6.0.0 (Current) Release #6383

Closed

aegyed91 mentioned this pull request May 2, 2016

crypto.pbkdf2 with digest param florianheinemann/password-hash-and-salt#6

Closed

trdarr mentioned this pull request May 18, 2016

Node 6 DeprecationWarning: crypto.pbkdf2 without specifying a digest is deprecated. jcoglan/vault#32

Closed

lSoleyl added a commit to lSoleyl/endecrypt that referenced this pull request Apr 11, 2018

Added missing digest argument

ab3f6f3

Passing no digest to pbkdf2() has been deprecated and throws an error with the newest node version. Default value for the digest was "sha1" see: nodejs/node#4047

lSoleyl mentioned this pull request Apr 11, 2018

Added missing digest argument to support new node versions dcodeIO/endecrypt#3

Open

tniessen added the deprecations Issues and PRs related to deprecations. label Sep 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

crypto: pbkdf.js deprecate digest overload. #4047

crypto: pbkdf.js deprecate digest overload. #4047

tomgco commented Nov 27, 2015

rvagg commented Nov 30, 2015

bnoordhuis Nov 30, 2015

tomgco Dec 4, 2015

bnoordhuis commented Dec 7, 2015

jasnell commented Dec 11, 2015

tomgco commented Jan 8, 2016

rvagg commented Jan 12, 2016

jasnell commented Jan 12, 2016

indutny Jan 12, 2016

indutny commented Jan 12, 2016

tomgco commented Jan 22, 2016

shigeki commented Jan 22, 2016

shigeki commented Jan 22, 2016

crypto: pbkdf.js deprecate digest overload. #4047

crypto: pbkdf.js deprecate digest overload. #4047

Conversation

tomgco commented Nov 27, 2015

rvagg commented Nov 30, 2015

bnoordhuis Nov 30, 2015

Choose a reason for hiding this comment

tomgco Dec 4, 2015

Choose a reason for hiding this comment

bnoordhuis commented Dec 7, 2015

jasnell commented Dec 11, 2015

tomgco commented Jan 8, 2016

rvagg commented Jan 12, 2016

jasnell commented Jan 12, 2016

indutny Jan 12, 2016

Choose a reason for hiding this comment

indutny commented Jan 12, 2016

tomgco commented Jan 22, 2016

shigeki commented Jan 22, 2016

shigeki commented Jan 22, 2016