CASSCPP-16 Revisit support for TLS 1.3 patch by Chintan Pansuriya; reviewed by Bret McGuire and Yifan Cai reference: https://github.com/apache/cassandra-cpp-driver/pull/591
diff --git a/include/cassandra.h b/include/cassandra.h index c51bfa2..dc169a1 100644 --- a/include/cassandra.h +++ b/include/cassandra.h
@@ -634,7 +634,8 @@ typedef enum CassSslTlsVersion_ { CASS_SSL_VERSION_TLS1 = 0x00, CASS_SSL_VERSION_TLS1_1 = 0x01, - CASS_SSL_VERSION_TLS1_2 = 0x02 + CASS_SSL_VERSION_TLS1_2 = 0x02, + CASS_SSL_VERSION_TLS1_3 = 0x03 } CassSslTlsVersion; typedef enum CassProtocolVersion_ {
diff --git a/src/socket.cpp b/src/socket.cpp index 76b1765..ca1a4ae 100644 --- a/src/socket.cpp +++ b/src/socket.cpp
@@ -225,6 +225,14 @@ uv_tcp_t* SocketWriteBase::tcp() { return &socket_->tcp_; } +SocketWriteBase::SocketWriteBase(Socket* socket) + : socket_(socket) + , is_flushed_(false) + , handler_generation_(socket->handler_generation()) { + req_.data = this; + buffers_.reserve(MIN_BUFFERS_SIZE); +} + void SocketWriteBase::on_close() { for (RequestVec::iterator i = requests_.begin(), end = requests_.end(); i != end; ++i) { (*i)->on_close(); @@ -267,7 +275,9 @@ socket->pending_writes_.remove(this); - if (socket->free_writes_.size() < socket->max_reusable_write_objects_) { + // Don't recycle a write created under a handler that's since been replaced. + if (handler_generation_ == socket->handler_generation_ && + socket->free_writes_.size() < socket->max_reusable_write_objects_) { clear(); socket->free_writes_.push_back(this); } else { @@ -278,7 +288,8 @@ } Socket::Socket(const Address& address, size_t max_reusable_write_objects) - : is_defunct_(false) + : handler_generation_(0) + , is_defunct_(false) , max_reusable_write_objects_(max_reusable_write_objects) , address_(address) { tcp_.data = this; @@ -288,6 +299,7 @@ void Socket::set_handler(SocketHandlerBase* handler) { handler_.reset(handler); + ++handler_generation_; cleanup_free_writes(); free_writes_.clear(); if (handler_) {
diff --git a/src/socket.hpp b/src/socket.hpp index b54e56c..c3f70cb 100644 --- a/src/socket.hpp +++ b/src/socket.hpp
@@ -206,12 +206,7 @@ * * @param The socket handling the write. */ - SocketWriteBase(Socket* socket) - : socket_(socket) - , is_flushed_(false) { - req_.data = this; - buffers_.reserve(MIN_BUFFERS_SIZE); - } + SocketWriteBase(Socket* socket); virtual ~SocketWriteBase() {} @@ -260,6 +255,8 @@ Socket* socket_; uv_write_t req_; bool is_flushed_; + // Socket's handler generation when this write was created, to avoid reuse under a new handler. + size_t handler_generation_; BufferVec buffers_; RequestVec requests_; }; @@ -299,6 +296,11 @@ void set_handler(SocketHandlerBase* handler); /** + * The number of times the socket's handler has been set. + */ + size_t handler_generation() const { return handler_generation_; } + + /** * Write a request to the socket and coalesce with outstanding requests. This * method doesn't flush. * @@ -367,6 +369,7 @@ uv_tcp_t tcp_; ScopedPtr<SocketHandlerBase> handler_; + size_t handler_generation_; SocketWriteBase::List pending_writes_; SocketWriteVec free_writes_;
diff --git a/src/socket_connector.cpp b/src/socket_connector.cpp index 3d396a4..d5fce7b 100644 --- a/src/socket_connector.cpp +++ b/src/socket_connector.cpp
@@ -65,6 +65,11 @@ delete request; if (status != 0) { connector_->on_error(SocketConnector::SOCKET_ERROR_WRITE, "Write error"); + return; + } + // TLS 1.3 can finish the handshake while this write was still queued. + if (connector_->ssl_session_->is_handshake_done()) { + connector_->ssl_handshake_finish(); } } @@ -106,7 +111,8 @@ : address_(address) , callback_(callback) , error_code_(SOCKET_OK) - , ssl_error_code_(CASS_OK) {} + , ssl_error_code_(CASS_OK) + , is_handshake_finished_(false) {} SocketConnector* SocketConnector::with_settings(const SocketSettings& settings) { settings_ = settings; @@ -207,23 +213,33 @@ } } - // Write any outgoing data created by the handshake process. + // Write any outgoing data created by the handshake process. Finishing is + // deferred to on_write() if the handshake is already done (e.g. TLS 1.3). char buf[SSL_HANDSHAKE_MAX_BUFFER_SIZE]; size_t size = ssl_session_->outgoing().read(buf, SSL_HANDSHAKE_MAX_BUFFER_SIZE); if (size > 0) { socket_->write_and_flush(new BufferSocketRequest(Buffer(buf, size))); - } else if (ssl_session_->is_handshake_done()) { // If the handshake process is done then verify - // the certificate and finish. - ssl_session_->verify(); - if (ssl_session_->has_error()) { - on_error(SOCKET_ERROR_SSL_VERIFY, - "Error verifying peer certificate: " + ssl_session_->error_message()); - return; - } - finish(); + } else if (ssl_session_->is_handshake_done()) { + ssl_handshake_finish(); } } +void SocketConnector::ssl_handshake_finish() { + // Handshake completion can be observed from both the deferred on_write() + // and a subsequent on_read() with TLS 1.3; only run this once. + if (is_handshake_finished_) return; + is_handshake_finished_ = true; + + // If the handshake process is done then verify the certificate and finish. + ssl_session_->verify(); + if (ssl_session_->has_error()) { + on_error(SOCKET_ERROR_SSL_VERIFY, + "Error verifying peer certificate: " + ssl_session_->error_message()); + return; + } + finish(); +} + void SocketConnector::finish() { if (socket_) socket_->set_handler(NULL); callback_(this);
diff --git a/src/socket_connector.hpp b/src/socket_connector.hpp index c7a1d33..da48007 100644 --- a/src/socket_connector.hpp +++ b/src/socket_connector.hpp
@@ -138,6 +138,7 @@ private: void internal_connect(uv_loop_t* loop); void ssl_handshake(); + void ssl_handshake_finish(); void finish(); void on_error(SocketError code, const String& message); @@ -167,6 +168,12 @@ ScopedPtr<SslSession> ssl_session_; + // Guards against ssl_handshake_finish() running more than once. With TLS 1.3 + // the handshake can be reported done both from a deferred on_write() (after + // the final client flight is flushed) and from a subsequent on_read() (e.g. + // post-handshake data arriving before that write completes). + bool is_handshake_finished_; + SocketSettings settings_; };
diff --git a/src/ssl/ssl_openssl_impl.cpp b/src/ssl/ssl_openssl_impl.cpp index 596fe90..badf3c8 100644 --- a/src/ssl/ssl_openssl_impl.cpp +++ b/src/ssl/ssl_openssl_impl.cpp
@@ -540,10 +540,6 @@ , trusted_store_(X509_STORE_new()) { SSL_CTX_set_cert_store(ssl_ctx_, trusted_store_); SSL_CTX_set_verify(ssl_ctx_, SSL_VERIFY_NONE, ssl_no_verify_callback); -#if (OPENSSL_VERSION_NUMBER >= 0x10100000L) - // Limit to TLS 1.2 for now. TLS 1.3 has broken the handshake code. - SSL_CTX_set_max_proto_version(ssl_ctx_, TLS1_2_VERSION); -#endif #if DEBUG_SSL SSL_CTX_set_info_callback(ssl_ctx_, ssl_info_callback); #endif @@ -632,6 +628,15 @@ case CassSslTlsVersion::CASS_SSL_VERSION_TLS1_2: method = TLS1_2_VERSION; break; +/* TLS 1.3 support came along with OpenSSL 1.1.1 so only add it if it's actually present. + + Note that this means the library can be compiled _without_ TLS 1.3 support if an older + OpenSSL lib is used. */ +#ifdef TLS1_3_VERSION + case CassSslTlsVersion::CASS_SSL_VERSION_TLS1_3: + method = TLS1_3_VERSION; + break; +#endif default: // unsupported version return CASS_ERROR_LIB_BAD_PARAMS;
diff --git a/tests/src/unit/http_server.cpp b/tests/src/unit/http_server.cpp index dcd6d60..4fa152b 100644 --- a/tests/src/unit/http_server.cpp +++ b/tests/src/unit/http_server.cpp
@@ -60,8 +60,8 @@ } bool Server::use_ssl(const String& key, const String& cert, const String& ca_cert /*= ""*/, - bool require_client_cert /*= false*/) { - return server_connection_->use_ssl(key, cert, ca_cert, require_client_cert); + bool require_client_cert /*= false*/, int max_tls_version /* = 0 */) { + return server_connection_->use_ssl(key, cert, ca_cert, require_client_cert, max_tls_version); } Server::ClientConnection::ClientConnection(internal::ServerConnection* server_connection,
diff --git a/tests/src/unit/http_server.hpp b/tests/src/unit/http_server.hpp index 48b7d6c..3e5f252 100644 --- a/tests/src/unit/http_server.hpp +++ b/tests/src/unit/http_server.hpp
@@ -65,7 +65,7 @@ } bool use_ssl(const String& key, const String& cert, const String& ca_cert = "", - bool require_client_cert = false); + bool require_client_cert = false, int max_tls_version = 0); void listen(); void close();
diff --git a/tests/src/unit/http_test.cpp b/tests/src/unit/http_test.cpp index f66db06..82d48b7 100644 --- a/tests/src/unit/http_test.cpp +++ b/tests/src/unit/http_test.cpp
@@ -52,12 +52,12 @@ return settings; } -void HttpTest::use_ssl(const String& ca_key, const String& ca_cert, const String& cn) { +void HttpTest::use_ssl(const String& ca_key, const String& ca_cert, const String& cn, int max_tls_version /* = 0 */) { #ifdef HAVE_OPENSSL key_ = mockssandra::Ssl::generate_key(); cert_ = mockssandra::Ssl::generate_cert(key_, cn, ca_cert, ca_key); ca_cert_ = ca_cert; - server_.use_ssl(key_, cert_, ca_cert_, true); + server_.use_ssl(key_, cert_, ca_cert_, true, max_tls_version); #endif }
diff --git a/tests/src/unit/http_test.hpp b/tests/src/unit/http_test.hpp index deee07a..f1cd709 100644 --- a/tests/src/unit/http_test.hpp +++ b/tests/src/unit/http_test.hpp
@@ -55,7 +55,7 @@ datastax::internal::core::SocketSettings use_ssl(const String& cn = HTTP_MOCK_HOSTNAME, bool is_server_using_ssl = true); - void use_ssl(const String& ca_cert, const String& ca_key, const String& cn); + void use_ssl(const String& ca_cert, const String& ca_key, const String& cn, int max_tls_version = 0); private: datastax::String ca_cert_;
diff --git a/tests/src/unit/mockssandra.cpp b/tests/src/unit/mockssandra.cpp index f30b22e..c99cf64 100644 --- a/tests/src/unit/mockssandra.cpp +++ b/tests/src/unit/mockssandra.cpp
@@ -444,23 +444,25 @@ } char buf[SSL_BUF_SIZE]; - bool data_written = false; int num_bytes; while ((num_bytes = BIO_read(outgoing_bio_, buf, sizeof(buf))) > 0) { - data_written = true; internal_write(buf, num_bytes); } - if (is_handshake_done() && data_written) { - return; // Handshake is not completed; ingore remaining data + if (!is_handshake_done()) { + return; // Handshake still isn't complete; wait for more data. } - } else { - char buf[SSL_BUF_SIZE]; - while ((rc = SSL_read(ssl_, buf, sizeof(buf))) > 0) { - on_read(buf, rc); - } - has_ssl_error(rc); + // Handshake just completed. With TLS 1.3 the client's first application + // data (e.g. OPTIONS) can arrive in the same read as the final handshake + // record, so fall through and drain any decrypted data below instead of + // dropping it. } + + char buf[SSL_BUF_SIZE]; + while ((rc = SSL_read(ssl_, buf, sizeof(buf))) > 0) { + on_read(buf, rc); + } + has_ssl_error(rc); } ServerConnection::ServerConnection(const Address& address, const ClientConnectionFactory& factory) @@ -491,7 +493,8 @@ bool ServerConnection::use_ssl(const String& key, const String& cert, const String& ca_cert /*= ""*/, - bool require_client_cert /*= false*/) { + bool require_client_cert /*= false*/, + int max_tls_version /* = 0 */) { if (ssl_context_) { SSL_CTX_free(ssl_context_); } @@ -505,6 +508,16 @@ SSL_CTX_set_default_passwd_cb(ssl_context_, on_password); SSL_CTX_set_verify(ssl_context_, SSL_VERIFY_NONE, NULL); + /* CASSCPP-16 + + Default max for TLS negotiation with clients will be whatever's supported by + the underlying OpenSSL impl (in order to maximally test the newest version) + but some tests may include behaviours that rely on earlier TLS versions. + */ + if (max_tls_version) { + SSL_CTX_set_max_proto_version(ssl_context_, max_tls_version); + } + { // Load server certificate Scoped<X509> x509(load_cert(cert)); if (!x509) return false;
diff --git a/tests/src/unit/mockssandra.hpp b/tests/src/unit/mockssandra.hpp index 26050fd..e5be182 100644 --- a/tests/src/unit/mockssandra.hpp +++ b/tests/src/unit/mockssandra.hpp
@@ -176,7 +176,7 @@ const ClientConnections& clients() const { return clients_; } bool use_ssl(const String& key, const String& cert, const String& ca_cert = "", - bool require_client_cert = false); + bool require_client_cert = false, int max_tls_version = 0); void weaken_ssl(); void listen(EventLoopGroup* event_loop_group);
diff --git a/tests/src/unit/tests/test_http_client.cpp b/tests/src/unit/tests/test_http_client.cpp index bba6ddb..350f0c6 100644 --- a/tests/src/unit/tests/test_http_client.cpp +++ b/tests/src/unit/tests/test_http_client.cpp
@@ -210,7 +210,13 @@ String ca_key = mockssandra::Ssl::generate_key(); String ca_cert = mockssandra::Ssl::generate_cert(ca_key, "CA"); - use_ssl(ca_key, ca_cert, HTTP_MOCK_HOSTNAME); + /* CASSCPP-16 + + Disable TLS 1.3 here so that we're only testing against TLS 1.2. Since client cert compares + don't happen until the first bit of client data is read this test would need to be restructured + to work with TLS 1.3. We can pursue this in later work but for now we just constrain this + test at the earlier TLS version. */ + use_ssl(ca_key, ca_cert, HTTP_MOCK_HOSTNAME, TLS1_2_VERSION); start_http_server(); bool is_failed = false; @@ -240,7 +246,13 @@ String client_key = mockssandra::Ssl::generate_key(); String client_cert = mockssandra::Ssl::generate_cert(client_key, ""); // Self-signed - use_ssl(ca_key, ca_cert, HTTP_MOCK_HOSTNAME); + /* CASSCPP-16 + + Disable TLS 1.3 here so that we're only testing against TLS 1.2. Since client cert compares + don't happen until the first bit of client data is read this test would need to be restructured + to work with TLS 1.3. We can pursue this in later work but for now we just constrain this + test at the earlier TLS version. */ + use_ssl(ca_key, ca_cert, HTTP_MOCK_HOSTNAME, TLS1_2_VERSION); start_http_server(); bool is_failed = false;
diff --git a/topics/building/README.md b/topics/building/README.md index 17a9998..5e01c03 100644 --- a/topics/building/README.md +++ b/topics/building/README.md
@@ -48,6 +48,11 @@ 3.x to be relatively painless. Note that two officially supported platforms (Ubuntu 22.04 and Rocky Linux 9.2) come with OpenSSL 3.x by default and the unit and integration tests all pass on these platforms. +### A Brief Note on TLS 1.3 +TLS 1.3 support was introduced to OpenSSL with the [1.1.1 release]. As of version 2.17.2 the driver includes support for +at least basic connectivity with TLS 1.3. Note that TLS 1.3 support will only be available if a version of OpenSSL >= 1.1.1 +is utilized at build-time. See [PR 591] for more on this point. + ## Linux/Mac OS The driver is known to build on Rocky Linux 8/9, Mac OS X 10.10/10.11 (Yosemite @@ -342,3 +347,5 @@ [OpenSSL]: https://www.openssl.org [zlib]: https://www.zlib.net [migration guide]: https://www.openssl.org/docs/man3.0/man7/migration_guide.html +[1.1.1 release]: https://github.com/openssl/openssl/wiki/TLS1.3 +[PR 591]: https://github.com/apache/cassandra-cpp-driver/pull/591 \ No newline at end of file