node icon indicating copy to clipboard operation
node copied to clipboard

crypto: add argon2() and argon2Sync() methods

Open ranisalt opened this issue 2 years ago • 14 comments

Argon2 is a password-based key derivation function that is designed to expensive both computationally and memory-wise in order to make force attacks unrewarding.

OpenSSL added support for the Argon2 algorithm in the 3.2.0 (see https://github.com/openssl/openssl/pull/12256). Add a js API modeled after crypto.scrypt() and crypto.scryptSync().

Related work:

  • argon2, a library currently available to support Argon2 in Node.js (disclaimer: I am the author)
  • Issue #34452 requests native support for Argon2, closed due to OpenSSL not implementing it yet

Caveats:

  • ~~OpenSSL v3.2.0 has not yet been released, so this is preliminary work based on the latest beta release (beta1 as of opening this pull request).~~ To test this pull request, you must have OpenSSL 3.2 or later installed. Run ./configure with the following flags: --shared-openssl --shared-openssl-libpath /usr/lib/openssl-3.2/ --shared-openssl-includes /usr/include/openssl-3.2 (adjust accordingly)
  • ~~It was not possible to enable passing the number of threads to the OpenSSL functions, Node.js hangs if you fire more than one async job at once. For now, it will run single threaded, which affects performance but results in the same hashes, so adding support later should be transparent to the user.~~ solved

ranisalt avatar Oct 24 '23 00:10 ranisalt

Review requested:

  • [ ] @nodejs/crypto
  • [ ] @nodejs/gyp

nodejs-github-bot avatar Oct 24 '23 00:10 nodejs-github-bot

cc @RafaelGSS @anonrig

khaosdoctor avatar Oct 24 '23 17:10 khaosdoctor

I'm curious: now that EVP_KDF is available, should we design a generic KDF API instead of adding separate APIs for different KDFs? We've had unfortunate differences across such APIs in the past (see, for example, #39471). What are your thoughts @ranisalt @bnoordhuis @panva @jasnell?

tniessen avatar Oct 24 '23 20:10 tniessen

@tniessen how to handle the different parameters used by each KDF? At some point you will need separate functions to set the parameter list, but fetching the KDF and applying it can be merged.

Also, what's the status on having a standard "password hash" function (like PHP) that is transparent to the user? Strong defaults, no parameters besides password, and returning encoded string.

ranisalt avatar Oct 24 '23 23:10 ranisalt

But doesnt argon2 give different results based on parallelism? If so, doesnt this mean that if somebody uses argon2 for his product he is forced to not use parallelism. Later if it is decided they want to use parallelism, it is not backwards compatible? Maybe should find a solution for parallelism to support this from start.

Uzlopak avatar Oct 24 '23 23:10 Uzlopak

how to handle the different parameters used by each KDF? At some point you will need separate functions to set the parameter list

@ranisalt Various cryptographic libraries do this in more or less elegant ways. EVP_KDF is one such API. The entire SubtleCrypto interface of the Web Crypto API is also independent of the underlying algorithm. In JavaScript, we typically use objects with varying sets of properties to adapt to different algorithms. Another example is crypto.generateKeyPair(), which supports a variety of algorithms.

Also, what's the status on having a standard "password hash" function (like PHP) that is transparent to the user? Strong defaults, no parameters besides password, and returning encoded string.

I think that's a separate discussion. As long as Node.js provides the primitives (such as Argon2), I think this can be a userland library.

But doesnt argon2 give different results based on parallelism? If so, doesnt this mean that if somebody uses argon2 for his product he is forced to not use parallelism.

@Uzlopak Yes, the result may depend upon the parallelism parameter. No, that does not mean that users cannot use parallelism.

Users are responsible for choosing appropriate parallelism parameters, and parallelism can always be emulated through sequential operations. Please refer to scrypt(), which also has a parallelism parameter.

tniessen avatar Oct 25 '23 00:10 tniessen

But doesnt argon2 give different results based on parallelism? If so, doesnt this mean that if somebody uses argon2 for his product he is forced to not use parallelism. Later if it is decided they want to use parallelism, it is not backwards compatible?

Argon2 has two parallelism options, lanes and threads. Changing lanes will change the resulting hash, as it changes the underlying memory layout that is used. Changing threads, however, does not change the hash, it just allows operating in lanes (which are independent) in parallel. Usually, you want both to be the same, but it doesn't have to, as long as you have lanes >= threads. The Argon2 RFC itself doesn't even mention threads, only lanes, it's an implementation detail.

@tniessen thanks for the suggestions. I think that a generic KDF function would be excellent.

ranisalt avatar Oct 25 '23 01:10 ranisalt

I'm curious: now that EVP_KDF is available, should we design a generic KDF API instead of adding separate APIs for different KDFs? We've had unfortunate differences across such APIs in the past (see, for example, #39471).

I'm sure supportive of such an API using EVP_KDF and crypto.kdf(identifier, { options }, callback) and crypto.kdfSync(identifier, { options }) would indeed be the signature I would expect.

Could even be, as we now have with crypto.sign, and crypto.verify, crypto.kdf(identifier, { options }, [callback]), just one method with the callback optional.

panva avatar Oct 25 '23 07:10 panva

Added parallelism support by creating a new OpenSSL context for each hash. This is needed for the async mode, otherwise there is a race condition when one job finishes and resets the thread pool size to 0 while another job is still running.

ranisalt avatar Nov 01 '23 14:11 ranisalt

I'm curious: now that EVP_KDF is available, should we design a generic KDF API instead of adding separate APIs for different KDFs?

Sorry I just noticed the mention on this. Yes, I'd be supportive of a generic API as opposed to protocol-specific APIs.

jasnell avatar Nov 11 '23 16:11 jasnell

Good news: OpenSSL 3.2 has finally been released

Bad news: there are incompatibilities with quictls' fork that won't be solved so soon.

ranisalt avatar Nov 29 '23 23:11 ranisalt

@nodejs/crypto does anyone know if this has moved forward in any way? As far as I've seen there is a PR that's being reviewed for a while, but I don't have any other details.

khaosdoctor avatar Jul 29 '24 09:07 khaosdoctor

This is still blocked while we wait for an LTS release of OpenSSL that contains the new APIs.

targos avatar Jul 29 '24 14:07 targos

OpenSSL has been launching LTS releases roughly every 3 years (2015, 2018, 2021) so it may be out soon, hopefully

I'm gonna rebase this PR soon

ranisalt avatar Jul 29 '24 14:07 ranisalt

FWIW I've now added testing Node.js against OpenSSL 3.2 to our Jenkins CI: ubuntu2204_sharedlibs_openssl32_x64 (as part of node-test-commit-linux-containered).

For Node.js, the position hasn't changed -- we'll continue to bundle OpenSSL 3.0 as it is LTS and will review when OpenSSL announce what the next LTS version of OpenSSL will be. The new additional testing will at least allow early detection of potential issues for people building Node.js from source against external OpenSSL (e.g. downstream Linux distributions).

richardlau avatar Nov 21 '24 15:11 richardlau

What's the status of this PR? It should be able to get merged now, right?

TheOneTheOnlyJJ avatar Nov 25 '24 08:11 TheOneTheOnlyJJ

Looks like the next LTS version could be in Apr 2025. fingers crossed.

https://github.com/openssl/openssl/discussions/23735

willfarrell avatar Feb 20 '25 18:02 willfarrell

Looking forward to this too!

lirantal avatar Feb 20 '25 20:02 lirantal

I will add KDF to ncrypto and then implement Argon2 on top of that when OpenSSL releases a new LTS

ranisalt avatar Mar 25 '25 09:03 ranisalt

New LTS released! https://openssl-library.org/post/2025-04-08-openssl-35-final-release/

Worth noting the new 2y release cycle for LTS, very nice to see.

willfarrell avatar Apr 08 '25 16:04 willfarrell

👀

ufwo avatar Jul 09 '25 03:07 ufwo

Rebased against the latest codebase 😰 added guards to build only when Argon2 is is available (OpenSSL 3.2+) and moved the hashing to ncrypto

ranisalt avatar Jul 13 '25 00:07 ranisalt

i want this in node so much.. i beg you, nodejs gods!

ufwo avatar Jul 25 '25 15:07 ufwo

OpenSSL 3.5 is now on main, can you squash the commits and rebase your PR against main? @ranisalt

panva avatar Jul 29 '25 20:07 panva

The https://github.com/nodejs/node/labels/notable-change label has been added by @panva.

Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section.

github-actions[bot] avatar Jul 29 '25 20:07 github-actions[bot]

@panva rebased and squashed!

ranisalt avatar Jul 29 '25 22:07 ranisalt

Codecov Report

:x: Patch coverage is 85.23490% with 44 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 89.87%. Comparing base (eaf1c15) to head (c9baa80). :warning: Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
src/crypto/crypto_argon2.cc 63.72% 25 Missing and 12 partials :warning:
lib/internal/crypto/argon2.js 97.29% 5 Missing :warning:
src/crypto/crypto_argon2.h 50.00% 2 Missing :warning:
Additional details and impacted files
@@           Coverage Diff            @@
##             main   #50353    +/-   ##
========================================
  Coverage   89.87%   89.87%            
========================================
  Files         661      664     +3     
  Lines      194645   194943   +298     
  Branches    38252    38293    +41     
========================================
+ Hits       174939   175214   +275     
+ Misses      12195    12187     -8     
- Partials     7511     7542    +31     
Files with missing lines Coverage Δ
lib/crypto.js 92.85% <100.00%> (+0.11%) :arrow_up:
lib/internal/errors.js 97.51% <100.00%> (+<0.01%) :arrow_up:
src/node_crypto.cc 81.81% <ø> (ø)
src/node_errors.h 87.50% <ø> (ø)
src/crypto/crypto_argon2.h 50.00% <50.00%> (ø)
lib/internal/crypto/argon2.js 97.29% <97.29%> (ø)
src/crypto/crypto_argon2.cc 63.72% <63.72%> (ø)

... and 34 files with indirect coverage changes

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Jul 30 '25 07:07 codecov[bot]

A few code paths, mostly related to passing invalid input to the C++ code, aren't covered by tests. Is this needed? These code paths should never be reached since we check the parameters in Node before passing to the module

ranisalt avatar Jul 30 '25 14:07 ranisalt

@ranisalt I've pushed changes wrt lint rules and more readable validations. I'm fully expecting this to still fail to compile with linked OpenSSL < 3.2 but let's have github CI pass first, then I can kick off Jenkins CI to see the failures. Afterwards I have notes on the exposed JS api itself.

Thank you for being patient, let's kick this along together from now on.

panva avatar Aug 01 '25 11:08 panva

cc @jasnell @anonrig @tniessen @nodejs/cpp-reviewers for a cpp review

panva avatar Aug 01 '25 12:08 panva