From 964207cbea1e1e230fbaeb562bb5f6cde45ca33f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Thu, 2 Jan 2020 11:34:28 +0200 Subject: [PATCH 1/5] MXS-2820: Return correct value on wrong password --- server/modules/authenticator/MySQLAuth/dbusers.cc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/server/modules/authenticator/MySQLAuth/dbusers.cc b/server/modules/authenticator/MySQLAuth/dbusers.cc index 3dfde6cae..873648517 100644 --- a/server/modules/authenticator/MySQLAuth/dbusers.cc +++ b/server/modules/authenticator/MySQLAuth/dbusers.cc @@ -415,6 +415,10 @@ int validate_mysql_user(MYSQL_AUTH* instance, rval = MXS_AUTH_FAILED_DB; } } + else + { + rval = MXS_AUTH_FAILED_WRONG_PASSWORD; + } } return rval; From aa83bc24ae2d9421865193a89df2e9ab134bef1c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Thu, 2 Jan 2020 12:00:10 +0200 Subject: [PATCH 2/5] MXS-2820: Log default database on auth failure The default database was not exposed in the warning that was logged when authentication failed. The authentication uses the username, host and the default database to find the user entry and the lack of the default database made it hard to know for sure which user entry a client should've matched against. --- .../authenticator/MySQLAuth/mysql_auth.cc | 97 +++++++++++-------- 1 file changed, 55 insertions(+), 42 deletions(-) diff --git a/server/modules/authenticator/MySQLAuth/mysql_auth.cc b/server/modules/authenticator/MySQLAuth/mysql_auth.cc index 33f012171..e836cde39 100644 --- a/server/modules/authenticator/MySQLAuth/mysql_auth.cc +++ b/server/modules/authenticator/MySQLAuth/mysql_auth.cc @@ -37,6 +37,8 @@ #include #include +#include + static void* mysql_auth_init(char** options); static bool mysql_auth_set_protocol_data(DCB* dcb, GWBUF* buf); static bool mysql_auth_is_client_ssl_capable(DCB* dcb); @@ -301,7 +303,58 @@ static GWBUF* gen_auth_switch_request_packet(MySQLProtocol* proto, MYSQL_session bufdata += GW_MYSQL_SCRAMBLE_SIZE; *bufdata = '\0'; return buffer; -}; +} + +static void log_auth_failure(DCB* dcb, int auth_ret) +{ + MYSQL_session* client_data = (MYSQL_session*)dcb->data; + std::ostringstream extra; + + if (auth_ret == MXS_AUTH_FAILED_DB) + { + extra << "Unknown database: " << client_data->db; + } + else if (auth_ret == MXS_AUTH_FAILED_WRONG_PASSWORD) + { + extra << "Wrong password."; + } + else + { + extra << "User not found."; + } + + + std::ostringstream host; + + if (dcb->path) + { + host << "[" << dcb->remote << "]:" << dcb->path; + } + else + { + host << "[" << dcb->remote << "]:" << dcb_get_port(dcb); + } + + std::ostringstream db; + + if (*client_data->db) + { + db << " to database '" << client_data->db << "'"; + } + + MXS_LOG_EVENT(maxscale::event::AUTHENTICATION_FAILURE, + "%s: login attempt for user '%s'@%s%s, authentication failed. %s", + dcb->service->name, client_data->user, host.str().c_str(), + db.str().c_str(), extra.str().c_str()); + + if (is_localhost_address(&dcb->ip) && !dcb->service->localhost_match_wildcard_host) + { + MXS_NOTICE("If you have a wildcard grant that covers this address, " + "try adding 'localhost_match_wildcard_host=true' for " + "service '%s'. ", dcb->service->name); + } +} + /** * @brief Authenticates a MySQL user who is a client to MaxScale. * @@ -365,47 +418,7 @@ static int mysql_auth_authenticate(DCB* dcb) } else if (dcb->service->log_auth_warnings) { - // The default failure is a `User not found` one - char extra[256] = "User not found."; - - if (auth_ret == MXS_AUTH_FAILED_DB) - { - snprintf(extra, sizeof(extra), "Unknown database: %s", client_data->db); - } - else if (auth_ret == MXS_AUTH_FAILED_WRONG_PASSWORD) - { - strcpy(extra, "Wrong password."); - } - - if (dcb->path) - { - MXS_LOG_EVENT(maxscale::event::AUTHENTICATION_FAILURE, - "%s: login attempt for user '%s'@[%s]:%s, authentication failed. %s", - dcb->service->name, - client_data->user, - dcb->remote, - dcb->path, - extra); - } - else - { - MXS_LOG_EVENT(maxscale::event::AUTHENTICATION_FAILURE, - "%s: login attempt for user '%s'@[%s]:%d, authentication failed. %s", - dcb->service->name, - client_data->user, - dcb->remote, - dcb_get_port(dcb), - extra); - } - - if (is_localhost_address(&dcb->ip) - && !dcb->service->localhost_match_wildcard_host) - { - MXS_NOTICE("If you have a wildcard grant that covers this address, " - "try adding 'localhost_match_wildcard_host=true' for " - "service '%s'. ", - dcb->service->name); - } + log_auth_failure(dcb, auth_ret); } /* let's free the auth_token now */ From 6306519e5e75575ba083ee2f0edfe7e624da5d26 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Thu, 2 Jan 2020 13:50:23 +0200 Subject: [PATCH 3/5] MXS-2710: Move client_count handling inside Session By incrementing the counters when the session is created, we know that the counter will always be decremented correctly. This does cause the listener session to be counted as an actual session but this is already present in the statistics calculations and is something we have to live with in 2.3 This change also makes it possible to overshoot the connection count limitation as the session creation is delayed until authentication fails. Both of these problems are fixed in 2.4. --- server/core/dcb.cc | 5 ---- server/core/session.cc | 23 ++++++++++--------- server/modules/filter/test/mock_session.cc | 3 +++ .../MySQL/mariadbclient/mysql_client.cc | 14 +++-------- 4 files changed, 18 insertions(+), 27 deletions(-) diff --git a/server/core/dcb.cc b/server/core/dcb.cc index f466d2135..de0572e6d 100644 --- a/server/core/dcb.cc +++ b/server/core/dcb.cc @@ -2533,11 +2533,6 @@ DCB* dcb_accept(DCB* dcb) } } - if (client_dcb) - { - mxb::atomic::add(&client_dcb->service->client_count, 1); - } - return client_dcb; } diff --git a/server/core/session.cc b/server/core/session.cc index 70da75a36..16b7c0654 100644 --- a/server/core/session.cc +++ b/server/core/session.cc @@ -361,18 +361,7 @@ private: static void session_free(MXS_SESSION* session) { MXS_INFO("Stopped %s client session [%" PRIu64 "]", session->service->name, session->ses_id); - Service* service = static_cast(session->service); - session_final_free(session); - bool should_destroy = !mxb::atomic::load(&service->active); - - if (mxb::atomic::add(&service->client_count, -1) == 1 && should_destroy) - { - // Destroy the service in the main routing worker thread - mxs::RoutingWorker* main_worker = mxs::RoutingWorker::get(mxs::RoutingWorker::MAIN); - main_worker->execute(std::unique_ptr(new ServiceDestroyTask(service)), - Worker::EXECUTE_AUTO); - } } static void session_final_free(MXS_SESSION* ses) @@ -1264,6 +1253,7 @@ Session::Session(SERVICE* service) mxb::atomic::add(&service->stats.n_current, 1, mxb::atomic::RELAXED); mxb_assert(service->stats.n_current >= 0); + mxb::atomic::add(&service->client_count, 1, mxb::atomic::RELAXED); } Session::~Session() @@ -1281,6 +1271,17 @@ Session::~Session() mxb::atomic::add(&service->stats.n_current, -1, mxb::atomic::RELAXED); mxb_assert(service->stats.n_current >= 0); + + bool should_destroy = !mxb::atomic::load(&service->active); + + if (mxb::atomic::add(&service->client_count, -1) == 1 && should_destroy) + { + // Destroy the service in the main routing worker thread + mxs::RoutingWorker* main_worker = mxs::RoutingWorker::get(mxs::RoutingWorker::MAIN); + main_worker->execute( + std::unique_ptr(new ServiceDestroyTask(static_cast(service))), + Worker::EXECUTE_AUTO); + } } namespace diff --git a/server/modules/filter/test/mock_session.cc b/server/modules/filter/test/mock_session.cc index aaa3d233a..c4b445795 100644 --- a/server/modules/filter/test/mock_session.cc +++ b/server/modules/filter/test/mock_session.cc @@ -44,6 +44,9 @@ Session::Session(Client* pClient) m_client_dcb.data = &m_mysql_session; service = &dummy_service; + + // This prevents the destruction of the dummy service + service->active = true; } Session::~Session() diff --git a/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc b/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc index 638a235ca..9bf24c96c 100644 --- a/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc +++ b/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc @@ -740,20 +740,12 @@ static void check_packet(DCB* dcb, GWBUF* buf, int bytes) */ static int gw_read_do_authentication(DCB* dcb, GWBUF* read_buffer, int nbytes_read) { - // If this function fails and dcb is closed, the service client count must be decremented manually. - // After authentication, the session closing code decrements the count. - auto decrement_and_close = [](DCB* dcb) - { - mxb::atomic::add(&dcb->service->client_count, -1); - dcb_close(dcb); - }; - MXB_AT_DEBUG(check_packet(dcb, read_buffer, nbytes_read)); /** Allocate the shared session structure */ if (dcb->data == NULL && (dcb->data = mysql_session_alloc()) == NULL) { - decrement_and_close(dcb); + dcb_close(dcb); return 1; } @@ -820,7 +812,7 @@ static int gw_read_do_authentication(DCB* dcb, GWBUF* read_buffer, int nbytes_re MYSQL_session* ses = (MYSQL_session*)dcb->data; if ((dcb->user = MXS_STRDUP(ses->user)) == NULL) { - decrement_and_close(dcb); + dcb_close(dcb); gwbuf_free(read_buffer); return 0; } @@ -873,7 +865,7 @@ static int gw_read_do_authentication(DCB* dcb, GWBUF* read_buffer, int nbytes_re /** * Close DCB and which will release MYSQL_session */ - decrement_and_close(dcb); + dcb_close(dcb); } /* One way or another, the buffer is now fully processed */ gwbuf_free(read_buffer); From edb49d6f35c0cc566764dc49c507d6bf775802bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Tue, 7 Jan 2020 09:57:15 +0200 Subject: [PATCH 4/5] MXS-2824: Document basic user privileges The documentation was not clear on what the difference between admin and basic accounts is. --- Documentation/REST-API/Resources-User.md | 4 ++++ maxctrl/lib/create.js | 3 ++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/Documentation/REST-API/Resources-User.md b/Documentation/REST-API/Resources-User.md index f10546cfe..306168b60 100644 --- a/Documentation/REST-API/Resources-User.md +++ b/Documentation/REST-API/Resources-User.md @@ -195,6 +195,10 @@ following fields. * `data.attributes.account` * Set to `admin` for administrative users and `basic` to read-only users +Only admin accounts can perform POST, PUT, DELETE and PATCH requests. If a basic +account performs one of the aforementioned request, the REST API will respond +with a `401 Unauthorized` error. + Here is an example request body defining the network user _my-user_ with the password _my-password_ that is allowed to execute only read-only operations. diff --git a/maxctrl/lib/create.js b/maxctrl/lib/create.js index 5fd9950c2..55210958c 100644 --- a/maxctrl/lib/create.js +++ b/maxctrl/lib/create.js @@ -319,7 +319,8 @@ exports.builder = function(yargs) { return yargs.epilog('The created user can be used with the MaxScale REST API as ' + 'well as the MaxAdmin network interface. By default the created ' + 'user will have read-only privileges. To make the user an ' + - 'administrative user, use the `--type=admin` option.') + 'administrative user, use the `--type=admin` option. ' + + 'Basic users can only perform `list` and `show` commands.') .usage('Usage: create user ') }, function(argv) { From 36b0196c3efb3fe94d204d98c6528ef646b09975 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Tue, 7 Jan 2020 10:13:28 +0200 Subject: [PATCH 5/5] MXS-2825: Fix basic user privileges All POST, PUT, DELETE and PATCH commands should be prevented regardless of whether they define a request body. --- server/core/admin.cc | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/server/core/admin.cc b/server/core/admin.cc index 379479d1d..a576820bb 100644 --- a/server/core/admin.cc +++ b/server/core/admin.cc @@ -73,9 +73,8 @@ static inline size_t request_data_length(MHD_Connection* connection) static bool modifies_data(MHD_Connection* connection, string method) { - return (method == MHD_HTTP_METHOD_POST || method == MHD_HTTP_METHOD_PUT - || method == MHD_HTTP_METHOD_DELETE || method == MHD_HTTP_METHOD_PATCH) - && request_data_length(connection); + return method == MHD_HTTP_METHOD_POST || method == MHD_HTTP_METHOD_PUT + || method == MHD_HTTP_METHOD_DELETE || method == MHD_HTTP_METHOD_PATCH; } static void send_auth_error(MHD_Connection* connection)