-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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
hkdf API is inconsistent with other supported KDFs #39471
Comments
The only non-breaking way out of it I see is
KDFs would having their own module is then the remaining inconsistency with the rest of the |
I disagree with it being an actual issue but, I won't block if someone wants to change things so long as hkdf is not removed. That said, there's no reason to expose a different module or make any breaking changes to the existing APIs. We can simply expose a single const pbkdf2 = crypto.kdfSync({
type: 'pbkdf2',
password,
salt,
iterations,
keyLength,
digest,
arrayBuffer: [true|false],
});
const scrypt = crypto.kdfSync({
type: 'scrypt',
password,
salt,
keyLength,
arrayBuffer: [true|false],
});
const hkdf = crypto.kdfSync({
type: 'hkdf',
digest,
password,
salt,
info,
keyLength,
arrayBuffer: [true|false],
); |
I don't have a strong opinion. I do believe that the current API is not ideal and should have aimed for consistency with Node.js and not with WebCrypto, but I don't see a way to solve that problem now without breaking existing code. Adding more APIs makes things more complicated and does not actually solve the problem at hand. I opened this issue mostly so that, when someone does come across this problem, they will find a resolution, even if that resolution is |
Why does replacing the current API not solve the problem at hand? |
PR-URL: #39474 Refs: #39471 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Zeyu Yang <[email protected]> Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: #39474 Refs: #39471 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Zeyu Yang <[email protected]> Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
I still think it's not ideal that we are being inconsistent in when we return |
Node.js supports three KDFs:
Output:
Inconsistencies:
ArrayBuffer
, not a Node.jsBuffer
. In the async case, the callback even has the same signature and same argument names(err, derivedKey)
, but they derived key is not aBuffer
when using HKDF.pbkdf2
.And this can absolutely lead to security issues. The output is computationally indistinguishable from a random bit sequence and all three KDFs are collision-resistant and preimage-resistant, therefore, the comparison of outputs does not necessarily need to be timing-safe. While not advisable, it is possible (depending on the application) to compare the outputs in a timing-unsafe manner without compromising the password
to compare the output of the KDF to a known correct value, e.g., as part of a login procedure. Now change
a
andb
toArrayBuffer
each, and suddenly, the condition is always true, sinceWhen scrypt was added, we had a lengthy discussion about designing KDF APIs. Quoting #21766 (comment):
Is there anything we can do about it now? Or simply label it
wontfix
and close the issue?Refs: #21766
Refs: #35093
Refs: #39453
The text was updated successfully, but these errors were encountered: