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: feature probe S2N_LIBCRYPTO_SUPPORTS_ENGINE #4878

Merged
merged 13 commits into from
Dec 5, 2024
Merged

Conversation

toidiu
Copy link
Contributor

@toidiu toidiu commented Nov 8, 2024

Description of changes:

Some platform are removing the openssl/engine.h header, which causes s2n-tls builds to fail (#4705, #4873).

This PR splits the static S2N_LIBCRYPTO_SUPPORTS_CUSTOM_RAND check into a:

  • runtime check if s2n-tls custom random is supported
  • feature-probe if the linked libcrypto supports ENGINE apis

Additional benefits:
The split limits the scope of the conditional compilation to ENGINE related features. The feature probe is also more comprehensive and flexible than the static check (eg. check for the openssl/engine.h header).

Existing checks (S2N_LIBCRYPTO_SUPPORTS_CUSTOM_RAND):

  • is_openssl: (runtime check)
  • not fips: (runtime check)

New checks:

  • Check if ENGINE related APIs are defined: (feature probe)
  • Check RAND_METHOD signature: (feature probe. due to awslc signature differences)

Testing:

I added a negative test for the feature probe. The positive test is missing due to unrelated feature probe failure on AL2. There is an issue for AL2 and I verified that only AL2 was failing by testing this commit in isolation. GeneralBatch Fail vs Pass after the test was removed.

Manual testing
I build s2n-tls linked to an openssl configured and build with the no-engine option.

Local build instructions **(Click to Expand)**
### build openssl
git clone [email protected]:openssl/openssl.git openssl_no_engine

pushd openssl_no_engine;
    ./Configure no-engine --prefix=/home/toidiu/projects/s2n-tls/local_toidiu_dir_libcrypto  /openssl_no_engine/install
    # can be used to check that `OPENSSL_NO_ENGINE` is disabled
    # ./configdata.pm --dump
    make -j 16
    make install
popd
# Build s2n-tls run unit tests.
 
CMAKE_PREFIX_PATH="local_toidiu_dir_libcrypto/openssl_no_engine/install" \
    cmake . -Bbuild -GNinja \
    -DBUILD_SHARED_LIBS=ON \
    -DCMAKE_BUILD_TYPE=Debug \
    -DCMAKE_EXPORT_COMPILE_COMMANDS=1 \
    -DUNSAFE_TREAT_WARNINGS_AS_ERRORS=ON \
    -DS2N_STACKTRACE=1

cmake --build ./build -j $(nproc)

CTEST_OUTPUT_ON_FAILURE=1 CTEST_PARALLEL_LEVEL=$(nproc) ninja -C build test

-------------------- Build output
...
-- feature S2N_LIBCRYPTO_SUPPORTS_ENGINE: FALSE

-------------------- All tests PASSED
...
100% tests passed, 0 tests failed out of 273

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

@github-actions github-actions bot added the s2n-core team label Nov 8, 2024
@toidiu toidiu changed the title Ak no engine feat: feature probe S2N_LIBCRYPTO_SUPPORTS_ENGINE Nov 8, 2024
tests/features/S2N_LIBCRYPTO_SUPPORTS_ENGINE.c Outdated Show resolved Hide resolved
tests/features/S2N_LIBCRYPTO_SUPPORTS_ENGINE.c Outdated Show resolved Hide resolved
tests/unit/s2n_random_test.c Outdated Show resolved Hide resolved
@toidiu
Copy link
Contributor Author

toidiu commented Nov 9, 2024

Questions:

  • Importantly while I tested this feature probe locally, I am not sure if we test OPENSSL_NO_ENGINE in CI. Should I add a new CI task for this or do we think it wont bring too much value?

@toidiu toidiu force-pushed the ak-no-engine branch 4 times, most recently from b329d11 to f051bbf Compare November 12, 2024 21:07
@toidiu toidiu marked this pull request as ready for review November 12, 2024 21:55
tests/features/S2N_LIBCRYPTO_SUPPORTS_ENGINE.c Outdated Show resolved Hide resolved
tests/unit/s2n_random_test.c Outdated Show resolved Hide resolved
tests/unit/s2n_override_openssl_random_test.c Outdated Show resolved Hide resolved
tests/unit/s2n_override_openssl_random_test.c Outdated Show resolved Hide resolved
utils/s2n_random.c Outdated Show resolved Hide resolved
utils/s2n_random.c Show resolved Hide resolved
utils/s2n_random.c Outdated Show resolved Hide resolved
@toidiu toidiu requested a review from lrstewart November 14, 2024 23:17
tests/features/S2N_LIBCRYPTO_SUPPORTS_ENGINE.c Outdated Show resolved Hide resolved
tests/features/S2N_LIBCRYPTO_SUPPORTS_ENGINE.c Outdated Show resolved Hide resolved
tests/features/S2N_LIBCRYPTO_SUPPORTS_ENGINE.c Outdated Show resolved Hide resolved
tests/unit/s2n_random_test.c Outdated Show resolved Hide resolved
tests/unit/s2n_random_test.c Outdated Show resolved Hide resolved
tests/unit/s2n_random_test.c Outdated Show resolved Hide resolved
utils/s2n_random.c Outdated Show resolved Hide resolved
utils/s2n_random.c Outdated Show resolved Hide resolved
tests/unit/s2n_random_test.c Outdated Show resolved Hide resolved
crypto/s2n_fips.c Outdated Show resolved Hide resolved
tests/unit/s2n_random_test.c Outdated Show resolved Hide resolved
utils/s2n_random.c Outdated Show resolved Hide resolved
tests/unit/s2n_random_test.c Outdated Show resolved Hide resolved
tests/unit/s2n_random_test.c Outdated Show resolved Hide resolved
tests/unit/s2n_random_test.c Outdated Show resolved Hide resolved
tests/features/S2N_LIBCRYPTO_SUPPORTS_ENGINE.c Outdated Show resolved Hide resolved
crypto/s2n_fips.c Outdated Show resolved Hide resolved
Comment on lines 51 to 52
bool s2n_libcrypto_is_fips(void);
bool s2n_libcrypto_is_openssl();
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: looks a bit wonky to specify void for one no-arg method but not for the other

Copy link
Contributor

Choose a reason for hiding this comment

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

we usually need void for compatibility with older compilers

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
bool s2n_libcrypto_is_fips(void);
bool s2n_libcrypto_is_openssl();
bool s2n_libcrypto_is_fips(void);
bool s2n_libcrypto_is_openssl(void);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this and opened an issue for others in our codebase #4952

@toidiu toidiu requested a review from camshaft December 4, 2024 23:35
utils/s2n_random.h Outdated Show resolved Hide resolved
toidiu and others added 2 commits December 4, 2024 15:41
@toidiu toidiu enabled auto-merge (squash) December 5, 2024 04:25
@toidiu toidiu merged commit 774462f into aws:main Dec 5, 2024
39 checks passed
@toidiu toidiu deleted the ak-no-engine branch December 5, 2024 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants