Rename {username} to {user_name}

Add caching/checking of search_params
Adjust comments/doc
Use special authentication logic from ExternalAuthenticators::checkLDAPCredentials

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 @@
                 <my_ldap_server>
                     <host>localhost</host>
                     <port>636</port>
-                    <bind_dn>uid={username},ou=users,dc=example,dc=com</bind_dn>
+                    <bind_dn>uid={user_name},ou=users,dc=example,dc=com</bind_dn>
                     <verification_cooldown>300</verification_cooldown>
                     <enable_tls>yes</enable_tls>
                     <tls_minimum_protocol_version>tls1.2</tls_minimum_protocol_version>
@@ -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 @@
                     </roles>
                     <role_mapping>
                         <base_dn>ou=groups,dc=example,dc=com</base_dn>
-                        <attribute>cn</attribute>
                         <scope>subtree</scope>
                         <search_filter>(&amp;(objectClass=groupOfNames)(member={bind_dn}))</search_filter>
+                        <attribute>cn</attribute>
                         <prefix>clickhouse_</prefix>
                     </role_mapping>
                 </ldap>

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);
-}
-
 }

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<LDAPServerParams> 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;

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<String, LDAPCacheEntry>; // user name   -> cache entry

src/Access/LDAPAccessStorage.cpp

@@ -1,5 +1,6 @@
 #include <Access/LDAPAccessStorage.h>
 #include <Access/AccessControlManager.h>
+#include <Access/ExternalAuthenticators.h>
 #include <Access/User.h>
 #include <Access/Role.h>
 #include <Access/LDAPClient.h>
@@ -364,9 +365,10 @@ std::set<String> 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<User>(*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))

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<String> 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;

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<char *>(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<char *>(search_params.attribute.c_str()), nullptr };
     ::timeval timeout = { params.search_timeout.count(), 0 };
     LDAPMessage* msgs = nullptr;

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<int>(scope));
+        boost::hash_combine(seed, search_filter);
+        boost::hash_combine(seed, attribute);
+        boost::hash_combine(seed, prefix);
+    }
 };
 
 using LDAPSearchParamsList = std::vector<LDAPSearchParams>;
@@ -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;
     }
 };
 

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):