crypto: pbkdf.js deprecate digest overload. by tomgco · Pull Request #4047 · nodejs/node · GitHub

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 👯

/cc @nodejs/crypto

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.

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

LGTM

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

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

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

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

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

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.

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.

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 <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>

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 <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>

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

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

crypto: pbkdf.js deprecate digest overload. by tomgco · Pull Request #4047 · nodejs/node · GitHub

Uh oh!

Uh oh!

crypto: pbkdf.js deprecate digest overload. - #4047

crypto: pbkdf.js deprecate digest overload.#4047tomgco wants to merge 1 commit intonodejs:masternodejs/node:masterfrom tomgco:pbkdf2-deprecate-overridetomgco/node:pbkdf2-deprecate-overrideCopy head branch name to clipboard

Conversation

tomgco commented Nov 27, 2015

Uh oh!

rvagg commented Nov 30, 2015

Uh oh!

bnoordhuis Nov 30, 2015

Choose a reason for hiding this comment

Uh oh!

tomgco Dec 4, 2015

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Dec 7, 2015

Uh oh!

jasnell commented Dec 11, 2015

Uh oh!

tomgco commented Jan 8, 2016

Uh oh!

rvagg commented Jan 12, 2016

Uh oh!

jasnell commented Jan 12, 2016

Uh oh!

indutny Jan 12, 2016

Choose a reason for hiding this comment

Uh oh!

indutny commented Jan 12, 2016

Uh oh!

tomgco commented Jan 22, 2016

Uh oh!

shigeki commented Jan 22, 2016

Uh oh!

shigeki commented Jan 22, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

crypto: pbkdf.js deprecate digest overload.#4047
tomgco wants to merge 1 commit into
nodejs:masterfrom
tomgco:pbkdf2-deprecate-override