-
-
Notifications
You must be signed in to change notification settings - Fork 243
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
Fix build and tests for wolfSSL #352
Conversation
Signed-off-by: Juliusz Sosinowicz <juliusz@wolfssl.com>
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.
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 |
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 was this header not required before? Would it be possible to just at the header?
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.
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.
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 |
Our compatibility layer headers are a set of macros that change the OpenSSL names to wolfSSL compatibility equivalents. Like |
This is failing in the OpenSSL without deprecated functions action. I think the solution would be for |
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 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.
this is deprecated with 3.0
I appreciate the review. I will try to get back to this PR next week. |
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.
playing with getting the right headers pulled in
🤞 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. |
Thanks for helping out here. I haven't had time to bring it across the finish line in the past month. |
Tested with wolfSSL master