Skip to content

crypto: deprecate digest == null in PBKDF2 - #22861

Closed
tniessen wants to merge 1 commit into
nodejs:masterfrom
tniessen:crypto-fix-pbkdf2-behavior-for-digest-null
Closed

crypto: deprecate digest == null in PBKDF2#22861
tniessen wants to merge 1 commit into
nodejs:masterfrom
tniessen:crypto-fix-pbkdf2-behavior-for-digest-null

Conversation

@tniessen

@tniessentniessen commented Sep 14, 2018

Copy link
Copy Markdown
Member

I assume that permitting digest === null was unintentional when digest === undefined was deprecated since their behavior was equivalent. The sha1 default for digest === null has somehow made it through refactoring of the PBKDF2 module multiple times, even though digest === undefined has been EOL for some time now.

This change deprecates setting digest to null so we can fix the behavior in Node.js 12 or so.

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

@tniessentniessen added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 14, 2018
@tniessentniessen added this to the 11.0.0 milestone Sep 14, 2018
@nodejs-github-botnodejs-github-bot added the crypto Issues and PRs related to the crypto subsystem. label Sep 14, 2018
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@tniessen

Copy link
Copy Markdown
MemberAuthor

cc @nodejs/tsc @nodejs/security-wg @nodejs/crypto

Comment threaddoc/api/deprecations.md Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

an -> a

Comment threadlib/internal/crypto/pbkdf2.js Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If I'm not wrong this is emitted every time check() is called, is this wanted?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

check is called once per crypto.pbkdf2 / crypto.pbkdf2Sync call. Would you prefer to only warn once throughout the whole execution?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, I think it's better. As is it may create too much noise.

@tniessentniessen added the deprecations Issues and PRs related to deprecations. label Sep 17, 2018
@tniessen

tniessen commented Sep 17, 2018

Copy link
Copy Markdown
MemberAuthor

@lpinca I rewrote it to use the deprecate function, that should take care of it. PTAL.

CI: https://ci.nodejs.org/job/node-test-pull-request/17234/
Resumed in https://ci.nodejs.org/job/node-test-pull-request/17244/
Resumed in https://ci.nodejs.org/job/node-test-pull-request/17266/

I assume that permitting digest === null was unintentional when
digest === undefined was deprecated since their behavior was
equivalent. The sha1 default for digest === null has somehow made it
through refactoring of the PBKDF2 module multiple times, even though
digest === undefined has been EOL for some time now.
This change deprecates setting digest to null so we can fix the
behavior in Node.js 12 or so.
@tniessen
tniessenforce-pushed the crypto-fix-pbkdf2-behavior-for-digest-null branch from 3aef5e5 to 6bcfb6fCompareSeptember 18, 2018 14:14
@tniessen

tniessen commented Sep 18, 2018

Copy link
Copy Markdown
MemberAuthor

@tniessen

tniessen commented Sep 19, 2018

Copy link
Copy Markdown
MemberAuthor

Landed in 19ad6b8, thanks for reviewing.

tniessen added a commit that referenced this pull request Sep 19, 2018
I assume that permitting digest === null was unintentional when
digest === undefined was deprecated since their behavior was
equivalent. The sha1 default for digest === null has somehow made it
through refactoring of the PBKDF2 module multiple times, even though
digest === undefined has been EOL for some time now.
This change deprecates setting digest to null so we can fix the
behavior in Node.js 12 or so.
PR-URL: #22861
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cryptoIssues and PRs related to the crypto subsystem.deprecationsIssues and PRs related to deprecations.semver-majorPRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@tniessen@nodejs-github-bot@jasnell@addaleax@lpinca@vsemozhetbyt