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

crypto: introduce crypto/promises #37218

Closed
wants to merge 1 commit into from
Closed

Conversation

panva
Copy link
Member

@panva panva commented Feb 3, 2021

The crypto.promises Experimental API provides an alternative set of asynchronous crypto methods that return Promise objects and execute their operations in libuv's threadpool. The API is accessible via require('crypto').promises or require('crypto/promises').

It does not bring all current crypto features but it's a start.

  • diffieHellman: async alternative to crypto.diffieHellman(...)
  • digest: async alternative to crypto.createHash(...).update(...).digest()
  • hmac: async alternative to crypto.createHmac(...).update(...).digest()
  • sign: async alternative to crypto.sign(...)
  • verify: async alternative to crypto.verify(...)

All methods are one-shot and use KeyObject (or CryptoKey) instances as key arguments exclusively.

TODO for this PR

  • (@jasnell) 🙏 prepare SignJob for the dsaEncoding option for (EC)DSA (one of kSigEncDER, kSigEncP1363)
  • (@jasnell) please see TODO(@jasnell) in test/parallel/test-crypto-promises-*.js
  • (@panva) tests
  • (@panva) more tests

Refs: #36181

TODO for future PRs

  • ❓ add simply promisified versions of generateKeyPair, hkdf, pbkdf2, randomBytes, randomFill, randomInt, scrypt
  • add symmetric encrypt/decrypt
  • add rsa privateDecrypt, publicEncrypt
  • suggestions

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. crypto Issues and PRs related to the crypto subsystem. labels Feb 3, 2021

Calculates the signature for `data` using the given private key and
algorithm. If `algorithm` is `null` or `undefined`, then the algorithm is
dependent upon the key type (especially Ed25519 and Ed448).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest moving the details about the defaults for algorithm into the bullet point in the argument list above

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tniessen is passing null|undefined used for anything but Ed25519 and Ed448?

Copy link
Member

@tniessen tniessen Feb 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't design this API. It would be used for SM2, too. But there's no decision yet whether SM2 will be added, see #37066.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies, i thought you did. @mscdex can you pitch in?

@jasnell
Copy link
Member

jasnell commented Feb 3, 2021

Woo! great start. Cursory read through looks good but I'll go through in detail tomorrow.

@benjamingr
Copy link
Member

Hey, random question - why do we need two promise APIs for crypto (WebCrypto and this). Are the capabilities significantly different?

const { asymmetricKeyType } = privateKey;

