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

Wrong OpenSSL CMake targets used #2874

Closed
gjasny opened this issue Feb 27, 2024 · 4 comments
Closed

Wrong OpenSSL CMake targets used #2874

gjasny opened this issue Feb 27, 2024 · 4 comments
Labels
bug This issue is a bug. closed-for-staleness Cmake Cmake related submissions p3 This is a minor priority issue response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 10 days.

Comments

@gjasny
Copy link

gjasny commented Feb 27, 2024

Describe the bug

Hi,

while working on packaging the 1.11 version for Conan (conan-io/conan-center-index#22905) I noticed that compilation cannot find any OpenSSL headers. The Conan package manager fiddles with some CMake default lookup rules which might reveal the error.

This is for version 1.11.273: In your CMakeLists.txt in line 208 you include external_dependencies

include(external_dependencies)

which does the OpenSSL crypto lib lookup and as part of that sets CRYPTO_LIBS to the imported target AWS::crypto:

add_definitions(-DENABLE_OPENSSL_ENCRYPTION)
message(STATUS "Encryption: LibCrypto")
set(CRYPTO_TARGET_NAME "AWS::crypto")
if(PLATFORM_ANDROID AND ANDROID_BUILD_OPENSSL)
set(BUILD_OPENSSL 1)
set(CRYPTO_TARGET_NAME "crypto")
set(USE_OPENSSL ON)
message(STATUS " Building Openssl as part of AWS SDK")
else()
find_package(crypto REQUIRED)
endif()
set(CRYPTO_LIBS ${CRYPTO_TARGET_NAME} ${ZLIB_LIBRARIES})
# ssl depends on libcrypto
set(CRYPTO_LIBS_ABSTRACT_NAME ${CRYPTO_TARGET_NAME} ssl z)

This is good because linking the CMake aws-cpp-sdk-all against the imported target later will apply both, the link libraries and the include dirs.

But in your CMakeLists.txt lines 250 you overwrite CRYPTO_LIBS with ${OPENSSL_LIBRARIES}.

aws-sdk-cpp/CMakeLists.txt

Lines 260 to 266 in 5929e20

if (ENABLE_BCRYPT_ENCRYPTION)
set(CRYPTO_LIBS Bcrypt)
set(CRYPTO_LIBS_ABSTRACT_NAME Bcrypt)
elseif (ENABLE_OPENSSL_ENCRYPTION)
set(CRYPTO_LIBS ${OPENSSL_LIBRARIES} ${ZLIB_LIBRARIES})
set(CRYPTO_LIBS_ABSTRACT_NAME crypto ssl z)
endif ()

The problem is that OPENSSL_LIBRARIES is empty and I also could not find any invocation of find_package(OpenSSL) which could have set it.

The same problem applies to CLIENT_LIBS.

Could you please take a look what's going on and which of the code blocks is the correct and desired one?

Expected Behavior

OpenSSL is properly used.

Current Behavior

[ 18%] Building CXX object src/aws-cpp-sdk-core/CMakeFiles/aws-cpp-sdk-core.dir/source/http/curl/CurlHttpClient.cpp.o
In file included from /Users/jenkins/w/prod-v2/bsr@2/98097/eacca/p/b/aws-s92c917073c641/b/build/Release/src/aws-cpp-sdk-core/ub_core.cpp:50:
In file included from /Users/jenkins/w/prod-v2/bsr@2/98097/eacca/p/b/aws-s92c917073c641/b/src/src/aws-cpp-sdk-core/source/utils/crypto/openssl/CryptoImpl.cpp:9:
/Users/jenkins/w/prod-v2/bsr@2/98097/eacca/p/b/aws-s92c917073c641/b/src/src/aws-cpp-sdk-core/include/aws/core/utils/crypto/openssl/CryptoImpl.h:12:10: fatal error: 'openssl/ossl_typ.h' file not found
#include <openssl/ossl_typ.h>
         ^~~~~~~~~~~~~~~~~~~~

Reproduction Steps

git clone https://github.com/gjasny/conan-center-index.git
git checkout aws-sdk-cpp/1.11.179
git reset --hard 026b203b4f46d6b637899e2b020cd41e914f9667
# install Conan 2
recipes/aws-sdk-cpp/conan create all/conanfile.py --version 1.11.179 --user local --channel local -pr:b=default -pr:h=default --build=missing

Possible Solution

I commented out the block which overwrites the discovered variables:
https://github.com/gjasny/conan-center-index/blob/6d49f275630c48a8f757b2ec0f58fe0ebb607b69/recipes/aws-sdk-cpp/all/patches/1.11.179-0002-fix-openssl-and-curl-lookup.patch#L1-L22

Additional Information/Context

No response

AWS CPP SDK version used

1.11.273

Compiler and Version used

Xcode 15

Operating System and version

macOS 14.3

@gjasny gjasny added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Feb 27, 2024
@jmklix
Copy link
Member

jmklix commented Mar 13, 2024

Thanks for opening this issue. We are currently in the process of reworking our cmake build process.

I'm not sure that your possible solution will fit all use cases, but I wanted to make sure I understand your request. Are you suggested commenting out this entire block?

    #[[
    if (ENABLE_BCRYPT_ENCRYPTION)
        set(CRYPTO_LIBS Bcrypt)
        set(CRYPTO_LIBS_ABSTRACT_NAME Bcrypt)
    elseif (ENABLE_OPENSSL_ENCRYPTION)
        set(CRYPTO_LIBS ${OPENSSL_LIBRARIES} ${ZLIB_LIBRARIES})
        set(CRYPTO_LIBS_ABSTRACT_NAME crypto ssl z)
    endif ()

    if (ENABLE_CURL_CLIENT)
        set(CLIENT_LIBS ${CURL_LIBRARIES})
        set(CLIENT_LIBS_ABSTRACT_NAME curl)
    elseif (ENABLE_WINDOWS_CLIENT)
        if (USE_IXML_HTTP_REQUEST_2)
            set(CLIENT_LIBS msxml6 runtimeobject)
            set(CLIENT_LIBS_ABSTRACT_NAME msxml6 runtimeobject)
            if (BYPASS_DEFAULT_PROXY)
                list(APPEND CLIENT_LIBS winhttp)
                list(APPEND CLIENT_LIBS_ABSTRACT_NAME winhttp)
            endif ()
        else ()
            set(CLIENT_LIBS Wininet winhttp)
            set(CLIENT_LIBS_ABSTRACT_NAME Wininet winhttp)
        endif ()
    endif ()
    ]]

