crypto: deprecate digest == null in PBKDF2

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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
nodejs · Sep 19, 2018 · 19ad6b8 · 19ad6b8
1 parent 92fd4fc
commit 19ad6b8
Show file tree

Hide file tree

Showing 4 changed files with 29 additions and 11 deletions.
diff --git a/doc/api/crypto.md b/doc/api/crypto.md
@@ -1786,8 +1786,8 @@ otherwise `err` will be `null`. By default, the successfully generated
 `derivedKey` will be passed to the callback as a [`Buffer`][]. An error will be
 thrown if any of the input arguments specify invalid values or types.
 
-If `digest` is `null`, `'sha1'` will be used. This behavior will be deprecated
-in a future version of Node.js.
+If `digest` is `null`, `'sha1'` will be used. This behavior is deprecated,
+please specify a `digest` explicitely.
 
 The `iterations` argument must be a number set as high as possible. The
 higher the number of iterations, the more secure the derived key will be,
@@ -1852,8 +1852,8 @@ applied to derive a key of the requested byte length (`keylen`) from the
 If an error occurs an `Error` will be thrown, otherwise the derived key will be
 returned as a [`Buffer`][].
 
-If `digest` is `null`, `'sha1'` will be used. This behavior will be deprecated
-in a future version of Node.js.
+If `digest` is `null`, `'sha1'` will be used. This behavior is deprecated,
+please specify a `digest` explicitely.
 
 The `iterations` argument must be a number set as high as possible. The
 higher the number of iterations, the more secure the derived key will be,

diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md
@@ -227,24 +227,31 @@ to the `constants` property exposed by the relevant module. For instance,
 ### DEP0009: crypto.pbkdf2 without digest
 <!-- YAML
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/22861
+    description: Runtime deprecation (for `digest === null`).
   - version: v8.0.0
     pr-url: https://github.com/nodejs/node/pull/11305
-    description: End-of-Life.
+    description: End-of-Life (for `digest === undefined`).
   - version: v6.12.0
     pr-url: https://github.com/nodejs/node/pull/10116
     description: A deprecation code has been assigned.
   - version: v6.0.0
     pr-url: https://github.com/nodejs/node/pull/4047
-    description: Runtime deprecation.
+    description: Runtime deprecation (for `digest === undefined`).
 -->
 
-Type: End-of-Life
+Type: Runtime
 
 Use of the [`crypto.pbkdf2()`][] API without specifying a digest was deprecated
 in Node.js 6.0 because the method defaulted to using the non-recommended
 `'SHA1'` digest. Previously, a deprecation warning was printed. Starting in
-Node.js 8.0.0, calling `crypto.pbkdf2()` or `crypto.pbkdf2Sync()` with an
-undefined `digest` will throw a `TypeError`.
+Node.js 8.0.0, calling `crypto.pbkdf2()` or `crypto.pbkdf2Sync()` with
+`digest` set to `undefined` will throw a `TypeError`.
+
+Beginning in Node.js REPLACEME, calling these functions with `digest` set to
+`null` will print a deprecation warning to align with the behavior when `digest`
+is `undefined`.
 
 <a id="DEP0010"></a>
 ### DEP0010: crypto.createCredentials

diff --git a/lib/internal/crypto/pbkdf2.js b/lib/internal/crypto/pbkdf2.js
@@ -5,6 +5,7 @@ const { AsyncWrap, Providers } = internalBinding('async_wrap');
 const { Buffer } = require('buffer');
 const { pbkdf2: _pbkdf2 } = internalBinding('crypto');
 const { validateUint32 } = require('internal/validators');
+const { deprecate } = require('internal/util');
 const {
   ERR_CRYPTO_INVALID_DIGEST,
   ERR_CRYPTO_PBKDF2_ERROR,
@@ -51,11 +52,16 @@ function pbkdf2Sync(password, salt, iterations, keylen, digest) {
   return keybuf.toString(encoding);
 }
 
+const defaultDigest = deprecate(() => 'sha1',
+                                'Calling pbkdf2 or pbkdf2Sync with "digest" ' +
+                                'set to null is deprecated.',
+                                'DEP0009');
+
 function check(password, salt, iterations, keylen, digest) {
   if (typeof digest !== 'string') {
     if (digest !== null)
       throw new ERR_INVALID_ARG_TYPE('digest', ['string', 'null'], digest);
-    digest = 'sha1';
+    digest = defaultDigest();
   }
 
   password = validateArrayBufferView(password, 'password');

diff --git a/test/parallel/test-crypto-pbkdf2.js b/test/parallel/test-crypto-pbkdf2.js
@@ -6,6 +6,11 @@ if (!common.hasCrypto)
 const assert = require('assert');
 const crypto = require('crypto');
 
+common.expectWarning(
+  'DeprecationWarning',
+  'Calling pbkdf2 or pbkdf2Sync with "digest" set to null is deprecated.',
+  'DEP0009');
+
 //
 // Test PBKDF2 with RFC 6070 test vectors (except #4)
 //
@@ -64,7 +69,7 @@ assert.throws(
 );
 
 assert.throws(
-  () => crypto.pbkdf2Sync('password', 'salt', -1, 20, null),
+  () => crypto.pbkdf2Sync('password', 'salt', -1, 20, 'sha1'),
   {
     code: 'ERR_OUT_OF_RANGE',
     name: 'RangeError [ERR_OUT_OF_RANGE]',