fix(mysqlx): reject re-auth on active session; fix two pre-existing test issues

## (A) Re-authentication on an active session is now rejected

Addresses the Important finding from the security re-review of PR #5651
(see https://github.com/sysown/proxysql/issues/5676).

After a successful login the session is in WAITING_CLIENT_XMSG. Before
this commit, dispatch_client_message routed
`SESS_AUTHENTICATE_START` and `SESS_AUTHENTICATE_CONTINUE` into the
auth handlers unconditionally. The handlers overwrote `username_`,
`identity_`, `target_hostgroup_`, `target_address_`, `target_port_` —
but they did NOT tear down `backend_conn_`. The next StmtExecute was
forwarded over the previous user's pooled backend connection. The
proxy then audited the query as user B while the backend executed it
as user A's role — a real identity-coherence / audit hazard.

The X Protocol uses `Mysqlx::Session::Reset` for re-auth on the same
connection, not direct re-auth. Reject `SESS_AUTHENTICATE_START` /
`SESS_AUTHENTICATE_CONTINUE` when `status_ == WAITING_CLIENT_XMSG`
with code 1845 (FATAL) and drop the session. Conformant with the spec.

Also clears `auth_challenge_` on successful auth (defense in depth):
the verified challenge is no longer reachable by a stale
AuthenticateContinue replay even before the dispatch-level guard
fires.

The unit test `test_error_severity_non_fatal` previously took a
shortcut: it manually set `status_=WAITING_CLIENT_XMSG` then sent
AUTHENTICATE_START to drive the auth flow. With the re-auth rejection
now in place, that shortcut hits the new guard and the test hangs
waiting for an auth challenge that never comes. Updated the test to
drive the auth flow naturally from CONNECTING_CLIENT (the state
init() leaves the session in), which exercises the same code paths
without the now-invalid pre-auth status_ override.

## (B) mysqlx_config_store_concurrent_unit-t: missing variables DDL

This was the source of 6 pre-existing test failures surfaced during
the ASAN run on PR #5651 (4, 11, 12, 13, 14, 15). All asserted that
load_from_runtime atomically replaces previously-loaded data. They
failed because the test fixture's create_runtime_db() did not create
a `runtime_mysqlx_variables` table, but `load_from_runtime` queries
that table near the end:

    SELECT variable_name, variable_value FROM runtime_mysqlx_variables

When the table doesn't exist, fetch_result returns false and
load_from_runtime short-circuits BEFORE swapping the new identities/
routes/endpoints into place. Every "second load replaces first"
assertion silently failed because the second load never actually
replaced anything — the swap never happened.

This is the same bug that was fixed in
`mysqlx_config_store_pure_unit-t.cpp` in commit 017496bc4. The
sibling test was missed at the time. Add the DDL here too with a
comment pointing at the underlying invariant. After this fix the
concurrent test reports 15/15 ok.

## Pre-existing test bug: X_FAST_FORWARD reference

`mysqlx_message_dispatch_unit-t.cpp:424` referenced
`MysqlxSession::X_FAST_FORWARD`, which was retired together with the
dormant MysqlxWorker path in commit 79cac4c97. The test failed to
compile with `-DMYSQLX_TEST_BUILD` once that macro was added. Replace
the comparison with `CONNECTING_CLIENT` (a still-extant earlier-than-
TLS state) so the assertion's intent — "TLS states come after the
basic states" — is preserved. (The other failures in this file
remain pre-existing and are not addressed here; they are out of
scope and tracked under issue #5679.)

Verified locally with WITHASAN=1: mysqlx_session_unit-t 60 oks (2
pre-existing not-ok #33,34), mysqlx_robustness_unit-t 74/74,
mysqlx_config_store_concurrent_unit-t 15/15.
pull/5700/head
Rene Cannao 2 months ago
parent 4bc7044710
commit 09c15d6d54

@ -644,6 +644,13 @@ void MysqlxSession::handler_auth_challenge_response() {
return;
}
// Defense in depth: clear the verified challenge so a stale value
// cannot be re-used by a misbehaving client. Combined with the
// re-auth rejection in dispatch_client_message, this leaves no
// path for AuthenticateContinue to be replayed against the same
// challenge after AuthenticateOk.
auth_challenge_.clear();
last_active_time_ = monotonic_time_ms();
send_auth_ok();
status_ = WAITING_CLIENT_XMSG;
@ -651,6 +658,26 @@ void MysqlxSession::handler_auth_challenge_response() {
}
int MysqlxSession::dispatch_client_message(uint8_t msg_type) {
// Re-authentication on an established session is not supported. The
// X Protocol uses Mysqlx::Session::Reset for that purpose; a direct
// AUTHENTICATE_START/CONTINUE after the session is already in
// WAITING_CLIENT_XMSG would otherwise overwrite username_,
// identity_, target_hostgroup_, target_address_, target_port_ —
// without tearing down backend_conn_, so the next StmtExecute would
// be forwarded over the previous user's pooled backend connection.
// That is an identity-coherence / audit hazard (the proxy bills the
// query as user B while the backend executes it as user A's role).
// Reject explicitly with a fatal error and drop the session.
if ((msg_type == Mysqlx::ClientMessages_Type_SESS_AUTHENTICATE_START ||
msg_type == Mysqlx::ClientMessages_Type_SESS_AUTHENTICATE_CONTINUE) &&
status_ == WAITING_CLIENT_XMSG) {
client_ds_.pop_frame();
send_error(1845, "Re-authentication is not supported on an active session; "
"use Mysqlx::Session::Reset to start a new session", true);
status_ = X_SESSION_CLOSING;
healthy = false;
return -1;
}
switch (msg_type) {
case Mysqlx::ClientMessages_Type_CON_CAPABILITIES_GET:
handler_capabilities_get(); return 0;

@ -50,6 +50,18 @@ const char kRuntimeMysqlxEndpointsDdl[] =
" PRIMARY KEY (hostname, mysql_port)"
" )";
// load_from_runtime also queries runtime_mysqlx_variables. Without the
// table, fetch_result returns false and the load short-circuits before
// swapping in the newly-loaded identities/routes — every assertion that
// depends on data actually being loaded silently fails. This DDL match
// ades the one in mysqlx_admin_schema.cpp; same fix as
// mysqlx_config_store_pure_unit-t.cpp received in commit 017496bc4.
const char kRuntimeMysqlxVariablesDdl[] =
"CREATE TABLE runtime_mysqlx_variables ("
" variable_name VARCHAR NOT NULL PRIMARY KEY,"
" variable_value VARCHAR NOT NULL DEFAULT ''"
" )";
std::unique_ptr<SQLite3DB> create_runtime_db() {
auto db = std::make_unique<SQLite3DB>();
db->open((char*)":memory:", SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE | SQLITE_OPEN_FULLMUTEX);
@ -58,6 +70,7 @@ std::unique_ptr<SQLite3DB> create_runtime_db() {
db->execute(kRuntimeMysqlxUsersDdl);
db->execute(kRuntimeMysqlxRoutesDdl);
db->execute(kRuntimeMysqlxEndpointsDdl);
db->execute(kRuntimeMysqlxVariablesDdl);
return db;
}

@ -421,7 +421,10 @@ static void test_dispatch_unknown_message() {
}
static void test_tls_states() {
ok(MysqlxSession::X_TLS_ACCEPT_INIT > MysqlxSession::X_FAST_FORWARD,
// X_FAST_FORWARD was retired with the dormant MysqlxWorker path
// (commit 79cac4c97); compare against a still-extant earlier state
// to keep the "TLS states are after the basic states" assertion.
ok(MysqlxSession::X_TLS_ACCEPT_INIT > MysqlxSession::CONNECTING_CLIENT,
"X_TLS_ACCEPT_INIT is valid enum value");
ok(MysqlxSession::X_TLS_ACCEPT_DONE > MysqlxSession::X_TLS_ACCEPT_CONT,
"X_TLS_ACCEPT_DONE > X_TLS_ACCEPT_CONT");

@ -396,7 +396,14 @@ static void test_error_severity_non_fatal() {
socketpair(AF_UNIX, SOCK_STREAM, 0, fds);
MysqlxSession sess;
sess.init(fds[0], nullptr);
sess.set_status(MysqlxSession::WAITING_CLIENT_XMSG);
// Drive the auth flow from CONNECTING_CLIENT (the natural startup
// state set by init()). Earlier versions of this test forced status_
// to WAITING_CLIENT_XMSG as a shortcut, but that pre-authenticated
// shortcut now collides with the re-auth rejection in
// dispatch_client_message(): re-authenticating an active session is
// no longer permitted (the X Protocol uses Mysqlx::Session::Reset
// for that purpose). The session naturally reaches X_AUTH_CHALLENGE_
// SENT after AUTHENTICATE_START anyway, so just don't override.
sess.to_process = true;
Mysqlx::Session::AuthenticateStart auth_start;

Loading…
Cancel
Save