This PR is expected to fix the flakes of
`//test/core/tsi:ssl_transport_security_test` when built under UBSAN.
Why is this needed? There are several tests in
`ssl_transport_security_test.cc` that involve doing many expensive
operations and PR #33638 recently added one more (namely, repeatedly
signing with an ECDSA key). The slow tests are already altered for MSAN
and TSAN, and now we need to do the same for UBSAN.
There is a bug in the SSL stack that was only partially fixed in #29176:
if more than 17kb is written to the BIO buffer, then everything over
17kb will be discarded, and the SSL handshake will fail with a bad
record mac error or hang if not enough bytes have arrived yet.
It's relatively uncommon to hit this bug, because the TLS handshake
messages need to be much larger than normal for you to have a chance of
hitting this bug. However, there was a separate bug in the SSL stack
(recently fixed in #33558) that causes the ServerHello produced by a
gRPC-C++ TLS server to grow linearly in size with the size of the trust
bundle; these 2 bugs combined to cause a large number of TLS handshake
failures for gRPC-C++ clients talking to gRPC-C++ servers when the
server had a large trust bundle.
This PR fixes the bug by ensuring that all bytes are successfully
written to the BIO buffer. An initial quick fix for this bug was planned
in #33611, but abandoned because we were worried about temporarily
doubling the memory footprint of all SSL channels.
The complexity in this PR is mostly in the test: it is fairly tricky to
force gRPC-C++'s SSL stack to generate a sufficiently large ServerHello
to trigger this bug.
This PR does the following: for the TLS server credentials, stops
calling `SSL_CTX_set_client_CA_list` by default in
`ssl_transport_security.cc`, and gives users a knob to re-enable calling
this API.
## What does the `SSL_CTX_set_client_CA_list` API do?
When this API is called, a gRPC TLS server sends the following data in
the ServerHello: for each certificate in the server's trust bundle, the
CA name in the certificate.
This API does not change the set of certificates trusted by the server
in any way. Rather, it is just providing a hint to the client about what
client certificate should be sent to the server.
## Why are we removing the use of `SSL_CTX_set_client_CA_list` by
default for the TLS server credentials?
Removing the use of this API by default has 2 benefits:
1. Calling this API makes gRPC TLS unusable for servers with a
sufficiently large trust bundle. Indeed, if the server trust bundle is
too large, then the server will always fail to build the ServerHello.
2. Calling this API is introducing a huge amount of overhead (1000s of
bytes) to each ServerHello, so removing this feature will improve
connection establishment latency for all users of the TLS server
credentials.
I'm fixing the ALTS/Envoy transport socket extension (which is currently
broken). Along the way, I'm trying to remove as many uses of gRPC
internals as possible (with the eventual goal of only relying on public
gRPC APIs and the alts_zero_copy_grpc_protector). To this end, I need to
remove the ExecCtx check in the alts_zero_copy_grpc_protector_create
function, so that Envoy can call into this function without needing to
create an ExecCtx.
The logic is straightforward: attempt to read the
`GRPC_ALTS_MAX_CONCURRENT_HANDSHAKES` environment variable and, if it
set to an integer, instantiate the handshake queues based on this
integer.
Based on go/grpc-alts-concurrent-handshake-cap.
This PR is a small code change with a lot of new test data.
[In OpenSSL, there are two flags that configure CRL checks. Coping
relevant
section:](https://www.openssl.org/docs/man1.0.2/man3/X509_VERIFY_PARAM_get_depth.html)
> - X509_V_FLAG_CRL_CHECK enables CRL checking for the certificate chain
leaf certificate. An error occurs if a suitable CRL cannot be found.
> - X509_V_FLAG_CRL_CHECK_ALL enables CRL checking for the entire
certificate chain.
We currently only set `X509_V_FLAG_CRL_CHECK`, so we will only ever
check if the leaf certificate is revoked. We should check the whole
chain. I am open to making this a user configuration if we want to do it
that way, but we certainly need to be able to check the whole chain.
So, this PR contains the small code change in
`ssl_transport_security.cc` to use the `X509_V_FLAG_CRL_CHECK_ALL` flag.
Then the rest of the changes are in tests. I've added all the necessary
files to have a chain built that looks as follows
`Root CA -> Revoked Intermediate CA -> Leaf Certificate`, and added a
test for this case as well.
You can verify that on master this new test will fail (i.e. the
handshake will succeed even though the intermediate CA is revoked) by
checking out this branch, running `git checkout master --
./src/core/tsi/ssl_transport_security.cc`, then running the test.
I also slightly reorganized test/core/tsi/test_creds/ so that the CRLs
are in their own directory, which is the way our API intends to accept
CRLs.
This check only works if all handshake RPCs have an OK status, and it's
racey e.g. if the client is cancelling handshake RPCs (being when an RPC
is cancelled, termination of the RPC at the client is asynchronous from
termination at the server, so the client can resume the queue before the
server RPC completes).
* Add info about ca cert used to verify chain.
The tsi_peer object will now contain the subject of the root/ca cert
that was used to verify the peer's chain during a handshake.
* temp investigation
* Fix issues relating to overlapping CRL callback
* formatting on ssl_transport_security.cc
* Swap ca_cert naming
* Use preverify_ok instead of numbers
* Continue some renaming, addressing pr comments
* Removed early return if peer property setting fails
* Continue renaming
* clang-tidy
* Fix clang problem
* clang fixes
* Add null check in tests
* More PR changes. Behavior change to include root cert extract when TSI_REQUEST_CLIENT_CERTIFICATE_AND_VERIFY
* Add intermediate ca, leaf cert, and test with them
* clang-tidy
* Basic formatting
* Add new keys to build for export
* Add new cert files to test BUILD
* build file style fix
* changes for chain test
* clang-format
* build clean
* Add $ to lines of code in README
* Add directive about X509_STORE_CTX_get0_chain
* formatting
* weighted_target and RLS: delegate to child picker on error
* security handshaker: include security connector type in error message
* update test
* fix sanity
* fix crash
* TSI: return handshaker error message for inclusion in RPC failure status
* remove TODO
* remove unnecessary code
* fix test
* fix tsan bug
* document parameter
* Provide means to control TCP frame sizes in response to high memory pressure
* static_cast
* fix sanity checks
* removing endpoint_write frame clipping. this could be added in a separate PR
* fix sanity checks
* remove unused parameter
* addressing review comments
* rename functions
* add a TODO
* fixing naming issues
* add changes to test
* Save ENOBUFS errno correctly in tcp_posix for subsequent handling
* updaing min_progress_size computation
* Revert "Save ENOBUFS errno correctly in tcp_posix for subsequent handling"
This reverts commit 5e1d10ac9b.
* fine tuning min progress size estimation and updating unit tests to verify returned min progress size
* Refactor end2end tests to exercise each EventEngine
* fix incorrect bazel_only exclusions
* Automated change: Fix sanity tests
* microbenchmark fix
* sanitize, fix iOS flub
* Automated change: Fix sanity tests
* iOS fix
* reviewer feedback
* first pass at excluding EventEngine test expansion
Also caught a few cases where we should not test pollers, but should
test all engines. And two cases where we likely shouldn't be testing
either product.
* end2end fuzzers to be fuzzed differently via EventEngine.
* sanitize
* reviewer feedback
* remove misleading comment
* reviewer feedback: comments
* EE test_init needs to play with our build system
* fix golden file test runner
Co-authored-by: drfloob <drfloob@users.noreply.github.com>
Matthew Stevenson
AJ Heller
Gregory Cooke
apolcyn
Gregory Cooke
Yash Tibrewal
Peter Foley
Craig Tiller
Yijie Ma
Cheng-Yu Chung
Zhan Jiang
Esun Kim
Mark D. Roth
Vignesh Babu
yihuaz
krestofur
ZhenLian