WIP: crypto: support multiple Crypto versions/libraries

[danbev]

The goal is to be able to support different versions/vendors of SSL/Crypto libraries. Motivation for this have been brought up in #11828.

This pull request is meant to be a proof of concept/spike to try to figure out one way of accomplishing this. Hopefully there is enough here to get some feedback and see if this is worth pursuing.

Steps taken:

1. Break dependency to OpenSSL

This has been done by extracting the usages of OpenSSL specific code and macros into src/node_crypto.h.

The type/version of the crypto library implementation is specified during configuration:

./configure --crypto-version=openssl_1_0_2e

This is currently the default if no value is specified. The crypto-version is used to define a variable named node_crypto_version that is then used to select the correct sources/tests to execute.

In addition, a variable named NODE_CRYPTO_VERSION will also be defined which is used in src/node.cc to set std::string crypto_version

A CryptoFactory was introduced that can be used to get a crypto implementation, for example:

auto crypto = crypto::CryptoFactory::Get(crypto_version, env);

obj->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "tls_npn"),
    Boolean::New(env->isolate(), crypto->HasNPN()));

2. Create Crypto implementation for OpenSSL 1_0_2e

Create a concrete Crypto implementation for openssl_1_0_2e

3. Create Crypto implementation for OpenSSL 1_1_0f

Create a concrete Crypto implementation for openssl_1_1_0f

4. Copy current implementation to src/crypto_impl/openssl/1_0_2e

The intention here is not to duplicate these classes for each implementation but instead use it as a starting point and verify that the project builds and test pass.

Next step is to begin extracting common parts and introduce datatypes that are common to all crypto implementations/version and move them into a suitable interface.

To build and run tests:

$ ./configure && make -j8 test

5. Copy current implementation to src/crypto_impl/openssl/1_1_0f

As mentioned in step 4, the intention is not to duplicate all this code use it as a starting point.

To build and run tests:

./configure --debug --shared-openssl --shared-openssl-libpath=/Users/danielbevenius/work/security/build_1_1_0f/lib --prefix=/Users/danielbevenius/work/nodejs/build --shared-openssl-includes=/Users/danielbevenius/work/security/build_1_1_0f/include --crypto-version=openssl_1_1_0f && make -j8 test

This step in currenly in progress
While I used as much as possible from #11828 there are some things I need to sort out to get all the test to pass.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

Affected core subsystem(s)

crypto

[gibfahn]

Related to #9376 (by @rvagg )

[danbev]

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

[BridgeAR]

@danbev are you still working on this? There are lots of conflicts and this definitely needs a rebase.

[BridgeAR]

Ping @danbev again

[danbev]

Ping @danbev again

Sorry about the late reply. I'll take a look at rebasing this next week and see if I can make some progress on it.

[bnoordhuis]

Motivation for this have been brought up in #11828.

Can you expand? On the face of it, this PR introduces a great deal of churn and complexity for mostly abstract benefit.

[danbev]

Can you expand? On the face of it, this PR introduces a great deal of churn and complexity for mostly abstract benefit.

The basic idea is to be able to support different version of OpenSSL (also different implementations but I'm not sure if there is any demand for that at all) so that supporting a new version of OpenSSL does not mean that code for a previous version has to be changed. This would also allow users to continue using an older version of OpenSSL on their systems.

The goal would be to avoid some of the troubles of upgrading from 1_0_2 to 1_1_0 in the future by doing this now.

@bnoordhuis Does this make sense or am I'm missing something that might prevent something like this (I might not have taken this far enough to discover other issues for example)?

[bnoordhuis]

I mean this in the nicest way possible but if you want to support multiple openssl versions, this is about the worst approach you could pick: it duplicates > 10,000 lines of code where a couple of #if OPENSSL_VERSION_NUMBER > ... would do the trick.

It might be a feasible approach if the goal is to support different crypto libraries, but my gut instinct is that there are enough openssl-isms leaking through in the JS API that any replacement would probably be limited to the point of DOA.

(And as you say, it's not even clear there is demand for it.)

[danbev]

it duplicates > 10,000 lines of code where a couple of #if OPENSSL_VERSION_NUMBER > ... would do the trick.

I agree, it does at the moment but the goal would be to refactor this and share code common between the same implementation versions where possible. I did not want to spend too much time on it before going any further as it might not be worth pursuing.

It might be a feasible approach if the goal is to support different crypto libraries, but my gut instinct is that there are enough openssl-isms leaking through in the JS API that any replacement would probably be limited to the point of DOA.

Ah I see, would breaking the openssl-ism in the JS API be an option or is this a huge undertaking and risk perhaps?

[bnoordhuis]

would breaking the openssl-ism in the JS API be an option

Only if it can be done without introducing too many backwards incompatible changes in the public JS API. What constitutes 'too many' depends on whom you ask; some will say any incompatible change.

Random example: the ciphers string parser. That's done by openssl, you'd need to replace that in full to be viable.

[jasnell]

Definitely a huge undertaking but an important one. I view it largely in the same way as something like N-API in that it would be a very significant chunk of work but with tangible benefits. That said, supporting multiple openssl versions, even with a good abstraction layer that reduced the openssl-isms that bleed out to JS likely would not be worth the time and effort. What likely would be worthwhile is supporting entirely different crypto backends (libressl for instance)

[bnoordhuis]

@danbev Are you still interested in pursuing this somehow? If not, can you close?

[danbev]

Are you still interested in pursuing this somehow? If not, can you close?

I am but perhaps in a different form and take a look at supporting entirely different crypto backends like @jasnell mentioned. But I'll close for now and hopefully come up with a new suggestions.