review suggestions

This commit is contained in:
Alexander Tokmakov 2022-05-23 21:55:17 +02:00
parent d0f998fb88
commit 3f44f34fe1
6 changed files with 22 additions and 20 deletions

View File

@ -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<String> & 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);
}
}

View File

@ -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<String> & user_name, const Poco::Net::SocketAddress & address_, const Exception & e);
/// Returns a reference to session ClientInfo.
ClientInfo & getClientInfo();

View File

@ -133,7 +133,7 @@ NamesAndTypesList SessionLogElement::getNamesAndTypes()
{"event_time", std::make_shared<DataTypeDateTime>()},
{"event_time_microseconds", std::make_shared<DataTypeDateTime64>(6)},
{"user", std::make_shared<DataTypeString>()},
{"user", std::make_shared<DataTypeNullable>(std::make_shared<DataTypeString>())},
{"auth_type", std::make_shared<DataTypeNullable>(std::move(identified_with_column))},
{"profiles", std::make_shared<DataTypeArray>(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<String> 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<String> ses
void SessionLog::addLoginFailure(
const UUID & auth_id,
const ClientInfo & info,
const String & user,
const std::optional<String> & 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;

View File

@ -46,7 +46,7 @@ struct SessionLogElement
time_t event_time{};
Decimal64 event_time_microseconds{};
String user;
std::optional<String> user;
std::optional<AuthenticationType> user_identified_with;
String external_auth_server;
Strings roles;
@ -73,7 +73,7 @@ class SessionLog : public SystemLog<SessionLogElement>
public:
void addLoginSuccess(const UUID & auth_id, std::optional<String> 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<String> & user, const Exception & reason);
void addLogOut(const UUID & auth_id, const UserPtr & login_user, const ClientInfo & client_info);
};

View File

@ -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());

View File

@ -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
}