-
-
Notifications
You must be signed in to change notification settings - Fork 35.4k
crypto: remove Argon2 KDF derivation from its job setup #62863
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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(); | ||
| } | ||
| } |
| 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That's bound to be even more flaky "under load" than a comparison that checks for "orders of magnitude" of difference (
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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 :-)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understood, but
Can we set a test file specific timeout?
I very much appreciate your suggestions, i'm just trying to figure out how to apply them in practice.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My understanding is that without this patch, the constructor of 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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
AdditionalConfigand 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 ✔️