Support CLIENT_HELLO split across multiple packets#12290
Support CLIENT_HELLO split across multiple packets#12290bneradt merged 2 commits intoapache:masterfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR updates TLS handshake processing to support CLIENT_HELLO messages arriving in multiple packets. Key changes include new test cases to simulate split and large CLIENT_HELLO packets, adjustments to proxy protocol checking, and enhanced debug logging for handshake buffering.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/gold_tests/tls/tls_tunnel.test.py | Added tests for split and large CLIENT_HELLO along with TimeOut updates. |
| tests/gold_tests/tls/split_client_hello.py | Introduced a script to send CLIENT_HELLO in multiple packets. |
| tests/gold_tests/tls/receive_split_client_hello.py | Added a script to receive CLIENT_HELLO and validate its length. |
| tests/gold_tests/tls/gold/tls-tunnel-metrics.gold | Updated expected metric numbers. |
| src/iocore/net/SSLNetVConnection.cc | Updated proxy protocol logic and added detailed debug messaging. |
| src/iocore/net/P_SSLNetVConnection.h | Added documentation and new members for handshake buffer management. |
Comments suppressed due to low confidence (1)
src/iocore/net/SSLNetVConnection.cc:584
- [nitpick] The conditional logic for updating the read BIO based on the CLIENT_HELLO state would benefit from an inline comment explaining why the buffers are retained while in the CLIENT_HELLO phase.
read.triggered = update_rbio(!in_client_hello);
51256ea to
d41cbaa
Compare
dbb78c8 to
2537ce6
Compare
Our TLS CLIENT_HELLO processing logic assumed all CLIENT_HELLO bytes came in a single TCP packet. However, with more recent cryptographic ciphers, the CLIENT_HELLO is often greater than the standard 1,500 byte MTU, so the CLIENT_HELLO is being delivered in multiple packets. This updates our logic to properly buffer and parse data across multiple socket reads. Fixes: apache#11758
2537ce6 to
cd9665c
Compare
| // finished. We need to keep our buffers updated until then in case we | ||
| // enter tunnel mode. | ||
| Dbg(dbg_ctl_ssl, "Updating our buffers, in CLIENT_HELLO: %s", in_client_hello ? "true" : "false"); | ||
| read.triggered = update_rbio(!in_client_hello); |
There was a problem hiding this comment.
Only transition to socket mode from our manually fed buffer if we're not still processing the CLIENT_HELLO. While we process the CLIENT_HELLO, we need to keep buffering it in case we want to replay it in the blind tunnel case.
| bool const in_client_hello = | ||
| this->get_handshake_hook_state() == TLSEventSupport::SSLHandshakeHookState::HANDSHAKE_HOOKS_CLIENT_HELLO; | ||
| // We only feed CLIENT_HELLO bytes into our temporary buffers. If we are past | ||
| // the CLIENT_HELLO, then no need to buffer. | ||
| if (in_client_hello && this->handShakeReader) { | ||
| if (BIO_eof(SSL_get_rbio(this->ssl))) { // No more data in the buffer | ||
| // Is this the first read? | ||
| if (!this->handShakeReader->is_read_avail_more_than(0) && !this->handShakeHolder->is_read_avail_more_than(0)) { | ||
| // Is this the first read? | ||
| #if TS_USE_TLS_ASYNC |
There was a problem hiding this comment.
If the CLIENT_HELLO is split on multiple packets, then we need to call this->read_raw_data for each packet (see 1246). Before this change, we assumed we would only ever need to call it once, thus we only called it if handShakeReader/Holder was empty. This change makes us do the raw read for each packet that comes in, so we can populate our buffered CLIENT_HELLO and feed it to the SSL socket.
| } | ||
| } else { | ||
| update_rbio(false); | ||
| } else if (retval == 0) { | ||
| // EOF, go away, we stopped in the handshake | ||
| SSLVCDebug(this, "SSL handshake error: EOF"); | ||
| return EVENT_ERROR; | ||
| } |
There was a problem hiding this comment.
the read_faw_data logic above does what update_rbio would have been doing (read from the socket and populate our buffers), so no need to call it here.
|
I guess no hope in getting this fix backported to older major versions? |
The cherry-pick for 9.x is non-trivial, but I can look into it. |
Support the situation in which handShakeReader is a chain of more than one IOBufferBlock.
fe5bf2c to
d9360f9
Compare
I made a 9.2.x backport: |
|
Cherry-picked to 10.1.x branch |
Our TLS CLIENT_HELLO processing logic assumed all CLIENT_HELLO bytes came in a single TCP packet. However, with more recent cryptographic ciphers, the CLIENT_HELLO is often greater than the standard 1,500 byte MTU, so the CLIENT_HELLO is being delivered in multiple packets. This updates our logic to properly buffer and parse data across multiple socket reads. Fixes: #11758 (cherry picked from commit 572ec5d)
Updating chart: The issue with Apache Traffic Server has been fixed. apache/trafficserver#12290
Updating chart: The issue with Apache Traffic Server has been fixed. apache/trafficserver#12290
This cherry picks a review comment update from: apache#12290
Our TLS CLIENT_HELLO processing logic assumed all CLIENT_HELLO bytes came in a single TCP packet. However, with more recent cryptographic ciphers, the CLIENT_HELLO is often greater than the standard 1,500 byte MTU, so the CLIENT_HELLO is being delivered in multiple packets. This updates our logic to properly buffer and parse data across multiple socket reads.
Fixes: #11758