crypto: reconcile oneshot sign/verify sync and async implementations

[panva]

This reconciles the crypto.sign and crypto.verify C++ implementation when running with or without a callback.

[panva]

cc @nodejs/crypto

[nodejs-github-bot]

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

💚

[tniessen]

@panva @danbev Re thread-safety:

• OpenSSL does not really make any guarantees. Key management itself is thread-safe (see e.g., CRYPTO_UP_REF), however, OpenSSL does not really tell us if the EVP APIs will be thread-safe, too.
• Documentation of thread safety openssl/openssl#2165 (comment) sounds like most things should be thread-safe, but also isn't particularly specific.
• It seems that any OpenSSL *_CTX should not be accessed by more than one thread, which makes sense. However, in our case, we don't share contexts, only keys.
• src: add mutex to ManagedEVPPKey class #36825 mentions evp_pkey_downgraded, which was recently replaced in openssl/openssl@b574c6a. The current implementation looks thread-safe at first sight.

Thinking further...

• If there really is no way to use EVP_PKEYs from multiple threads, then why do they implement thread-safe reference counters?
• Assuming there really is no way to use EVP_PKEYs from multiple threads, then we should be able to clone EVP_PKEY instances before using them to prevent mutexes from negatively affecting performance.

[danbev]

I'm aware of these and I saw that evp_pkey_downgrade was recently replaced. But even with it gone I think we might put ourselves in the same situation we have now where we are using objects that are not guaranteed to be thread safe.

I think the suggestions here are good and worth pursuing. What I would like is we get the OpenSSL 3.0 CI job in place before we start this work. It was time consuming to get all the tests to pass against OpenSSL 3.0 and the threading issue was only one of them. At the moment any changes to crypto could break the tests and I have to go chasing them down afterwards.

[panva]

@danbev are you tracking the places where parallel performance may have been affected by these openssl3 related changes?

If not we have to look at all crypto async APIs (that's for sure this one here and all of webcrypto) and bench them with the same keyobject vs unique keyobjects, before and after the changes were put in place and figure out a way forward if performance was impacted like this API here.

Ideally this would've been done before the changes landed in a release.

[danbev]

are you tracking the places where parallel performance may have been affected by these openssl3 related changes?

Anywhere an EVP_PKEY is used from multiple threads. There was an assumption that this was not required with versions prior to OpenSSL 3.0, but there was never such a guarantee from OpenSSL about this as far as I understand it.

[panva]

@danbev @tniessen I've added a parallel libuv threaded test and the only failure with openssl3 is the one from #38136. Can I assume the lock is therefore not necessary and this can proceed?

[nodejs-github-bot]

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

[jasnell]

Excellent work

[mcollina]

lgtm

[mcollina]

Why is this identified as semver-major?

[panva]

Why is this identified as semver-major?

It isn't anymore. At first it looked like there were going to be minor differences.

[panva]

Landed in e8cb644