if (asymmetricKeyType === 'dh') {
return new Promise((resolve, reject) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use util.promisify to avoid this new Promise dance

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this to be more efficient than the promisify implementation. Not a hill I want to die on tho.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's exactly as efficient since in recent version this code is what promisify generates - but not a hill to die on for me either was just trying to comment :]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/nodejs/node/blob/v15.8.0/lib/internal/util.js#L292-L326 sure it has the same outcome, but i think it's handling way more cases which we don't need.

@panva
Copy link
Member Author

panva commented Feb 4, 2021

Hey, random question - why do we need two promise APIs for crypto (WebCrypto and this). Are the capabilities significantly different?

@benjamingr that's a good question. From my POV we have

crypto

  • ➕ efficient streaming oriented APIs
  • ➕ algorithm selection as broad as openssl offers
  • ➖ when not used in small chunks blocks the event loop, for some algorithms significantly

crypto.webcrypto

  • ➕ web compatibility, great for universal modules
  • ➕ one-shot non-blocking libuv's threadpool utilizing APIs
  • ➖ algorithm selection is limited
  • ➖ the API itself is inefficient when you need to re-use a cryptographic keys for different algorithms dynamically, you effectively have to subtle.importKey all the time, Promises are over-utilized, especially for those key operations which are sync anyway.

crypto.promises (this proposal)

  • ➕ one-shot non-blocking libuv's threadpool utilizing APIs
  • ➕ algorithm selection as broad as openssl offers

It boils down to:

@benjamingr
Copy link
Member

I... honestly don't use these APIs in crypto enough to judge whether or not this is a good idea given we have WebCrypto.

I'll stick to code comments here instead then :]

@panva
Copy link
Member Author

panva commented Feb 4, 2021

I... honestly don't use these APIs in crypto enough to judge whether or not this is a good idea given we have WebCrypto.

I'll stick to code comments here instead then :]

I'm using these APIs heavily in my jose module. Given how these security tokens are transmitted you end up using the crypto module as a one-shot anyway - inefficient given it executes in the main thread. At the same time WebCrypto does not deliver the algorithm support and spends a ton of time resolving useless promises for the sake of conforming to the specification.

@benjamingr
Copy link
Member

At the same time WebCrypto does not deliver the algorithm support and spends a ton of time resolving useless promises for the sake of conforming to the specification.

I thought one of the design goals of WebCrypto was to not specify a fixed set of algorithms and allow platforms (like Node.js) to extend it with additional algorithms. Is that not the case?

Also - is resolving promises actually expensive in the context of crypto (promises are pretty cheap in modern Node.js and crypto is pretty expensive).

@jasnell
Copy link
Member

jasnell commented Feb 4, 2021

@panva :

the API itself is inefficient when you need to re-use a cryptographic keys for different algorithms dynamically, you effectively have to subtle.importKey all the time, Promises are over-utilized, especially for those key operations which are sync anyway.

This is actually exactly the reason why I made CryptoKey a wrapper around KeyObject and provided a way of wrapping/unwrapping those (e.g. subtle.importKey('node.keyObject', ...) and KeyObject.from()) We can have a simple sync operation on KeyObject to get a CryptoKey from it which will make re-use drop dead simple.

@benjamingr :

I thought one of the design goals of WebCrypto was to not specify a fixed set of algorithms and allow platforms (like Node.js) to extend it with additional algorithms. Is that not the case?

Eh... the Web Crypto provides an extensibility model then recommends against using it but then the working group for Web Crypto went inactive without defining a clear model for how new algorithms are defined, which is always fun. So long as it's clear that the things we are adding are Node.js specific (e.g. the NODE- prefix) then we should be fine, and eventually we'll want to write up some basic specs for the extensions we define in case other Web Crypto environments want to pick them up (I can imagine deno, for instance, possibly wanting to pick up a few of them down the road).

@benjamingr :

I... honestly don't use these APIs in crypto enough to judge whether or not this is a good idea given we have WebCrypto.

Personally I'm fine with doing everything as extensions to Web Crypto. It's not the best API but it's the interoperable/portable one. There are some inefficiencies in the design but now that we have an implementation we can start working on optimizations within that implementation. My emphasis thus far has been on providing the functionality before working on the performance.

@panva
Copy link
Member Author

panva commented Feb 4, 2021

We can have a simple sync operation on KeyObject to get a CryptoKey from it which will make re-use drop dead simple.

Something like CryptoKey.from(keyObject, webCryptoAlgorithm, usages, extractable)?

Personally I'm fine with doing everything as extensions to Web Crypto. It's not the best API but it's the interoperable/portable one. There are some inefficiencies in the design but now that we have an implementation we can start working on optimizations within that implementation. My emphasis thus far has been on providing the functionality before working on the performance.

As far as extensions, at least for the use cases I have in mind, that I think would help bring webcrypto on-par with what i had in mind for this module - deriveBits ECDH with any supported curve from crypto.getCurves(), digest and hmac with any crypto.getHashes() (e.g. shake256 with outputLength configurable) , and RSA1_5 public encrypt/private decrypt.

@benjamingr
Copy link
Member

I feel strongly that if it can be done with WebCrypto we should strive to have 2 and not 3 promise APIs.

However, I am quite ignorant on the topic in general - I only know WebCrypto and Node's crypto from a user's point of view, I read very little of the code - so please take everything I say here with a huge grain of salt :D

@jasnell
Copy link
Member

jasnell commented Feb 4, 2021

Something like CryptoKey.from(keyObject, webCryptoAlgorithm, usages, extractable)?

I'd prefer for it not to be on CryptoKey or the standards wonks will send a flogging party after me. Let's put it on KeyObject... e.g.

KeyObject.prototype.toCryptoKey(webCryptoAlgorithm, usages, extractable)

@panva
Copy link
Member Author

panva commented Feb 4, 2021

Something like CryptoKey.from(keyObject, webCryptoAlgorithm, usages, extractable)?

I'd prefer for it not to be on CryptoKey or the standards wonks will send a flogging party after me. Let's put it on KeyObject... e.g.

KeyObject.prototype.toCryptoKey(webCryptoAlgorithm, usages, extractable)

NB: This achieves that while still running all the importKey checks. It throws DOMException instead of node's errors though. That being said if all that save's us is one promise, might as well keep the footprint low and not bother.

@tniessen
Copy link
Member

tniessen commented Feb 5, 2021

Something like CryptoKey.from(keyObject, webCryptoAlgorithm, usages, extractable)?

I'd prefer for it not to be on CryptoKey or the standards wonks will send a flogging party after me. Let's put it on KeyObject... e.g.

KeyObject.prototype.toCryptoKey(webCryptoAlgorithm, usages, extractable)

While we are at it, I have to say I'm not happy that #35093 added KeyObject.from. It is public yet undocumented and has a super generic name yet serves a super specific purpose. A purpose that should rarely be necessary if WebCrypto actually accomplished its goals of feature completeness and portability.

Similar concerns applied to the changes to generateKeyPair in #36879, but those were addressed after I opened #37055 and before the changes were released.

I added the KeyObject class and many APIs, including generateKeyPair, long after WebCrypto was released, to narrowly mirror OpenSSL capabilities while still being Node.js-ish APIs. WebCrypto has a different philosophy, and as @jasnell said, a dysfunctional extensibility model. We added WebCrypto to improve interopability of JavaScript across different runtimes, not because it's a good standard. However, we ultimately end up with a small portable feature set and a larger non-portable feature set.

@tniessen
Copy link
Member

tniessen commented Feb 5, 2021

Now, regarding this PR, I wonder how much has changed from @jasnell's #678 (comment).

We already have crypto and WebCrypto, and slow crypto APIs are already either broken up into small stream-based pieces, or can run asynchronously using callbacks or promisified. sign and verify might be an exception, and I believe @jasnell is working on a new MAC API.

Part of the reason the one-shot sign and verify functions were added was performance: Using the one-shot functions for thousands of small buffers is much faster than createSign().update().sign(). For large amounts of data, the streaming APIs should be used. I suspect, and #678 (comment) appears to have confirmed that at some point, that promises will only add more performance overhead without actually solving anything.

For large amounts of data, the existing one-shot APIs can be slow (which is why the streaming APIs should be used instead). However, users will still pass large amounts of data to one-shot functions, to WebCrypto, and they will also pass it to crypto/promises. That's often a terrible idea. The data must be placed entirely in a continuous memory area, which can cause memory requirements to skyrocket. But not only that, since the API is asynchronous, it must duplicate the data to prevent modifications while it is running. So that doubles the memory requirement.

WebCrypto was added to Node.js, which already provides a promise-based API, no matter how inefficient that can be at times. The async callback-based functions in the crypto submodule can already be promisified. Yet we are talking about adding a third API now.

We're lacking one-shot APIs that are not blocking the event loop, that's what this proposal provides.

If that really is a big problem, then we should probably add required features to the crypto module. If it really does matter much, then I don't think it makes sense to only add new features to crypto/promises and not crypto. And at that point, it would just be a matter of promisifying the APIs again.

If the additional memory requirements and the performance degradation due to copying is not an issue, but blocking the main thread is, applications can already use worker threads to not block the main thread.

Adding a third, separate crypto API despite already having crypto and WebCrypto adds another maintenance burden if it's anything more than a promisified crypto. So it should really provide large benefits and not only exist because of rare edge cases.

@panva
Copy link
Member Author

panva commented Feb 5, 2021

@tniessen efficient streams but blocking the main thread ✖️ slow one-shot ✖️ WebCrypto w/ limited algorithm support - you're saying if a crypto use doesn't fall into these one should just go manage their own worker resources (and in doing so waste cycles transferring key objects between threads)? Given that the libuv implementation already exists but just isn't exposed i find these choices insufficient.

Using stream APIs isn't always applicable, i encounter all of the data needed to be processed to be already present (e.g. in req. headers, payloads, etc) more than streams - plus they block the main thread. Existing one-shots have the same downside as using the stream API in a oneshot kind of way, webcrypto doesn't have the algorithms and curves necessary.

So bottom line the best we can do is tell developers to use webcrypto when the algorithm is supported, otherwise go and manage your own worker_threads implementation. 😭

@tniessen
Copy link
Member

tniessen commented Feb 5, 2021

you're saying if a crypto use doesn't fall into these one should just go manage their own worker resources (and in doing so waste cycles transferring key objects between threads)?

@panva no, what I said is

If that really is a big problem, then we should probably add required features to the crypto module. If it really does matter much, then I don't think it makes sense to only add new features to crypto/promises and not crypto. And at that point, it would just be a matter of promisifying the APIs again.

I can see the use cases, but they need to justify the added complexity.

@panva panva added the stalled Issues and PRs that are stalled. label Feb 24, 2021
@github-actions
Copy link
Contributor

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

@panva
Copy link
Member Author

panva commented Feb 24, 2021

After discussing with @tniessen (here and privately) and @benjamingr i'm closing this. It makes more sense to extend the existing crypto module with callback methods that are ready to be promisified then to continue down this path.

See #37500 for crypto.sign/crypto.verify optional callback inclusion.

@panva panva closed this Feb 24, 2021
@panva panva deleted the crypto-promises branch October 13, 2022 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. crypto Issues and PRs related to the crypto subsystem. stalled Issues and PRs that are stalled.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants