This is an automated email from the ASF dual-hosted git repository. csringhofer pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/impala.git
commit 51126259bea2491316decef63d584f713d02a9a6 Author: Tamas Mate <[email protected]> AuthorDate: Fri Aug 5 15:07:23 2022 +0200 IMPALA-11436: Change search bind authentication parameters Impala's search bind authentication intends to mimic Spring's behaviour. However, the login username and user dn paremeters were swapped for group searches compared to Spring. This change intends to align these parameters. For user search, Spring uses {0} to replace the login username. Meanwhile, during group search {0} is used to replace the login user dn and {1} is used to replace the login username. Testing: - Ran LdapSearchBindImpalaShellTest frontend tests Change-Id: I9808566a348f7c6200b0571fbc05e67f720f2075 Reviewed-on: http://gerrit.cloudera.org:8080/18819 Reviewed-by: Impala Public Jenkins <[email protected]> Tested-by: Impala Public Jenkins <[email protected]> --- be/src/util/ldap-search-bind.cc | 27 +++++++++++----------- docs/topics/impala_ldap.xml | 18 ++++++++++----- .../LdapSearchBindImpalaShellTest.java | 8 +++---- 3 files changed, 30 insertions(+), 23 deletions(-) diff --git a/be/src/util/ldap-search-bind.cc b/be/src/util/ldap-search-bind.cc index 9c47965d1..d3c6d9c1d 100644 --- a/be/src/util/ldap-search-bind.cc +++ b/be/src/util/ldap-search-bind.cc @@ -42,11 +42,12 @@ using std::string; namespace impala { // Permitted patterns in the user/group filter -const string USER_NAME_PATTERN = "{0}"; -const string USER_DN_PATTERN = "{1}"; +const string USER_SEARCH_LOGIN_NAME_PATTERN = "{0}"; +const string GROUP_SEARCH_LOGIN_NAME_PATTERN = "{1}"; +const string GROUP_SEARCH_USER_DN_PATTERN = "{0}"; // Default ldap filters const string DEFAULT_USER_FILTER = "(&(objectClass=user)(sAMAccountName={0}))"; -const string DEFAULT_GROUP_FILTER = "(&(objectClass=group)(member={1})"; +const string DEFAULT_GROUP_FILTER = "(&(objectClass=group)(member={0})"; Status LdapSearchBind::ValidateFlags() { RETURN_IF_ERROR(ImpalaLdap::ValidateFlags()); @@ -92,10 +93,10 @@ bool LdapSearchBind::LdapCheckPass(const char* user, const char* pass, unsigned if (!success) return false; VLOG(2) << "LDAP bind successful"; - // Escape special characters and replace the USER_NAME_PATTERN in the filter. + // Escape special characters and replace the USER_SEARCH_LOGIN_NAME_PATTERN. string filter = string(user_filter_); string escaped_user = EscapeFilterProperty(user); - replace_all(filter, USER_NAME_PATTERN, escaped_user); + replace_all(filter, USER_SEARCH_LOGIN_NAME_PATTERN, escaped_user); // Execute the LDAP search and try to retrieve the user dn VLOG(1) << "Trying LDAP user search for: " << user; @@ -134,15 +135,15 @@ bool LdapSearchBind::LdapCheckFilters(string username) { if (!success) return false; VLOG(2) << "LDAP bind successful"; - // Substitute the USER_NAME_PATTERN and USER_DN_PATTERN patterns for the group search. - // USER_DN_PATTERN requires to determine the user dn and therefore an additional LDAP - // search. + // Substitute the USER_SEARCH_LOGIN_NAME_PATTERN and GROUP_SEARCH_USER_DN_PATTERN + // patterns for the group search. This needs an additional LDAP search to determine + // the GROUP_SEARCH_USER_DN_PATTERN. string group_filter = group_filter_; string escaped_username = EscapeFilterProperty(username); - replace_all(group_filter, USER_NAME_PATTERN, escaped_username); - if (group_filter.find(USER_DN_PATTERN) != string::npos) { + replace_all(group_filter, GROUP_SEARCH_LOGIN_NAME_PATTERN, escaped_username); + if (group_filter.find(GROUP_SEARCH_USER_DN_PATTERN) != string::npos) { string user_filter = user_filter_; - replace_all(user_filter, USER_NAME_PATTERN, escaped_username); + replace_all(user_filter, USER_SEARCH_LOGIN_NAME_PATTERN, escaped_username); vector<string> user_dns = LdapSearchObject(ld, FLAGS_ldap_user_search_basedn.c_str(), user_filter.c_str()); if (user_dns.size() != 1) { @@ -153,9 +154,9 @@ bool LdapSearchBind::LdapCheckFilters(string username) { ldap_unbind_ext(ld, nullptr, nullptr); return false; } - // Escape the characters in the the DN then replace the USER_DN_PATTERN. + // Escape the characters in the the DN then replace the GROUP_SEARCH_DN_PATTERN. string escaped_user_dn = EscapeFilterProperty(user_dns[0]); - replace_all(group_filter, USER_DN_PATTERN, escaped_user_dn); + replace_all(group_filter, GROUP_SEARCH_USER_DN_PATTERN, escaped_user_dn); } // Execute LDAP search for the group diff --git a/docs/topics/impala_ldap.xml b/docs/topics/impala_ldap.xml index e4d651a7e..6bac3614f 100644 --- a/docs/topics/impala_ldap.xml +++ b/docs/topics/impala_ldap.xml @@ -323,7 +323,7 @@ under the License. <dd> <p> - A comma separated list of user names. If specified, users must be + A comma separated list of usernames. If specified, users must be on this list for authentication to succeed </p> </dd> @@ -459,7 +459,7 @@ under the License. <p> LDAP filter that will be used during LDAP search, it can contain <codeph>{0}</codeph> pattern which will be replaced with the - user name. The default value is <codeph> + username. The default value is <codeph> (&(objectClass=user)(sAMAccountName={0}))</codeph>. </p> </dd> @@ -475,11 +475,17 @@ under the License. <dd> <p> LDAP filter that will be used during LDAP group search, it can - contain <codeph>{0}</codeph> pattern which will be replaced with - the user name and/or <codeph>{1}</codeph> which will be replaced + contain <codeph>{1}</codeph> pattern which will be replaced with + the username and/or <codeph>{0}</codeph> which will be replaced with the user DN. The default value is <codeph> - (&(objectClass=group)(member={1})</codeph>. + (&(objectClass=group)(member={0})</codeph>. </p> + <note> + The behaviour of this flag has been changed between Impala 4.1 and Impala 4.2 + to comply with Spring LDAP. Previously <codeph>{0}</codeph> was for username + and <codeph>{1}</codeph> for user dn, these paramters were swapped, now + <codeph>{0}</codeph> is for user dn and <codeph>{1}</codeph> is for username. + </note> </dd> </dlentry> @@ -617,7 +623,7 @@ under the License. Sets the user. Per Active Directory, the user is the short username, not the full LDAP distinguished name. If your LDAP settings include a search base, use the <codeph>--ldap_bind_pattern</codeph> on the <cmdname>impalad</cmdname> daemon to - translate the short user name from <cmdname>impala-shell</cmdname> automatically + translate the short username from <cmdname>impala-shell</cmdname> automatically to the fully qualified name. </p> </dd> diff --git a/fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java b/fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java index b92a8f9bf..38d74540c 100644 --- a/fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java +++ b/fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java @@ -71,7 +71,7 @@ public class LdapSearchBindImpalaShellTest extends LdapImpalaShellTest { setUp(String.format("--ldap_user_search_basedn=dc=myorg,dc=com " + "--ldap_group_search_basedn=ou=Groups,dc=myorg,dc=com " + "--ldap_user_filter=(&(objectClass=person)(cn={0})(!(cn=%s))) " - + "--ldap_group_filter=(uniqueMember={1})", + + "--ldap_group_filter=(uniqueMember={0})", TEST_USER_2)); testLdapFiltersImpl(); } @@ -89,7 +89,7 @@ public class LdapSearchBindImpalaShellTest extends LdapImpalaShellTest { setUp(String.format("--ldap_user_search_basedn=dc=myorg,dc=com " + "--ldap_group_search_basedn=ou=Groups,dc=myorg,dc=com " + "--ldap_user_filter=(&(objectClass=person)(cn={0})(!(cn=%s))) " - + "--ldap_group_filter=(&(cn=%s)(uniqueMember={1}))", + + "--ldap_group_filter=(&(cn=%s)(uniqueMember={0}))", TEST_USER_2, TEST_USER_GROUP)); testLdapFiltersImpl(); } @@ -107,7 +107,7 @@ public class LdapSearchBindImpalaShellTest extends LdapImpalaShellTest { setUp(String.format("--ldap_user_search_basedn=dc=myorg,dc=com " + "--ldap_group_search_basedn=ou=Groups,dc=myorg,dc=com " + "--ldap_user_filter=(&(objectClass=person)(cn={0})(!(cn=Test2Ldap))) " - + "--ldap_group_filter=(&(cn=group1)(uniqueMember={1})) " + + "--ldap_group_filter=(&(cn=group1)(uniqueMember={0})) " + "--authorized_proxy_user_config=%s=* ", TEST_USER_4)); testLdapFiltersWithProxyImpl(); @@ -145,7 +145,7 @@ public class LdapSearchBindImpalaShellTest extends LdapImpalaShellTest { setUp("--ldap_user_search_basedn=dc=myorg,dc=com " + "--ldap_group_search_basedn=ou=Groups,dc=myorg,dc=com " + "--ldap_user_filter=(cn={0}) " - + "--ldap_group_filter=(uniqueMember={1}) "); + + "--ldap_group_filter=(uniqueMember={0}) "); String query = "select logged_in_user()"; // Authentications should succeed with user who has escaped character in its DN
