Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 0 additions & 14 deletions src/crypto/crypto_argon2.cc
Original file line number Diff line number Diff line change
Expand Up @@ -104,20 +104,6 @@ Maybe<void> Argon2Traits::AdditionalConfig(
config->type =
static_cast<ncrypto::Argon2Type>(args[offset + 8].As<Uint32>()->Value());

if (!ncrypto::argon2(config->pass,
config->salt,
config->lanes,
config->keylen,
config->memcost,
config->iter,
config->version,
config->secret,
config->ad,
config->type)) {
THROW_ERR_CRYPTO_INVALID_ARGON2_PARAMS(env);
return Nothing<void>();
}

Copy link
Copy Markdown
Contributor

@ranisalt ranisalt Apr 21, 2026

Choose a reason for hiding this comment

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

I particularly don't mind, but the other KDF functions are checking the params in AdditionalConfig and Argon2 now does not. Probably not relevant to the end user. Catching an execution error later on is not an issue as your new tests cover this. Would ✔️

return JustVoid();
}

Expand Down
2 changes: 0 additions & 2 deletions src/node_errors.h
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ void OOMErrorHandler(const char* location, const v8::OOMDetails& details);
V(ERR_CRYPTO_CUSTOM_ENGINE_NOT_SUPPORTED, Error) \
V(ERR_CRYPTO_INCOMPATIBLE_KEY_OPTIONS, Error) \
V(ERR_CRYPTO_INITIALIZATION_FAILED, Error) \
V(ERR_CRYPTO_INVALID_ARGON2_PARAMS, TypeError) \
V(ERR_CRYPTO_INVALID_AUTH_TAG, TypeError) \
V(ERR_CRYPTO_INVALID_COUNTER, TypeError) \
V(ERR_CRYPTO_INVALID_CURVE, TypeError) \
Expand Down Expand Up @@ -198,7 +197,6 @@ ERRORS_WITH_CODE(V)
V(ERR_CRYPTO_INCOMPATIBLE_KEY_OPTIONS, \
"The selected key encoding is incompatible with the key type") \
V(ERR_CRYPTO_INITIALIZATION_FAILED, "Initialization failed") \
V(ERR_CRYPTO_INVALID_ARGON2_PARAMS, "Invalid Argon2 params") \
V(ERR_CRYPTO_INVALID_AUTH_TAG, "Invalid authentication tag") \
V(ERR_CRYPTO_INVALID_COUNTER, "Invalid counter") \
V(ERR_CRYPTO_INVALID_CURVE, "Invalid EC curve name") \
Expand Down
59 changes: 59 additions & 0 deletions test/parallel/test-crypto-argon2-job.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
// Flags: --expose-internals --no-warnings
'use strict';
const common = require('../common');
if (!common.hasCrypto)
common.skip('missing crypto');

const { hasOpenSSL } = require('../common/crypto');

if (!hasOpenSSL(3, 2))
common.skip('requires OpenSSL >= 3.2');

// Exercises the native Argon2 job directly via internalBinding, bypassing
// the JS validators, to ensure that if invalid parameters ever reach the
// native layer they produce a clean error from the KDF rather than crashing,
// in both sync and async modes.

const assert = require('node:assert');
const { internalBinding } = require('internal/test/binding');
const {
Argon2Job,
kCryptoJobAsync,
kCryptoJobSync,
kTypeArgon2id,
} = internalBinding('crypto');

const pass = Buffer.from('password');
const salt = Buffer.alloc(16, 0x02);
const empty = Buffer.alloc(0);

// Parameters that OpenSSL's Argon2 KDF rejects.
const badParams = [
{ lanes: 0, keylen: 32, memcost: 16, iter: 1 }, // lanes < 1
{ lanes: 1, keylen: 32, memcost: 0, iter: 1 }, // memcost == 0
{ lanes: 1, keylen: 32, memcost: 16, iter: 0 }, // iter == 0
];

for (const { lanes, keylen, memcost, iter } of badParams) {
{
const job = new Argon2Job(
kCryptoJobSync, pass, salt, lanes, keylen, memcost, iter,
empty, empty, kTypeArgon2id);
const { 0: err, 1: result } = job.run();
assert.ok(err);
assert.match(err.message, /Argon2 derivation failed/);
assert.strictEqual(result, undefined);
}

{
const job = new Argon2Job(
kCryptoJobAsync, pass, salt, lanes, keylen, memcost, iter,
empty, empty, kTypeArgon2id);
job.ondone = common.mustCall((err, result) => {
assert.ok(err);
assert.match(err.message, /Argon2 derivation failed/);
assert.strictEqual(result, undefined);
});
job.run();
}
}
81 changes: 81 additions & 0 deletions test/pummel/test-crypto-argon2-nonblocking-constructor.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
// Flags: --expose-internals --no-warnings
'use strict';
const common = require('../common');
if (!common.hasCrypto)
common.skip('missing crypto');

const { hasOpenSSL } = require('../common/crypto');

if (!hasOpenSSL(3, 2))
common.skip('requires OpenSSL >= 3.2');

// Regression test for https://github.com/nodejs/node/issues/62861.
// `AdditionalConfig` used to invoke the full Argon2 KDF synchronously inside
// `new Argon2Job(...)` for the purpose of getting a parameter validation
// error.
//
// The fix removes the redundant KDF call from the constructor. This test
// asserts that the constructor returns in a small fraction of the time a
// full sync job takes. Pre-fix the constructor ran the KDF, and job.run()
// then ran it a second time in DeriveBits; post-fix the constructor does
// no KDF work at all.

