From 634de22cae26f7a2173ae8e60f0c3c7a875f621a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Wed, 10 Jul 2024 18:38:00 +0200 Subject: [PATCH] crypto: remove ERR_CRYPTO_SCRYPT_INVALID_PARAMETER It is confusing to have both ERR_CRYPTO_SCRYPT_INVALID_PARAMETER and ERR_CRYPTO_INVALID_SCRYPT_PARAMS. The former was the original error code, added in 371103dae8b97264471e17de1989199ffcd2718e, but parameter validation gradually changed and now produces ERR_CRYPTO_INVALID_SCRYPT_PARAMS for all parameter validation errors coming from OpenSSL, as well as different error codes for validation errors coming from JavaScript. The only remaining use of ERR_CRYPTO_SCRYPT_INVALID_PARAMETER is in the validation logic that ensures that no two synonymous options were passed. We already have an error code for that particular case, ERR_INCOMPATIBLE_OPTION_PAIR, so replace these last instances of ERR_CRYPTO_SCRYPT_INVALID_PARAMETER with that error code and remove ERR_CRYPTO_SCRYPT_INVALID_PARAMETER. If there ever is need again for such an error code, we can just use ERR_CRYPTO_INVALID_SCRYPT_PARAMS. Refs: https://github.com/nodejs/node/pull/35093 Refs: https://github.com/nodejs/node/pull/21525 Refs: https://github.com/nodejs/node/pull/20816 PR-URL: https://github.com/nodejs/node/pull/53305 Reviewed-By: James M Snell Reviewed-By: Filip Skokan Reviewed-By: Luigi Pinca Reviewed-By: Richard Lau Reviewed-By: Benjamin Gruenbaum --- doc/api/errors.md | 23 +++++++++++++++-------- lib/internal/crypto/scrypt.js | 8 ++++---- lib/internal/errors.js | 1 - test/parallel/test-crypto-scrypt.js | 22 +++++++++++++++++++--- 4 files changed, 38 insertions(+), 16 deletions(-) diff --git a/doc/api/errors.md b/doc/api/errors.md index 716bc9bec37de2..22be02830f0d98 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -1010,7 +1010,8 @@ An invalid message length was provided. added: v15.0.0 --> -Invalid scrypt algorithm parameters were provided. +One or more [`crypto.scrypt()`][] or [`crypto.scryptSync()`][] parameters are +outside their legal range. @@ -1070,13 +1071,6 @@ A crypto operation failed for an otherwise unspecified reason. The PBKDF2 algorithm failed for unspecified reasons. OpenSSL does not provide more details and therefore neither does Node.js. - - -### `ERR_CRYPTO_SCRYPT_INVALID_PARAMETER` - -One or more [`crypto.scrypt()`][] or [`crypto.scryptSync()`][] parameters are -outside their legal range. - ### `ERR_CRYPTO_SCRYPT_NOT_SUPPORTED` @@ -3271,6 +3265,18 @@ The UTF-16 encoding was used with [`hash.digest()`][]. While the causing the method to return a string rather than a `Buffer`, the UTF-16 encoding (e.g. `ucs` or `utf16le`) is not supported. + + +### `ERR_CRYPTO_SCRYPT_INVALID_PARAMETER` + + + +An incompatible combination of options was passed to [`crypto.scrypt()`][] or +[`crypto.scryptSync()`][]. New versions of Node.js use the error code +[`ERR_INCOMPATIBLE_OPTION_PAIR`][] instead, which is consistent with other APIs. + ### `ERR_FS_INVALID_SYMLINK_TYPE` @@ -4046,6 +4052,7 @@ An error occurred trying to allocate memory. This should never happen. [`--no-addons`]: cli.md#--no-addons [`--unhandled-rejections`]: cli.md#--unhandled-rejectionsmode [`Class: assert.AssertionError`]: assert.md#class-assertassertionerror +[`ERR_INCOMPATIBLE_OPTION_PAIR`]: #err_incompatible_option_pair [`ERR_INVALID_ARG_TYPE`]: #err_invalid_arg_type [`ERR_MISSING_MESSAGE_PORT_IN_TRANSFER_LIST`]: #err_missing_message_port_in_transfer_list [`ERR_MISSING_TRANSFERABLE_IN_TRANSFER_LIST`]: #err_missing_transferable_in_transfer_list diff --git a/lib/internal/crypto/scrypt.js b/lib/internal/crypto/scrypt.js index a90d317c552331..ce170dff3b8344 100644 --- a/lib/internal/crypto/scrypt.js +++ b/lib/internal/crypto/scrypt.js @@ -21,8 +21,8 @@ const { const { codes: { - ERR_CRYPTO_SCRYPT_INVALID_PARAMETER, ERR_CRYPTO_SCRYPT_NOT_SUPPORTED, + ERR_INCOMPATIBLE_OPTION_PAIR, }, } = require('internal/errors'); @@ -92,7 +92,7 @@ function check(password, salt, keylen, options) { validateUint32(N, 'N'); } if (options.cost !== undefined) { - if (has_N) throw new ERR_CRYPTO_SCRYPT_INVALID_PARAMETER(); + if (has_N) throw new ERR_INCOMPATIBLE_OPTION_PAIR('N', 'cost'); N = options.cost; validateUint32(N, 'cost'); } @@ -102,7 +102,7 @@ function check(password, salt, keylen, options) { validateUint32(r, 'r'); } if (options.blockSize !== undefined) { - if (has_r) throw new ERR_CRYPTO_SCRYPT_INVALID_PARAMETER(); + if (has_r) throw new ERR_INCOMPATIBLE_OPTION_PAIR('r', 'blockSize'); r = options.blockSize; validateUint32(r, 'blockSize'); } @@ -112,7 +112,7 @@ function check(password, salt, keylen, options) { validateUint32(p, 'p'); } if (options.parallelization !== undefined) { - if (has_p) throw new ERR_CRYPTO_SCRYPT_INVALID_PARAMETER(); + if (has_p) throw new ERR_INCOMPATIBLE_OPTION_PAIR('p', 'parallelization'); p = options.parallelization; validateUint32(p, 'parallelization'); } diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 7dede891721547..a8543b51a6da4b 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -1168,7 +1168,6 @@ E('ERR_CRYPTO_INVALID_KEY_OBJECT_TYPE', 'Invalid key object type %s, expected %s.', TypeError); E('ERR_CRYPTO_INVALID_STATE', 'Invalid state for operation %s', Error); E('ERR_CRYPTO_PBKDF2_ERROR', 'PBKDF2 error', Error); -E('ERR_CRYPTO_SCRYPT_INVALID_PARAMETER', 'Invalid scrypt parameter', Error); E('ERR_CRYPTO_SCRYPT_NOT_SUPPORTED', 'Scrypt algorithm not supported', Error); // Switch to TypeError. The current implementation does not seem right. E('ERR_CRYPTO_SIGN_KEY_REQUIRED', 'No key provided to sign', Error); diff --git a/test/parallel/test-crypto-scrypt.js b/test/parallel/test-crypto-scrypt.js index 61bd65fc92678c..0cf6efb8eed997 100644 --- a/test/parallel/test-crypto-scrypt.js +++ b/test/parallel/test-crypto-scrypt.js @@ -93,12 +93,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 ]; @@ -176,6 +180,18 @@ 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); +} + for (const options of toobig) { const expected = { message: /Invalid scrypt params:.*memory limit exceeded/,