ssl: support libssls with no ENGINE implementation

[chrisnovakovic]

OpenSSL can be built without ENGINE support, and some libssl-compatible forks (e.g. BoringSSL) don't contain any ENGINE implementation at all - guard all references to the ENGINE API using OPENSSL_NO_ENGINE so these libssls can be used with librdkafka.

[edenhill]

Please rebase this on latest master, and verify that it builds correctly with LibreSSL.

Thanks!

[chrisnovakovic]

While rebasing on master, I also refactored the changes to src/rdkafka_ssl.c - now there's an OPENSSL_HAS_ENGINE macro that eliminates the need to repeat the conditional every time.

The following table lists the result of the 0124_openssl_invalid_engine test with various libssls before and after applying this PR:

libssl	master	#3535
OpenSSL 1.0.2u	SKIPPED	SKIPPED
OpenSSL 1.1.1m with ENGINE	PASSED	PASSED
OpenSSL 1.1.1m without ENGINE	Build failure	SKIPPED
LibreSSL 3.3.4	SKIPPED	PASSED

SKIPPED indicates that the test was skipped with the reason Configuration property "ssl.engine.location" not supported in this build: OpenSSL with ENGINE support not available at build time.

Note that the test isn't skipped on LibreSSL any more - LibreSSL 3.3 does contain an ENGINE component, and it should probably be used if enabled (e.g. so it can make use of AES-NI).

[edenhill]

Some style-fixes still needed (run make style-fix)

[chrisnovakovic]

Unfortunately it seems that clang-format bounces between

        .unsupported = "OpenSSL with ENGINE support not available at build "   \
                       "time"

and

        .unsupported =                                                         \
            "OpenSSL with ENGINE support not available at build "              \
            "time"

as recommendations - using one always causes it to suggest the other. There's no way that's not a bug in clang-format, but I'll just reword that string so it fits on a single line.

[chrisnovakovic]

Tried to get clever by using a string that was unambiguously over the 80-column limit, but clang-format wasn't having any of that either. Reverted to a shorter, single-line message and it looks like we're there now. Sorry for all the noise!

[edenhill]

Thanks for all your work on this @chrisnovakovic!

We're a bit too close to the 1.9.0 release to merge this, but we'll make sure to include it in a 1.9.1 release (scheduled for June).

[chrisnovakovic]

Rebased on master and edited to account for the changes made to accommodate OpenSSL 3.0 - #4026 defines WITH_SSL_ENGINE (making most of this PR redundant), but it doesn't consider the value of OPENSSL_NO_ENGINE when defining WITH_SSL_ENGINE, so librdkafka still fails to link to libssls that don't have ENGINE support due to missing symbols.

[martirosyanH]

We need this change as well in order to build librdkafka.

[cla-assistant]

All committers have signed the CLA.

[mhdawson]

@emasab this seems like a small change that was intended to land a while back based on the comment in - #3535 (comment)

Not having this is causing some compile failures when trying to use https://github.com/Blizzard/node-rdkafka on some newer Linux distros that define OPENSSL_NO_ENGINE

[showuon]

@chrisnovakovic , the changelog should be updated to the latest release. Could you help update it?
@edenhill , any other comments to this PR?
Thanks.

[showuon]

I've created #4852 to not include the change log. We should add it when making sure it will be merged.

[chrisnovakovic]

I'll rebase this shortly. Sorry for the delay.

[confluent-cla-assistant]

🎉 All Contributor License Agreements have been signed. Ready to merge.
✅ chrisnovakovic
_{Please push an empty commit if you would like to re-run the checks to verify CLA status for all contributors.}

[chrisnovakovic]

Rebased on master. Based on f55f3ec, I'm assuming v2.6.0 is the next release, so that's the version I credited the changes to in the changelog - happy to modify it again if that's incorrect.

[showuon]

@milindl @emasab @anchitj , call for review for this 1 line change PR, and had already approved by @edenhill before. Without this change, it will impact some users adopt librdkafka. Thank you.

[calvin2021y]

any update?

[mhdawson]

@emasab I've seen you landing PR's recently so I thought I'd see if you thought there was any chance this one would make it to the top of the priority list any time soon?

[nhaq-confluent]

Hey everyone thanks for the input and the PR, we will take a look and aim to have it in our upcoming 2.7.0 release

[emasab]

@chrisnovakovic Thanks a lot for the PR and sorry for the long wait. It looks good, could you just add
#include <openssl/rand.h> to rdkafka_admin.h inside the #if WITH_SSL. As rdkafka_admin.c is making use of RAND_priv_bytes for secure random salt generation and rand.h is included by engine.h at the moment (Thanks @remicollet for checking this in #4911)

[chrisnovakovic]

@chrisnovakovic Thanks a lot for the PR and sorry for the long wait. It looks good, could you just add #include <openssl/rand.h> to rdkafka_admin.h inside the #if WITH_SSL. As rdkafka_admin.c is making use of RAND_priv_bytes for secure random salt generation and rand.h is included by engine.h at the moment (Thanks @remicollet for checking this in #4911)

Good call. I'll rebase and update in a moment.

[chrisnovakovic]

Also updated the commit message and the changelog line in the latest commit.

[emasab]

/sem-approve

[emasab]

LGTM, Thanks @chrisnovakovic @remicollet ! As our policy expects a second review for external contributors, I'm asking for it from my team.

[emasab]

How to test:

# 1) in mklove/modules/configure.libssl add "no-deprecated no-engine" at line 98

# mklove has a problem as inherits CFLAGS for building the dependencies too
# but OpenSSL has a warning about floating point comparison so cannot be built with -Werror
# so we need to unset CFLAGS if they include some flag that is not compatible
# with the dependencies.
unset CFLAGS
make distclean
./configure --install-deps --source-deps-only --enable-static --disable-lz4-ext --enable-strip
# run make with -Werror
export CFLAGS="-Werror"
# make will work with OPENSSL_NO_ENGINE
make
. tests/_venv/bin/activate
# run SSL tests
(cd tests && python3 -m trivup.clusters.KafkaCluster --version 3.4.0 --ssl --cmd 'TESTS=0133,0097 make')
# now checkout current version
git checkout master
# make will fail with OPENSSL_NO_ENGINE
make

[pranavrth]

/sem-approve

[pranavrth]

LGTM!. Thanks.