Fix build and tests for wolfSSL by julek-wolfssl · Pull Request #352

Fix build and tests for wolfSSL by julek-wolfssl · Pull Request #352 · Thalhammer/jwt-cpp

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 <juliusz@wolfssl.com>

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

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?

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 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.

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.

/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.

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.

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

use the new macro that was required

🤞 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.