CASSCPP-16 Fix TLS 1.3 handshake race with in-flight Finished write - #591
cpansuriya-simba wants to merge 3 commits into
Conversation
…assandra.h). Handled the new value in OpenSslContext::set_min_protocol_version() (ssl_openssl_impl.cpp), mapping it to TLS1_3_VERSION so cass_ssl_set_min_protocol_version() can enforce a TLS 1.3 minimum.
absurdfarce
left a comment
There was a problem hiding this comment.
Heart of the change (the handling of the 1 round-trip handshake in TLS 1.3) makes good sense to me. I've also confirmed that this change allows a test client to communicate with a Apache Cassandra 5.0.4 server configured to use TLS 1.3 exclusively. To validate backwards compat this change allows the same configuration to function with TLS 1.2 as well... so we didn't break anything there.
I do have a few questions/observations that we should at least give some time to:
- Some of the unit tests are currently failing with this change. Noted on the latest Jenkins run and it looks to be fairly consistent across all four test platforms. I'll have some details on these failures momentarily.
- Do we need explicit unit/integration tests for the TLS 1.2 vs. TLS 1.3 case? Answering this probably involves a deeper dive into the current test infrastructure than I've undertaken so far.
- I think we should be careful about saying this release support TLS 1.3 in it's entirety. There's a lot of things that come along with TLS 1.3 and I'm not sure we cover everything in the changes in this PR. I'm willing to listen if somebody thinks that's wrong... that's just the way it looked to me on first blush. To be clear: even saying we support the streamlined handshake introduced in TLS 1.3 is (a) technically correct while also being (b) a significant achievement.
| SocketWriteBase::SocketWriteBase(Socket* socket) | ||
| : socket_(socket) | ||
| , is_flushed_(false) | ||
| , handler_generation_(socket->handler_generation()) { |
There was a problem hiding this comment.
This was the only part I felt like I didn't quite understand. Can you speak more to what the goal of this notion of "handler generations" is @cpansuriya-simba? Maybe I'm missing something obvious (in fact I probably am) but I wasn't immediately clear on why you'd need a mechanism like this.
There was a problem hiding this comment.
Given full explanation below. Want to suppress it but full explanation is batter so I keep it as it is.
handler_generation_() captures a "snapshot" of which handler was active on the Socket at the moment this write object was created, so it can be compared later before recycling it. Here's why it became necessary for TLS 1.3:
The reuse mechanism that existed before: Socket keeps a pool of completed SocketWriteBase objects (free_writes_) to avoid re-allocating on every write. When a write finishes (handle_write()), if under the pool limit, the object is cleared and pushed back into free_writes_ for the next write request — regardless of what that next request is for.
Why that's dangerous now: During the SSL handshake, Socket's handler is a plain SocketHandler, so handshake bytes are written using a plain SocketWrite object. Once the handshake finishes, set_handler() swaps the socket over to the real SslSocketHandler for encrypted application traffic.
With TLS 1.2's handshake, "handshake done" and "the last handshake write completing" were separated enough in timing that this ordering issue didn't surface. With TLS 1.3's 1-RTT handshake, is_handshake_done() can become true in the very same step that produces the client's Finished message — so we now deliberately defer calling finish() (which swaps the handler) until after that final handshake write's on_write callback actually fires (see the earlier ssl_handshake_finish() change). That means the handler swap and the completion of the handshake's own write object happen close together, right around when that write object would normally be recycled into free_writes_.
The bug this prevents: Without tracking which handler-generation a write object belongs to, that now-completed plain handshake write object could get recycled into free_writes_ right as (or after) the handler switches to SslSocketHandler. The next application write would then pop that stale plain SocketWrite object from the pool instead of creating a proper encrypted SslSocketWrite, causing application data to be written unencrypted, in plaintext straight over the socket.
What the line actually does: handler_generation_(socket->handler_generation()) records the handler's generation counter (bumped once per set_handler() call) at construction time. Later, in handle_write(), the object is only put back in the free pool if handler_generation_ == socket->handler_generation_ — i.e., the handler hasn't changed since this write object was created. If it has changed, the object is simply deleted instead of reused, forcing a fresh, correctly-typed write object (SslSocketWrite) to be created for the next request.
|
An example of some of the test failures I've seen (pulled from the Jenkins runs): There are a few others but these two stood out right away. These tests aren't failing on a base run of current trunk. I don't know that we necessarily have to fix every test that breaks here but I'd like to at least understand what is breaking and what (if anything) we need to change to make Jenkins happy. |
|
Ping @yifan-c for visibility as to what's going on here |
|
Because of below reason, we show 2 test failures |
Problem
The driver pinned OpenSSL to TLS 1.2 because the handshake state machine assumed a 2-RTT flow where writes and "handshake done" are always sequential. TLS 1.3's 1-RTT handshake breaks that assumption: SSL_connect() can report completion in the same call that produces the outgoing Finished message, so "done" and "still have data to write" happen simultaneously rather than one after another.
Root cause
The old code only checked is_handshake_done() when there was nothing left to write (size == 0). Under TLS 1.3, that check could pass while the Finished message write was still in flight — causing the driver to verify the peer cert and hand the socket off to the post-handshake handler/write-recycling pool before the write actually completed, racing with (and risking corruption of) that write.
Fix
Net effect
TLS 1.3 connections now complete the handshake safely, with no race on the final handshake write and no stale pre-handoff write object leaking into the encrypted session.