Skip to content
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

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

theg4sh
Copy link
Contributor

@theg4sh theg4sh commented Apr 14, 2024

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

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.
@theg4sh theg4sh requested a review from segoon as a code owner April 14, 2024 14:20
@theg4sh theg4sh marked this pull request as draft April 14, 2024 14:20
@theg4sh
Copy link
Contributor Author

theg4sh commented Apr 14, 2024

Latest build log in attach.
build-failed.log

@@ -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")
Copy link
Contributor Author

@theg4sh theg4sh Apr 14, 2024

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.

@theg4sh theg4sh changed the title feat crypto: support wolfssl library feat crypto: support wolfssl library, help wanted Apr 14, 2024
@@ -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")
Copy link
Collaborator

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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

garbage? or conditional code?

Copy link
Contributor Author

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}
Copy link
Collaborator

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"
Copy link
Collaborator

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?

Copy link
Contributor Author

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())) {
Copy link
Collaborator

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"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is it possible?

Copy link
Contributor Author

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())) {
Copy link
Collaborator

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/
Copy link
Contributor Author

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)
Copy link
Contributor Author

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"
Copy link
Contributor Author

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"
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support WolfSSL
2 participants