-
Notifications
You must be signed in to change notification settings - Fork 294
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
feat crypto: support wolfssl library, help wanted #523
base: develop
Are you sure you want to change the base?
feat crypto: support wolfssl library, help wanted #523
Conversation
closes userver-framework#498 This PR provides minimal changes which replaces internal usage of openssl. It does not checked for other libraries used in userver. Some openssl features does not implemented by wolfssl itself. (has found some typos during investigation, see wolfSSL/wolfssl#7423). BTW, wolfssl v5.7.0-stable requires few small patches which also included into the PR. Finally, "short path" to support wolfssl is not available, because of the lib does not implements some of used functions. It also does not provide `ENGINE_*`, but have not been tried with wolfengine lib yet. PR is building with commands: ``` mkdir -p build_debug cd build_debug cmake \ -Wdev \ -DCMAKE_CXX_COMPILER=clang++-17 \ -DUSERVER_FEATURE_WOLFSSL=ON \ -DUSERVER_DOWNLOAD_PACKAGE_WOLFSSL=ON \ -DUSERVER_FEATURE_GRPC=OFF \ -DUSERVER_FEATURE_POSTGRESQL=OFF \ -DUSERVER_FEATURE_MYSQL=OFF \ -DUSERVER_FEATURE_STACKTRACE=OFF \ -DUSERVER_FEATURE_CLICKHOUSE=OFF \ -DUSERVER_USE_LD=lld \ .. ``` Patch might be re-applied with command: ```(test -d build_debug/_deps/wolfssl-src && cd build_debug/_deps/wolfssl-src && git checkout -- .)``` Final error will be attached in comments to this PR. Help needed.
Latest build log in attach. |
@@ -172,6 +173,13 @@ option(USERVER_FEATURE_MYSQL "Provide asynchronous driver for MariaDB/MySQL" "${ | |||
|
|||
option(USERVER_FEATURE_UBOOST_CORO "Use vendored boost context instead of a system one" ON) | |||
|
|||
if (USERVER_FEATURE_WOLFSSL) | |||
include(cmake/SetupWolfSSL.cmake) | |||
add_compile_definitions("OPENSSL_EXTRA=1") |
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.
this compile definitions added directly because it is required by wolfssl headers.
@@ -172,6 +173,13 @@ option(USERVER_FEATURE_MYSQL "Provide asynchronous driver for MariaDB/MySQL" "${ | |||
|
|||
option(USERVER_FEATURE_UBOOST_CORO "Use vendored boost context instead of a system one" ON) | |||
|
|||
if (USERVER_FEATURE_WOLFSSL) | |||
include(cmake/SetupWolfSSL.cmake) | |||
add_compile_definitions("OPENSSL_EXTRA=1") |
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.
target_compile_definitions
"CMAKE_C_FLAGS -Wall -Wextra -O2 -DOPENSSL_ALL -DOPENSSL_EXTRA" | ||
) | ||
|
||
#add_library(WolfSSL INTERFACE) |
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.
garbage? or conditional code?
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.
Had experimented with, mostly a garbage.
The PR still in draft state and I'll clear this when (and if) all job will be done.
) | ||
|
||
if (USERVER_FEATURE_WOLFSSL) | ||
target_link_libraries(${PROJECT_NAME} |
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.
the same code snippet is copied multiple times
it's better to declare INTERFACE target, link openssl/wolfssl targets, and define all required macros in this interface library
@@ -81,6 +91,12 @@ int CurveStringToNid(const std::string_view& curve_str) { | |||
return it->second; | |||
} | |||
|
|||
#ifdef USERVER_FEATURE_WOLFSSL | |||
#ifndef EC_KEY_set_public_key_affine_coordinates | |||
#warning "EC_KEY_set_public_key_affine_coordinates does not provided by wolfSSL" |
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.
is it an old wolfssl version limitation?
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.
At the point of PR, 5.7.0 is the latest stable version.
WolfSSL does not provide function with suffix _coordinates
, but EC_KEY_set_public_key
See https://github.com/wolfSSL/wolfssl/blob/master/wolfssl/openssl/ec.h
@@ -41,6 +48,13 @@ std::optional<std::string> GetPemStringImpl(EVP_PKEY* key, | |||
|
|||
std::string result; | |||
result.resize(BIO_pending(membio.get())); | |||
#ifdef USERVER_FEATURE_WOLFSSL | |||
#warning "BIO_read_ex does not supported by wolfSSL" | |||
if (1 != BIO_read(membio.get(), result.data(), result.size())) { |
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 guess we may use BIO_read() in openssl too as we don't care about readbytes
#define EVP_PKEY_CTX_set_rsa_pss_saltlen(...) 0 | ||
#endif | ||
#ifndef EVP_PKEY_CTX_set_rsa_mgf1_md | ||
#warning "EVP_PKEY_CTX_set_rsa_mgf1_md() is undefined by wolfSSL" |
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.
why is it possible?
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.
These functions are missed in the header wolfssl/openssl/evp.h. Sadly, buy I have no idea currently how to make workaround for these functions.
@@ -31,6 +36,13 @@ std::optional<std::string> Certificate::GetPemString() const { | |||
|
|||
std::string result; | |||
result.resize(BIO_pending(membio.get())); | |||
#ifdef USERVER_FEATURE_WOLFSSL | |||
#warning "BIO_read_ex does not supported by wolfSSL" | |||
if (1 != BIO_read(membio.get(), result.data(), result.size())) { |
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.
same here, we can use BIO_read() in both cases
@@ -13,7 +13,6 @@ | |||
/cmake-build-*/ | |||
/cmake/cmake_generated/ | |||
/docs/ | |||
/third_party/ |
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.
this line was added eariler by mistake.
"CMAKE_C_FLAGS -Wall -Wextra -O2 -DOPENSSL_ALL -DOPENSSL_EXTRA" | ||
) | ||
|
||
#add_library(WolfSSL INTERFACE) |
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.
Had experimented with, mostly a garbage.
The PR still in draft state and I'll clear this when (and if) all job will be done.
@@ -81,6 +91,12 @@ int CurveStringToNid(const std::string_view& curve_str) { | |||
return it->second; | |||
} | |||
|
|||
#ifdef USERVER_FEATURE_WOLFSSL | |||
#ifndef EC_KEY_set_public_key_affine_coordinates | |||
#warning "EC_KEY_set_public_key_affine_coordinates does not provided by wolfSSL" |
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.
At the point of PR, 5.7.0 is the latest stable version.
WolfSSL does not provide function with suffix _coordinates
, but EC_KEY_set_public_key
See https://github.com/wolfSSL/wolfssl/blob/master/wolfssl/openssl/ec.h
#define EVP_PKEY_CTX_set_rsa_pss_saltlen(...) 0 | ||
#endif | ||
#ifndef EVP_PKEY_CTX_set_rsa_mgf1_md | ||
#warning "EVP_PKEY_CTX_set_rsa_mgf1_md() is undefined by wolfSSL" |
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.
These functions are missed in the header wolfssl/openssl/evp.h. Sadly, buy I have no idea currently how to make workaround for these functions.
closes #498
Currently, this PR won't build by the reason described below.
This PR provides minimal changes which replaces internal usage of openssl. It does not checked for other libraries used in userver. Some openssl features does not implemented by wolfssl itself. (has found some typos during investigation, see wolfSSL/wolfssl#7423).
BTW, wolfssl v5.7.0-stable requires few small patches which also included into the PR.
Finally, "short path" to support wolfssl is not available, because of the lib does not implements some of used functions. It also does not provide
ENGINE_*
, but have not been tried with wolfengine lib yet.Worst thing is that wolfssl does not provides any CMS_* analogue to soft migrate onto this library, so, need some code branching using available functions in wolfssl.
PR is building with commands:
mkdir -p build_debug cd build_debug cmake \ -Wdev \ -DCMAKE_CXX_COMPILER=clang++-17 \ -DUSERVER_FEATURE_WOLFSSL=ON \ -DUSERVER_DOWNLOAD_PACKAGE_WOLFSSL=ON \ -DUSERVER_FEATURE_GRPC=OFF \ -DUSERVER_FEATURE_POSTGRESQL=OFF \ -DUSERVER_FEATURE_MYSQL=OFF \ -DUSERVER_FEATURE_STACKTRACE=OFF \ -DUSERVER_FEATURE_CLICKHOUSE=OFF \ -DUSERVER_USE_LD=lld \ ..
Patch might be re-applied with command:
(test -d build_debug/_deps/wolfssl-src && cd build_debug/_deps/wolfssl-src && git checkout -- .)
Current errors will be attached in comments to this PR.
Help needed with re-implementation of current crypto's features using wolfssl.
In my opinion, wolfssl is not production ready yet because of a lot of issues https://github.com/wolfSSL/wolfssl/labels/bug