New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crypto: upgrade pbkdf2 without digest to an error #11305

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
9 participants
@jasnell
Member

jasnell commented Feb 10, 2017

Commit a116358 added a deprecation warning when pbkdf2 was called without an explicit digest argument. This was because the default digest is sha1, which is not-recommended from a security point of view. This upgrades it to a runtime error when digest is undefined per the plan discussed in the original issue.

Ref: a116358

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
Affected core subsystem(s)

crypto

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell
Member

jasnell commented Feb 10, 2017

@bnoordhuis

LGTM with style nits and a suggestion.

Show outdated Hide outdated lib/crypto.js
Show outdated Hide outdated lib/crypto.js
Show outdated Hide outdated doc/api/deprecations.md
Show outdated Hide outdated doc/api/deprecations.md
Show outdated Hide outdated lib/crypto.js
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 11, 2017

Member

to be honest, not sure what we should do with null. the current code only prints the deprecation notice if it's undefined so this only throws if it's undefined. The assumption, I believe, is that passing null is more explicit and is therefore ok.

Member

jasnell commented Feb 11, 2017

to be honest, not sure what we should do with null. the current code only prints the deprecation notice if it's undefined so this only throws if it's undefined. The assumption, I believe, is that passing null is more explicit and is therefore ok.

@indutny

LGTM

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell
Member

jasnell commented Feb 12, 2017

Updated to address feedback. New CI: https://ci.nodejs.org/job/node-test-pull-request/6365/

crypto: upgrade pbkdf2 without digest to an error
Commit a116358 added a deprecation warning when pbkdf2 was called without an
explicit `digest` argument. This was because the default digest is `sha1`,
which is not-recommended from a security point of view. This upgrades it
to a runtime error when `digest` is undefined per the plan discussed in
the original issue.

Ref: a116358
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 13, 2017

Member

@nodejs/ctc ... any further thoughts?

Member

jasnell commented Feb 13, 2017

@nodejs/ctc ... any further thoughts?

@@ -562,15 +556,17 @@ exports.pbkdf2 = function(password,
exports.pbkdf2Sync = function(password, salt, iterations, keylen, digest) {

This comment has been minimized.

@thefourtheye

thefourtheye Feb 13, 2017

Contributor

Let's simply do exports.pbkdf2Sync = pbkdf2

@thefourtheye

thefourtheye Feb 13, 2017

Contributor

Let's simply do exports.pbkdf2Sync = pbkdf2

This comment has been minimized.

@jasnell

jasnell Feb 13, 2017

Member

That would change the signature of pbkdf2Sync (it would accept the callback argument and change the value of pbkdf2Sync.length).

@jasnell

jasnell Feb 13, 2017

Member

That would change the signature of pbkdf2Sync (it would accept the callback argument and change the value of pbkdf2Sync.length).

@Fishrock123

Concept and API (error) change LGTM, no comment on the code

jasnell added a commit that referenced this pull request Feb 13, 2017

crypto: upgrade pbkdf2 without digest to an error
Commit a116358 added a deprecation warning when pbkdf2 was called without an
explicit `digest` argument. This was because the default digest is `sha1`,
which is not-recommended from a security point of view. This upgrades it
to a runtime error when `digest` is undefined per the plan discussed in
the original issue.

Ref: a116358

PR-URL: #11305
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 13, 2017

Member

Landed in 9f74184

Member

jasnell commented Feb 13, 2017

Landed in 9f74184

@jasnell jasnell closed this Feb 13, 2017

@yosuke-furukawa yosuke-furukawa referenced this pull request Feb 15, 2017

Closed

test: improve crypto coverage #11280

3 of 3 tasks complete

addaleax added a commit to addaleax/express that referenced this pull request Feb 15, 2017

examples: fix pbkdf2 invocation
Calling `crypto.pbkdf2()` without a digest has been deprecated in Node
and is scheduled to be broken in Node 8.

Fix this by actually passing a digest.

ref: nodejs/node#11305
@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Feb 15, 2017

Member

@jasnell This breaks CITGM because it was used in the express tests in the deprecated way… it’s “just” in an example script, but we really should have run that on this :/

I opened expressjs/express#3207 but there might be another problem with express + Node master… I’m looking into it

Member

addaleax commented Feb 15, 2017

@jasnell This breaks CITGM because it was used in the express tests in the deprecated way… it’s “just” in an example script, but we really should have run that on this :/

I opened expressjs/express#3207 but there might be another problem with express + Node master… I’m looking into it

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 15, 2017

Member

Argh... Ok.

Member

jasnell commented Feb 15, 2017

Argh... Ok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment