crypto: pbkdf.js deprecate digest overload. by tomgco · Pull Request #4047 · nodejs/node (original) (raw)

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 👯

mscdex added the crypto

Issues and PRs related to the crypto subsystem.

label

Nov 27, 2015

bnoordhuis

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

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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 added the semver-major

PRs that contain breaking changes and should be released in the next major version.

label

Dec 7, 2015

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

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

indutny

@@ -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');

Choose a reason for hiding this comment

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

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.

shigeki pushed a commit that referenced this pull request

Jan 22, 2016

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.

scovetta pushed a commit to scovetta/node that referenced this pull request

Apr 2, 2016

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

jasnell added a commit that referenced this pull request

Apr 26, 2016