From 3f44f34fe1fa53fb6177d11b431361f23dcbca1b Mon Sep 17 00:00:00 2001 From: Alexander Tokmakov Date: Mon, 23 May 2022 21:55:17 +0200 Subject: [PATCH] review suggestions --- src/Interpreters/Session.cpp | 6 +++--- src/Interpreters/Session.h | 2 +- src/Interpreters/SessionLog.cpp | 15 ++++++++++----- src/Interpreters/SessionLog.h | 4 ++-- src/Interpreters/SystemLog.cpp | 2 +- src/Server/TCPHandler.cpp | 13 +++++-------- 6 files changed, 22 insertions(+), 20 deletions(-) diff --git a/src/Interpreters/Session.cpp b/src/Interpreters/Session.cpp index 7ea58f51ba3..bc5ae369114 100644 --- a/src/Interpreters/Session.cpp +++ b/src/Interpreters/Session.cpp @@ -317,7 +317,7 @@ void Session::authenticate(const Credentials & credentials_, const Poco::Net::So } catch (const Exception & e) { - onAuthenticationFailure(credentials_, address, e); + onAuthenticationFailure(credentials_.getUserName(), address, e); throw; } @@ -325,7 +325,7 @@ void Session::authenticate(const Credentials & credentials_, const Poco::Net::So prepared_client_info->current_address = address; } -void Session::onAuthenticationFailure(const Credentials & credentials_, const Poco::Net::SocketAddress & address_, const Exception & e) +void Session::onAuthenticationFailure(const std::optional & user_name, const Poco::Net::SocketAddress & address_, const Exception & e) { LOG_DEBUG(log, "{} Authentication failed with error: {}", toString(auth_id), e.what()); if (auto session_log = getSessionLog()) @@ -333,7 +333,7 @@ void Session::onAuthenticationFailure(const Credentials & credentials_, const Po /// Add source address to the log auto info_for_log = *prepared_client_info; info_for_log.current_address = address_; - session_log->addLoginFailure(auth_id, info_for_log, credentials_.getUserName(), e); + session_log->addLoginFailure(auth_id, info_for_log, user_name, e); } } diff --git a/src/Interpreters/Session.h b/src/Interpreters/Session.h index ec5d50f8147..8de76349b7d 100644 --- a/src/Interpreters/Session.h +++ b/src/Interpreters/Session.h @@ -52,7 +52,7 @@ public: void authenticate(const Credentials & credentials_, const Poco::Net::SocketAddress & address_); /// Writes a row about login failure into session log (if enabled) - void onAuthenticationFailure(const Credentials & credentials_, const Poco::Net::SocketAddress & address_, const Exception & e); + void onAuthenticationFailure(const std::optional & user_name, const Poco::Net::SocketAddress & address_, const Exception & e); /// Returns a reference to session ClientInfo. ClientInfo & getClientInfo(); diff --git a/src/Interpreters/SessionLog.cpp b/src/Interpreters/SessionLog.cpp index 554c6416d33..5f303ab64b4 100644 --- a/src/Interpreters/SessionLog.cpp +++ b/src/Interpreters/SessionLog.cpp @@ -133,7 +133,7 @@ NamesAndTypesList SessionLogElement::getNamesAndTypes() {"event_time", std::make_shared()}, {"event_time_microseconds", std::make_shared(6)}, - {"user", std::make_shared()}, + {"user", std::make_shared(std::make_shared())}, {"auth_type", std::make_shared(std::move(identified_with_column))}, {"profiles", std::make_shared(lc_string_datatype)}, @@ -169,7 +169,8 @@ void SessionLogElement::appendToBlock(MutableColumns & columns) const columns[i++]->insert(event_time); columns[i++]->insert(event_time_microseconds); - columns[i++]->insert(user); + assert((user && user_identified_with) || client_info.interface == ClientInfo::Interface::TCP_INTERSERVER); + columns[i++]->insert(user ? Field(*user) : Field()); columns[i++]->insert(user_identified_with ? Field(*user_identified_with) : Field()); fillColumnArray(profiles, *columns[i++]); @@ -217,9 +218,11 @@ void SessionLog::addLoginSuccess(const UUID & auth_id, std::optional ses DB::SessionLogElement log_entry(auth_id, SESSION_LOGIN_SUCCESS); log_entry.client_info = client_info; - log_entry.user = login_user ? login_user->getName() : USER_INTERSERVER_MARKER; if (login_user) + { + log_entry.user = login_user->getName(); log_entry.user_identified_with = login_user->auth_data.getType(); + } log_entry.external_auth_server = login_user ? login_user->auth_data.getLDAPServerName() : ""; if (session_id) @@ -240,7 +243,7 @@ void SessionLog::addLoginSuccess(const UUID & auth_id, std::optional ses void SessionLog::addLoginFailure( const UUID & auth_id, const ClientInfo & info, - const String & user, + const std::optional & user, const Exception & reason) { SessionLogElement log_entry(auth_id, SESSION_LOGIN_FAILURE); @@ -256,9 +259,11 @@ void SessionLog::addLoginFailure( void SessionLog::addLogOut(const UUID & auth_id, const UserPtr & login_user, const ClientInfo & client_info) { auto log_entry = SessionLogElement(auth_id, SESSION_LOGOUT); - log_entry.user = login_user ? login_user->getName() : USER_INTERSERVER_MARKER; if (login_user) + { + log_entry.user = login_user->getName(); log_entry.user_identified_with = login_user->auth_data.getType(); + } log_entry.external_auth_server = login_user ? login_user->auth_data.getLDAPServerName() : ""; log_entry.client_info = client_info; diff --git a/src/Interpreters/SessionLog.h b/src/Interpreters/SessionLog.h index 92074540ec4..990c7ffea01 100644 --- a/src/Interpreters/SessionLog.h +++ b/src/Interpreters/SessionLog.h @@ -46,7 +46,7 @@ struct SessionLogElement time_t event_time{}; Decimal64 event_time_microseconds{}; - String user; + std::optional user; std::optional user_identified_with; String external_auth_server; Strings roles; @@ -73,7 +73,7 @@ class SessionLog : public SystemLog public: void addLoginSuccess(const UUID & auth_id, std::optional session_id, const Context & login_context, const UserPtr & login_user); - void addLoginFailure(const UUID & auth_id, const ClientInfo & info, const String & user, const Exception & reason); + void addLoginFailure(const UUID & auth_id, const ClientInfo & info, const std::optional & user, const Exception & reason); void addLogOut(const UUID & auth_id, const UserPtr & login_user, const ClientInfo & client_info); }; diff --git a/src/Interpreters/SystemLog.cpp b/src/Interpreters/SystemLog.cpp index 3c47f972538..3fc5dda0672 100644 --- a/src/Interpreters/SystemLog.cpp +++ b/src/Interpreters/SystemLog.cpp @@ -234,7 +234,7 @@ SystemLogs::SystemLogs(ContextPtr global_context, const Poco::Util::AbstractConf if (session_log) { logs.emplace_back(session_log.get()); - global_context->addWarningMessage("Table system.session_log exists. It's unreliable and may contain garbage. Do not use it for any kind of security monitoring."); + global_context->addWarningMessage("Table system.session_log is enabled. It's unreliable and may contain garbage. Do not use it for any kind of security monitoring."); } if (transactions_info_log) logs.emplace_back(transactions_info_log.get()); diff --git a/src/Server/TCPHandler.cpp b/src/Server/TCPHandler.cpp index a74d58b848a..dd8bb78d6a3 100644 --- a/src/Server/TCPHandler.cpp +++ b/src/Server/TCPHandler.cpp @@ -1039,7 +1039,7 @@ void TCPHandler::receiveHello() (!user.empty() ? ", user: " + user : "") ); - is_interserver_mode = (user == USER_INTERSERVER_MARKER); + is_interserver_mode = (user == USER_INTERSERVER_MARKER) && password.empty(); if (is_interserver_mode) { receiveClusterNameAndSalt(); @@ -1274,19 +1274,16 @@ void TCPHandler::receiveQuery() readStringBinary(state.query, *in); - /// TODO unify interserver authentication (currently this code looks like a backdoor at a first glance) + /// TODO Unify interserver authentication (and make sure that it's secure enough) if (is_interserver_mode) { client_info.interface = ClientInfo::Interface::TCP_INTERSERVER; #if USE_SSL - String user_for_session_log = client_info.initial_user; - if (user_for_session_log.empty()) - user_for_session_log = USER_INTERSERVER_MARKER; String cluster_secret = server.context()->getCluster(cluster)->getSecret(); if (salt.empty() || cluster_secret.empty()) { auto exception = Exception(ErrorCodes::AUTHENTICATION_FAILED, "Interserver authentication failed"); - session->onAuthenticationFailure(AlwaysAllowCredentials{USER_INTERSERVER_MARKER}, socket().peerAddress(), exception); + session->onAuthenticationFailure(/* user_name */ std::nullopt, socket().peerAddress(), exception); throw exception; /// NOLINT } @@ -1303,7 +1300,7 @@ void TCPHandler::receiveQuery() if (calculated_hash != received_hash) { auto exception = Exception(ErrorCodes::AUTHENTICATION_FAILED, "Interserver authentication failed"); - session->onAuthenticationFailure(AlwaysAllowCredentials{USER_INTERSERVER_MARKER}, socket().peerAddress(), exception); + session->onAuthenticationFailure(/* user_name */ std::nullopt, socket().peerAddress(), exception); throw exception; /// NOLINT } @@ -1322,7 +1319,7 @@ void TCPHandler::receiveQuery() auto exception = Exception( "Inter-server secret support is disabled, because ClickHouse was built without SSL library", ErrorCodes::AUTHENTICATION_FAILED); - session->onAuthenticationFailure(AlwaysAllowCredentials{USER_INTERSERVER_MARKER}, socket().peerAddress(), exception); + session->onAuthenticationFailure(/* user_name */ std::nullopt, socket().peerAddress(), exception); throw exception; /// NOLINT #endif }