feat: feature probe S2N_LIBCRYPTO_SUPPORTS_ENGINE #4878

toidiu · 2024-11-08T03:57:33Z

Review this PR first. This PR was getting too large so I split some changes into its own PR.

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.

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.

tests/features/S2N_LIBCRYPTO_SUPPORTS_ENGINE.c

tests/unit/s2n_random_test.c

toidiu · 2024-11-09T01:06:10Z

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?

tests/features/S2N_LIBCRYPTO_SUPPORTS_ENGINE.c

tests/unit/s2n_random_test.c

tests/unit/s2n_override_openssl_random_test.c

utils/s2n_random.c

tests/unit/s2n_override_openssl_random_test.c

utils/s2n_random.c

tests/features/S2N_LIBCRYPTO_SUPPORTS_ENGINE.c

tests/unit/s2n_random_test.c

utils/s2n_random.c

toidiu · 2024-11-15T23:50:09Z

tests/unit/s2n_random_test.c

+         * We expect ENGINE APIs to be available if `OPENSSL_NO_ENGINE` is not
+         * defined.
+         */
+    #if !defined(OPENSSL_NO_ENGINE) && !defined(S2N_LIBCRYPTO_SUPPORTS_ENGINE)


This should be positive.. "if is_openssl_1_0_2 we want the feature probe to be true".

crypto/s2n_fips.c

tests/unit/s2n_random_test.c

lrstewart · 2024-11-21T22:41:54Z

utils/s2n_random.c

+bool s2n_libcrypto_is_likely_openssl()
+{
+    return !s2n_libcrypto_is_boringssl() && !s2n_libcrypto_is_libressl() && !s2n_libcrypto_is_awslc();
+}


Does this method belong in this module?

And I recognize that I suggested this name, but I still don't like it. Maybe we need to just commit to "s2n_libcrypto_is_openssl", since that's how we'll treat this method, even if it's not 100% guaranteed accurate. Maybe a comment is sufficient to clarify that point?

Also, consider how you could make this more readable:

Suggested change

bool s2n_libcrypto_is_likely_openssl()

{

return !s2n_libcrypto_is_boringssl() && !s2n_libcrypto_is_libressl() && !s2n_libcrypto_is_awslc();

}

bool s2n_libcrypto_is_likely_openssl()

{

bool is_other_libcrypto = s2n_libcrypto_is_boringssl() || s2n_libcrypto_is_libressl() || s2n_libcrypto_is_awslc();

return !is_other_libcrypto;

}

I wonder if we can also fix the awkward "is_likely" thing by inverting this method completely. ~~We can write a completely definitive "s2n_libcrypto_is_not_openssl" method. But negative names can be tricky in their own way 🤔~~ Actually I guess it's not definitive either, since we could not cover one option. Maybe "s2n_libcrypto_is_known_variant"?

Does this method belong in this module?

Maybe we need to just commit to "s2n_libcrypto_is_openssl"

s2n_libcrypto_is_openssl is risky because we cant actually be 100% sure it is OpenSSL. Because of the assumptions, I decided to place the method is in this file because I didnt want it to be generally available across the codebase.

AI: search of other uses and move to a single global location so that we can find and update it in the future.

tests/unit/s2n_random_test.c

lrstewart · 2024-11-21T22:50:10Z

tests/unit/s2n_random_test.c

+#if !defined(S2N_LIBCRYPTO_SUPPORTS_ENGINE)
+        EXPECT_FALSE(s2n_supports_custom_rand());
+#endif
+
+#if defined(OPENSSL_NO_ENGINE)
+        EXPECT_FALSE(s2n_supports_custom_rand());
+#endif


How/where is OPENSSL_NO_ENGINE defined? Have you tested with a libcrypto with engine support disabled to confirm it's defined where you expect it to be defined?

Nit: I still don't think these are particularly useful tests and they might not be worth the conditional compilation.

Have you tested with a libcrypto with engine support disabled to confirm it's defined where you expect it to be defined?

Yep, I actually included build instructions on how to do this. It the expandable section Local build instructions.

How/where is OPENSSL_NO_ENGINE defined?

