search

Thalhammer / jwt-cpp

A header only library for creating and validating json web tokens in c++
https://thalhammer.github.io/jwt-cpp/
MIT License
864 stars 235 forks source link

Compilation fails with OpenSSL 3.0 and OPENSSL_NO_DEPRECATED #223

Closed janblome closed 2 years ago

janblome commented 2 years ago

What happened?

Using OpenSSL 3.0.1 with OPENSSL_NO_DEPRECATED defined (as document here) results in compilation errors in jwt.h.

How To Reproduce?

#define OPENSSL_NO_DEPRECATED
#include "jwt-cpp/jwt.h"

int main() { return 0; }

Version

0.6.0

What OS are you seeing the problem on?

Windows

What compiler are you seeing the problem on?

MSVC

Relevant log output

jwt.h(1410): error C2065: 'RSA_PKCS1_PSS_PADDING': undeclared identifier
jwt.h(1410): error C3861: 'EVP_PKEY_CTX_set_rsa_padding': identifier not found
jwt.h(1417): error C3861: 'EVP_PKEY_CTX_set_rsa_pss_saltlen': identifier not found
jwt.h(1464): error C2065: 'RSA_PKCS1_PSS_PADDING': undeclared identifier
jwt.h(1464): error C3861: 'EVP_PKEY_CTX_set_rsa_padding': identifier not found
jwt.h(1471): error C3861: 'EVP_PKEY_CTX_set_rsa_pss_saltlen': identifier not found

Code of Conduct

janblome commented 2 years ago

I think this is caused by a missing include. The man page for EVP_PKEY_CTX_set_rsa_padding places the function in <openssl/rsa.h>, which does not seem to be included in jwt.h. Explicitly including it before jwt.h fixes the issue for me:

#define OPENSSL_NO_DEPRECATED
#include <openssl/rsa.h>
#include "jwt-cpp/jwt.h"

int main() { return 0; }
prince-chrismc commented 2 years ago

Looking at OpenSSL, at least one is not deprecated https://github.com/openssl/openssl/blob/1bf649b5998ac98511203f54ce954eccfaf75467/include/openssl/rsa.h#L119 so I am curious is the problem is platform specific....

Can you please share how you are building, installing and linking against OpenSSL. There's no standard way on windows AFAIK.

We test this configuration on Linux and it's passing https://github.com/Thalhammer/jwt-cpp/runs/5853386571?check_suite_focus=true

I am curious what the windows vs Linux is 🤔

https://github.com/Thalhammer/jwt-cpp/blob/e0f89e042d524b6c6f1ad44928b6797f25e7a8fd/.github/workflows/ssl.yml#L53

janblome commented 2 years ago

Thanks for the quick response! I'm using Conan to pull both OpenSSL and jwt-cpp itself from ConanCenter. My CMakeLists.txt looks like this:

cmake_minimum_required(VERSION 3.16)
project(jwt-cpp-demo)

include("conan.cmake")

conan_cmake_configure(
    REQUIRES jwt-cpp/0.6.0
    REQUIRES openssl/3.0.1
    GENERATORS cmake
)
conan_cmake_autodetect(
    CONAN_SETTINGS
)
conan_cmake_install(
    PATH_OR_REFERENCE .
    REMOTE conancenter
    SETTINGS ${CONAN_SETTINGS}
)

include("${CMAKE_BINARY_DIR}/conanbuildinfo.cmake")
conan_basic_setup(TARGETS)

add_executable(
    demo
    "source/main.cpp"
)
target_link_libraries(
    demo
    CONAN_PKG::jwt-cpp
    CONAN_PKG::openssl
)
target_compile_definitions(
    demo
    PRIVATE JWT_DISABLE_PICOJSON
    #PRIVATE OPENSSL_NO_DEPRECATED
)

The main.cpp reference there just contains the code I put in the "How To Reproduce" section.

Since this way of pulling dependencies is platform independent this should also work on Linux. I just tried it in a Ubuntu Docker container and I do get similar errors there (using gcc):

