crypto: improve random-bytes performance by ~60%

[anonrig]

By using the same method of randomInt, I added to reduce the random byte generation for key size less than 8 * 1024 bytes

Some of the tests are failing due to the async nature of caching. I appreciate any help for resolving those.

                                         confidence improvement accuracy (*)   (**)  (***)
crypto/randomBytes.js n=1000 size=1024          ***     11.23 %       ±1.66% ±2.21% ±2.87%
crypto/randomBytes.js n=1000 size=524288                 0.24 %       ±2.20% ±2.93% ±3.82%
crypto/randomBytes.js n=1000 size=64            ***     62.44 %       ±5.18% ±6.94% ±9.13%
crypto/randomBytes.js n=1000 size=8192            *     -1.85 %       ±1.43% ±1.90% ±2.47%

Be aware that when doing many comparisons the risk of a false-positive result increases.
In this case, there are 4 comparisons, you can thus expect the following amount of false-positive results:
  0.20 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.04 false positives, when considering a   1% risk acceptance (**, ***),
  0.00 false positives, when considering a 0.1% risk acceptance (***)

[nodejs-github-bot]

Review requested:

• @nodejs/crypto

[mcollina]

lgtm

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/46606/

[jasnell]

My guess here is that in certain cases now the async context may or may not be the same. @nodejs/async_hooks folks, I just want to confirm with y'all that this looks correct?

[Flarna]

Removing these asserts makes this test quite useless. It no longer tests what was tested before.

After this change the random byte comes from the cache therefore no RANDOMBYTESREQUEST is done at all.
To keep the behavior of the test as before it's needed to request more random bytes as the cache has.

[jasnell]

It would be good to add a test that ensures that, when the cache is used, the results of multiple random-bytes calls return different results each time without any overlap to ensure that the cache is advancing the way that it should.

[tniessen]

IIRC I was the one to implement caching for randomInt() and I believe it was not without some concerns voiced in that PR back then.

My main concern here is that, unlike randomInt(), the randomBytes() and randomFill() functions are frequently used to generate symmetric keys etc. Having such secrets in the JS heap, potentially long before being used, seems risky to me, especially because the JS heap lacks any sort of protection.

I assume that the main performance bottleneck is crossing the JS/C++ border, and V8 fast API calls would be a good approach to reducing that performance bottleneck without having to cache secret data in the JS heap.

[jasnell]

Yeah, caching random bytes unconditionally could lead to problems. The cache could be set up to use the openssl secure heap if that is enabled for the process (https://nodejs.org/dist/latest-v18.x/docs/api/cli.html#--secure-heapn) which would at least separate it from the rest of the js heap space. But if we're going to add this, it would be good to make use of the cache optional (and likely off by default)

[tniessen]

Another motivation for caching randomInt() state was that randomInt() is rarely used only once. Main use cases such as Fisher-Yates need many random integers, so caching helps a lot.

randomBytes() and randomFill(), on the other hand, are rarely called in a loop.

The cache could be set up to use the openssl secure heap if that is enabled for the process (https://nodejs.org/dist/latest-v18.x/docs/api/cli.html#--secure-heapn) which would at least separate it from the rest of the js heap space.

It could, but secure heap is almost always disabled, and even unsupported on many operating systems IIRC.

[anonrig]

I can only think of two solutions to address the heap-related issues:

1. Reducing the cache size and speeding up operations with an input of less than the cache size.
2. Having { useCache: Boolean } as an option of randomBytes and make caching opt-in.

[anonrig]

By the way; can we add request-ci label again, since I've fixed the failing tests after it has been added?

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/46610/

[mcollina]

I assume that the main performance bottleneck is crossing the JS/C++ border, and V8 fast API calls would be a good approach to reducing that performance bottleneck without having to cache secret data in the JS heap.

@anonrig this can be a good approach to try.

[bnoordhuis]

I don't want to sound too negative but I strongly feel this is optimizing the wrong thing.

It's extremely important to maintain the security properties of the cryptographic random number generator. The more speed hacks you pile on, the harder it becomes to understand and the easier it becomes for bugs to slip in.

I'm also skeptical it needs to be super fast because even a dedicated keygen server probably doesn't use more than a few hundred bytes of entropy per second.

[anonrig]

I assume that the main performance bottleneck is crossing the JS/C++ border, and V8 fast API calls would be a good approach to reducing that performance bottleneck without having to cache secret data in the JS heap.

Is there any documentation for v8 Fast API? I couldn't find docs or usage of fast api.

[tniessen]

There unfortunately is very little documentation beyond https://github.com/nodejs/node/blob/main/deps/v8/include/v8-fast-api-calls.h.