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

Fix build and tests for wolfSSL #352

Merged
merged 16 commits into from
Jul 29, 2024

Conversation

julek-wolfssl
Copy link
Contributor

@julek-wolfssl julek-wolfssl commented Jun 5, 2024

Tested with wolfSSL master

cmake -B build -DJWT_SSL_LIBRARY:STRING=wolfSSL -DJWT_BUILD_TESTS=ON  .
make -j -C build
./build/tests/jwt-cpp-test

Signed-off-by: Juliusz Sosinowicz <[email protected]>
Copy link
Collaborator

@prince-chrismc prince-chrismc left a comment

Choose a reason for hiding this comment

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

Thanks a ton for this! prince-chrismc#35 (comment) I've been staring at thos for a while so I really appreciate this.

I have a couple questions since I don't use wolfssl anymore I don't understand all the changes

CMakeLists.txt Outdated
@@ -125,7 +125,8 @@ if(${JWT_SSL_LIBRARY} MATCHES "wolfSSL")
target_link_libraries(jwt-cpp INTERFACE PkgConfig::wolfssl)
# This is required to access OpenSSL compatibility API
target_include_directories(jwt-cpp INTERFACE ${wolfssl_INCLUDE_DIRS})
target_compile_definitions(jwt-cpp INTERFACE OPENSSL_EXTRA OPENSSL_ALL)
# EXTERNAL_OPTS_OPENVPN is necessary so that wolfssl/options.h is included automatically
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why was this header not required before? Would it be possible to just at the header?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was always necessary to include wolfssl/options.h. Notice that in your current builds you have

In file included from /usr/local/include/wolfssl/openssl/bn.h:33,
                 from /usr/local/include/wolfssl/openssl/ec.h:27,
                 from /home/runner/work/jwt-cpp/jwt-cpp/include/jwt-cpp/jwt.h:15,
                 from /home/runner/work/jwt-cpp/jwt-cpp/tests/JwksTest.cpp:1:
/usr/local/include/wolfssl/wolfcrypt/settings.h:2369:14: warning: #warning "For timing resistance / side-channel attack prevention consider using harden options" [-Wcpp]
 2369 |             #warning "For timing resistance / side-channel attack prevention consider using harden options"
      |              ^~~~~~~

which is a dead giveaway that wolfssl/options.h is missing. I think by defining OPENSSL_EXTRA OPENSSL_ALL you were able to somewhat build with wolfSSL but I don't know how the tests are "passing". I didn't investigate this.

tests/OpenSSLErrorTest.cpp Show resolved Hide resolved
@Thalhammer
Copy link
Owner

Great to see someone from inside wolfSSL taking the time to improve support in opensource libraries.

I noticed the symbol names for wolfSSL are different from openssl (hence the new SYMBOL_NAME macro), which I assume are then converted to the OpenSSL ones using a define. The OpenSSL Error test works by providing a definition for the weakly defined symbols in openssl (which is also why it doesn't work with static linking). Given wolfssl has different symbol names, shouldn't the naming of the methods change as well, not just the methods they resolve to ? Keep in mind I have never worked with wolfSSL before, so I might be talking nonsense.

@julek-wolfssl
Copy link
Contributor Author

julek-wolfssl commented Jun 6, 2024

Given wolfssl has different symbol names, shouldn't the naming of the methods change as well, not just the methods they resolve to ?

Our compatibility layer headers are a set of macros that change the OpenSSL names to wolfSSL compatibility equivalents. Like SSL_new -> wolfSSL_new (#define SSL_new wolfSSL_new inside wolfssl/openssl/ssl.h.

@julek-wolfssl
Copy link
Contributor Author

/home/runner/work/jwt-cpp/jwt-cpp/tests/OpenSSLErrorTest.cpp:340:1: error: ‘RSA’ does not name a type
  340 | RSA* EVP_PKEY_get1_RSA(EVP_PKEY* pkey) {
      | ^~~

This is failing in the OpenSSL without deprecated functions action. I think the solution would be for EVP_PKEY_get1_RSA to not be used when building against OpenSSL with deprecated functions removed. But I also don't think that this is the PR to do this in.

Copy link
Collaborator

@prince-chrismc prince-chrismc left a comment

Choose a reason for hiding this comment

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

This is so amazing helpful. Sorry for pushing the limits of the compatibility layer 🤣 but I see wolfSSL/wolfssl#7618 so hopefully it was not too bad.

The only change I am unsure about is the new defines added to CMake, I think that could be avoided, the openssl and libressl are not used --- it makes it inconsistent with how it was being referenced.

I have no issue with disabling the tests that are not support or have conflicts. The rest of the changes make sense but I did have a few questions just to confirm.

Thanks again for helping to improve this.

include/jwt-cpp/jwt.h Outdated Show resolved Hide resolved
include/jwt-cpp/jwt.h Outdated Show resolved Hide resolved
tests/OpenSSLErrorTest.cpp Outdated Show resolved Hide resolved
tests/OpenSSLErrorTest.cpp Outdated Show resolved Hide resolved
@julek-wolfssl
Copy link
Contributor Author

I appreciate the review. I will try to get back to this PR next week.

Copy link
Collaborator

@prince-chrismc prince-chrismc left a comment

Choose a reason for hiding this comment

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

playing with getting the right headers pulled in

.github/workflows/ssl.yml Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
docs/install.md Outdated Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@prince-chrismc
Copy link
Collaborator

🤞 this should be ready. Thanks so much for contributing to both side to make the integration better 🚀

I wont be updating the docs with the new tested version because I have a PR to update the the SSL versions which would conflict.

@julek-wolfssl
Copy link
Contributor Author

Thanks for helping out here. I haven't had time to bring it across the finish line in the past month.

@prince-chrismc prince-chrismc merged commit a968dfe into Thalhammer:master Jul 29, 2024
60 checks passed
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.

3 participants