jwt.h: In member function 'std::string jwt::algorithm::pss::sign(const string&, std::error_code&) const':
jwt.h:1410:43: error: 'RSA_PKCS1_PSS_PADDING' was not declared in this scope
 1410 |     if (EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_PSS_PADDING) <= 0) {
      |                                           ^~~~~~~~~~~~~~~~~~~~~
jwt.h:1410:9: error: 'EVP_PKEY_CTX_set_rsa_padding' was not declared in this scope; did you mean 'EVP_PKEY_CTX_set_app_data'?
 1410 |     if (EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_PSS_PADDING) <= 0) {
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |         EVP_PKEY_CTX_set_app_data
jwt.h:1417:9: error: 'EVP_PKEY_CTX_set_rsa_pss_saltlen' was not declared in this scope; did you mean 'EVP_PKEY_CTX_set_app_data'?
 1417 |     if (EVP_PKEY_CTX_set_rsa_pss_saltlen(ctx, -1) <= 0) {
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |         EVP_PKEY_CTX_set_app_data
jwt.h: In member function 'void jwt::algorithm::pss::verify(const string&, const string&, std::error_code&) const':
jwt.h:1464:43: error: 'RSA_PKCS1_PSS_PADDING' was not declared in this scope
 1464 |     if (EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_PSS_PADDING) <= 0) {
      |                                           ^~~~~~~~~~~~~~~~~~~~~
jwt.h:1464:9: error: 'EVP_PKEY_CTX_set_rsa_padding' was not declared in this scope; did you mean 'EVP_PKEY_CTX_set_app_data'?
 1464 |     if (EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_PSS_PADDING) <= 0) {
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |         EVP_PKEY_CTX_set_app_data
jwt.h:1471:9: error: 'EVP_PKEY_CTX_set_rsa_pss_saltlen' was not declared in this scope; did you mean 'EVP_PKEY_CTX_set_app_data'?
 1471 |     if (EVP_PKEY_CTX_set_rsa_pss_saltlen(ctx, -1) <= 0) {
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |         EVP_PKEY_CTX_set_app_data

I didn't realize this project had a specific test for this, that's very cool! From the snippet you posted it looks like the test is setting OPENSSL_NO_DEPRECATED_3_0 instead of OPENSSL_NO_DEPRECATED. And indeed, when I set only OPENSSL_NO_DEPRECATED_3_0 then the compiler errors are gone. But from a quick search I can't find any documentation on this define, so it is not clear to me what the difference between the two is.

prince-chrismc commented 2 years ago

Perfect amazing. I should be able to repro the problem.

Thanks for all the details ❤️

Thalhammer commented 2 years ago

Might be that we rely on transient includes from openssl which are no longer included if OPENSSL_NO_DEPRECATED is defined. Seems like other projects had similar issues: https://github.com/chriskohlhoff/asio/issues/126 According to https://github.com/openssl/openssl/blob/44fde441937fc8db8ea6a7ac2e7c683ad9d5f8e0/include/openssl/macros.h OPENSSL_NO_DEPRECATED causes all of the following to be defined:

OPENSSL_NO_DEPRECATED_3_0
OPENSSL_NO_DEPRECATED_1_1_1
OPENSSL_NO_DEPRECATED_1_1_0
OPENSSL_NO_DEPRECATED_1_0_2
OPENSSL_NO_DEPRECATED_1_0_1
OPENSSL_NO_DEPRECATED_1_0_0
OPENSSL_NO_DEPRECATED_0_9_8

which might help to narrow down the issue.

janblome commented 2 years ago

I looked a bit through the OpenSSL headers and might have an idea of what is happening here. The missing functions the compiler complains about are defined in openssl/rsa.h, which is indirectly included by jwt.h like this:

As @Thalhammer pointed out setting OPENSSL_NO_DEPRECATED also sets OPENSSL_NO_DEPRECATED_1_1_0, so that would cause openssl/rsa.h to not be included. Which also explains why only setting OPENSSL_NO_DEPRECATED_3_0 doesn't cause the error I was seeing.

So my proposed fix for this would be to simply add an explicit include for openssl/rsa.h in jwt.h. I also checked OpenSSL 1.0.2 and the header already exists there, so don't think this would break compatibility with any supported versions. Optionally I would also suggest to change the test to use OPENSSL_NO_DEPRECATED, since that seems to be the more strict setting.

If you agree with this approach I could prepare a pull request with these changes 🙂.

prince-chrismc commented 2 years ago

Please feel free to try opening a PR! ❤️

I gave it a quick test but it seems there are more deprecated API calls https://github.com/prince-chrismc/jwt-cpp/runs/6382096564?check_suite_focus=true

https://www.openssl.org/docs/man3.0/man3/EC_KEY_new.html#HISTORY

EVP_EC_gen() was added in OpenSSL 3.0. All other functions described here were deprecated in OpenSSL 3.0. For replacement see EVP_PKEY-EC(7).

janblome commented 2 years ago

Thanks a lot! I took a look at the file with the deprecated function calls. If I understand that file correctly it re-defines some OpenSSL functions to be able to change their behavior for testing. But as far as I can tell the re-definitions of the deprecated functions aren't actually needed since the library itself doesn't use them anymore 😄. I also checked the variables used to change their behavior and they seem to also not be used anywhere. So I think the functions can simply be removed.

I tried that in a fork, and it looks like all the checks succeeded there: janblome#1. So I also create a PR here: #228.

prince-chrismc commented 2 years ago

But as far as I can tell the re-definitions of the deprecated functions aren't actually needed since the library itself doesn't use them anymore 😄

Thank you so much for looking deeper into that and getting a PR submitted!!!

I triggered the CI and it should be merged soon 🤞