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

feat: update scripts to use OpenSSL 3 #4992

Closed
wants to merge 2 commits into from

Conversation

chenrui333
Copy link
Contributor

@chenrui333 chenrui333 commented Jan 3, 2025

Description of changes:

since 1.3.22 release, homebrew has switched to use openssl@3, also [email protected] has been disabled on the homebrew side for quite some time.

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

@lrstewart lrstewart requested a review from dougch January 3, 2025 07:24
Copy link
Contributor

@lrstewart lrstewart left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

You can ignore the "Performance Regression Test" failure-- it looks like that's failing for several PRs right now and is probably just misbehaving.

For the "macOS build and test S2n" failure, I think your problem is that script calls s2n_setup_env.sh, which sets S2N_LIBCRYPTO to "openssl-1.1.1" by default. s2n_build_test then complains that S2N_LIBCRYPTO was set to "openssl-1.1.1" but you actually built against openssl-3. You should be able to fix this by just setting S2N_LIBCRYPTO to "openssl-3.0" in the s2n_osx.sh script.

Copy link
Contributor

@dougch dougch left a comment

Choose a reason for hiding this comment

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

Thanks for starting this! Our s2n_build_test is checking to make sure that the openssl version matches the S2N_LIBCRYPTO environment variable set in CI and fails, because we're currently assuming 3.0, but brew is installing 3.4.

Getting this working might require adding more versions to s2n_build_test.c:

        { .libcrypto = "openssl-3.1", .is_openssl = true },
        { .libcrypto = "openssl-3.2", .is_openssl = true },
        { .libcrypto = "openssl-3.3", .is_openssl = true },
        { .libcrypto = "openssl-3.4", .is_openssl = true },

BREWINSTLLPATH=$(brew --prefix openssl@1.1)
OPENSSL_1_1_1_INSTALL_DIR="${BREWINSTLLPATH:-"/usr/local/Cellar/openssl@1.1/1.1.1?"}"
BREWINSTLLPATH=$(brew --prefix openssl@3)
OPENSSL_3_INSTALL_DIR="${BREWINSTLLPATH:-"/usr/local/Cellar/openssl@3/3.0.0?"}"
Copy link
Contributor

Choose a reason for hiding this comment

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

homebrew paths have apparently changed:

Suggested change
OPENSSL_3_INSTALL_DIR="${BREWINSTLLPATH:-"/usr/local/Cellar/openssl@3/3.0.0?"}"
OPENSSL_3_INSTALL_DIR="${BREWINSTLLPATH:-"/opt/homebrew/Cellar/openssl@3/3.?"}"

@@ -17,22 +17,22 @@ set -eu
source codebuild/bin/s2n_setup_env.sh
Copy link
Contributor

@dougch dougch Jan 8, 2025

Choose a reason for hiding this comment

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

We really need to deprecate the s2n_setup_env script, it's making some assumptions...set this value explicitly:

Suggested change
source codebuild/bin/s2n_setup_env.sh
export S2N_LIBCRYPTO=openssl-3.4

@dougch
Copy link
Contributor

dougch commented Jan 16, 2025

Now I'm emotionally invested in this... applying my suggestions to this in #5041

@dougch
Copy link
Contributor

dougch commented Jan 16, 2025

Closing in favor of #5041

@dougch dougch closed this Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants