diff --git a/programs/server/config.xml b/programs/server/config.xml index 98b1801372b..09a53a6589e 100644 --- a/programs/server/config.xml +++ b/programs/server/config.xml @@ -322,7 +322,7 @@ host - LDAP server hostname or IP, this parameter is mandatory and cannot be empty. port - LDAP server port, default is 636 if enable_tls is set to true, 389 otherwise. bind_dn - template used to construct the DN to bind to. - The resulting DN will be constructed by replacing all '{username}' substrings of the template with the actual + The resulting DN will be constructed by replacing all '{user_name}' substrings of the template with the actual user name during each authentication attempt. verification_cooldown - a period of time, in seconds, after a successful bind attempt, during which a user will be assumed to be successfully authenticated for all consecutive requests without contacting the LDAP server. @@ -344,7 +344,7 @@ localhost 636 - uid={username},ou=users,dc=example,dc=com + uid={user_name},ou=users,dc=example,dc=com 300 yes tls1.2 @@ -377,20 +377,23 @@ If no roles are specified here or assigned during role mapping (below), user will not be able to perform any actions after authentication. role_mapping - section with LDAP search parameters and mapping rules. - The list of strings (values of attributes) returned by the search will be transformed and the resulting strings - will be treated as local role names and assigned to the user. + When a user authenticates, while still bound to LDAP, an LDAP search is performed using search_filter and the + name of the logged in user. For each entry found during that search, the value of the specified attribute is + extracted. For each attribute value that has the specified prefix, the prefix is removed, and the rest of the + value becomes the name of a local role defined in ClickHouse, which is expected to be created beforehand by + CREATE ROLE command. There can be multiple 'role_mapping' sections defined inside the same 'ldap' section. All of them will be applied. base_dn - template used to construct the base DN for the LDAP search. - The resulting DN will be constructed by replacing all '{username}' and '{bind_dn}' substrings + The resulting DN will be constructed by replacing all '{user_name}' and '{bind_dn}' substrings of the template with the actual user name and bind DN during each LDAP search. - attribute - attribute name whose values will be returned by the LDAP search. scope - scope of the LDAP search. Accepted values are: 'base', 'one_level', 'children', 'subtree' (the default). search_filter - template used to construct the search filter for the LDAP search. - The resulting filter will be constructed by replacing all '{username}', '{bind_dn}', and '{base_dn}' + The resulting filter will be constructed by replacing all '{user_name}', '{bind_dn}', and '{base_dn}' substrings of the template with the actual user name, bind DN, and base DN during each LDAP search. Note, that the special characters must be escaped properly in XML. + attribute - attribute name whose values will be returned by the LDAP search. prefix - prefix, that will be expected to be in front of each string in the original list of strings returned by the LDAP search. Prefix will be removed from the original strings and resulting strings will be treated as local role names. Empty, by default. @@ -403,9 +406,9 @@ ou=groups,dc=example,dc=com - cn subtree (&(objectClass=groupOfNames)(member={bind_dn})) + cn clickhouse_ diff --git a/src/Access/Authentication.cpp b/src/Access/Authentication.cpp index 3b67563f6eb..19c40c068b4 100644 --- a/src/Access/Authentication.cpp +++ b/src/Access/Authentication.cpp @@ -88,17 +88,4 @@ bool Authentication::isCorrectPassword(const String & user_, const String & pass throw Exception("Cannot check if the password is correct for authentication type " + toString(type), ErrorCodes::NOT_IMPLEMENTED); } -bool Authentication::isCorrectPasswordLDAP(const String & password_, const String & user_, const ExternalAuthenticators & external_authenticators, const LDAPSearchParamsList * search_params, LDAPSearchResultsList * search_results) const -{ - if (type != LDAP_SERVER) - throw Exception("Cannot check if the password is correct using LDAP logic for authentication type " + toString(type), ErrorCodes::BAD_ARGUMENTS); - - auto ldap_server_params = external_authenticators.getLDAPServerParams(server_name); - ldap_server_params.user = user_; - ldap_server_params.password = password_; - - LDAPSimpleAuthClient ldap_client(ldap_server_params); - return ldap_client.authenticate(search_params, search_results); -} - } diff --git a/src/Access/ExternalAuthenticators.cpp b/src/Access/ExternalAuthenticators.cpp index e13341ff3f9..6f66f4303e1 100644 --- a/src/Access/ExternalAuthenticators.cpp +++ b/src/Access/ExternalAuthenticators.cpp @@ -63,7 +63,7 @@ auto parseLDAPServer(const Poco::Util::AbstractConfiguration & config, const Str { const auto auth_dn_prefix = config.getString(ldap_server_config + ".auth_dn_prefix"); const auto auth_dn_suffix = config.getString(ldap_server_config + ".auth_dn_suffix"); - params.bind_dn = auth_dn_prefix + "{username}" + auth_dn_suffix; + params.bind_dn = auth_dn_prefix + "{user_name}" + auth_dn_suffix; } if (has_verification_cooldown) @@ -177,7 +177,8 @@ void ExternalAuthenticators::setConfiguration(const Poco::Util::AbstractConfigur } } -bool ExternalAuthenticators::checkLDAPCredentials(const String & server, const String & user_name, const String & password) const +bool ExternalAuthenticators::checkLDAPCredentials(const String & server, const String & user_name, const String & password, + const LDAPSearchParamsList * search_params, LDAPSearchResultsList * search_results) const { std::optional params; std::size_t params_hash = 0; @@ -193,7 +194,15 @@ bool ExternalAuthenticators::checkLDAPCredentials(const String & server, const S params = pit->second; params->user = user_name; params->password = password; - params_hash = params->getCoreHash(); + + params->combineCoreHash(params_hash); + if (search_params) + { + for (const auto & params_instance : *search_params) + { + params_instance.combineHash(params_hash); + } + } // Check the cache, but only if the caching is enabled at all. if (params->verification_cooldown > std::chrono::seconds{0}) @@ -217,9 +226,19 @@ bool ExternalAuthenticators::checkLDAPCredentials(const String & server, const S // Check if we can safely "reuse" the result of the previous successful password verification. entry.last_successful_params_hash == params_hash && last_check_period >= std::chrono::seconds{0} && - last_check_period <= params->verification_cooldown + last_check_period <= params->verification_cooldown && + + // Ensure that search_params are compatible. + ( + search_params == nullptr ? + entry.last_successful_search_results.empty() : + search_params->size() == entry.last_successful_search_results.size() + ) ) { + if (search_results) + *search_results = entry.last_successful_search_results; + return true; } @@ -236,7 +255,7 @@ bool ExternalAuthenticators::checkLDAPCredentials(const String & server, const S } LDAPSimpleAuthClient client(params.value()); - const auto result = client.check(); + const auto result = client.authenticate(search_params, search_results); const auto current_check_timestamp = std::chrono::steady_clock::now(); // Update the cache, but only if this is the latest check and the server is still configured in a compatible way. @@ -253,8 +272,18 @@ bool ExternalAuthenticators::checkLDAPCredentials(const String & server, const S new_params.user = user_name; new_params.password = password; + std::size_t new_params_hash = 0; + new_params.combineCoreHash(new_params_hash); + if (search_params) + { + for (const auto & params_instance : *search_params) + { + params_instance.combineHash(new_params_hash); + } + } + // If the critical server params have changed while we were checking the password, we discard the current result. - if (params_hash != new_params.getCoreHash()) + if (params_hash != new_params_hash) return false; auto & entry = ldap_server_caches[server][user_name]; @@ -262,8 +291,20 @@ bool ExternalAuthenticators::checkLDAPCredentials(const String & server, const S { entry.last_successful_params_hash = params_hash; entry.last_successful_authentication_timestamp = current_check_timestamp; + + if (search_results) + entry.last_successful_search_results = *search_results; + else + entry.last_successful_search_results.clear(); } - else if (entry.last_successful_params_hash != params_hash) + else if ( + entry.last_successful_params_hash != params_hash || + ( + search_params == nullptr ? + !entry.last_successful_search_results.empty() : + search_params->size() != entry.last_successful_search_results.size() + ) + ) { // Somehow a newer check with different params/password succeeded, so the current result is obsolete and we discard it. return false; diff --git a/src/Access/ExternalAuthenticators.h b/src/Access/ExternalAuthenticators.h index fa618c92b3f..abcc8e8d10d 100644 --- a/src/Access/ExternalAuthenticators.h +++ b/src/Access/ExternalAuthenticators.h @@ -28,13 +28,15 @@ class ExternalAuthenticators public: void reset(); void setConfiguration(const Poco::Util::AbstractConfiguration & config, Poco::Logger * log); - bool checkLDAPCredentials(const String & server, const String & user_name, const String & password) const; + bool checkLDAPCredentials(const String & server, const String & user_name, const String & password, + const LDAPSearchParamsList * search_params = nullptr, LDAPSearchResultsList * search_results = nullptr) const; private: struct LDAPCacheEntry { std::size_t last_successful_params_hash = 0; std::chrono::steady_clock::time_point last_successful_authentication_timestamp; + LDAPSearchResultsList last_successful_search_results; }; using LDAPServerCache = std::unordered_map; // user name -> cache entry diff --git a/src/Access/LDAPAccessStorage.cpp b/src/Access/LDAPAccessStorage.cpp index a787c704999..2602422a59a 100644 --- a/src/Access/LDAPAccessStorage.cpp +++ b/src/Access/LDAPAccessStorage.cpp @@ -1,5 +1,6 @@ #include #include +#include #include #include #include @@ -364,9 +365,10 @@ std::set LDAPAccessStorage::mapExternalRolesNoLock(const LDAPSearchResul } -bool LDAPAccessStorage::isPasswordCorrectLDAPNoLock(const User & user, const String & password, const ExternalAuthenticators & external_authenticators, LDAPSearchResultsList & search_results) const +bool LDAPAccessStorage::isPasswordCorrectLDAPNoLock(const String & user_name, const String & password, + const ExternalAuthenticators & external_authenticators, LDAPSearchResultsList & search_results) const { - return user.authentication.isCorrectPasswordLDAP(password, user.getName(), external_authenticators, &role_search_params, &search_results); + return external_authenticators.checkLDAPCredentials(ldap_server, user_name, password, &role_search_params, &search_results); } @@ -521,7 +523,7 @@ UUID LDAPAccessStorage::loginImpl(const String & user_name, const String & passw { auto user = memory_storage.read(*id); - if (!isPasswordCorrectLDAPNoLock(*user, password, external_authenticators, external_roles)) + if (!isPasswordCorrectLDAPNoLock(user->getName(), password, external_authenticators, external_roles)) throwInvalidPassword(); if (!isAddressAllowedImpl(*user, address)) @@ -540,7 +542,7 @@ UUID LDAPAccessStorage::loginImpl(const String & user_name, const String & passw user->authentication = Authentication(Authentication::Type::LDAP_SERVER); user->authentication.setServerName(ldap_server); - if (!isPasswordCorrectLDAPNoLock(*user, password, external_authenticators, external_roles)) + if (!isPasswordCorrectLDAPNoLock(user->getName(), password, external_authenticators, external_roles)) throwInvalidPassword(); if (!isAddressAllowedImpl(*user, address)) diff --git a/src/Access/LDAPAccessStorage.h b/src/Access/LDAPAccessStorage.h index cce50fd03aa..b3d82d1e86b 100644 --- a/src/Access/LDAPAccessStorage.h +++ b/src/Access/LDAPAccessStorage.h @@ -69,7 +69,8 @@ private: void assignRolesNoLock(User & user, const LDAPSearchResultsList & external_roles, const std::size_t external_roles_hash) const; void updateAssignedRolesNoLock(const UUID & id, const String & user_name, const LDAPSearchResultsList & external_roles) const; std::set mapExternalRolesNoLock(const LDAPSearchResultsList & external_roles) const; - bool isPasswordCorrectLDAPNoLock(const User & user, const String & password, const ExternalAuthenticators & external_authenticators, LDAPSearchResultsList & search_results) const; + bool isPasswordCorrectLDAPNoLock(const String & user_name, const String & password, + const ExternalAuthenticators & external_authenticators, LDAPSearchResultsList & search_results) const; mutable std::recursive_mutex mutex; AccessControlManager * access_control_manager = nullptr; diff --git a/src/Access/LDAPClient.cpp b/src/Access/LDAPClient.cpp index cba74fbbb89..41756aebb9a 100644 --- a/src/Access/LDAPClient.cpp +++ b/src/Access/LDAPClient.cpp @@ -271,8 +271,8 @@ void LDAPClient::openConnection() { case LDAPServerParams::SASLMechanism::SIMPLE: { - const auto escaped_username = escapeForLDAP(params.user); - const auto bind_dn = replacePlaceholders(params.bind_dn, { {"{username}", escaped_username} }); + const auto escaped_user_name = escapeForLDAP(params.user); + const auto bind_dn = replacePlaceholders(params.bind_dn, { {"{user_name}", escaped_user_name} }); ::berval cred; cred.bv_val = const_cast(params.password.c_str()); @@ -314,10 +314,10 @@ LDAPSearchResults LDAPClient::search(const LDAPSearchParams & search_params) case LDAPSearchParams::Scope::CHILDREN: scope = LDAP_SCOPE_CHILDREN; break; } - const auto escaped_username = escapeForLDAP(params.user); - const auto bind_dn = replacePlaceholders(params.bind_dn, { {"{username}", escaped_username} }); - const auto base_dn = replacePlaceholders(search_params.base_dn, { {"{username}", escaped_username}, {"{bind_dn}", bind_dn} }); - const auto search_filter = replacePlaceholders(search_params.search_filter, { {"{username}", escaped_username}, {"{bind_dn}", bind_dn}, {"{base_dn}", base_dn} }); + const auto escaped_user_name = escapeForLDAP(params.user); + const auto bind_dn = replacePlaceholders(params.bind_dn, { {"{user_name}", escaped_user_name} }); + const auto base_dn = replacePlaceholders(search_params.base_dn, { {"{user_name}", escaped_user_name}, {"{bind_dn}", bind_dn} }); + const auto search_filter = replacePlaceholders(search_params.search_filter, { {"{user_name}", escaped_user_name}, {"{bind_dn}", bind_dn}, {"{base_dn}", base_dn} }); char * attrs[] = { const_cast(search_params.attribute.c_str()), nullptr }; ::timeval timeout = { params.search_timeout.count(), 0 }; LDAPMessage* msgs = nullptr; diff --git a/src/Access/LDAPParams.h b/src/Access/LDAPParams.h index 426e81719bc..5181b2d1621 100644 --- a/src/Access/LDAPParams.h +++ b/src/Access/LDAPParams.h @@ -23,10 +23,19 @@ struct LDAPSearchParams }; String base_dn; + Scope scope = Scope::SUBTREE; String search_filter; String attribute = "cn"; - Scope scope = Scope::SUBTREE; String prefix; + + void combineHash(std::size_t & seed) const + { + boost::hash_combine(seed, base_dn); + boost::hash_combine(seed, static_cast(scope)); + boost::hash_combine(seed, search_filter); + boost::hash_combine(seed, attribute); + boost::hash_combine(seed, prefix); + } }; using LDAPSearchParamsList = std::vector; @@ -98,18 +107,13 @@ struct LDAPServerParams std::chrono::seconds search_timeout{20}; std::uint32_t search_limit = 100; - std::size_t getCoreHash() const + void combineCoreHash(std::size_t & seed) const { - std::size_t seed = 0; - boost::hash_combine(seed, host); boost::hash_combine(seed, port); - boost::hash_combine(seed, auth_dn_prefix); - boost::hash_combine(seed, auth_dn_suffix); + boost::hash_combine(seed, bind_dn); boost::hash_combine(seed, user); boost::hash_combine(seed, password); - - return seed; } }; diff --git a/tests/testflows/ldap/external_user_directory/tests/common.py b/tests/testflows/ldap/external_user_directory/tests/common.py index 6d8a97e8611..e1ee4f99545 100644 --- a/tests/testflows/ldap/external_user_directory/tests/common.py +++ b/tests/testflows/ldap/external_user_directory/tests/common.py @@ -77,7 +77,7 @@ def verify_ldap_user_exists(server, username, password): with By("searching LDAP database"): ldap_node = current().context.cluster.node(server) r = ldap_node.command( - f"ldapwhoami -H ldap://localhost -D 'cn={username},ou=users,dc=company,dc=com' -w {password}") + f"ldapwhoami -H ldap://localhost -D 'cn={user_name},ou=users,dc=company,dc=com' -w {password}") assert r.exitcode == 0, error() def create_ldap_external_user_directory_config_content(server=None, roles=None, **kwargs):