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

ci: add openssl111 to LD_LIBRARY_PATH for integv2 testing #3464

Merged
merged 3 commits into from
Sep 7, 2022

Conversation

dougch
Copy link
Contributor

@dougch dougch commented Aug 24, 2022

Resolved issues:

Partial for #3465

Description of changes:

For integration testing on Linux distributions that don't use openssl-1.1.1 as their default OS libcrypto, trying to use the openssl-1.1.1 binary fails when S2N_LIBCRYPTO is not openssl-1.1.1.

This is because we're not adding the lib path to LD_LIBRARY_PATH, and we've modified PATH so that our hand rolled openssl binary comes before the distribution provided binary.

Repro steps

On ubuntu22 with gcc-9 installed:

S2N_LIBCRYPTO=awslc ./codebuild/bin/install_default_dependencies.sh
S2N_BUILD_PRESET=awslc_gcc9 TOX_TEST_NAME=test_early_data TESTS=integrationv2 ./codebuild/bin/s2n_codebuild.sh
...
Stderr: openssl: error while loading shared libraries: libssl.so.1.1: cannot open shared object file: No such file or directory

ad-hoc CodeBuild job

Call-outs:

Longer term, a refactor of the integv2 tests is needed, likely bundled with the move to cmake, where we should stop putting openssl-1.1.1 first in the PATH.

Testing:

How is this change tested (unit tests, fuzz tests, etc.)? Are there any testing steps to be verified by the reviewer?

Is this a refactor change? If so, how have you proved that the intended behavior hasn't changed?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@dougch dougch requested a review from goatgoose August 24, 2022 21:44
@github-actions github-actions bot added the s2n-core team label Aug 24, 2022
@dougch dougch marked this pull request as ready for review August 24, 2022 21:45
@dougch dougch requested a review from a team as a code owner August 24, 2022 21:45
@dougch dougch mentioned this pull request Aug 24, 2022
7 tasks
@dougch dougch requested a review from toidiu August 24, 2022 23:28
@@ -28,7 +28,7 @@ endif
define run_tox
( \
DYLD_LIBRARY_PATH="$(LIBCRYPTO_ROOT)/lib:$$DYLD_LIBRARY_PATH" \
LD_LIBRARY_PATH="$(LIBCRYPTO_ROOT)/lib:"$(S2N_ROOT)/test-deps/gnutls37/nettle/lib":$$LD_LIBRARY_PATH" \
LD_LIBRARY_PATH="$(LIBCRYPTO_ROOT)/lib:"$(S2N_ROOT)/test-deps/openssl-1.1.1/lib":"$(S2N_ROOT)/test-deps/gnutls37/nettle/lib":$$LD_LIBRARY_PATH" \
Copy link
Contributor

Choose a reason for hiding this comment

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

why cant we just have LD_LIBRARY_PATH="$(LIBCRYPTO_ROOT)/lib? seems like we should always specify the LIBCRYPTO_ROOT in our testing. If that doesnt exist then failing seems like the better option rather than continuing with 111 or gnutls37/nettle/lib (which is default i assume?)

Copy link
Contributor Author

@dougch dougch Aug 25, 2022

Choose a reason for hiding this comment

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

We're using the openssl utility regardless of what S2N_LIBCRYPTO is set to, and ld can't find the correct libcrypto.so file, because it's in a non-standard location and it's not in LD_LIBRARY_PATH.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alternative fix is to remove OPENSSL_1_1_1_INSTALL_DIR from the PATH here, but this means we'll be using openssl3 on ubuntu22/AL2022 and I wasn't sure if that would cause other issues/concerns.

@dougch dougch requested a review from toidiu August 25, 2022 17:45
@dougch dougch merged commit e28ae9a into aws:main Sep 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants