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

Add OpenSSL support to main branch #256

Draft
wants to merge 63 commits into
base: main
Choose a base branch
from

Conversation

tedjpoole
Copy link
Contributor

This pull request just cherry-picks our OpenSSL related commits from the release/v1.31 branch into the main branch.

@tedjpoole tedjpoole force-pushed the add-openssl branch 2 times, most recently from a266cda to 9ffd7aa Compare September 30, 2024 13:18
tedjpoole and others added 28 commits October 3, 2024 14:51
Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
…b89d846ec53f2)

BoringSSL Commit ca1690e221677cea3fb946f324eb89d846ec53f2
Now in the bssl-compat/third_party/boringssl/ directory
According to https://boringssl.googlesource.com/boringssl/+/HEAD/INCORPORATING.md

Disabled the configure/build for BoringSSL because (1) it can't be done on all
platforms, and (2) we no longer need to configure/build BoringSSL to obtain it's
crypto_test_data.cc file because it is now checked in.

Removed the pre installation of go into the builder image. This was only being done
as a work around to support the BoringSSL configure/build, but that requirement has
now gone.

Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
…_rsa_key_usage

Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
`maxmind` is causing the build to not honor the flag `--//bazel:http3=False`: The
define `ENVOY_ENABLE_QUIC` is still being passed to the compiler. This
causes code that rely on the presence (or not) of that define to behave
wrongly.

I am not 100% sure of what causes it, but Bazel doc says 1) to not use
`bind` and 2) that `bind` and `select` do not play well together: https://bazel.build/reference/be/workspace#bind

By removing the `bind` and pointing directly to the actual dependency in
`maxmind` BUILD file, we fix this issue.

Backport of envoyproxy/envoy#33638

Signed-off-by: Jonh Wendell <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
We can now use the original OpenSSL functions as Envoy has stopped
accessing the internal struct fields of BIO_METHOD (relevant change in
Envoy was in 0ff3fcb). This change also
removes our wrapper functions to deal with this behavior and the tests
for them.

Signed-off-by: Daniel Grimm <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Daniel Grimm <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
* Only supports synchronous (pass or fail) verification, which is enough to
accommodate the default certificate validator.

* Also fixed/extended the implementation of SSL_get_peer_full_cert_chain()
so that (1) it's return value now has the correct ownership semantics, and
(2) it works in the context of a SSL_CTX_set_custom_verify() callback.

Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
…s options)

Note that this really is a misuse of the "boringssl=fips" define, and the "nofips" tag.
However, pretending that we are building on a FIPS version of BoringSSL has the side
effect of compiling out QUIC support, which is what we want to achieve.

At some point, when a newer version of BoringSSL FIPS does support building QUIC,
this misuse of these options will almost certainly stop working. At that point,
we will need to fix the //bazel:http3=False option.

Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
Eliminated the need for the openssl/do_ci.sh script, so the
upstream ci/do_ci.sh script should now be used directly instead.

Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
For this test to pass, it requires OpenSSL's legacy provider, so that the
RC2-40-CBC encryption algorithm is available. Previously, this was achieved
via an OpenSSL configuration file, pointed to by the OPENSSL_CONF env var,
which was set up in openssl/do_ci.sh script. But since the openssl/do_ci.sh
script no longer exists, we have to load (and unload) the legacy provider
programatically instead.

Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
Since we do not currently support async cert validation,
the following tests have been disabled:

SslIntegrationTest.AsyncCertValidationSucceeds
SslIntegrationTest.AsyncCertValidationSucceedsWithLocalAddress
SslIntegrationTest.AsyncCertValidationAfterTearDown
SslIntegrationTest.AsyncCertValidationAfterSslShutdown

Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
…tests

By making BoringSSL's ssl_private_key_method_st struct defintion, and a few
extra functions, available in bssl-compat, it is now possible to compile all of
Envoy's private key method provider mplementation and test code. The main
reason for this is to minimise the number of diffs wrt upstream.

Clearly, because the private key method provider mechanism isn't actually
implemented on OpenSSL, all the tests which actually excercise the private key
method provider will fail, so they are all disabled.

Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
Switching OpenSSL between FIPS and non-FIPS is a config choice that is made
during deployment. Therefore, FIPS vs non-FIPS mode has no affect during build
time. Therefore the envoy-openssl binary has no concept of being built for one
mode or the other.

Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
…f BoringSSL

Some tests check for things like JA3 fingerprints and/or received byte
counts, which vary between BoringSSL and OpenSSL due to slightly
different client hello contents etc.

Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
The ErrTest.test_SSL_R_NO_SUITABLE_SIGNATURE_ALGORITHM test was failing
to compile because it was referring to SSL_R_NO_SUITABLE_SIGNATURE_ALGORITHM
rather than the prefixed ossl_SSL_R_NO_SUITABLE_SIGNATURE_ALGORITHM.

The previously generated implementations of SSL_CTX_get_session_cache_mode()
and X509_STORE_CTX_get0_chain() have been replaced with hand written ones,
with the addition of some const casting to remove compiler warnings.

Finally, the OpenSSL version is increased from 3.0.8 to 3.0.13

Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
A non-null callback was previously disallowed simply because there were no tests.
However, when building Envoy with google grpc, the callback capability is
required, and without it some of the grpc_client_integration_test fails.

Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
* Boringssl s390x fix

* rules python fix for s390x

* Update to minimum python version supported on s390x

Signed-off-by: Surender Yadav <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
NishikantThorat and others added 29 commits October 3, 2024 14:51
Signed-off-by: Nishikant Thorat <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Surender Yadav <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Swapnali911 <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
The proxy-wasm-cpp-host BUILD file adds the -lcrypto option to the link line if
either --define=crypto=system is specified to bazel, or if building on s390x.
This effectively means that on s390x, the -lcrypto linker option is *always*
added and there's no way to remove it.

This updated patch removes that special s390x case, so that -lcrypto is *not*
added to the link line unless --define=crypto=system is specified, the same
as for all other architectures. In the context of envoy-openssl, this means
that proxy-wasm-cpp-host, along with everything else, gets linked against
libbssl-compat.a rather than libcrypto.so, which is what we need.

Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
Fixes OSSM-6786

Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
Also added a test

Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
Fixes OSSM-6809

Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
The TLS inspector listener filter installs a servername callback (using
SSL_CTX_set_tlsext_servername_callback()). That callback obtains the server
name and then halts the handshake by returning SSL_TLSEXT_ERR_ALERT_FATAL.
It does this because once it has obtained the server name, it has no need
to progress the handshake any further because it's only "peeking" at the
received data, and not actually doing the "real" handshake. In upstream
envoy, on BoringSSL, this is OK, but on OpenSSL the SSL_TLSEXT_ERR_ALERT_FATAL
return value causes a "callback failed" error message to be logged. It turns
out this error message is innocuous, but it is unsigtly and distracting, so
this commit removes it by returning SSL_TLSEXT_ERR_OK instead.

Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
The previous one did not apply to the new commit

Signed-off-by: Daniel Grimm <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
…called in jwt_verify lib

Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
We will enable it with enabling quic support in envoy-openssl.

Signed-off-by: Zuzana Miklankova <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
These quiche ci_tests currently fail to compile against the bssl-compat
layer, becasue it doesn't provide enough of the required BoringSSL API.

Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
Since https://bugzilla.redhat.com/show_bug.cgi?id=1724250 is already
fixed we can reenable TLSv1.3 for FIPS mode as a default max TLS server
version.

Signed-off-by: Zuzana Miklankova <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Signed-off-by: Ted Poole <[email protected]>
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.

8 participants