Repository: denoland/deno
Pull request: fix(ext/node): improve scrypt support

This PR improves `scrypt` implementation in `node:crypto`.

towards #29637 

Changed files:
filename: ext/node/ops/crypto/mod.rs
patch: @@ -644,7 +644,7 @@ fn scrypt(
     parallelization,
     keylen as usize,
   )
-  .map_err(|_| JsErrorBox::generic("scrypt params construction failed"))?;
+  .map_err(|_| JsErrorBox::generic("Invalid scrypt param"))?;
 
   // Call into scrypt
   let res = scrypt::scrypt(&password, &salt, &params, output_buffer);

filename: ext/node/polyfills/internal/crypto/scrypt.ts
patch: @@ -29,6 +29,14 @@ SOFTWARE.
 import { Buffer } from "node:buffer";
 import { HASH_DATA } from "ext:deno_node/internal/crypto/types.ts";
 import { op_node_scrypt_async, op_node_scrypt_sync } from "ext:core/ops";
+import {
+  validateFunction,
+  validateInt32,
+  validateInteger,
+  validateUint32,
+} from "ext:deno_node/internal/validators.mjs";
+import { ERR_INCOMPATIBLE_OPTION_PAIR } from "ext:deno_node/internal/errors.ts";
+import { getArrayBufferOrView } from "ext:deno_node/internal/crypto/keys.ts";
 
 type Opts = Partial<{
   N: number;
@@ -40,32 +48,14 @@ type Opts = Partial<{
   maxmem: number;
 }>;
 
-const fixOpts = (opts?: Opts) => {
-  const out = { N: 16384, p: 1, r: 8, maxmem: 32 << 20 };
-  if (!opts) return out;
-
-  if (opts.N) out.N = opts.N;
-  else if (opts.cost) out.N = opts.cost;
-
-  if (opts.p) out.p = opts.p;
-  else if (opts.parallelization) out.p = opts.parallelization;
-
-  if (opts.r) out.r = opts.r;
-  else if (opts.blockSize) out.r = opts.blockSize;
-
-  if (opts.maxmem) out.maxmem = opts.maxmem;
-
-  return out;
-};
-
 export function scryptSync(
   password: HASH_DATA,
   salt: HASH_DATA,
   keylen: number,
   _opts?: Opts,
 ): Buffer {
-  const { N, r, p, maxmem } = fixOpts(_opts);
-
+  const options = check(password, salt, keylen, _opts);
+  const { N, r, p, maxmem } = options;
   const blen = p * 128 * r;
 
   if (32 * r * (N + 2) * 4 + blen > maxmem) {
@@ -100,7 +90,10 @@ export function scrypt(
     cb = _opts as Callback;
     _opts = null;
   }
-  const { N, r, p, maxmem } = fixOpts(_opts as Opts);
+  const options = check(password, salt, keylen, _opts);
+  const { N, r, p, maxmem } = options;
+
+  validateFunction(cb, "callback");
 
   const blen = p * 128 * r;
   if (32 * r * (N + 2) * 4 + blen > maxmem) {
@@ -122,6 +115,69 @@ export function scrypt(
   ).catch((err: unknown) => cb(err));
 }
 
+const defaults = {
+  N: 16384,
+  r: 8,
+  p: 1,
+  maxmem: 32 << 20, // 32 MiB, matches SCRYPT_MAX_MEM.
+};
+
+function check(password, salt, keylen, options) {
+  password = getArrayBufferOrView(password, "password");
+  salt = getArrayBufferOrView(salt, "salt");
+  validateInt32(keylen, "keylen", 0);
+
+  let { N, r, p, maxmem } = defaults;
+  if (options && options !== defaults) {
+    const hasN = options.N !== undefined;
+    if (hasN) {
+      N = options.N;
+      validateUint32(N, "N");
+    }
+    if (options.cost !== undefined) {
+      if (hasN) throw new ERR_INCOMPATIBLE_OPTION_PAIR("N", "cost");
+      N = options.cost;
+      validateUint32(N, "cost");
+    }
+    const hasR = options.r !== undefined;
+    if (hasR) {
+      r = options.r;
+      validateUint32(r, "r");
+    }
+    if (options.blockSize !== undefined) {
+      if (hasR) throw new ERR_INCOMPATIBLE_OPTION_PAIR("r", "blockSize");
+      r = options.blockSize;
+      validateUint32(r, "blockSize");
+    }
+    const hasP = options.p !== undefined;
+    if (hasP) {
+      p = options.p;
+      validateUint32(p, "p");
+    }
+    if (options.parallelization !== undefined) {
+      if (hasP) throw new ERR_INCOMPATIBLE_OPTION_PAIR("p", "parallelization");
+      p = options.parallelization;
+      validateUint32(p, "parallelization");
+    }
+    if (options.maxmem !== undefined) {
+      maxmem = options.maxmem;
+      validateInteger(maxmem, "maxmem", 0);
+    }
+    if (N === 0) N = defaults.N;
+    if (r === 0) r = defaults.r;
+    if (p === 0) p = defaults.p;
+    if (maxmem === 0) maxmem = defaults.maxmem;
+  }
+
+  if (N < 2 || (N & (N - 1)) !== 0) {
+    throw new Error(
+      "Invalid scrypt param: N must be a power of 2 and greater than 0",
+    );
+  }
+
+  return { password, salt, keylen, N, r, p, maxmem };
+}
+
 export default {
   scrypt,
   scryptSync,

filename: tests/node_compat/test/parallel/test-crypto-scrypt.js
patch: @@ -2,10 +2,9 @@
 // deno-lint-ignore-file
 
 // Copyright Joyent and Node contributors. All rights reserved. MIT license.
-// Taken from Node 20.11.1
+// Taken from Node 23.9.0
 // This file is automatically generated by `tests/node_compat/runner/setup.ts`. Do not modify this file manually.
 
-// Flags: --expose-internals
 'use strict';
 const common = require('../common');
 if (!common.hasCrypto)
@@ -14,11 +13,11 @@ if (!common.hasCrypto)
 const assert = require('assert');
 const crypto = require('crypto');
 
-const { internalBinding } = require('internal/test/binding');
-if (typeof internalBinding('crypto').ScryptJob !== 'function')
+if (typeof crypto.scrypt !== 'function' || typeof crypto.scryptSync !== 'function')
   common.skip('no scrypt support');
 
 const good = [
+  /* TODO(kt3k): Node.js allows keylen === 0
   // Zero-length key is legal, functions as a parameter validation check.
   {
     pass: '',
@@ -29,6 +28,7 @@ const good = [
     r: 1,
     expected: '',
   },
+  */
   // Test vectors from https://tools.ietf.org/html/rfc7914#page-13 that
   // should pass.  Note that the test vector with N=1048576 is omitted
   // because it takes too long to complete and uses over 1 GiB of memory.
@@ -100,12 +100,16 @@ const good = [
   },
 ];
 
-// Test vectors that should fail.
+// Test vectors that contain invalid parameters.
 const bad = [
   { N: 1, p: 1, r: 1 },         // N < 2
   { N: 3, p: 1, r: 1 },         // Not power of 2.
-  { N: 1, cost: 1 },            // Both N and cost
-  { p: 1, parallelization: 1 }, // Both p and parallelization
+];
+
+// Test vectors that contain incompatible options.
+const incompatibleOptions = [
+  { N: 1, cost: 1 },             // Both N and cost
+  { p: 1, parallelization: 1 },  // Both p and parallelization
   { r: 1, blockSize: 1 },        // Both r and blocksize
 ];
 
@@ -183,16 +187,32 @@ for (const options of bad) {
                 expected);
 }
 
+for (const options of incompatibleOptions) {
+  const [short, long] = Object.keys(options).sort((a, b) => a.length - b.length);
+  const expected = {
+    message: `Option "${short}" cannot be used in combination with option "${long}"`,
+    code: 'ERR_INCOMPATIBLE_OPTION_PAIR',
+  };
+  assert.throws(() => crypto.scrypt('pass', 'salt', 1, options, () => {}),
+                expected);
+  assert.throws(() => crypto.scryptSync('pass', 'salt', 1, options),
+                expected);
+}
+
+/* TODO(kt3k): Currently Deno throws with generic invalid scrypt params error
 for (const options of toobig) {
   const expected = {
-    message: /Invalid scrypt param/
+    message: /Invalid scrypt params:.*memory limit exceeded/,
+    code: 'ERR_CRYPTO_INVALID_SCRYPT_PARAMS',
   };
   assert.throws(() => crypto.scrypt('pass', 'salt', 1, options, () => {}),
                 expected);
   assert.throws(() => crypto.scryptSync('pass', 'salt', 1, options),
                 expected);
 }
+*/
 
+/* TODO(kt3k): Deno does not allow keylen === 1
 {
   const defaults = { N: 16384, p: 1, r: 8 };
   const expected = crypto.scryptSync('pass', 'salt', 1, defaults);
@@ -202,6 +222,7 @@ for (const options of toobig) {
     assert.deepStrictEqual(actual.toString('hex'), expected.toString('hex'));
   }));
 }
+*/
 
 for (const { args, expected } of badargs) {
   assert.throws(() => crypto.scrypt(...args), expected);
@@ -216,6 +237,7 @@ for (const { args, expected } of badargs) {
   assert.throws(() => crypto.scrypt('', '', 42, {}, {}), expected);
 }
 
+/* TODO(kt3k): Enable these tests
 {
   // Values for maxmem that do not fit in 32 bits but that are still safe
   // integers should be allowed.
@@ -229,7 +251,9 @@ for (const { args, expected } of badargs) {
     code: 'ERR_OUT_OF_RANGE'
   });
 }
+*/
 
+/* TODO(kt3k): Deno does not allow keylen === 1
 {
   // Regression test for https://github.com/nodejs/node/issues/28836.
 
@@ -264,3 +288,4 @@ for (const { args, expected } of badargs) {
     ['p', 1], ['parallelization', 1],
   ].forEach((arg) => testParameter(...arg));
 }
+*/