const assert = require('node:assert');
const { internalBinding } = require('internal/test/binding');
const {
Argon2Job,
kCryptoJobAsync,
kCryptoJobSync,
kTypeArgon2id,
} = internalBinding('crypto');

const pass = Buffer.from('password');
const salt = Buffer.alloc(16, 0x02);
const empty = Buffer.alloc(0);

// Tuned so a single-threaded Argon2id derivation is expensive enough that
// scheduler/GC noise is negligible compared to it.
const kdfArgs = [
pass,
salt,
1, // lanes
32, // keylen
65536, // memcost
20, // iter
empty, // secret
empty, // associatedData
kTypeArgon2id,
];

// For each mode, measure the constructor and the derivation separately and
// assert that the constructor is a small fraction of the derivation. Pre-fix
// the constructor ran a full KDF, so ctorNs was comparable to runNs. Post-fix
// the constructor does no KDF work and must be orders of magnitude faster.
//
// For async mode the derivation happens on the thread pool, so runNs is
// measured from the start of run() until ondone fires.

// Sync: run() derives on the main thread and returns when done.
{
const ctorStart = process.hrtime.bigint();
const job = new Argon2Job(kCryptoJobSync, ...kdfArgs);
const ctorNs = process.hrtime.bigint() - ctorStart;
const runStart = process.hrtime.bigint();
const { 0: err } = job.run();
const runNs = process.hrtime.bigint() - runStart;
assert.strictEqual(err, undefined);
assert.ok(ctorNs * 10n < runNs);
}

// Async: run() dispatches to the thread pool; measure until ondone fires.
{
const ctorStart = process.hrtime.bigint();
const job = new Argon2Job(kCryptoJobAsync, ...kdfArgs);
const ctorNs = process.hrtime.bigint() - ctorStart;
const runStart = process.hrtime.bigint();
job.ondone = common.mustSucceed(() => {
const runNs = process.hrtime.bigint() - runStart;
assert.ok(ctorNs * 10n < runNs);
});
job.run();
}
Comment on lines +50 to +81
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.

This sort of test is bound to become flaky at some point since it relies on timing of CPU-bound operations, which is inherently unpredictable if the system is under load.

Would it be possible to instead configure the job with unreasonably large parameters such that, if the job were to be run, the test would crash or timeout? We can still measure how long the constructor call takes and fail the test if it takes more than, say, five seconds.

Copy link
Copy Markdown
Member Author

@panva panva Apr 21, 2026

Choose a reason for hiding this comment

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

  • that's why it's in pummel, not parallel.
  • load would affect both equally

We can still measure how long the constructor call takes

That's bound to be even more flaky "under load" than a comparison that checks for "orders of magnitude" of difference (* 10n)

Would it be possible to instead configure the job with unreasonably large parameters such that, if the job were to be run, the test would crash or timeout?

I feel uneasy about intentionally introducing tests that time out instead of fail.

I'm ready to forego the test entirely if it indeed turns out to be flaky.

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.

Just in case it was not clear, my suggestion would be to skip running the job altogether and instead to only call the constructor. In the absence of a bug, that should terminate almost instantaneously. If there is a bug that causes the Argon2 computation to be performed during job setup, only then would the test take a considerable amount of time, or perhaps even time out. Catching that unlikely case by allowing a very long duration for the constructor call (five seconds or perhaps even longer) sounds less likely to end up being flaky to me than a relative comparison of the time of the constructor call versus the job execution.

In any case, this is not a blocking suggestion, just my two cents :-)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I understood, but

Catching that unlikely case by allowing a very long duration for the constructor call (five seconds or perhaps even longer) sounds less likely to end up being flaky to me than a relative comparison of the time of the constructor call versus the job execution

Can we set a test file specific timeout?

In any case, this is not a blocking suggestion, just my two cents :-)

I very much appreciate your suggestions, i'm just trying to figure out how to apply them in practice.

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.

My understanding is that without this patch, the constructor of Argon2Job will already execute the Argon2 hash function, and this test is supposed to ensure that it does not.

const ctorStart = process.hrtime.bigint();
const job = new Argon2Job(kCryptoJobSync, ...hugeKdfArgsThatWouldTakeForever);
const ctorNs = process.hrtime.bigint() - ctorStart;
assert.ok(ctorNs < 60_000_000_000n);

If hugeKdfArgsThatWouldTakeForever.passes is sufficiently large (e.g., the maximum permitted value), the test should fail (without your patch) on any realistic hardware, either because the constructor does terminate but takes longer than 60 seconds or, and this is more likely, the test is executed using the standard Node.js test framework and will be killed due to a timeout. With the patch in place, it should terminate immediately.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The config would have to actually be one "that takes forever", in which case only the nodejs test framework can kill it because these two will never run

const ctorNs = process.hrtime.bigint() - ctorStart;
assert.ok(ctorNs < 60_000_000_000n);

And that's the part i'm uneasy about, relying on the test runner to timeout the test/kill the process rather than the test being self contained.

If the config was one that doesn't take forever and we tried to find a hard value that it musn't take longer then than we're looking at flakyness again.

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.

One could add a worker thread that kills the process after a timeout, but personally, I think it is fair to rely on the test runner to kill the process in the unlikely case that this precise bug will ever resurface. If you feel uneasy about that, it is perfectly fine to leave the test as is.

Loading