-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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: implement crypto.hash() #51044
Conversation
Review requested:
|
Interested, could the same be done for hmac? |
Results of comparing the one-shot digest implementation from this, the
On Ubuntu + Intel(R) Xeon(R) Platinum 8280 CPU @ 2.70GHz
|
Yes though I think for caching EVP_MAC for maximum speed we need to do the init-update-final ourselves because for HAMC there's only |
I suppose this supersedes #42233? |
If the goal is to "reducing as much unnecessary overhead as possible when computing a one-shot digest synchronously" then I think yes. With an implementation based on I don't know if the current API is the best, however. But I think that might already be enough for maybe 80% of the use case (this assumes the string input is decoded as UTF8 - would the need of custom input decoding really be that common? With this API users can still decode their strings with other encodings into a buffer and use that as input if they really need a different encoding, though) |
444fec1
to
114f2f9
Compare
114f2f9
to
163a5a6
Compare
163a5a6
to
2f9c4fe
Compare
Added docs and renamed this to |
Pinging @nodejs/cpp-reviewers @nodejs/crypto again.. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't really vouch for the C++ code, but JS code and benchmark LGTM 👍 Could you add a test that validates the correct error is returned when passing arguments with wrong types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing work, I'm sure many libraries will see a good impact on performance with this new method.
@nodejs/performance Ping to get more attention/approvals.
This patch introduces a helper crypto.hash() that computes a digest from the input at one shot. This can be 1.2-1.6x faster than the object-based createHash() for smaller inputs (<= 5MB) that are readily available (not streamed) and incur less memory overhead since no intermediate objects will be created.
Notable changes: build: * (SEMVER-MINOR) build opt to set local location of headers (Michael Dawson) #51525 crypto: * (SEMVER-MINOR) implement crypto.hash() (Joyee Cheung) #51044 * update root certificates to NSS 3.98 (Node.js GitHub Bot) #51794 doc: * add zcbenz to collaborators (Cheng Zhao) #51812 * add lemire to collaborators (Daniel Lemire) #51572 http2: * (SEMVER-MINOR) add h2 compat support for appendHeader (Tim Perry) #51412 * (SEMVER-MINOR) add server handshake utility (snek) #51172 * (SEMVER-MINOR) receive customsettings (Marten Richter) #51323 lib: * (SEMVER-MINOR) move encodingsMap to internal/util (Joyee Cheung) #51044 sea: * (SEMVER-MINOR) support sea.getRawAsset() (Joyee Cheung) #50960 * (SEMVER-MINOR) support embedding assets (Joyee Cheung) #50960 src: * (SEMVER-MINOR) print string content better in BlobDeserializer (Joyee Cheung) #50960 * (SEMVER-MINOR) support multi-line values for .env file (IlyasShabi) #51289 * (SEMVER-MINOR) add `process.loadEnvFile` and `util.parseEnv` (Yagiz Nizipli) #51476 * (SEMVER-MINOR) do not coerce dotenv paths (Tobias Nießen) #51425 stream: * (SEMVER-MINOR) implement `min` option for `ReadableStreamBYOBReader.read` (Mattias Buelens) #50888 util: * (SEMVER-MINOR) add styleText API to text formatting (Rafael Gonzaga) #51850 vm: * (SEMVER-MINOR) support using the default loader to handle dynamic import() (Joyee Cheung) #51244 PR-URL: #51932
The default encoding being |
We could at least mark this as experimental if it isn't already. |
From what I can tell, when people do one-shot hashing, most of the time they want the result to be encoded in a hex string - or from a quick search in popular packages, it seems rare for them to want the result to be returned in a buffer. What exactly makes this default awkward other than that it follows what most users want instead of what other APIs already do? Personally it doesn't seem very necessary to me to return the hash in buffer by default in other APIs either, though they are already what they are. The hash is small and fixed-sized, and creating a buffer is not cheap. Encoding it to string isn't, either - for less than a megabyte of input, the buffer + string allocation are actually more expensive than the actual hashing (and the most common use case of this that I can see in top packages is hashing JS files, which are normally around this size). |
I agree we can mark it as 1.2, though, if users do think defaulting to buffer makes sense, we can switch to buffer before we make it 2. |
Notable changes: build: * (SEMVER-MINOR) build opt to set local location of headers (Michael Dawson) #51525 crypto: * (SEMVER-MINOR) implement crypto.hash() (Joyee Cheung) #51044 * update root certificates to NSS 3.98 (Node.js GitHub Bot) #51794 doc: * add zcbenz to collaborators (Cheng Zhao) #51812 * add lemire to collaborators (Daniel Lemire) #51572 http2: * (SEMVER-MINOR) add h2 compat support for appendHeader (Tim Perry) #51412 * (SEMVER-MINOR) add server handshake utility (snek) #51172 * (SEMVER-MINOR) receive customsettings (Marten Richter) #51323 lib: * (SEMVER-MINOR) move encodingsMap to internal/util (Joyee Cheung) #51044 sea: * (SEMVER-MINOR) support sea.getRawAsset() (Joyee Cheung) #50960 * (SEMVER-MINOR) support embedding assets (Joyee Cheung) #50960 src: * (SEMVER-MINOR) print string content better in BlobDeserializer (Joyee Cheung) #50960 * (SEMVER-MINOR) support multi-line values for .env file (IlyasShabi) #51289 * (SEMVER-MINOR) add `process.loadEnvFile` and `util.parseEnv` (Yagiz Nizipli) #51476 * (SEMVER-MINOR) do not coerce dotenv paths (Tobias Nießen) #51425 stream: * (SEMVER-MINOR) implement `min` option for `ReadableStreamBYOBReader.read` (Mattias Buelens) #50888 util: * (SEMVER-MINOR) add styleText API to text formatting (Rafael Gonzaga) #51850 vm: * (SEMVER-MINOR) support using the default loader to handle dynamic import() (Joyee Cheung) #51244 PR-URL: #51932
Notable changes: build: * (SEMVER-MINOR) build opt to set local location of headers (Michael Dawson) #51525 crypto: * (SEMVER-MINOR) implement crypto.hash() (Joyee Cheung) #51044 * update root certificates to NSS 3.98 (Node.js GitHub Bot) #51794 doc: * add zcbenz to collaborators (Cheng Zhao) #51812 * add lemire to collaborators (Daniel Lemire) #51572 http2: * (SEMVER-MINOR) add h2 compat support for appendHeader (Tim Perry) #51412 * (SEMVER-MINOR) add server handshake utility (snek) #51172 * (SEMVER-MINOR) receive customsettings (Marten Richter) #51323 lib: * (SEMVER-MINOR) move encodingsMap to internal/util (Joyee Cheung) #51044 sea: * (SEMVER-MINOR) support sea.getRawAsset() (Joyee Cheung) #50960 * (SEMVER-MINOR) support embedding assets (Joyee Cheung) #50960 src: * (SEMVER-MINOR) print string content better in BlobDeserializer (Joyee Cheung) #50960 * (SEMVER-MINOR) support multi-line values for .env file (IlyasShabi) #51289 * (SEMVER-MINOR) add `process.loadEnvFile` and `util.parseEnv` (Yagiz Nizipli) #51476 * (SEMVER-MINOR) do not coerce dotenv paths (Tobias Nießen) #51425 stream: * (SEMVER-MINOR) implement `min` option for `ReadableStreamBYOBReader.read` (Mattias Buelens) #50888 util: * (SEMVER-MINOR) add styleText API to text formatting (Rafael Gonzaga) #51850 vm: * (SEMVER-MINOR) support using the default loader to handle dynamic import() (Joyee Cheung) #51244 PR-URL: #51932
Notable changes: build: * (SEMVER-MINOR) build opt to set local location of headers (Michael Dawson) #51525 crypto: * (SEMVER-MINOR) implement crypto.hash() (Joyee Cheung) #51044 * update root certificates to NSS 3.98 (Node.js GitHub Bot) #51794 doc: * add zcbenz to collaborators (Cheng Zhao) #51812 * add lemire to collaborators (Daniel Lemire) #51572 http2: * (SEMVER-MINOR) add h2 compat support for appendHeader (Tim Perry) #51412 * (SEMVER-MINOR) add server handshake utility (snek) #51172 * (SEMVER-MINOR) receive customsettings (Marten Richter) #51323 lib: * (SEMVER-MINOR) move encodingsMap to internal/util (Joyee Cheung) #51044 sea: * (SEMVER-MINOR) support sea.getRawAsset() (Joyee Cheung) #50960 * (SEMVER-MINOR) support embedding assets (Joyee Cheung) #50960 src: * (SEMVER-MINOR) print string content better in BlobDeserializer (Joyee Cheung) #50960 * (SEMVER-MINOR) support multi-line values for .env file (IlyasShabi) #51289 * (SEMVER-MINOR) add `process.loadEnvFile` and `util.parseEnv` (Yagiz Nizipli) #51476 * (SEMVER-MINOR) do not coerce dotenv paths (Tobias Nießen) #51425 stream: * (SEMVER-MINOR) implement `min` option for `ReadableStreamBYOBReader.read` (Mattias Buelens) #50888 util: * (SEMVER-MINOR) add styleText API to text formatting (Rafael Gonzaga) #51850 vm: * (SEMVER-MINOR) support using the default loader to handle dynamic import() (Joyee Cheung) #51244 PR-URL: #51932
PR-URL: #51044 Refs: nodejs/performance#136 Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
This patch introduces a helper crypto.hash() that computes a digest from the input at one shot. This can be 1.2-1.6x faster than the object-based createHash() for smaller inputs (<= 5MB) that are readily available (not streamed) and incur less memory overhead since no intermediate objects will be created. PR-URL: #51044 Refs: nodejs/performance#136 Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
PR-URL: #51044 Refs: nodejs/performance#136 Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
This patch introduces a helper crypto.hash() that computes a digest from the input at one shot. This can be 1.2-1.6x faster than the object-based createHash() for smaller inputs (<= 5MB) that are readily available (not streamed) and incur less memory overhead since no intermediate objects will be created. PR-URL: #51044 Refs: nodejs/performance#136 Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Notable changes: build: * (SEMVER-MINOR) build opt to set local location of headers (Michael Dawson) #51525 crypto: * (SEMVER-MINOR) implement crypto.hash() (Joyee Cheung) #51044 * update root certificates to NSS 3.98 (Node.js GitHub Bot) #51794 doc: * add lemire to collaborators (Daniel Lemire) #51572 * add zcbenz to collaborators (Cheng Zhao) #51812 lib: * (SEMVER-MINOR) move encodingsMap to internal/util (Joyee Cheung) #51044 sea: * (SEMVER-MINOR) support sea.getRawAsset() (Joyee Cheung) #50960 * (SEMVER-MINOR) support embedding assets (Joyee Cheung) #50960 src: * (SEMVER-MINOR) print string content better in BlobDeserializer (Joyee Cheung) #50960 util: * (SEMVER-MINOR) add styleText API to text formatting (Rafael Gonzaga) #51850 vm: * (SEMVER-MINOR) support using the default loader to handle dynamic import() (Joyee Cheung) #51244 PR-URL: #52212
PR-URL: nodejs#51044 Refs: nodejs/performance#136 Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
This patch introduces a helper crypto.hash() that computes a digest from the input at one shot. This can be 1.2-1.6x faster than the object-based createHash() for smaller inputs (<= 5MB) that are readily available (not streamed) and incur less memory overhead since no intermediate objects will be created. PR-URL: nodejs#51044 Refs: nodejs/performance#136 Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Notable changes: build: * (SEMVER-MINOR) build opt to set local location of headers (Michael Dawson) nodejs#51525 crypto: * (SEMVER-MINOR) implement crypto.hash() (Joyee Cheung) nodejs#51044 * update root certificates to NSS 3.98 (Node.js GitHub Bot) nodejs#51794 doc: * add zcbenz to collaborators (Cheng Zhao) nodejs#51812 * add lemire to collaborators (Daniel Lemire) nodejs#51572 http2: * (SEMVER-MINOR) add h2 compat support for appendHeader (Tim Perry) nodejs#51412 * (SEMVER-MINOR) add server handshake utility (snek) nodejs#51172 * (SEMVER-MINOR) receive customsettings (Marten Richter) nodejs#51323 lib: * (SEMVER-MINOR) move encodingsMap to internal/util (Joyee Cheung) nodejs#51044 sea: * (SEMVER-MINOR) support sea.getRawAsset() (Joyee Cheung) nodejs#50960 * (SEMVER-MINOR) support embedding assets (Joyee Cheung) nodejs#50960 src: * (SEMVER-MINOR) print string content better in BlobDeserializer (Joyee Cheung) nodejs#50960 * (SEMVER-MINOR) support multi-line values for .env file (IlyasShabi) nodejs#51289 * (SEMVER-MINOR) add `process.loadEnvFile` and `util.parseEnv` (Yagiz Nizipli) nodejs#51476 * (SEMVER-MINOR) do not coerce dotenv paths (Tobias Nießen) nodejs#51425 stream: * (SEMVER-MINOR) implement `min` option for `ReadableStreamBYOBReader.read` (Mattias Buelens) nodejs#50888 util: * (SEMVER-MINOR) add styleText API to text formatting (Rafael Gonzaga) nodejs#51850 vm: * (SEMVER-MINOR) support using the default loader to handle dynamic import() (Joyee Cheung) nodejs#51244 PR-URL: nodejs#51932
Notable changes: build: * (SEMVER-MINOR) build opt to set local location of headers (Michael Dawson) #51525 crypto: * (SEMVER-MINOR) implement crypto.hash() (Joyee Cheung) #51044 * update root certificates to NSS 3.98 (Node.js GitHub Bot) #51794 doc: * add lemire to collaborators (Daniel Lemire) #51572 * add zcbenz to collaborators (Cheng Zhao) #51812 lib: * (SEMVER-MINOR) move encodingsMap to internal/util (Joyee Cheung) #51044 sea: * (SEMVER-MINOR) support sea.getRawAsset() (Joyee Cheung) #50960 * (SEMVER-MINOR) support embedding assets (Joyee Cheung) #50960 src: * (SEMVER-MINOR) print string content better in BlobDeserializer (Joyee Cheung) #50960 util: * (SEMVER-MINOR) add styleText API to text formatting (Rafael Gonzaga) #51850 vm: * (SEMVER-MINOR) support using the default loader to handle dynamic import() (Joyee Cheung) #51244 PR-URL: #52212
It looks like this commit broke my code. I use algorithm from external OpenSSL engine and now I get error:0308010C:digital envelope routines::unsupported |
@GauriSpears I think that should be caused by #51034 - can you open an issue with a reproduction? |
@joyeecheung , you are right. Not all of digest algos can be retrieved via EVP_MD_fetch and EVP_get_digestbyname should be used to get them. I've fixed the same problem in OpenSSL last year: |
The caching does EVP_get_digestbyname first and if EVP_MD_fetch doesn't return a result, it uses the one returned by EVP_get_digestbyname, otherwise it uses the one fetched by EVP_MD_fetch, so it should work as long as EVP_get_digestbyname provides valid results. We also have some tests for setEngine, but not a lot. If the caching somehow broke your use case, can you provide a reproduction? |
This is great, thanks @joyeecheung and other contributors!! 🙌 For TypeScript users, Error message:
cc DT maintainers / contributors to Edit: this has been added in |
I'm trying to dig into your code. From the first sight it seems really equivalent to what happens in OpenSSL. Could you explain: when the caching of algorithm list is done? And how it shoud work if setEngine loads engine with new algorithms? |
That was explained in #51034 (comment)
It's supposed to keep trying It seems a bit off topic for this PR. Can you open a separate issue for this? Ideally with a reproduction. |
Notable changes: build: * (SEMVER-MINOR) build opt to set local location of headers (Michael Dawson) nodejs#51525 crypto: * (SEMVER-MINOR) implement crypto.hash() (Joyee Cheung) nodejs#51044 * update root certificates to NSS 3.98 (Node.js GitHub Bot) nodejs#51794 doc: * add zcbenz to collaborators (Cheng Zhao) nodejs#51812 * add lemire to collaborators (Daniel Lemire) nodejs#51572 http2: * (SEMVER-MINOR) add h2 compat support for appendHeader (Tim Perry) nodejs#51412 * (SEMVER-MINOR) add server handshake utility (snek) nodejs#51172 * (SEMVER-MINOR) receive customsettings (Marten Richter) nodejs#51323 lib: * (SEMVER-MINOR) move encodingsMap to internal/util (Joyee Cheung) nodejs#51044 sea: * (SEMVER-MINOR) support sea.getRawAsset() (Joyee Cheung) nodejs#50960 * (SEMVER-MINOR) support embedding assets (Joyee Cheung) nodejs#50960 src: * (SEMVER-MINOR) print string content better in BlobDeserializer (Joyee Cheung) nodejs#50960 * (SEMVER-MINOR) support multi-line values for .env file (IlyasShabi) nodejs#51289 * (SEMVER-MINOR) add `process.loadEnvFile` and `util.parseEnv` (Yagiz Nizipli) nodejs#51476 * (SEMVER-MINOR) do not coerce dotenv paths (Tobias Nießen) nodejs#51425 stream: * (SEMVER-MINOR) implement `min` option for `ReadableStreamBYOBReader.read` (Mattias Buelens) nodejs#50888 util: * (SEMVER-MINOR) add styleText API to text formatting (Rafael Gonzaga) nodejs#51850 vm: * (SEMVER-MINOR) support using the default loader to handle dynamic import() (Joyee Cheung) nodejs#51244 PR-URL: nodejs#51932
crypto: implement crypto.hash()
This patch introduces a helper crypto.hash() that computes
a digest from the input at one shot. This can be 1.2-2x faster
than the object-based createHash() for smaller inputs (<= 5MB)
that are readily available (not streamed) and incur less memory
overhead since no intermediate objects will be created.
Refs: nodejs/performance#136
From the benchmark CI https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1493/console
Benchmark results from macOS + M2
Benchmark results from Ubuntu + Intel(R) Xeon(R) Platinum 8280 CPU @ 2.70GHz