Fix build of s2n_random.c if OPENSSL_NO_ENGINE

[neverpanic]

Resolved issues:

Resolves awslabs/aws-crt-python#583

Description of changes:

OpenSSL does not create the openssl/engine.h header when compiled with the no-engine option, which creates the OPENSSL_NO_ENGINE preprocessor symbol.

Call-outs:

I have not tested this.

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.

[jmayclin]

Hi @neverpanic ! Thanks for the PR, and sorry for the delay in review. I just want to verify that this works locally before approving it. I should have an update tomorrow!

[jmayclin]

Hi @neverpanic , could you provide more details about the exact issue that you were encountering and how to reproduce it?

• What version of openssl were you using?
• can you share how it was compiled?

I am having difficulty reproducing the issue. I checked out the latest version of openssl and compiled from source using

./Configure no-engine --prefix=/home/ec2-user/workspace/openssl-install
make -j 16
make install

I double checked the openssl config with perl configdata.pm --dump to confirm that the engine feature was disabled.

Disabled features:

    <SNIP>
    egd                    [default]        OPENSSL_NO_EGD
    engine                 [option]         OPENSSL_NO_ENGINE (skip engines, crypto/engine)
    external-tests         [default]        OPENSSL_NO_EXTERNAL_TESTS
    <SNIP>

However, this does appear to generate the header file.

$ find openssl-install/ -name engine.h
openssl-install/include/openssl/engine.h

And I can successfully build and test s2n-tls

cmake . \
    -B build \
    -D CMAKE_PREFIX_PATH=/home/ec2-user/workspace/openssl-install \
    -D CMAKE_C_COMPILER=clang
cmake --build ./build -j $(nproc)
CTEST_PARALLEL_LEVEL=$(nproc) make -C build test ARGS="--output-on-failure"

[neverpanic]

My apologies, I should have verified my assumptions. Turns out OpenSSL does always install the header, but CentOS 10 Stream hides it, because it does not want to offer ENGINE support, but also did not outright want to break existing binaries compiled on other platforms, so does not actually build with no-engine, but edits the openssl/configuration.h header to define OPENSSL_NO_ENGINE and removes the openssl/engine.h header.

This breaks building against ENGINE symbols (i.e., removes the API), which is the intended outcome, but preserves the ABI for binary compatibility.

You can use the quay.io/centos/centos:stream10-development container image to test:

$ podman run --rm -it quay.io/centos/centos:stream10-development bash
[root@65d0868ff621 /]# dnf install -y git cmake clang openssl-devel
[…]
[root@65d0868ff621 /]# git clone https://github.com/aws/s2n-tls.git
[…]
[root@65d0868ff621 /]# cd s2n-tls/
[root@65d0868ff621 s2n-tls]# git submodule update --recursive --init
Submodule 'tests/cbmc/aws-verification-model-for-libcrypto' (https://github.com/awslabs/aws-verification-model-for-libcrypto.git) registered for path 'tests/cbmc/aws-verification-model-for-libcrypto'
Cloning into '/s2n-tls/tests/cbmc/aws-verification-model-for-libcrypto'...
Submodule path 'tests/cbmc/aws-verification-model-for-libcrypto': checked out 'd732f7257fc569e1be32d34037b111af0a058ab0'
[root@65d0868ff621 s2n-tls]# cmake . -Bbuild -DCMAKE_C_COMPILER=clang
[…]
[root@65d0868ff621 s2n-tls]# cmake --build ./build -j$(nproc)
[…]
/s2n-tls/utils/s2n_random.c:50:10: fatal error: 'openssl/engine.h' file not found
   50 | #include <openssl/engine.h>
      |          ^~~~~~~~~~~~~~~~~~
[…]

There's an additional use of openssl/engine.h in tests/unit/s2n_override_openssl_random_test.c that this PR does not patch. With that fixed, build and tests pass.

[jmayclin]

but edits the openssl/configuration.h header to define OPENSSL_NO_ENGINE and removes the openssl/engine.h header.

Oh, very interesting. Is it necessary to do both? On mainline openssl, all the symbols in openssl/engine.h are gated behind an ifndef OPENSSL_NO_ENGINE, so I'd expect that defining OPENSSL_NO_ENGINE would have the same effect as deleting the header.

https://github.com/openssl/openssl/blob/e91384d5b0547bf797e2b44976f142d146c4e650/include/openssl/engine.h#L22

My explicit ask would be: If centos's goal is to prevent libraries from building against openssl engine features, I think that they could do this less disruptively by only editing openssl/configuration.h to define OPENSSL_NO_ENGINE. This would still prevent libraries from using any of the engine symbols, but would avoid breaking builds that include the header without depending on any of the symbols. Is this a reasonable solution?

Edit: My own comment made me suspicious, because if OPENSSL_NO_ENGINE effectively removed the symbols, then how was my build/test of s2n-tls succeeding? So I just deleted the entire header file and looks like the s2n-tls build/test still succeeds, meaning that (for my current environment) s2n-tls doesn't actually seem to be relying on any of those symbols. I'm guessing this is related to some platform/libcrypto specific ways that we handle randomness. I'll try digging into this to find a more satisfying answer.

[neverpanic]

Oh, very interesting. Is it necessary to do both? On mainline openssl, all the symbols in openssl/engine.h are gated behind an ifndef OPENSSL_NO_ENGINE, so I'd expect that defining OPENSSL_NO_ENGINE would have the same effect as deleting the header.

Unfortunately undeclared functions are not always treated as errors by compilers. Missing includes are.

My explicit ask would be: If centos's goal is to prevent libraries from building against openssl engine features, I think that they could do this less disruptively by only editing openssl/configuration.h to define OPENSSL_NO_ENGINE. This would still prevent libraries from using any of the engine symbols, but would avoid breaking builds that include the header without depending on any of the symbols. Is this a reasonable solution?

Builds that ignore warnings about undeclared symbols would likely still pass and continue to use the ENGINE functionality if the header was present.

[jmayclin]

Sorry for the delay in response.

I've been discussing this over with the team, and we're a little worried about the PR.

From my earlier investigation, s2n-tls already supports OPENSSL_NO_ENGINE. What it doesn't support is the platform specific modifications that CentOS Stream 10 makes to the OpenSSL installation. As a general rule we try and avoid narrow #ifdef's, especially for things that we aren't testing/validating in CI. This all feels rather "non-standard", which is something we also try to avoid 😄

I'm curious if this has affected any other libraries? My expectation is that this change would break a decent number of other libraries, since openssl/engine.h has always been unconditionally available.

[neverpanic]

I'm curious if this has affected any other libraries? My expectation is that this change would break a decent number of other libraries, since openssl/engine.h has always been unconditionally available.

It's a pretty even split, I'd say. There are quite a few components that never include openssl/engine.h if OPENSSL_NO_ENGINE is defined, and about as many that do. From my impression (don't have numbers, sorry), among those that do, most upstream projects were fine with including it conditionally based on the OPENSSL_NO_ENGINE preprocessor symbol.

[jmayclin]

Hi @neverpanic,

After discussion with the team, we will be closing this PR. This is part of our general policy of not accepting any fixes that we don’t have continuously validated in CI. Additionally, we have concerns about adding build complexity to support such a narrow usecase.

Please do let us know if you find any issues with the OPENSSL_NO_ENGINE support. We expect to gracefully handle that case and we’d be happy to help debug those!