OPENSSL_NO_ENGINE is lightly documented (search for OPENSSL_NO_ENGINE in https://wiki.openssl.org/index.php/Compilation_and_Installation) and the expected behavior when openssl is build with no-engine which makes me partial to including it.

Based on this comment, the OPENSSL_NO_ENGINE define comes from openssl/configuration.h. Its not clear if that header if being included and hence if this test is executing correctly.

Lesson: we should only rely on s2n defines (s2n-feature probe define or an explicit s2n-cmake define). Relying on an external define, i.e. OPENSSL_NO_ENGINE, requires that we be certain that the correct header is included. Since removing the header can very subtly break code, we should avoid it in general.

lrstewart · 2024-11-21T23:35:06Z

tests/unit/s2n_random_test.c

+        EXPECT_NOT_NULL(rand_method);
+        EXPECT_NOT_EQUAL((s2n_rand_meth_fn_type) rand_method->bytes, (s2n_rand_meth_fn_type) s2n_openssl_compat_rand);


Defining a type for this is probably overkill. I'd forgotten that you can't use void* for functions and there's no function version of uintptr_t -_- Still, you should be able to just do:

Suggested change

EXPECT_NOT_NULL(rand_method);

EXPECT_NOT_EQUAL((s2n_rand_meth_fn_type) rand_method->bytes, (s2n_rand_meth_fn_type) s2n_openssl_compat_rand);

EXPECT_NOT_NULL(rand_method);

EXPECT_NOT_EQUAL((void (*)(void)) rand_method->bytes, (void (*)(void)) s2n_openssl_compat_rand);

It just looks sillier than void* or uintptr_t.

Originally I used void* but that resulted in valgrind warnings about casting to void*. We could wrap the cast with diagnostic ignore, but that would be uglier imo. Not sure if there is a better option.

lrstewart · 2024-11-21T23:39:40Z

tests/features/S2N_LIBCRYPTO_SUPPORTS_ENGINE.c

+/* LibreSSL requires the rand.h file.
+ *
+ * https://github.com/aws/s2n-tls/issues/153#issuecomment-129651643
+ */
+#include <openssl/rand.h>


Wait, why does our feature probe include this? We don't include <openssl/rand.h> in utils/s2n_random.c, so wouldn't this produce a false positive where it looks like LibreSSL supports custom rand, but we fail to compile because we didn't include this file?

Did you test with LibreSSL?

We do include <openssl/rand.h>.

The positive probe test was failing for LibreSSL, which is how I discovered this.

crypto/s2n_fips.c

This reverts commit ab66d00.

github-actions bot added the s2n-core team label Nov 8, 2024

toidiu changed the title ~~Ak no engine~~ feat: feature probe S2N_LIBCRYPTO_SUPPORTS_ENGINE Nov 8, 2024

toidiu force-pushed the ak-no-engine branch from 6f05229 to 8410c6d Compare November 8, 2024 04:21

camshaft reviewed Nov 9, 2024

View reviewed changes

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 force-pushed the ak-no-engine branch 4 times, most recently from b329d11 to f051bbf Compare November 12, 2024 21:07

toidiu requested review from camshaft, lrstewart and torben-hansen November 12, 2024 21:55

toidiu marked this pull request as ready for review November 12, 2024 21:55

toidiu force-pushed the ak-no-engine branch from cb3f7ac to b5df39f Compare November 12, 2024 22:04

lrstewart reviewed Nov 12, 2024

View reviewed changes

toidiu commented Nov 13, 2024

View reviewed changes

tests/unit/s2n_override_openssl_random_test.c Outdated Show resolved Hide resolved

camshaft reviewed Nov 14, 2024

View reviewed changes

utils/s2n_random.c Outdated Show resolved Hide resolved

camshaft approved these changes Nov 14, 2024

View reviewed changes

toidiu commented Nov 14, 2024

View reviewed changes

tests/features/S2N_LIBCRYPTO_SUPPORTS_ENGINE.c Outdated Show resolved Hide resolved

toidiu requested a review from lrstewart November 14, 2024 23:17

lrstewart reviewed Nov 15, 2024

View reviewed changes

toidiu force-pushed the ak-no-engine branch from 2614a58 to 074e249 Compare November 15, 2024 18:57

toidiu commented Nov 15, 2024

View reviewed changes

toidiu force-pushed the ak-no-engine branch from 04a8319 to 7c546ed Compare November 16, 2024 23:19

toidiu commented Nov 18, 2024

View reviewed changes

crypto/s2n_fips.c Outdated Show resolved Hide resolved

toidiu commented Nov 18, 2024

View reviewed changes

tests/unit/s2n_random_test.c Outdated Show resolved Hide resolved

toidiu mentioned this pull request Nov 18, 2024

Fix build without openssl/engine.h #4873

Open

toidiu requested a review from lrstewart November 19, 2024 17:41

lrstewart reviewed Nov 21, 2024

View reviewed changes

toidiu mentioned this pull request Nov 23, 2024

Feature probes incorrectly fail on AL2 due to zlib issues #4900

Open

5 tasks

toidiu added 2 commits November 22, 2024 22:29

chore: add s2n_libcrypto_is_openssl helper, docs, fix test

f2d3a14

feat: feature probe S2N_LIBCRYPTO_SUPPORTS_ENGINE

28030d0

toidiu force-pushed the ak-no-engine branch from 4aa3c9c to 28030d0 Compare November 23, 2024 17:01

toidiu added 2 commits November 23, 2024 09:14

positive feature probe

ab66d00

Revert "positive feature probe"

d45e824

This reverts commit ab66d00.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: feature probe S2N_LIBCRYPTO_SUPPORTS_ENGINE #4878

feat: feature probe S2N_LIBCRYPTO_SUPPORTS_ENGINE #4878

toidiu commented Nov 8, 2024 •

edited

Loading

toidiu commented Nov 9, 2024

toidiu Nov 15, 2024

lrstewart Nov 21, 2024 •

edited

Loading

toidiu Nov 22, 2024

toidiu Nov 22, 2024

lrstewart Nov 21, 2024

toidiu Nov 22, 2024 •

edited

Loading

toidiu Nov 23, 2024

lrstewart Nov 21, 2024

toidiu Nov 22, 2024 •

edited

Loading

lrstewart Nov 21, 2024

toidiu Nov 22, 2024

		EXPECT_NOT_NULL(rand_method);
		EXPECT_NOT_EQUAL((s2n_rand_meth_fn_type) rand_method->bytes, (s2n_rand_meth_fn_type) s2n_openssl_compat_rand);

feat: feature probe S2N_LIBCRYPTO_SUPPORTS_ENGINE #4878

Are you sure you want to change the base?

feat: feature probe S2N_LIBCRYPTO_SUPPORTS_ENGINE #4878

Conversation

toidiu commented Nov 8, 2024 • edited Loading

Description of changes:

Testing:

toidiu commented Nov 9, 2024

Choose a reason for hiding this comment

lrstewart Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toidiu Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toidiu Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toidiu commented Nov 8, 2024 •

edited

Loading

lrstewart Nov 21, 2024 •

edited

Loading

toidiu Nov 22, 2024 •

edited

Loading

toidiu Nov 22, 2024 •

edited

Loading