From 5b62ea4cbca6ef02d5f16b0de81a2a6b9950e0a2 Mon Sep 17 00:00:00 2001 From: lmccay Date: Mon, 11 Jul 2022 01:03:44 -0400 Subject: [PATCH 1/6] =?UTF-8?q?HADOOP-18074=20-=20Partial/Incomplete=20gro?= =?UTF-8?q?ups=20list=20can=20be=20returned=20in=20LDAP=E2=80=A6=20(#4503)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../hadoop/security/LdapGroupsMapping.java | 6 +- .../TestLdapGroupsMappingWithOneQuery.java | 100 +++++++++++++++--- 2 files changed, 93 insertions(+), 13 deletions(-) diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/LdapGroupsMapping.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/LdapGroupsMapping.java index e4d6225fe87d5..0adb5b5125792 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/LdapGroupsMapping.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/LdapGroupsMapping.java @@ -58,6 +58,7 @@ import javax.net.ssl.TrustManager; import javax.net.ssl.TrustManagerFactory; +import org.apache.hadoop.classification.VisibleForTesting; import org.apache.hadoop.thirdparty.com.google.common.collect.Iterators; import org.apache.hadoop.classification.InterfaceAudience; import org.apache.hadoop.classification.InterfaceStability; @@ -458,7 +459,8 @@ private NamingEnumeration lookupPosixGroup(SearchResult result, * @return a list of strings representing group names of the user. * @throws NamingException if unable to find group names */ - private List lookupGroup(SearchResult result, DirContext c, + @VisibleForTesting + Set lookupGroup(SearchResult result, DirContext c, int goUpHierarchy) throws NamingException { List groups = new ArrayList<>(); @@ -544,6 +546,8 @@ List doGetGroups(String user, int goUpHierarchy) } } catch (NamingException e) { // If the first lookup failed, fall back to the typical scenario. + // In order to force the fallback, we need to reset groups collection. + groups.clear(); LOG.info("Failed to get groups from the first lookup. Initiating " + "the second LDAP query using the user's DN.", e); } diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestLdapGroupsMappingWithOneQuery.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestLdapGroupsMappingWithOneQuery.java index 7ae802e26d36d..8686d5c6e3b46 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestLdapGroupsMappingWithOneQuery.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestLdapGroupsMappingWithOneQuery.java @@ -18,19 +18,22 @@ package org.apache.hadoop.security; +import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import java.util.Set; import javax.naming.NamingEnumeration; import javax.naming.NamingException; import javax.naming.directory.Attribute; +import javax.naming.directory.DirContext; import javax.naming.directory.SearchControls; import javax.naming.directory.SearchResult; import org.apache.hadoop.conf.Configuration; import org.junit.Assert; -import org.junit.Before; import org.junit.Test; +import org.mockito.stubbing.Stubber; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; @@ -49,48 +52,121 @@ public class TestLdapGroupsMappingWithOneQuery extends TestLdapGroupsMappingBase { - @Before - public void setupMocks() throws NamingException { + public void setupMocks(List listOfDNs) throws NamingException { Attribute groupDN = mock(Attribute.class); NamingEnumeration groupNames = getGroupNames(); doReturn(groupNames).when(groupDN).getAll(); - String groupName1 = "CN=abc,DC=foo,DC=bar,DC=com"; - String groupName2 = "CN=xyz,DC=foo,DC=bar,DC=com"; - String groupName3 = "CN=sss,CN=foo,DC=bar,DC=com"; - doReturn(groupName1).doReturn(groupName2).doReturn(groupName3). - when(groupNames).next(); - when(groupNames.hasMore()).thenReturn(true).thenReturn(true). - thenReturn(true).thenReturn(false); + buildListOfGroupDNs(listOfDNs).when(groupNames).next(); + when(groupNames.hasMore()). + thenReturn(true).thenReturn(true). + thenReturn(true).thenReturn(false); when(getAttributes().get(eq("memberOf"))).thenReturn(groupDN); } + /** + * Build and return a list of individually added group DNs such + * that calls to .next() will result in a single value each time. + * + * @param listOfDNs + * @return the stubber to use for the .when().next() call + */ + private Stubber buildListOfGroupDNs(List listOfDNs) { + Stubber stubber = null; + for (String s : listOfDNs) { + if (stubber != null) { + stubber.doReturn(s); + } else { + stubber = doReturn(s); + } + } + return stubber; + } + @Test public void testGetGroups() throws NamingException { // given a user whose ldap query returns a user object with three "memberOf" // properties, return an array of strings representing its groups. String[] testGroups = new String[] {"abc", "xyz", "sss"}; doTestGetGroups(Arrays.asList(testGroups)); + + // test fallback triggered by NamingException + doTestGetGroupsWithFallback(); } private void doTestGetGroups(List expectedGroups) throws NamingException { + List groupDns = new ArrayList<>(); + groupDns.add("CN=abc,DC=foo,DC=bar,DC=com"); + groupDns.add("CN=xyz,DC=foo,DC=bar,DC=com"); + groupDns.add("CN=sss,DC=foo,DC=bar,DC=com"); + + setupMocks(groupDns); String ldapUrl = "ldap://test"; Configuration conf = getBaseConf(ldapUrl); // enable single-query lookup conf.set(LdapGroupsMapping.MEMBEROF_ATTR_KEY, "memberOf"); - LdapGroupsMapping groupsMapping = getGroupsMapping(); + TestLdapGroupsMapping groupsMapping = new TestLdapGroupsMapping(); groupsMapping.setConf(conf); // Username is arbitrary, since the spy is mocked to respond the same, // regardless of input List groups = groupsMapping.getGroups("some_user"); Assert.assertEquals(expectedGroups, groups); + Assert.assertFalse("Second LDAP query should NOT have been called.", + groupsMapping.isSecondaryQueryCalled()); // We should have only made one query because single-query lookup is enabled verify(getContext(), times(1)).search(anyString(), anyString(), any(Object[].class), any(SearchControls.class)); } -} \ No newline at end of file + + private void doTestGetGroupsWithFallback() + throws NamingException { + List groupDns = new ArrayList<>(); + groupDns.add("CN=abc,DC=foo,DC=bar,DC=com"); + groupDns.add("CN=xyz,DC=foo,DC=bar,DC=com"); + groupDns.add("ipaUniqueID=e4a9a634-bb24-11ec-aec1-06ede52b5fe1," + + "CN=sudo,DC=foo,DC=bar,DC=com"); + setupMocks(groupDns); + String ldapUrl = "ldap://test"; + Configuration conf = getBaseConf(ldapUrl); + // enable single-query lookup + conf.set(LdapGroupsMapping.MEMBEROF_ATTR_KEY, "memberOf"); + conf.set(LdapGroupsMapping.LDAP_NUM_ATTEMPTS_KEY, "1"); + + TestLdapGroupsMapping groupsMapping = new TestLdapGroupsMapping(); + groupsMapping.setConf(conf); + // Username is arbitrary, since the spy is mocked to respond the same, + // regardless of input + List groups = groupsMapping.getGroups("some_user"); + + // expected to be empty due to invalid memberOf + Assert.assertEquals(0, groups.size()); + + // expect secondary query to be called: getGroups() + Assert.assertTrue("Second LDAP query should have been called.", + groupsMapping.isSecondaryQueryCalled()); + + // We should have fallen back to the second query because first threw + // NamingException expected count is 3 since testGetGroups calls + // doTestGetGroups and doTestGetGroupsWithFallback in succession and + // the count is across both test scenarios. + verify(getContext(), times(3)).search(anyString(), anyString(), + any(Object[].class), any(SearchControls.class)); + } + + private static final class TestLdapGroupsMapping extends LdapGroupsMapping { + private boolean secondaryQueryCalled = false; + public boolean isSecondaryQueryCalled() { + return secondaryQueryCalled; + } + Set lookupGroup(SearchResult result, DirContext c, + int goUpHierarchy) throws NamingException { + secondaryQueryCalled = true; + return super.lookupGroup(result, c, goUpHierarchy); + } + } +} From e84edd9dba696fe529688dcd5cebd61d310eb349 Mon Sep 17 00:00:00 2001 From: Larry McCay Date: Tue, 12 Jul 2022 12:24:14 -0400 Subject: [PATCH 2/6] HADOOP-18074 - fix compliation problem on branch-3.3 --- .../main/java/org/apache/hadoop/security/LdapGroupsMapping.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/LdapGroupsMapping.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/LdapGroupsMapping.java index 0adb5b5125792..07ce0de70c23c 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/LdapGroupsMapping.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/LdapGroupsMapping.java @@ -460,7 +460,7 @@ private NamingEnumeration lookupPosixGroup(SearchResult result, * @throws NamingException if unable to find group names */ @VisibleForTesting - Set lookupGroup(SearchResult result, DirContext c, + List lookupGroup(SearchResult result, DirContext c, int goUpHierarchy) throws NamingException { List groups = new ArrayList<>(); From 25abfacebce19b07594abe6af9b77eb1868e972a Mon Sep 17 00:00:00 2001 From: Larry McCay Date: Tue, 12 Jul 2022 14:26:09 -0400 Subject: [PATCH 3/6] HADOOP-18074 - address test compilation problem on branch-3.3 --- .../hadoop/security/TestLdapGroupsMappingWithOneQuery.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestLdapGroupsMappingWithOneQuery.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestLdapGroupsMappingWithOneQuery.java index 8686d5c6e3b46..40df401386ce0 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestLdapGroupsMappingWithOneQuery.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestLdapGroupsMappingWithOneQuery.java @@ -163,7 +163,7 @@ private static final class TestLdapGroupsMapping extends LdapGroupsMapping { public boolean isSecondaryQueryCalled() { return secondaryQueryCalled; } - Set lookupGroup(SearchResult result, DirContext c, + List lookupGroup(SearchResult result, DirContext c, int goUpHierarchy) throws NamingException { secondaryQueryCalled = true; return super.lookupGroup(result, c, goUpHierarchy); From 92bfea890ad34c6d2ffc1e6833f15fab34a481dd Mon Sep 17 00:00:00 2001 From: Larry McCay Date: Tue, 12 Jul 2022 21:10:20 -0400 Subject: [PATCH 4/6] HADOOP-18074 - fix spotbug for NPE on branch-3.3 --- .../java/org/apache/hadoop/security/LdapGroupsMapping.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/LdapGroupsMapping.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/LdapGroupsMapping.java index 07ce0de70c23c..0627ee4cfe7c1 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/LdapGroupsMapping.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/LdapGroupsMapping.java @@ -547,7 +547,9 @@ List doGetGroups(String user, int goUpHierarchy) } catch (NamingException e) { // If the first lookup failed, fall back to the typical scenario. // In order to force the fallback, we need to reset groups collection. - groups.clear(); + if (groups != null) { + groups.clear(); + } LOG.info("Failed to get groups from the first lookup. Initiating " + "the second LDAP query using the user's DN.", e); } From 22ebd4e5ef15cdc5064414cd70118b77a5d24009 Mon Sep 17 00:00:00 2001 From: Larry McCay Date: Wed, 13 Jul 2022 20:58:02 -0400 Subject: [PATCH 5/6] HADOOP-18074 - clean up of unnecessary null variable --- .../org/apache/hadoop/security/LdapGroupsMapping.java | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/LdapGroupsMapping.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/LdapGroupsMapping.java index 0627ee4cfe7c1..e751ed6a3730f 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/LdapGroupsMapping.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/LdapGroupsMapping.java @@ -512,6 +512,7 @@ List lookupGroup(SearchResult result, DirContext c, List doGetGroups(String user, int goUpHierarchy) throws NamingException { DirContext c = getDirContext(); + List groups = new ArrayList<>(); // Search for the user. We'll only ever need to look at the first result NamingEnumeration results = c.search(userbaseDN, @@ -520,11 +521,10 @@ List doGetGroups(String user, int goUpHierarchy) if (!results.hasMoreElements()) { LOG.debug("doGetGroups({}) returned no groups because the " + "user is not found.", user); - return new ArrayList<>(); + return groups; } SearchResult result = results.nextElement(); - List groups = null; if (useOneQuery) { try { /** @@ -538,7 +538,6 @@ List doGetGroups(String user, int goUpHierarchy) memberOfAttr + "' attribute." + "Returned user object: " + result.toString()); } - groups = new ArrayList<>(); NamingEnumeration groupEnumeration = groupDNAttr.getAll(); while (groupEnumeration.hasMore()) { String groupDN = groupEnumeration.next().toString(); @@ -547,14 +546,12 @@ List doGetGroups(String user, int goUpHierarchy) } catch (NamingException e) { // If the first lookup failed, fall back to the typical scenario. // In order to force the fallback, we need to reset groups collection. - if (groups != null) { - groups.clear(); - } + groups.clear(); LOG.info("Failed to get groups from the first lookup. Initiating " + "the second LDAP query using the user's DN.", e); } } - if (groups == null || groups.isEmpty() || goUpHierarchy > 0) { + if (groups.isEmpty() || goUpHierarchy > 0) { groups = lookupGroup(result, c, goUpHierarchy); } LOG.debug("doGetGroups({}) returned {}", user, groups); From e03105bdafdd061db7d69ed2e3ac2bc5bb2fc0c0 Mon Sep 17 00:00:00 2001 From: Larry McCay Date: Thu, 14 Jul 2022 00:45:09 -0400 Subject: [PATCH 6/6] HADOOP-18074 - fix checkstyle unused import for branch-3.3 --- .../hadoop/security/TestLdapGroupsMappingWithOneQuery.java | 1 - 1 file changed, 1 deletion(-) diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestLdapGroupsMappingWithOneQuery.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestLdapGroupsMappingWithOneQuery.java index 40df401386ce0..c86f1768b7f01 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestLdapGroupsMappingWithOneQuery.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestLdapGroupsMappingWithOneQuery.java @@ -21,7 +21,6 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.List; -import java.util.Set; import javax.naming.NamingEnumeration; import javax.naming.NamingException;