Migrate hkdf, pbkdf2, scrypt to OpenSSL EVP_KDF functions #57824

ranisalt · 2025-04-10T17:32:53Z

Long ago in #50353 it was suggested to create a generic KDF function to support all functions depending on parameters, and it turns out that's how OpenSSL does it anyway. I created a generic interface to implement all KDF functions (currently hkdf, pbkdf2, scrypt, in the future argon2 when 3.5 is merged)

There's a few issues with the implementation currently:

parameters can be 32 or 64 bit, signed or unsigned integers but I'm treating everything as uint32 as that's what can be passed to the current KDF functions, need to figure out how to pass other sizes and "signednesses"
errors have changed and it's not possible for example to get invalid digest errors, it will just be a generic OpenSSL error
PBKDF2 is giving the wrong output, I'm probably passing some parameter incorrectly. HKDF and scrypt are correct

nodejs-github-bot · 2025-04-10T17:32:58Z

Review requested:

@nodejs/crypto
@nodejs/gyp
@nodejs/security-wg

ranisalt · 2025-04-10T17:35:18Z

deps/ncrypto/ncrypto.cc

+KdfCtxPointer KdfCtxPointer::FromName(std::string_view name) {
+  auto kdf = EVP_KDF_fetch(nullptr, name.data(), nullptr);
+  auto ctx = EVP_KDF_CTX_new(kdf);
+  return KdfCtxPointer{ctx};


The KDF context stores a reference to the KDF, that's why we don't need to store it ourselves. It's also possible to fetch the KDF from the context if needed

ranisalt · 2025-04-10T17:37:54Z

deps/ncrypto/ncrypto.h

+ public:
+  using ParamType =
+      std::variant<uint32_t, double, std::string, std::vector<char>>;
+  using Params = std::map<std::string, ParamType>;


Maybe this can be a vector of pairs and we assume no one will add the same param twice (it will take the value of the first match)

ranisalt · 2025-04-10T17:38:57Z

lib/internal/crypto/hkdf.js

+    "HKDF",
+    {
+      digest: hash,
+      key: key.export({ format: 'buffer' }),


I am not sure about this. OpenSSL expects an octet string and manipulating a key object in C++ sounded complicated

ranisalt · 2025-04-10T17:40:52Z

lib/internal/crypto/hkdf.js

+  const job = new KDFJob(kCryptoJobAsync,
+    "HKDF",
+    {
+      digest: hash,


Would it be OK to rename the parameters to write digest, here? I tried to not change the public interface at all

ranisalt · 2025-04-10T17:42:17Z

src/crypto/crypto_kdf.cc

+  // if (!salt.CheckSizeInt32()) [[unlikely]] {
+  //   THROW_ERR_OUT_OF_RANGE(env, "salt is too large");
+  //   return v8::Nothing<void>();
+  // }


I can still do these checks by checking if the param name is "pass" or "salt" but I think it's pointless

Similarly, these changes will need to be ifdef guarded.

ranisalt · 2025-04-10T17:43:45Z

test/parallel/test-crypto-hkdf.js

+  // assert.throws(
+  //   () => hkdfSync('sha512', 'a', '', '', 64 * 255 + 1), {
+  //     code: 'ERR_CRYPTO_INVALID_KEYLEN'
+  //   });


These aren't throwing from OpenSSL, not sure why they should throw here

tniessen · 2025-04-10T17:56:10Z

I did indeed suggest this in #50353 (comment), but I am not sure if we have a timeline for when we will stop supporting OpenSSL 1.1.1... maybe @panva knows. From what I remember, EVP_KDF is only available in OpenSSL 3.

jasnell

Unfortunately, we need to remember that Node.js has to continue to be able to support downlevel versions of openssl (dynamic lib) and boringssl (for electron). The ncrypto dependency in particular is used not only by Node.js but also Cloudflare workers and bun (and possibly others by now). Because of that, the original implementation here needs to be preserved and the new EVP_KDF impl needs to be guarded with #ifdef flags that enable it only when a supporting version of openssl is used.

The new implementations are likely best added in addition to rather than as a replacement for the original implementations.

panva · 2025-04-10T18:05:20Z

I am not sure if we have a timeline for when we will stop supporting OpenSSL 1.1.1... maybe @panva knows

I don't know the latest, @jasnell was proposing a swift downfall of 1.1.1 support but we clearly cannot do it for 24, so possibly in 25 it's deprecated and 26 it's gone? Refs #56733

The new implementations are likely best added in addition to rather than as a replacement for the original implementations.

👌

jasnell · 2025-04-10T18:14:47Z

... so possibly in 25 it's deprecated and 26 it's gone?

That's my current thinking yes.

ranisalt · 2025-04-11T07:14:13Z

Cool, so I'll revert the removals and keep it alongside. When it's time to drop support, there's a large code surface to prune :)

jasnell · 2025-04-12T15:29:46Z

deps/ncrypto/ncrypto.cc

@@ -3080,6 +3080,69 @@ bool CipherCtxPointer::getAeadTag(size_t len, unsigned char* out) {

 // ============================================================================

+KdfCtxPointer KdfCtxPointer::FromName(std::string_view name) {


This will need to be ifdef guarded so it only compiles when openssl/boringssl versions that support EVP_KDF

jasnell · 2025-04-12T15:39:01Z

src/crypto/crypto_kdf.cc

+  // }
+
+  auto paramsObj = args[offset + 1].As<v8::Object>();
+  auto propertyNames = paramsObj->GetOwnPropertyNames(env->context()).ToLocalChecked();


We've been working on moving as much as possible away from using ToLocalChecked(). in code paths that will be hit at runtime in order to allow errors to propagate correctly. These should follow this pattern instead:

MaybeLocal<Array> propertyNames; if (!paramsObj->GetOwnPropertyNames(env->context()).ToLocal(&propertyNames)) { return Nothing<void>(); }

Here and below.

jasnell · 2025-04-12T15:50:35Z

src/crypto/crypto_kdf.cc

+      ArrayBufferOrViewContents<char> abv(value);
+      params->kdfParams[name] = std::vector<char>(abv.data(), abv.data() + abv.size());
+    }
+  }


Hmm... this feels a bit dangerous to me... This essentially means that the parameter names are tied to the underlying openssl version. If openssl happens to change names for whatever reason then the public API also changes. I know that is unlikely but I think it's important to try not to tie ourselves too closely to a specific openssl version, especially considering that boringssl also needs to be supported in the mix here and they could actually choose different names just to be Extra Spicy if they wanted to.

It's rather more work to do but I think I'd prefer to define an intermediary struct that we control the naming of and have ncrypto map that to whatever openssl/boringssl version is being used. So something like...

// in ncrypto struct KDFParams { std::string_view pass; std::string_view salt; size_t iterations // ... and so on };

ncrypto::KDFParams params; v8::Local<v8::Value> value; if (!paramsObj->Get(context, env->pass_string()).ToLocal(&value)) { return Nothing<void>(); } Utf8Value str(isolate, value); params.pass = *str; // ... ctx.setParams(params);

@jasnell that structure would have to support all parameters for all KDFs, is that the suggestion?

Alternatively, we can make a "translation" layer from Node to OpenSSL so the parameters are stable from Node independent of the OpenSSL backend

Either that or there would be separate structs for each KDF algorithm we support, with only the parameters for that algorithm. Yes, it's way more of a pain but definitely buffers more from differences across individual openssl and boringssl versions.

jasnell · 2025-04-12T15:52:07Z

src/crypto/crypto_kdf.cc

+  }
+
+  if (!params->kdfCtx.setParams(params->kdfParams)) {
+    if (uint32_t err = ERR_peek_last_error(); err != 0) {


This function should be using ncrypto::ClearErrorOnReturn or ncrypto::MarkPopErrorOnReturn

jasnell · 2025-04-12T15:55:22Z

src/crypto/crypto_kdf.cc

+template<class... Ts>
+struct overloads : Ts... { using Ts::operator()...; };
+
+void KDFConfig::MemoryInfo(MemoryTracker* tracker) const {


Take a look at MemoryRetainerTraits ... it was recently added to simplify tracking for types that cannot be modified to implement MemoryRetainer https://github.com/nodejs/node/blob/main/src/memory_tracker.h#L141-L166 ... that said tho, I really think the params need to be modeled as an ncrypto defined struct that would make this easier.

Migrate hkdf, pbkdf2, scrypt to OpenSSL EVP_KDF functions

0a37338

nodejs-github-bot added lib / src needs-ci labels Apr 10, 2025

ranisalt commented Apr 10, 2025

View reviewed changes

jasnell requested changes Apr 10, 2025

View reviewed changes

Restore crypto hkdf, pbkdf2, scrypt implementations

3f48658

ranisalt force-pushed the ncrypto/kdf branch from ff9f74a to 3f48658 Compare April 11, 2025 19:32

jasnell reviewed Apr 12, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate hkdf, pbkdf2, scrypt to OpenSSL EVP_KDF functions #57824

Migrate hkdf, pbkdf2, scrypt to OpenSSL EVP_KDF functions #57824

ranisalt commented Apr 10, 2025

nodejs-github-bot commented Apr 10, 2025

ranisalt Apr 10, 2025

ranisalt Apr 10, 2025

ranisalt Apr 10, 2025

ranisalt Apr 10, 2025

ranisalt Apr 10, 2025

jasnell Apr 12, 2025

ranisalt Apr 10, 2025

tniessen commented Apr 10, 2025

jasnell left a comment •

edited

Loading

panva commented Apr 10, 2025

jasnell commented Apr 10, 2025

ranisalt commented Apr 11, 2025

jasnell Apr 12, 2025

jasnell Apr 12, 2025 •

edited

Loading

jasnell Apr 12, 2025 •

edited

Loading

ranisalt Apr 12, 2025

jasnell Apr 12, 2025

jasnell Apr 12, 2025

jasnell Apr 12, 2025 •

edited

Loading

		@@ -3080,6 +3080,69 @@ bool CipherCtxPointer::getAeadTag(size_t len, unsigned char* out) {

		// ============================================================================

		KdfCtxPointer KdfCtxPointer::FromName(std::string_view name) {

Migrate hkdf, pbkdf2, scrypt to OpenSSL EVP_KDF functions #57824

Are you sure you want to change the base?

Migrate hkdf, pbkdf2, scrypt to OpenSSL EVP_KDF functions #57824

Conversation

ranisalt commented Apr 10, 2025

nodejs-github-bot commented Apr 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tniessen commented Apr 10, 2025

jasnell left a comment • edited Loading

Choose a reason for hiding this comment

panva commented Apr 10, 2025

jasnell commented Apr 10, 2025

ranisalt commented Apr 11, 2025

Choose a reason for hiding this comment

jasnell Apr 12, 2025 • edited Loading

Choose a reason for hiding this comment

jasnell Apr 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell Apr 12, 2025 • edited Loading

Choose a reason for hiding this comment

jasnell left a comment •

edited

Loading

jasnell Apr 12, 2025 •

edited

Loading

jasnell Apr 12, 2025 •

edited

Loading

jasnell Apr 12, 2025 •

edited

Loading