diff --git a/plugins/mysqlx/include/mysqlx_session.h b/plugins/mysqlx/include/mysqlx_session.h index 720b9c218..c8b23b0a3 100644 --- a/plugins/mysqlx/include/mysqlx_session.h +++ b/plugins/mysqlx/include/mysqlx_session.h @@ -78,7 +78,15 @@ public: int get_fd() const { return client_ds_.get_fd(); } MysqlxDataStream& client_ds() { return client_ds_; } - MysqlxDataStream& server_ds() { return server_ds_; } + // Session-level accessor for the backend data stream. When a backend + // connection is attached, this proxies to MysqlxConnection::backend_ds() + // so the SSL* established during the optional backend TLS handshake is + // preserved for the rest of the session. Falls back to an uninitialized + // placeholder (fd == -1) when no backend is attached so that pollers and + // tests can safely query get_fd()/get_status() without crashing. + MysqlxDataStream& server_ds() { + return backend_conn_ ? backend_conn_->backend_ds() : server_ds_placeholder_; + } MysqlxConnection*& backend_conn() { return backend_conn_; } void set_credential_lookup(MysqlxCredentialLookup lookup) { credential_lookup_ = lookup; } @@ -123,7 +131,13 @@ private: bool is_terminal_for_state(uint8_t msg_type) const; MysqlxDataStream client_ds_; - MysqlxDataStream server_ds_; + // Placeholder stream returned by server_ds() when no backend connection + // is attached. Intentionally never init()'d during the data-plane phase: + // the real backend stream lives on MysqlxConnection::backend_ds_, which + // owns the SSL* from the optional backend TLS handshake. Rewrapping the + // raw fd here after auth would discard that SSL* and silently regress + // TLS-wrapped sessions to cleartext I/O. + MysqlxDataStream server_ds_placeholder_; MysqlxConnection* backend_conn_; Mysqlx_Thread* thread_ptr_; Status status_; diff --git a/plugins/mysqlx/src/mysqlx_session.cpp b/plugins/mysqlx/src/mysqlx_session.cpp index 52d02cd49..f999a3f1b 100644 --- a/plugins/mysqlx/src/mysqlx_session.cpp +++ b/plugins/mysqlx/src/mysqlx_session.cpp @@ -450,26 +450,29 @@ void MysqlxSession::handler_waiting_client_msg() { } void MysqlxSession::forward_to_backend() { - if (server_ds_.get_status() != XDS_READY) { - if (!backend_conn_ || backend_conn_->get_state() != MysqlxConnection::IDLE) { - status_ = CONNECTING_SERVER; - to_process = true; - return; - } - server_ds_.init(XDS_BACKEND, backend_conn_->get_fd()); + if (!backend_conn_ || backend_conn_->get_state() != MysqlxConnection::IDLE) { + status_ = CONNECTING_SERVER; + to_process = true; + return; } + // Data-plane reads/writes must go through MysqlxConnection::backend_ds(), + // which owns the SSL* established during the optional backend TLS + // handshake. Wrapping backend_conn_->get_fd() in a fresh MysqlxDataStream + // here would discard that SSL* and silently fall back to cleartext I/O. + MysqlxDataStream& be_ds = backend_conn_->backend_ds(); + if (client_ds_.has_complete_frame()) { const auto& frame = client_ds_.front_frame(); if (frame.size() > 5) { - server_ds_.enqueue_frame(frame[4], frame.data() + 5, frame.size() - 5); + be_ds.enqueue_frame(frame[4], frame.data() + 5, frame.size() - 5); } else { - server_ds_.enqueue_frame(frame[4], nullptr, 0); + be_ds.enqueue_frame(frame[4], nullptr, 0); } client_ds_.pop_frame(); } - server_ds_.write_to_net(); + be_ds.write_to_net(); status_ = WAITING_SERVER_XMSG; } @@ -518,14 +521,16 @@ bool MysqlxSession::is_terminal_for_state(uint8_t msg_type) const { } void MysqlxSession::handler_waiting_server_msg() { - if (server_ds_.get_fd() < 0) { + // Go through backend_conn_->backend_ds() so TLS state is preserved. + if (!backend_conn_ || backend_conn_->backend_ds().get_fd() < 0) { return_backend_to_pool(); status_ = WAITING_CLIENT_XMSG; to_process = true; return; } + MysqlxDataStream& be_ds = backend_conn_->backend_ds(); - ssize_t r = server_ds_.read_from_net(); + ssize_t r = be_ds.read_from_net(); if (r == 0) { send_error(2013, "Lost connection to backend during query"); return_backend_to_pool(); @@ -540,12 +545,12 @@ void MysqlxSession::handler_waiting_server_msg() { } bool got_terminal = false; - while (server_ds_.has_complete_frame()) { - const auto& frame = server_ds_.front_frame(); + while (be_ds.has_complete_frame()) { + const auto& frame = be_ds.front_frame(); uint8_t msg_type = frame[4]; forward_frame_to_client(msg_type, frame); - server_ds_.pop_frame(); + be_ds.pop_frame(); if (msg_type != Mysqlx::ServerMessages_Type_NOTICE && is_terminal_for_state(msg_type)) { @@ -567,14 +572,16 @@ void MysqlxSession::handler_fast_forward() { } void MysqlxSession::handler_session_reset_waiting() { - if (server_ds_.get_fd() < 0) { + // Go through backend_conn_->backend_ds() so TLS state is preserved. + if (!backend_conn_ || backend_conn_->backend_ds().get_fd() < 0) { return_backend_to_pool(); status_ = WAITING_CLIENT_XMSG; to_process = true; return; } + MysqlxDataStream& be_ds = backend_conn_->backend_ds(); - ssize_t r = server_ds_.read_from_net(); + ssize_t r = be_ds.read_from_net(); if (r == 0 || (r < 0 && errno != EAGAIN && errno != EWOULDBLOCK)) { return_backend_to_pool(); status_ = WAITING_CLIENT_XMSG; @@ -582,22 +589,20 @@ void MysqlxSession::handler_session_reset_waiting() { return; } - while (server_ds_.has_complete_frame()) { - const auto& frame = server_ds_.front_frame(); + while (be_ds.has_complete_frame()) { + const auto& frame = be_ds.front_frame(); uint8_t msg_type = frame[4]; if (msg_type == Mysqlx::ServerMessages_Type_NOTICE) { forward_frame_to_client(msg_type, frame); - server_ds_.pop_frame(); + be_ds.pop_frame(); continue; } if (msg_type == Mysqlx::ServerMessages_Type_OK) { - server_ds_.pop_frame(); - if (backend_conn_) { - backend_conn_->set_has_prepared_statement(false); - backend_conn_->set_in_transaction(false); - } + be_ds.pop_frame(); + backend_conn_->set_has_prepared_statement(false); + backend_conn_->set_in_transaction(false); return_backend_to_pool(); last_active_time_ = monotonic_time_ms(); status_ = WAITING_CLIENT_XMSG; @@ -607,7 +612,7 @@ void MysqlxSession::handler_session_reset_waiting() { if (msg_type == Mysqlx::ServerMessages_Type_ERROR) { forward_frame_to_client(msg_type, frame); - server_ds_.pop_frame(); + be_ds.pop_frame(); client_ds_.write_to_net(); return_backend_to_pool(); status_ = WAITING_CLIENT_XMSG; @@ -615,7 +620,7 @@ void MysqlxSession::handler_session_reset_waiting() { return; } - server_ds_.pop_frame(); + be_ds.pop_frame(); } } @@ -668,7 +673,10 @@ void MysqlxSession::handler_connecting_server() { } if (backend_conn_) { - server_ds_.init(XDS_BACKEND, backend_conn_->get_fd()); + // Cached connection already has its MysqlxDataStream + // (backend_conn_->backend_ds()) initialized, including any + // SSL* from the original handshake. Do NOT rewrap the raw fd + // here: that would bypass the TLS-aware stream entirely. status_ = WAITING_CLIENT_XMSG; to_process = true; return; @@ -740,7 +748,14 @@ void MysqlxSession::handler_connecting_server() { } } - server_ds_.init(XDS_BACKEND, backend_conn_->get_fd()); + // Intentionally do NOT reinitialize a session-owned MysqlxDataStream + // around backend_conn_->get_fd() here. The backend auth path, including + // the optional TLS handshake, runs against backend_conn_->backend_ds(), + // which holds the SSL* for the session's entire lifetime. Wrapping the + // raw fd in a fresh plain stream would discard that SSL* and cause + // cleartext I/O on a TLS-wrapped socket. All data-plane code paths + // below (forward_to_backend, handler_waiting_server_msg, etc.) must go + // through backend_conn_->backend_ds() instead. backend_conn_->set_state(MysqlxConnection::IDLE); backend_conn_->set_reusable(true); status_ = WAITING_CLIENT_XMSG; @@ -750,12 +765,19 @@ void MysqlxSession::handler_connecting_server() { void MysqlxSession::return_backend_to_pool() { if (!backend_conn_) return; if (thread_ptr_) { + // The cached connection retains its MysqlxDataStream (including + // the SSL* established during the optional backend TLS handshake), + // so a subsequent session can resume using it without tearing the + // TLS session down. MysqlxConnection::reset() intentionally leaves + // backend_ds_ untouched. Do NOT reset/destroy the SSL state here. thread_ptr_->return_connection_to_cache(backend_conn_); } else { delete backend_conn_; } backend_conn_ = nullptr; - server_ds_ = MysqlxDataStream(); + // The session-owned placeholder carries no data-plane state and does + // not need to be reset. server_ds() will again return the placeholder + // (with fd == -1) until another backend is attached. } void MysqlxSession::send_error(int code, const char* msg, bool fatal) { diff --git a/test/tap/tests/unit/Makefile b/test/tap/tests/unit/Makefile index 2b0dab331..51c96e3b2 100644 --- a/test/tap/tests/unit/Makefile +++ b/test/tap/tests/unit/Makefile @@ -524,8 +524,8 @@ mysqlx_credential_verify_unit-t: mysqlx_credential_verify_unit-t.cpp $(PROXYSQL_ $(IDIRS) $(LDIRS) $(OPT) $(LIBPROXYSQLAR_FULL) $(STATIC_LIBS) \ $(MYLIBS) -lprotobuf -lssl -lcrypto $(ALLOW_MULTI_DEF) -o $@ -mysqlx_robustness_unit-t: mysqlx_robustness_unit-t.cpp $(PROXYSQL_PATH)/plugins/mysqlx/src/mysqlx_session.cpp $(PROXYSQL_PATH)/plugins/mysqlx/src/mysqlx_data_stream.cpp $(PROXYSQL_PATH)/plugins/mysqlx/src/mysqlx_connection.cpp $(PROXYSQL_PATH)/plugins/mysqlx/src/mysqlx_thread.cpp $(PROXYSQL_PATH)/plugins/mysqlx/src/mysqlx_protocol.cpp $(MYSQLX_PROTO_OBJS) $(TEST_HELPERS_OBJ) $(LIBPROXYSQLAR) - $(CXX) $< $(PROXYSQL_PATH)/plugins/mysqlx/src/mysqlx_session.cpp $(PROXYSQL_PATH)/plugins/mysqlx/src/mysqlx_data_stream.cpp $(PROXYSQL_PATH)/plugins/mysqlx/src/mysqlx_connection.cpp $(PROXYSQL_PATH)/plugins/mysqlx/src/mysqlx_thread.cpp $(PROXYSQL_PATH)/plugins/mysqlx/src/mysqlx_protocol.cpp $(MYSQLX_PROTO_OBJS) $(TEST_HELPERS_OBJ) \ +mysqlx_robustness_unit-t: mysqlx_robustness_unit-t.cpp $(PROXYSQL_PATH)/plugins/mysqlx/src/mysqlx_session.cpp $(PROXYSQL_PATH)/plugins/mysqlx/src/mysqlx_data_stream.cpp $(PROXYSQL_PATH)/plugins/mysqlx/src/mysqlx_connection.cpp $(PROXYSQL_PATH)/plugins/mysqlx/src/mysqlx_thread.cpp $(PROXYSQL_PATH)/plugins/mysqlx/src/mysqlx_protocol.cpp $(PROXYSQL_PATH)/plugins/mysqlx/src/mysqlx_config_store.cpp $(MYSQLX_PROTO_OBJS) $(TEST_HELPERS_OBJ) $(LIBPROXYSQLAR) + $(CXX) $< $(PROXYSQL_PATH)/plugins/mysqlx/src/mysqlx_session.cpp $(PROXYSQL_PATH)/plugins/mysqlx/src/mysqlx_data_stream.cpp $(PROXYSQL_PATH)/plugins/mysqlx/src/mysqlx_connection.cpp $(PROXYSQL_PATH)/plugins/mysqlx/src/mysqlx_thread.cpp $(PROXYSQL_PATH)/plugins/mysqlx/src/mysqlx_protocol.cpp $(PROXYSQL_PATH)/plugins/mysqlx/src/mysqlx_config_store.cpp $(MYSQLX_PROTO_OBJS) $(TEST_HELPERS_OBJ) \ -I$(PROXYSQL_PATH)/plugins/mysqlx/include -I$(MYSQLX_PROTO_DIR) \ $(IDIRS) $(LDIRS) $(OPT) $(LIBPROXYSQLAR_FULL) $(STATIC_LIBS) \ $(MYLIBS) -lprotobuf -lssl -lcrypto $(ALLOW_MULTI_DEF) -lpthread -o $@ diff --git a/test/tap/tests/unit/mysqlx_robustness_unit-t.cpp b/test/tap/tests/unit/mysqlx_robustness_unit-t.cpp index ddee5c21e..f9fec2d03 100644 --- a/test/tap/tests/unit/mysqlx_robustness_unit-t.cpp +++ b/test/tap/tests/unit/mysqlx_robustness_unit-t.cpp @@ -525,6 +525,66 @@ static void test_client_disconnect_detected() { close(fds[0]); } +// Regression guard: after backend auth completes, the data-plane reads/writes +// must route through backend_conn_->backend_ds(), not a session-owned stream +// re-wrapped around the raw backend fd. The TLS handshake (when backend TLS +// is required) runs against backend_conn_->backend_ds() and the resulting +// SSL* lives on that stream; rewrapping the fd would discard it and cause +// cleartext I/O on a TLS-expecting socket. +// +// We can't cheaply drive a full TLS handshake inside a unit test, so we +// instead assert the structural invariant: MysqlxSession::server_ds() is an +// alias for MysqlxConnection::backend_ds() whenever a backend is attached, +// and the session's placeholder stream is never initialized post-auth. +static void test_server_ds_aliases_backend_conn_backend_ds() { + int client_fds[2], backend_fds[2]; + socketpair(AF_UNIX, SOCK_STREAM, 0, client_fds); + socketpair(AF_UNIX, SOCK_STREAM, 0, backend_fds); + + MysqlxSession sess; + setup_authenticated_session(client_fds, sess); + + // With no backend attached yet, server_ds() returns the session's + // placeholder: fd == -1 and status never set to something data-plane + // readers would treat as live. + ok(sess.backend_conn() == nullptr, + "pre-attach: no backend connection is associated with the session"); + ok(sess.server_ds().get_fd() == -1, + "pre-attach: server_ds() placeholder has fd == -1"); + + // Attach a backend and initialize its backend_ds() (as the real auth + // path does via MysqlxConnection::init_backend_ds()). + MysqlxConnection* conn = new MysqlxConnection(); + conn->set_fd(backend_fds[0]); + conn->set_state(MysqlxConnection::IDLE); + conn->set_reusable(true); + conn->init_backend_ds(backend_fds[0]); + sess.backend_conn() = conn; + + // The critical invariant: server_ds() now aliases backend_conn_->backend_ds(). + // If this ever regresses to wrapping the raw fd in a fresh stream, the + // TLS state living on backend_conn_->backend_ds_ would be bypassed. + ok(&sess.server_ds() == &sess.backend_conn()->backend_ds(), + "post-attach: server_ds() aliases backend_conn_->backend_ds() (TLS state preserved)"); + ok(sess.server_ds().get_fd() == backend_fds[0], + "post-attach: server_ds().get_fd() reflects the backend fd"); + + // return_backend_to_pool (via session close) must not tear down the + // TLS-aware stream on the cached connection. Driving this path via the + // session handler while the backend connection is null would invoke + // return_backend_to_pool(); here we simply verify that after clearing + // the backend pointer the placeholder is once again what server_ds() + // returns and its fd is still -1. + delete conn; + sess.backend_conn() = nullptr; + ok(sess.server_ds().get_fd() == -1, + "post-detach: server_ds() placeholder has fd == -1 (never re-initialized)"); + + detach_session_fds(sess); + close(client_fds[0]); close(client_fds[1]); + close(backend_fds[0]); close(backend_fds[1]); +} + static void test_forward_empty_frame() { int client_fds[2], backend_fds[2]; socketpair(AF_UNIX, SOCK_STREAM, 0, client_fds); @@ -567,7 +627,7 @@ static void test_forward_empty_frame() { } int main() { - plan(33); + plan(38); test_server_response_terminal_frame(); test_server_response_non_terminal_keeps_waiting(); @@ -581,6 +641,7 @@ int main() { test_return_backend_no_thread(); test_connection_limit_config(); test_client_disconnect_detected(); + test_server_ds_aliases_backend_conn_backend_ds(); test_forward_empty_frame(); return exit_status();