Skip to content
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

Proposal: generic key derivation function for crypto #50391

Open
ranisalt opened this issue Oct 25, 2023 · 10 comments
Open

Proposal: generic key derivation function for crypto #50391

ranisalt opened this issue Oct 25, 2023 · 10 comments
Labels
crypto Issues and PRs related to the crypto subsystem. feature request Issues that request new features to be added to Node.js.

Comments

@ranisalt
Copy link

What is the problem this feature will solve?

The crypto library currently lacks a generic key derivation function similar to OpenSSL's EVP_KDF methods and resembling WebCrypto's deriveKey. This proposal aims to introduce a generic key derivation function to the crypto library, enabling developers to derive keys for various cryptographic purposes with simplicity.

The current situation, with separate functions for each supported KDF, can lead to inconsistencies in the API. Each KDF function might have its unique set of parameters and usage patterns, making it challenging for developers to work seamlessly with different key derivation methods.

What is the feature you are proposing to solve the problem?

I propose the addition of a new function named kdf to the crypto library. This function will take the following parameters:

  • algorithm: An object specifying the key derivation algorithm to use. This object should contain information about the algorithm's name, mode, parameters, and any other necessary details, allowing developers to choose from a variety of key derivation algorithms, including those available in OpenSSL 3.0. This object is akin to the first parameter of Web Crypto's deriveKey.
  • baseKey: The base key from which the derived key will be generated (e.g. the password to be hashed). This base key can be of any type supported by the Crypto library, such as an ArrayBuffer, Buffer, or other relevant types.
  • salt: A Buffer containing the salt to be used in the key derivation process, enhancing the security of the derived key.
  • callback: An optional function to be run with the key after the derivation is complete. If undefined, run the job synchronously (as suggested by @panva).

Example Usage

Here is an example of how the crypto.kdf function might be used in Node.js:

const { kdf } = require('node:crypto');

const baseKey = 'hunter2';

const algorithm = {
  name: 'scrypt',
  cost: 32768,
};

/** 
 * Using PBKDF2 instead:
 *
 * const algorithm = {
 *   name: 'pbkdf2',
 *   iterations: 100000,
 *   digest: 'sha-512',
 * };
 */

try {
  const derivedKey = kdf(algorithm, baseKey, crypto.randomBytes(16));
  // derivedKey is a Buffer
} catch (err) {
  console.err('An error has occurred!', err);
}

What alternatives have you considered?

It is possible to keep adding functions to the crypto library as new KDF algorithms are supported, but the more options there are, the more duplication there will be as apart from the parameters available, KDF functions are quite similar in interface and behavior: you pass a key and a salt and it returns a hash.

@ranisalt ranisalt added the feature request Issues that request new features to be added to Node.js. label Oct 25, 2023
@bnoordhuis
Copy link
Member

An optional function to be run with the key after the derivation is complete. If undefined, run the job synchronously

I don't think that's a good idea - and I'm the one who introduced that pattern in the crypto module when I added randomBytes(). Mistake in hindsight, synchronous code should stand out like a sore thumb.

Also, there's a good counter example: randomFill() vs. randomFillSync().

@ranisalt
Copy link
Author

@bnoordhuis would kdf and kdfSync, doing the obvious work, be a good option instead? I'm fine with either.

How about returning a Promise? It seems to be the new standard, and there's callbackify for anyone running legacy code.

@panva
Copy link
Member

panva commented Oct 25, 2023

For the sake of consistency in the crypto module -> callbacks.

@bnoordhuis
Copy link
Member

Yes. People can call util.promisify(...) to get a promise-based version.

For the record, I'm not vehemently opposed but I'm lukewarm about introducing a general multiplexing KDF API. The thing is you're almost always interested in a single concrete KDF. Once you've picked one, you don't switch lightly.

From that perspective a general API just doesn't seem that useful, only a source of potential confusion.

@khaosdoctor
Copy link
Member

While I fully support the easiness of changing and creating different hashes using different KDFs, I also agree with @bnoordhuis on that it's going to create confusion. So I'm proposing a new approach, adding the generic KDF function internally only.

In this use case the idea would be to have a single internal entrypoint for all KDFs like @ranisalt mentioned, but we have different facades for which algorithm is supported or not. In my POV this would have the following pros:

  • It would be easier to add new KDFs to the library as needed, like crypto: add argon2() and argon2Sync() methods #50353, this would have been an easier implementation
  • There is the possibility of implementing new algorithms or deprecating old ones by deprecating the facade without modifying internal behaviour (and then correct/deprecate the internal one as well)
  • If one day, the chance arises that we decide to change the crypto module to something generic, it's a matter of exposing the internal KDF and removing the facades
  • All KDFs have the same output, varying inputs

But, on the other hand, this could (and potentially will) be a big refactor in the code base since we would need to fiddle with all the KDFs for the sake of consistency as well, but it can be done one by one after the initial implementation.

What do you think?

@tniessen tniessen added the crypto Issues and PRs related to the crypto subsystem. label Oct 26, 2023
@tniessen
Copy link
Member

For the record, I'm not vehemently opposed but I'm lukewarm about introducing a general multiplexing KDF API. The thing is you're almost always interested in a single concrete KDF. Once you've picked one, you don't switch lightly.

I don't disagree @bnoordhuis. However, with the current direction OpenSSL 3 is taking, I wouldn't be surprised if there were some KDFs implemented through OpenSSL providers at some point, and a generic KDF API might allow users to interface with those in Node.js, similar to how we resolve ciphers, hash functions, etc.

Copy link
Contributor

There has been no activity on this feature request for 5 months. To help maintain relevant open issues, please add the never-stale Mark issue so that it is never considered stale label or close this issue if it should be closed. If not, the issue will be automatically closed 6 months after the last non-automated comment.
For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Apr 24, 2024
@ranisalt
Copy link
Author

I wouldn't be surprised if there were some KDFs implemented through OpenSSL providers at some point

If I understand it correctly, that's already how they are implemented. #50353 uses generic KDF facilities

@github-actions github-actions bot removed the stale label Apr 25, 2024
Copy link
Contributor

There has been no activity on this feature request for 5 months. To help maintain relevant open issues, please add the never-stale Mark issue so that it is never considered stale label or close this issue if it should be closed. If not, the issue will be automatically closed 6 months after the last non-automated comment.
For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Oct 22, 2024
@ranisalt
Copy link
Author

ranisalt commented Nov 4, 2024

This may be more interesting/consistent now that crypto.hash is a thing

@github-actions github-actions bot removed the stale label Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. feature request Issues that request new features to be added to Node.js.
Projects
Status: Awaiting Triage
Development

No branches or pull requests

5 participants