Add support for wolfSSL TLS backend #867
Conversation
There was a problem hiding this comment.
@kerbert101 Do you mind adding a GitHub action job that sets up a VM with WolfSSL and runs the tests on it? The relevant places to change would be
nats.c/.github/workflows/build-test.yml
Line 98 in 7a3a385
nats.c/.github/workflows/on-pr-debug.yml
Line 50 in 7a3a385
Coverage-WolfSSL job. I can do it myself, but would need to a) help with the setup of WolfSSL if its not trivial, and b) permission to push to your fork/branch.
There was a problem hiding this comment.
Even if you were to add GitHub actions as instructed by @levb, the test would still use OpenSSL since you did not add the WolfSSL include/link in the test's CMakeFile.
If we are going to support another backend, we need to have more generic "SSL" build variables that will represent the chosen SSL library.
My other concern is that it looks like WolfSSL is GPLv2 and may not be compatible with Apache 2.0 license. But I don't know enough about it to know if it would be "safe" in this context.
| endif(NATS_BUILD_WITH_TLS AND NOT NATS_BUILD_WITH_WOLFSSL) | ||
|
|
||
| if(NATS_BUILD_WITH_WOLFSSL) | ||
| if(DEFINED ENV{NATS_WOLFSSL_DIR}) |
There was a problem hiding this comment.
All this is a bit complex, no? Look at what we do for OpenSSL: find_package(OpenSSL REQUIRED). Is there no equivalent for that?
| add_definitions(-D${NATS_OS}) | ||
| add_definitions(-D_REENTRANT) | ||
| if(NATS_BUILD_WITH_TLS) | ||
|
|
||
| if(NATS_BUILD_WITH_TLS OR NATS_BUILD_WITH_WOLFSSL) |
There was a problem hiding this comment.
Since I don't see where you would set NATS_BUILD_WITH_TLS to "false" when using WolfSSL, then not sure the "OR" is needed here, but we should also question if the following settings make sense if using WolfSSL library.
| @@ -10,6 +10,10 @@ if(NATS_BUILD_WITH_TLS) | |||
| set(NATS_OPENSSL_LIBS "${OPENSSL_LIBRARIES}") | |||
| endif(NATS_BUILD_WITH_TLS) | |||
|
|
|||
| if(NATS_BUILD_WITH_WOLFSSL) | |||
There was a problem hiding this comment.
But then just above we possibly include OpenSSL libraries, is that the right thing to do?
| @@ -37,15 +41,15 @@ endif(NATS_BUILD_STREAMING) | |||
| # -------------------------------------- | |||
| if(NATS_BUILD_LIB_SHARED) | |||
| add_library(nats SHARED ${SOURCES} ${GLIB_SOURCES} ${PS_SOURCES} ${S_SOURCES}) | |||
| target_link_libraries(nats ${NATS_OPENSSL_LIBS} ${NATS_EXTRA_LIB} ${NATS_PROTOBUF_LIBRARIES} ${NATS_SODIUM_LIBRARIES}) | |||
| target_link_libraries(nats ${NATS_OPENSSL_LIBS} ${NATS_WOLFSSL_LIBRARIES} ${NATS_EXTRA_LIB} ${NATS_PROTOBUF_LIBRARIES} ${NATS_SODIUM_LIBRARIES}) | |||
There was a problem hiding this comment.
If we support different SSL libraries, I think we should have a NATS_SSL_LIBS that would be set to either one.
| set_target_properties(nats PROPERTIES | ||
| VERSION ${NATS_VERSION_MAJOR}.${NATS_VERSION_MINOR}.${NATS_VERSION_PATCH} | ||
| SOVERSION ${NATS_VERSION_MAJOR}.${NATS_VERSION_MINOR}) | ||
| endif(NATS_BUILD_LIB_SHARED) | ||
|
|
||
| if(NATS_BUILD_LIB_STATIC) | ||
| add_library(nats_static STATIC ${SOURCES} ${GLIB_SOURCES} ${PS_SOURCES} ${S_SOURCES}) | ||
| target_link_libraries(nats_static ${NATS_OPENSSL_LIBS} ${NATS_PROTOBUF_LIBRARIES} ${NATS_SODIUM_LIBRARIES}) | ||
| target_link_libraries(nats_static ${NATS_OPENSSL_LIBS} ${NATS_WOLFSSL_LIBRARIES} ${NATS_PROTOBUF_LIBRARIES} ${NATS_SODIUM_LIBRARIES}) |
| @@ -44,6 +44,10 @@ | |||
| #define NATS_EVENT_ACTION_ADD (true) | |||
| #define NATS_EVENT_ACTION_REMOVE (false) | |||
|
|
|||
| #ifdef NATS_HAS_WOLFSSL | |||
| typedef int (*SSL_verify_cb)(int preverify_ok, X509_STORE_CTX *x509_ctx); | |||
There was a problem hiding this comment.
Is that not defined in WolfSSL?
| #if defined(NATS_USE_OPENSSL_1_1) | ||
| // the SSL_set_hostflags function is not available in the | ||
| // wolfSSL OpenSSL compatibility layer, so use the new function instead | ||
| #if defined(NATS_USE_OPENSSL_1_1) && !defined(NATS_HAS_WOLFSSL) |
There was a problem hiding this comment.
Related to the comment in the main CMakeFile.txt. If NATS_USE_OPENSSL_1_1 does not make sense in the context of WolfSSL, then it should not have been set in the make file.
|
@levb In principle, I can fix the tests—I don't expect that to be a problem. I see that the protobuf and sodium libraries are included in the @kozlovic: I agree with your comments. I reused the block for protobuf-c to find wolfSSL. It might be improved, but I haven’t fully looked into it yet. The 1.1.1 flag should indeed probably be set to false by default for the wolfSSL implementation. Regarding the licenses: I currently don't have enough knowledge to say whether combining GPLv2 and Apache 2.0 is possible. It might be a good idea to first consider whether it's desirable to support multiple TLS backends. |
That for sure would NOT be good, because unlike those, we can distribute and have the LICENSE file attached. Having the wolfSSL library (GPLv2) in the nats.c.deps repo (Apache 2.0) would not be legal.
Agreed that it should be the first thing to decide on. Maybe @levb and the rest of the team can discuss that and get back to you. |
This adds support for wolfSSL as the TLS backend. It uses the OpenSSL Compatibility Layer of wolfSSL (wolfSSL must be compiled with --enable-opensslextra)
More information about the compatibility layer:
https://www.wolfssl.com/documentation/manuals/wolfssl/chapter13.html