From 14b849489ad60ef146d213bd350bb912b0aa5e84 Mon Sep 17 00:00:00 2001 From: Jitendra Pandey Date: Wed, 15 Jun 2016 11:41:49 -0700 Subject: [PATCH] HADOOP-12291. Add support for nested groups in LdapGroupsMapping. Contributed by Esther Kundin. --- .../hadoop/security/LdapGroupsMapping.java | 114 +++++++++++++++--- .../src/main/resources/core-default.xml | 13 ++ .../security/TestLdapGroupsMapping.java | 62 ++++++++-- .../security/TestLdapGroupsMappingBase.java | 33 ++++- .../TestLdapGroupsMappingWithPosixGroup.java | 2 +- 5 files changed, 198 insertions(+), 26 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 da87369bbb4..5a0b1d9d4c1 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 @@ -25,6 +25,9 @@ import java.util.ArrayList; import java.util.Collections; import java.util.Hashtable; import java.util.List; +import java.util.HashSet; +import java.util.Collection; +import java.util.Set; import javax.naming.Context; import javax.naming.NamingEnumeration; @@ -66,9 +69,11 @@ import org.apache.hadoop.conf.Configuration; * is used for searching users or groups which returns more results than are * allowed by the server, an exception will be thrown. * - * The implementation also does not attempt to resolve group hierarchies. In - * order to be considered a member of a group, the user must be an explicit - * member in LDAP. + * The implementation attempts to resolve group hierarchies, + * to a configurable limit. + * If the limit is 0, in order to be considered a member of a group, + * the user must be an explicit member in LDAP. Otherwise, it will traverse the + * group hierarchy n levels up. */ @InterfaceAudience.LimitedPrivate({"HDFS", "MapReduce"}) @InterfaceStability.Evolving @@ -156,6 +161,13 @@ public class LdapGroupsMapping public static final String GROUP_NAME_ATTR_KEY = LDAP_CONFIG_PREFIX + ".search.attr.group.name"; public static final String GROUP_NAME_ATTR_DEFAULT = "cn"; + /* + * How many levels to traverse when checking for groups in the org hierarchy + */ + public static final String GROUP_HIERARCHY_LEVELS_KEY = + LDAP_CONFIG_PREFIX + ".search.group.hierarchy.levels"; + public static final int GROUP_HIERARCHY_LEVELS_DEFAULT = 0; + /* * LDAP attribute names to use when doing posix-like lookups */ @@ -208,6 +220,7 @@ public class LdapGroupsMapping private String memberOfAttr; private String groupMemberAttr; private String groupNameAttr; + private int groupHierarchyLevels; private String posixUidAttr; private String posixGidAttr; private boolean isPosix; @@ -234,7 +247,7 @@ public class LdapGroupsMapping */ for(int retry = 0; retry < RECONNECT_RETRY_COUNT; retry++) { try { - return doGetGroups(user); + return doGetGroups(user, groupHierarchyLevels); } catch (NamingException e) { LOG.warn("Failed to get groups for user " + user + " (retry=" + retry + ") by " + e); @@ -324,9 +337,11 @@ public class LdapGroupsMapping * @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) + private List lookupGroup(SearchResult result, DirContext c, + int goUpHierarchy) throws NamingException { List groups = new ArrayList(); + Set groupDNs = new HashSet(); NamingEnumeration groupResults = null; // perform the second LDAP query @@ -345,12 +360,14 @@ public class LdapGroupsMapping if (groupResults != null) { while (groupResults.hasMoreElements()) { SearchResult groupResult = groupResults.nextElement(); - Attribute groupName = groupResult.getAttributes().get(groupNameAttr); - if (groupName == null) { - throw new NamingException("The group object does not have " + - "attribute '" + groupNameAttr + "'."); - } - groups.add(groupName.get().toString()); + getGroupNames(groupResult, groups, groupDNs, goUpHierarchy > 0); + } + if (goUpHierarchy > 0 && !isPosix) { + // convert groups to a set to ensure uniqueness + Set groupset = new HashSet(groups); + goUpGroupHierarchy(groupDNs, goUpHierarchy, groupset); + // convert set back to list for compatibility + groups = new ArrayList(groupset); } } return groups; @@ -369,7 +386,8 @@ public class LdapGroupsMapping * return an empty string array. * @throws NamingException if unable to get group names */ - List doGetGroups(String user) throws NamingException { + List doGetGroups(String user, int goUpHierarchy) + throws NamingException { DirContext c = getDirContext(); // Search for the user. We'll only ever need to look at the first result @@ -378,7 +396,7 @@ public class LdapGroupsMapping // return empty list if the user can not be found. if (!results.hasMoreElements()) { if (LOG.isDebugEnabled()) { - LOG.debug("doGetGroups(" + user + ") return no groups because the " + + LOG.debug("doGetGroups(" + user + ") returned no groups because the " + "user is not found."); } return new ArrayList(); @@ -411,15 +429,76 @@ public class LdapGroupsMapping "the second LDAP query using the user's DN.", e); } } - if (groups == null || groups.isEmpty()) { - groups = lookupGroup(result, c); + if (groups == null || groups.isEmpty() || goUpHierarchy > 0) { + groups = lookupGroup(result, c, goUpHierarchy); } if (LOG.isDebugEnabled()) { - LOG.debug("doGetGroups(" + user + ") return " + groups); + LOG.debug("doGetGroups(" + user + ") returned " + groups); } return groups; } + /* Helper function to get group name from search results. + */ + void getGroupNames(SearchResult groupResult, Collection groups, + Collection groupDNs, boolean doGetDNs) + throws NamingException { + Attribute groupName = groupResult.getAttributes().get(groupNameAttr); + if (groupName == null) { + throw new NamingException("The group object does not have " + + "attribute '" + groupNameAttr + "'."); + } + groups.add(groupName.get().toString()); + if (doGetDNs) { + groupDNs.add(groupResult.getNameInNamespace()); + } + } + + /* Implementation for walking up the ldap hierarchy + * This function will iteratively find the super-group memebership of + * groups listed in groupDNs and add them to + * the groups set. It will walk up the hierarchy goUpHierarchy levels. + * Note: This is an expensive operation and settings higher than 1 + * are NOT recommended as they will impact both the speed and + * memory usage of all operations. + * The maximum time for this function will be bounded by the ldap query + * timeout and the number of ldap queries that it will make, which is + * max(Recur Depth in LDAP, goUpHierarcy) * DIRECTORY_SEARCH_TIMEOUT + * + * @param ctx - The context for contacting the ldap server + * @param groupDNs - the distinguished name of the groups whose parents we + * want to look up + * @param goUpHierarchy - the number of levels to go up, + * @param groups - Output variable to store all groups that will be added + */ + void goUpGroupHierarchy(Set groupDNs, + int goUpHierarchy, + Set groups) + throws NamingException { + if (goUpHierarchy <= 0 || groups.isEmpty()) { + return; + } + DirContext context = getDirContext(); + Set nextLevelGroups = new HashSet(); + StringBuilder filter = new StringBuilder(); + filter.append("(&").append(groupSearchFilter).append("(|"); + for (String dn : groupDNs) { + filter.append("(").append(groupMemberAttr).append("=") + .append(dn).append(")"); + } + filter.append("))"); + LOG.debug("Ldap group query string: " + filter.toString()); + NamingEnumeration groupResults = + context.search(baseDN, + filter.toString(), + SEARCH_CONTROLS); + while (groupResults.hasMoreElements()) { + SearchResult groupResult = groupResults.nextElement(); + getGroupNames(groupResult, groups, nextLevelGroups, true); + } + goUpGroupHierarchy(nextLevelGroups, goUpHierarchy - 1, groups); + } + DirContext getDirContext() throws NamingException { if (ctx == null) { // Set up the initial environment for LDAP connectivity @@ -446,7 +525,6 @@ public class LdapGroupsMapping ctx = new InitialDirContext(env); } - return ctx; } @@ -513,6 +591,8 @@ public class LdapGroupsMapping conf.get(GROUP_MEMBERSHIP_ATTR_KEY, GROUP_MEMBERSHIP_ATTR_DEFAULT); groupNameAttr = conf.get(GROUP_NAME_ATTR_KEY, GROUP_NAME_ATTR_DEFAULT); + groupHierarchyLevels = + conf.getInt(GROUP_HIERARCHY_LEVELS_KEY, GROUP_HIERARCHY_LEVELS_DEFAULT); posixUidAttr = conf.get(POSIX_UID_ATTR_KEY, POSIX_UID_ATTR_DEFAULT); posixGidAttr = diff --git a/hadoop-common-project/hadoop-common/src/main/resources/core-default.xml b/hadoop-common-project/hadoop-common/src/main/resources/core-default.xml index 490f1de36c2..8097c0f417b 100644 --- a/hadoop-common-project/hadoop-common/src/main/resources/core-default.xml +++ b/hadoop-common-project/hadoop-common/src/main/resources/core-default.xml @@ -324,6 +324,19 @@ + + hadoop.security.group.mapping.ldap.search.group.hierarchy.levels + 0 + + The number of levels to go up the group hierarchy when determining + which groups a user is part of. 0 Will represent checking just the + group that the user belongs to. Each additional level will raise the + time it takes to exectue a query by at most + hadoop.security.group.mapping.ldap.directory.search.timeout. + The default will usually be appropriate for all LDAP systems. + + + hadoop.security.group.mapping.ldap.posix.attr.uid.name uidNumber diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestLdapGroupsMapping.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestLdapGroupsMapping.java index 9f9f9943cae..131b4e6d928 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestLdapGroupsMapping.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestLdapGroupsMapping.java @@ -39,6 +39,7 @@ import java.net.Socket; import java.util.Arrays; import java.util.List; import java.util.concurrent.CountDownLatch; +import java.util.HashSet; import javax.naming.CommunicationException; import javax.naming.NamingException; @@ -91,8 +92,23 @@ public class TestLdapGroupsMapping extends TestLdapGroupsMappingBase { when(getContext().search(anyString(), anyString(), any(Object[].class), any(SearchControls.class))) .thenReturn(getUserNames(), getGroupNames()); - - doTestGetGroups(Arrays.asList(testGroups), 2); + doTestGetGroups(Arrays.asList(getTestGroups()), 2); + } + + @Test + public void testGetGroupsWithHierarchy() throws IOException, NamingException { + // The search functionality of the mock context is reused, so we will + // return the user NamingEnumeration first, and then the group + // The parent search is run once for each level, and is a different search + // The parent group is returned once for each group, yet the final list + // should be unique + when(getContext().search(anyString(), anyString(), any(Object[].class), + any(SearchControls.class))) + .thenReturn(getUserNames(), getGroupNames()); + when(getContext().search(anyString(), anyString(), + any(SearchControls.class))) + .thenReturn(getParentGroupNames()); + doTestGetGroupsWithParent(Arrays.asList(getTestParentGroups()), 2, 1); } @Test @@ -104,8 +120,10 @@ public class TestLdapGroupsMapping extends TestLdapGroupsMappingBase { .thenThrow(new CommunicationException("Connection is closed")) .thenReturn(getUserNames(), getGroupNames()); - // Although connection is down but after reconnected it still should retrieve the result groups - doTestGetGroups(Arrays.asList(testGroups), 1 + 2); // 1 is the first failure call + // Although connection is down but after reconnected + // it still should retrieve the result groups + // 1 is the first failure call + doTestGetGroups(Arrays.asList(getTestGroups()), 1 + 2); } @Test @@ -139,7 +157,37 @@ public class TestLdapGroupsMapping extends TestLdapGroupsMappingBase { any(Object[].class), any(SearchControls.class)); } - + + private void doTestGetGroupsWithParent(List expectedGroups, + int searchTimesGroup, int searchTimesParentGroup) + throws IOException, NamingException { + Configuration conf = new Configuration(); + // Set this, so we don't throw an exception + conf.set(LdapGroupsMapping.LDAP_URL_KEY, "ldap://test"); + // Set the config to get parents 1 level up + conf.setInt(LdapGroupsMapping.GROUP_HIERARCHY_LEVELS_KEY, 1); + + LdapGroupsMapping groupsMapping = getGroupsMapping(); + groupsMapping.setConf(conf); + // Username is arbitrary, since the spy is mocked to respond the same, + // regardless of input + List groups = groupsMapping.getGroups("some_user"); + + // compare lists, ignoring the order + Assert.assertEquals(new HashSet(expectedGroups), + new HashSet(groups)); + + // We should have searched for a user, and group + verify(getContext(), times(searchTimesGroup)).search(anyString(), + anyString(), + any(Object[].class), + any(SearchControls.class)); + // One groups search for the parent group should have been done + verify(getContext(), times(searchTimesParentGroup)).search(anyString(), + anyString(), + any(SearchControls.class)); + } + @Test public void testExtractPassword() throws IOException { File testDir = GenericTestUtils.getTestDir(); @@ -246,7 +294,7 @@ public class TestLdapGroupsMapping extends TestLdapGroupsMappingBase { mapping.setConf(conf); try { - mapping.doGetGroups("hadoop"); + mapping.doGetGroups("hadoop", 1); fail("The LDAP query should have timed out!"); } catch (NamingException ne) { LOG.debug("Got the exception while LDAP querying: ", ne); @@ -302,7 +350,7 @@ public class TestLdapGroupsMapping extends TestLdapGroupsMappingBase { mapping.setConf(conf); try { - mapping.doGetGroups("hadoop"); + mapping.doGetGroups("hadoop", 1); fail("The LDAP query should have timed out!"); } catch (NamingException ne) { LOG.debug("Got the exception while LDAP querying: ", ne); diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestLdapGroupsMappingBase.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestLdapGroupsMappingBase.java index 75e3bf19e5d..51d36732736 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestLdapGroupsMappingBase.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestLdapGroupsMappingBase.java @@ -46,13 +46,17 @@ public class TestLdapGroupsMappingBase { @Mock private NamingEnumeration groupNames; @Mock + private NamingEnumeration parentGroupNames; + @Mock private SearchResult userSearchResult; @Mock private Attributes attributes; @Spy private LdapGroupsMapping groupsMapping = new LdapGroupsMapping(); - protected String[] testGroups = new String[] {"group1", "group2"}; + private String[] testGroups = new String[] {"group1", "group2"}; + private String[] testParentGroups = + new String[] {"group1", "group2", "group1_1"}; @Before public void setupMocksBase() throws NamingException { @@ -93,6 +97,24 @@ public class TestLdapGroupsMappingBase { thenReturn(getUserSearchResult()); when(getUserSearchResult().getAttributes()).thenReturn(getAttributes()); + // Define results for groups 1 level up + SearchResult parentGroupResult = mock(SearchResult.class); + + // only one parent group + when(parentGroupNames.hasMoreElements()).thenReturn(true, false); + when(parentGroupNames.nextElement()). + thenReturn(parentGroupResult); + + // Define the attribute for the parent group + Attribute parentGroup1Attr = new BasicAttribute("cn"); + parentGroup1Attr.add(testParentGroups[2]); + Attributes parentGroup1Attrs = new BasicAttributes(); + parentGroup1Attrs.put(parentGroup1Attr); + + // attach the attributes to the result + when(parentGroupResult.getAttributes()).thenReturn(parentGroup1Attrs); + when(parentGroupResult.getNameInNamespace()). + thenReturn("CN=some_group,DC=test,DC=com"); } protected DirContext getContext() { @@ -117,4 +139,13 @@ public class TestLdapGroupsMappingBase { protected LdapGroupsMapping getGroupsMapping() { return groupsMapping; } + protected String[] getTestGroups() { + return testGroups; + } + protected NamingEnumeration getParentGroupNames() { + return parentGroupNames; + } + protected String[] getTestParentGroups() { + return testParentGroups; + } } diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestLdapGroupsMappingWithPosixGroup.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestLdapGroupsMappingWithPosixGroup.java index 332eed4283f..17d28a5827c 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestLdapGroupsMappingWithPosixGroup.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestLdapGroupsMappingWithPosixGroup.java @@ -69,7 +69,7 @@ public class TestLdapGroupsMappingWithPosixGroup any(Object[].class), any(SearchControls.class))) .thenReturn(getUserNames(), getGroupNames()); - doTestGetGroups(Arrays.asList(testGroups), 2); + doTestGetGroups(Arrays.asList(getTestGroups()), 2); } private void doTestGetGroups(List expectedGroups, int searchTimes)