@jmklix jmklix added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 10 days. p3 This is a minor priority issue Cmake Cmake related submissions and removed needs-triage This issue or PR still needs to be triaged. labels Mar 13, 2024
Copy link

Greetings! It looks like this issue hasn’t been active in longer than a week. We encourage you to check if this is still an issue in the latest release. Because it has been longer than a week since the last update on this, and in the absence of more information, we will be closing this issue soon. If you find that this is still a problem, please feel free to provide a comment or add an upvote to prevent automatic closure, or if the issue is already closed, please feel free to open a new one.

@github-actions github-actions bot added closing-soon This issue will automatically close in 4 days unless further comments are made. closed-for-staleness and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels Mar 24, 2024
@MouhK
Copy link

MouhK commented Aug 20, 2024

Any update on this issue. I have the same problem

@jmklix
Copy link
Member

jmklix commented Aug 20, 2024

Yes, this should be fixed with this patch. We updated this sdk to not depend on crypto directly, but rather use the common runtime's(aws-c-common) dependency on crypto. Updating this sdk to the latest version should fix any problems that you might be running into with this. If it doesn't fix it for you please open a new issue/discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. closed-for-staleness Cmake Cmake related submissions p3 This is a minor priority issue response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 10 days.
Projects
None yet
Development

No branches or pull requests

3 participants