From 5590adeffa27324269e339f3c758bc32516bb35e Mon Sep 17 00:00:00 2001 From: Alexander Tokmakov Date: Tue, 17 May 2022 22:53:17 +0200 Subject: [PATCH] fix more trash --- src/Interpreters/Session.cpp | 14 ++++++----- src/Interpreters/Session.h | 2 +- src/Parsers/Access/ParserUserNameWithHost.cpp | 11 ++++++++- src/Server/TCPHandler.cpp | 6 ++--- tests/config/config.d/session_log.xml | 7 ++++++ tests/config/install.sh | 1 + .../0_stateless/01119_session_log.reference | 13 ++++++++++ .../queries/0_stateless/01119_session_log.sql | 24 +++++++++++++++++++ .../0_stateless/02242_delete_user_race.sh | 6 ++--- 9 files changed, 69 insertions(+), 15 deletions(-) create mode 100644 tests/config/config.d/session_log.xml create mode 100644 tests/queries/0_stateless/01119_session_log.reference create mode 100644 tests/queries/0_stateless/01119_session_log.sql diff --git a/src/Interpreters/Session.cpp b/src/Interpreters/Session.cpp index 65b4f496e88..8cc263132b2 100644 --- a/src/Interpreters/Session.cpp +++ b/src/Interpreters/Session.cpp @@ -319,10 +319,7 @@ void Session::authenticate(const Credentials & credentials_, const Poco::Net::So } catch (const Exception & e) { - onAuthenticationFailure(credentials_, e); - LOG_DEBUG(log, "{} Authentication failed with error: {}", toString(auth_id), e.what()); - if (auto session_log = getSessionLog()) - session_log->addLoginFailure(auth_id, *prepared_client_info, credentials_.getUserName(), e); + onAuthenticationFailure(credentials_, address, e); throw; } @@ -337,11 +334,16 @@ void Session::authenticateInterserverFake() is_internal_interserver_query = true; } -void Session::onAuthenticationFailure(const Credentials & credentials_, const Exception & e) +void Session::onAuthenticationFailure(const Credentials & credentials_, 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()) - session_log->addLoginFailure(auth_id, *prepared_client_info, credentials_.getUserName(), e); + { + /// 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); + } } ClientInfo & Session::getClientInfo() diff --git a/src/Interpreters/Session.h b/src/Interpreters/Session.h index 6c130a06cac..030d3f740ed 100644 --- a/src/Interpreters/Session.h +++ b/src/Interpreters/Session.h @@ -55,7 +55,7 @@ public: void authenticateInterserverFake(); /// Writes a row about login failure into session log (if enabled) - void onAuthenticationFailure(const Credentials & credentials_, const Exception & e); + void onAuthenticationFailure(const Credentials & credentials_, const Poco::Net::SocketAddress & address_, const Exception & e); /// Returns a reference to session ClientInfo. ClientInfo & getClientInfo(); diff --git a/src/Parsers/Access/ParserUserNameWithHost.cpp b/src/Parsers/Access/ParserUserNameWithHost.cpp index c9c655fecc4..2fd112647d1 100644 --- a/src/Parsers/Access/ParserUserNameWithHost.cpp +++ b/src/Parsers/Access/ParserUserNameWithHost.cpp @@ -8,6 +8,12 @@ namespace DB { + +namespace ErrorCodes +{ + extern const int BAD_ARGUMENTS; +} + namespace { bool parseUserNameWithHost(IParserBase::Pos & pos, Expected & expected, std::shared_ptr & ast) @@ -18,7 +24,10 @@ namespace if (!parseIdentifierOrStringLiteral(pos, expected, base_name)) return false; - boost::algorithm::trim(base_name); + /// Previously username was silently trimmed. Now we throw an exception instead. + /// But it's not clear why spaces were not allowed. + if (base_name.empty() || base_name.starts_with(' ') || base_name.ends_with(' ')) + throw Exception(ErrorCodes::BAD_ARGUMENTS, "User name cannot start or end with spaces and cannot be empty"); String host_pattern; if (ParserToken{TokenType::At}.ignore(pos, expected)) diff --git a/src/Server/TCPHandler.cpp b/src/Server/TCPHandler.cpp index 6c0e3cd5125..afe507f2293 100644 --- a/src/Server/TCPHandler.cpp +++ b/src/Server/TCPHandler.cpp @@ -1277,7 +1277,7 @@ void TCPHandler::receiveQuery() if (salt.empty()) { auto exception = Exception(ErrorCodes::AUTHENTICATION_FAILED, "Interserver authentication failed"); - session->onAuthenticationFailure(AlwaysAllowCredentials{USER_INTERSERVER_MARKER}, exception); + session->onAuthenticationFailure(AlwaysAllowCredentials{USER_INTERSERVER_MARKER}, client_info.initial_address, exception); throw exception; } @@ -1294,7 +1294,7 @@ void TCPHandler::receiveQuery() if (calculated_hash != received_hash) { auto exception = Exception(ErrorCodes::AUTHENTICATION_FAILED, "Interserver authentication failed"); - session->onAuthenticationFailure(AlwaysAllowCredentials{USER_INTERSERVER_MARKER}, exception); + session->onAuthenticationFailure(AlwaysAllowCredentials{USER_INTERSERVER_MARKER}, client_info.initial_address, exception); throw exception; } @@ -1312,7 +1312,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}, exception); + session->onAuthenticationFailure(AlwaysAllowCredentials{USER_INTERSERVER_MARKER}, client_info.initial_address, exception); throw exception; #endif } diff --git a/tests/config/config.d/session_log.xml b/tests/config/config.d/session_log.xml new file mode 100644 index 00000000000..b3db05580d6 --- /dev/null +++ b/tests/config/config.d/session_log.xml @@ -0,0 +1,7 @@ + + + system + session_log
+ 100000 + + \ No newline at end of file diff --git a/tests/config/install.sh b/tests/config/install.sh index 3bc1967ab1f..d6f91b499af 100755 --- a/tests/config/install.sh +++ b/tests/config/install.sh @@ -44,6 +44,7 @@ ln -sf $SRC_PATH/config.d/logger.xml $DEST_SERVER_PATH/config.d/ ln -sf $SRC_PATH/config.d/named_collection.xml $DEST_SERVER_PATH/config.d/ ln -sf $SRC_PATH/config.d/ssl_certs.xml $DEST_SERVER_PATH/config.d/ ln -sf $SRC_PATH/config.d/filesystem_cache_log.xml $DEST_SERVER_PATH/config.d/ +ln -sf $SRC_PATH/config.d/session_log.xml $DEST_SERVER_PATH/config.d/ ln -sf $SRC_PATH/users.d/log_queries.xml $DEST_SERVER_PATH/users.d/ ln -sf $SRC_PATH/users.d/readonly.xml $DEST_SERVER_PATH/users.d/ diff --git a/tests/queries/0_stateless/01119_session_log.reference b/tests/queries/0_stateless/01119_session_log.reference new file mode 100644 index 00000000000..bd38ba7c934 --- /dev/null +++ b/tests/queries/0_stateless/01119_session_log.reference @@ -0,0 +1,13 @@ +0 +1 +LoginFailure NO_PASSWORD 1 1 TCP +LoginFailure NO_PASSWORD 1 1 HTTP +LoginFailure INTERSERVER SECRET NO_PASSWORD 1 1 HTTP +LoginFailure default NO_PASSWORD 1 1 TCP +LoginFailure default NO_PASSWORD 1 1 HTTP +LoginFailure nonexistsnt_user_1119 NO_PASSWORD 1 1 TCP +LoginFailure nonexistsnt_user_1119 NO_PASSWORD 1 1 HTTP +LoginSuccess default PLAINTEXT_PASSWORD 1 1 TCP +LoginSuccess default PLAINTEXT_PASSWORD 1 1 HTTP +Logout default NO_PASSWORD 1 1 TCP +Logout default NO_PASSWORD 1 1 HTTP diff --git a/tests/queries/0_stateless/01119_session_log.sql b/tests/queries/0_stateless/01119_session_log.sql new file mode 100644 index 00000000000..64723fa1033 --- /dev/null +++ b/tests/queries/0_stateless/01119_session_log.sql @@ -0,0 +1,24 @@ +create user " "; -- { clientError BAD_ARGUMENTS } +create user ' spaces'; -- { clientError BAD_ARGUMENTS } +create user 'spaces '; -- { clientError BAD_ARGUMENTS } +create user ` INTERSERVER SECRET `; -- { clientError BAD_ARGUMENTS } +create user ''; -- { clientError BAD_ARGUMENTS } +alter user default rename to " spaces "; -- { clientError BAD_ARGUMENTS } +alter user default rename to ''; -- { clientError BAD_ARGUMENTS } + +select * from remote('127.0.0.2', system, one, 'default', ''); +select * from remote('127.0.0.2', system, one, 'default', 'wrong password'); -- { serverError AUTHENTICATION_FAILED } +select * from remote('127.0.0.2', system, one, 'nonexistsnt_user_1119', ''); -- { serverError AUTHENTICATION_FAILED } +set receive_timeout=1; +select * from remote('127.0.0.2', system, one, ' INTERSERVER SECRET ', ''); -- { serverError NO_REMOTE_SHARD_AVAILABLE } +set receive_timeout=300; +select * from remote('127.0.0.2', system, one, ' ', ''); -- { serverError AUTHENTICATION_FAILED } + +select * from url('http://127.0.0.1:8123/?query=select+1&user=default', LineAsString, 's String'); +select * from url('http://127.0.0.1:8123/?query=select+1&user=default&password=wrong', LineAsString, 's String'); -- { serverError RECEIVED_ERROR_FROM_REMOTE_IO_SERVER } +select * from url('http://127.0.0.1:8123/?query=select+1&user=nonexistsnt_user_1119', LineAsString, 's String'); -- { serverError RECEIVED_ERROR_FROM_REMOTE_IO_SERVER } +select * from url('http://127.0.0.1:8123/?query=select+1&user=+INTERSERVER+SECRET+', LineAsString, 's String'); -- { serverError RECEIVED_ERROR_FROM_REMOTE_IO_SERVER } +select * from url('http://127.0.0.1:8123/?query=select+1&user=+++', LineAsString, 's String'); -- { serverError RECEIVED_ERROR_FROM_REMOTE_IO_SERVER } + +system flush logs; +select distinct type, user, auth_type, toString(client_address)!='::ffff:0.0.0.0', client_port!=0, interface from system.session_log where event_time >= now() - interval 5 minute order by type, user, interface; diff --git a/tests/queries/0_stateless/02242_delete_user_race.sh b/tests/queries/0_stateless/02242_delete_user_race.sh index 66cab7500e9..ad5f2c5d46c 100755 --- a/tests/queries/0_stateless/02242_delete_user_race.sh +++ b/tests/queries/0_stateless/02242_delete_user_race.sh @@ -56,7 +56,5 @@ done wait -# $CLICKHOUSE_CLIENT -q "DROP ROLE IF EXISTS test_role_02242" -# $CLICKHOUSE_CLIENT -q "DROP USER IF EXISTS test_user_02242" - -# wait +$CLICKHOUSE_CLIENT -q "DROP ROLE IF EXISTS test_role_02242" +$CLICKHOUSE_CLIENT -q "DROP USER IF EXISTS test_user_02242"