hcq/MaxScale
hcq/MaxScale
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

MXS-1826: Fix COM_CHANGE_USER regression

Browse Source 
The re-authentication done in MaxScale caused multiple error packets to be
sent for the same COM_CHANGE_USER. In addition to this, the failure of
authentication did not terminate the client connection.

The change in behavior requires the test case to be changed as well.
This commit is contained in:
Markus Mäkelä 2018-05-08 15:10:19 +03:00
parent 5eb6718b75
commit 00581e7f35
No known key found for this signature in database
GPG Key ID: 72D48FCE664F7B19
3 changed files with 61 additions and 11 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
maxscale-system-test/change_user.cpp
View File

@ -31,6 +31,10 @@ int main(int argc, char *argv[])
Test->try_query(Test->maxscales->conn_rwsplit[0], (char *) "DROP TABLE IF EXISTS t1");
Test->try_query(Test->maxscales->conn_rwsplit[0], (char *) "CREATE TABLE t1 (x1 int, fl int)");
Test->maxscales->restart_maxscale();
sleep(2);
Test->maxscales->connect_maxscale(0);
Test->tprintf("Changing user... \n");
Test->add_result(mysql_change_user(Test->maxscales->conn_rwsplit[0], (char *) "user", (char *) "pass2", (char *) "test") ,
"changing user failed \n");
@ -60,9 +64,9 @@ int main(int argc, char *argv[])
Test->add_result(1, "There is no proper error message\n");
}
Test->tprintf("Trying INSERT again (expecting success - use change should fail)... \n");
Test->try_query(Test->maxscales->conn_rwsplit[0], (char *) "INSERT INTO t1 VALUES (77, 13);");
Test->tprintf("Trying INSERT again (expecting failure - change user should have failed)...");
Test->add_result(!execute_query(Test->maxscales->conn_rwsplit[0], (char *) "INSERT INTO t1 VALUES (77, 13);"),
"Query should fail, MaxScale should disconnect on auth failure");
Test->tprintf("Changing user with wrong password using ReadConn \n");
if (mysql_change_user(Test->maxscales->conn_slave[0], (char *) "user", (char *) "wrong_pass2", (char *) "test") == 0)
@ -79,6 +83,7 @@ int main(int argc, char *argv[])
Test->add_result(mysql_change_user(Test->maxscales->conn_slave[0], (char *) "user", (char *) "pass2", (char *) "test") ,
"changing user failed \n");
Test->maxscales->connect_maxscale(0);
Test->try_query(Test->maxscales->conn_rwsplit[0], (char *) "DROP USER user@'%%';");
execute_query_silent(Test->maxscales->conn_rwsplit[0], "DROP TABLE test.t1");

8
server/modules/protocol/MySQL/mariadbbackend/mysql_backend.c
View File

@ -859,6 +859,14 @@ gw_read_and_write(DCB *dcb)
}
else
{
/**
* The client protocol always requests an authentication method
* switch to the same plugin to be compatible with most connectors.
*
* To prevent packet sequence number mismatch, always return a sequence
* of 3 for the final response to a COM_CHANGE_USER.
*/
GWBUF_DATA(read_buffer)[3] = 0x3;
proto->changing_user = false;
}
}

53
server/modules/protocol/MySQL/mariadbclient/mysql_client.cc
View File

@ -1513,19 +1513,36 @@ static bool reauthenticate_client(MXS_SESSION* session, GWBUF* packetbuf)
if (session->client_dcb->authfunc.reauthenticate)
{
MySQLProtocol* proto = (MySQLProtocol*)session->client_dcb->protocol;
MYSQL_session* data = (MYSQL_session*)session->client_dcb->data;
uint8_t client_sha1[MYSQL_SCRAMBLE_LEN] = {};
uint8_t payload[gwbuf_length(packetbuf) - MYSQL_HEADER_LEN];
gwbuf_copy_data(packetbuf, MYSQL_HEADER_LEN, sizeof(payload), payload);
// Will contains extra data but the username is null-terminated
char user[gwbuf_length(proto->stored_query) - MYSQL_HEADER_LEN - 1];
gwbuf_copy_data(proto->stored_query, MYSQL_HEADER_LEN + 1,
sizeof(user), (uint8_t*)user);
// Copy the new username to the session data
MYSQL_session* data = (MYSQL_session*)session->client_dcb->data;
strcpy(data->user, user);
int rc = session->client_dcb->authfunc.reauthenticate(session->client_dcb, data->user,
payload, sizeof(payload),
proto->scramble, sizeof(proto->scramble),
client_sha1, sizeof(client_sha1));
if (!(rval = rc == MXS_AUTH_SUCCEEDED))
if (rc == MXS_AUTH_SUCCEEDED)
{
// Re-authentication successful, route the original COM_CHANGE_USER
rval = true;
}
else
{
/**
* Authentication failed. To prevent the COM_CHANGE_USER from reaching
* the backend servers (and possibly causing problems) the client
* connection will be closed.
*
* First packet is COM_CHANGE_USER, the second is AuthSwitchRequest,
* third is the response and the fourth is the following error.
*/
@ -1656,14 +1673,34 @@ static int route_by_statement(MXS_SESSION* session, uint64_t capabilities, GWBUF
{
changed_user = true;
send_auth_switch_request_packet(session->client_dcb);
// Store the original COM_CHANGE_USER for later
proto->stored_query = packetbuf;
packetbuf = NULL;
}
else if (proto->changing_user)
{
proto->changing_user = false;
bool ok = reauthenticate_client(session, packetbuf);
gwbuf_free(packetbuf);
packetbuf = proto->stored_query;
proto->stored_query = NULL;
if (ok)
{
// Authentication was successful, route the original COM_CHANGE_USER
rc = 1;
}
else
{
// Authentication failed, close the connection
rc = 0;
gwbuf_free(packetbuf);
packetbuf = NULL;
}
}
if (proto->changing_user)
{
rc = reauthenticate_client(session, packetbuf) ? 1 : 0;
gwbuf_free(packetbuf);
}
else
if (packetbuf)
{
/** Route query */
rc = MXS_SESSION_ROUTE_QUERY(session, packetbuf);