SEC-1823: User roles in AD should handle nested groups #2053

Open
spring-issuemaster opened this Issue Sep 21, 2011 · 10 comments

3 participants

@spring-issuemaster

Rick Jensen (Migrated from SEC-1823) said:

With Active Directory (AD), groups can be nested within each other. In fact, in larger organizations, it is quite common to use nested groups to manage users.

The default AD authorities populator only looks at a user's memberOf property to populate the list of granted authorities, using the group name as the authority name. The memberOf property is only populated with groups that the user is directly a member of, and does not include groups that the user is a member of through nesting of groups.

For example, you have the following group structure:

MyApplicationAdmins (group)
Members: DomainAdmins
DomainAdmins (group)
Members: User1
User1 (user)

Currently, the ActiveDirectoryLdapAuthenticationProvider will only populate the user's GrantedAuthorities with DomainAdmins, since that is the only group the user is directly a member of. Instead, the user should have a GrantedAuthorities list that contains both DomainAdmins and MyApplicationAdmins, since the user is in both groups through nesting.

With generic LDAP, there is no way to get nested groups other than by walking the LDAP tree, which requires multiple calls to the LDAP server and is very expensive. With AD however, there is a special matching rule object identifier that will walk a chain of ancestry objects. This page (http://msdn.microsoft.com/en-us/library/aa746475%28VS.85%29.aspx) describes it in more detail.

An example of how to use the filter to get all groups a user is in would be this filter:

(&(objectClass=group)(member:1.2.840.113556.1.4.1941:={0}))

where {0} is the DN of the user.

To enable this, the loadUserAuthorities(...) method of the ActiveDirectoryLdapAuthenticationProvider needs to be modified. Since the full nested group information is not available in the DirContextOperations object, another AD server call is required, but it is only a single call and it fully populates the GrantedAuthorities with all of the groups a user is in, both directly and via nesting.

Here is an example of the updated method: (note: this was put together hastily and likely has issues, but it serves as a concrete starting point)

/**
 * Creates the user authority list from the values of the {@code memberOf} attribute obtained from the user's
 * Active Directory entry in a nested fashion.
 * @throws NamingException 
 */
public Collection<? extends GrantedAuthority> loadUserAuthorities(DirContextOperations userData, String username, String password) throws NamingException {
    SearchControls searchControls = new SearchControls();
    searchControls.setSearchScope(SearchControls.SUBTREE_SCOPE);

    String filter = "(&(objectClass=group)(member:1.2.840.113556.1.4.1941:={0}))";

    final String bindPrincipal = createBindPrincipal(username, domain);
    String searchRoot = rootDn != null ? rootDn : searchRootFromPrincipal(bindPrincipal);


    DirContext ctx = bindAsUser(username, password);

    final DistinguishedName ctxBaseDn = new DistinguishedName(ctx.getNameInNamespace());

    final DistinguishedName searchBaseDn = new DistinguishedName(searchRoot);

    final NamingEnumeration<SearchResult> resultsEnum = ctx.search(searchBaseDn, filter, new Object[]{userData.getDn()}, searchControls);

    if (logger.isDebugEnabled()) {
        logger.debug("Searching for entry under DN '" + ctxBaseDn
                + "', base = '" + searchBaseDn + "', filter = '" + filter + "'");
    }

    ArrayList<GrantedAuthority> authorities = new ArrayList<GrantedAuthority>();
    try {
        while (resultsEnum.hasMore()) {
            SearchResult searchResult = resultsEnum.next();
            // Work out the DN of the matched entry
            DistinguishedName dn = new DistinguishedName(new CompositeName(searchResult.getName()));

            if (searchRoot.length() > 0) {
                dn.prepend(searchBaseDn);
            }

            if (logger.isDebugEnabled()) {
                logger.debug("Found DN: " + dn);
            }

            authorities.add(new SimpleGrantedAuthority(dn.removeLast().getValue()));
        }
    } catch (PartialResultException e) {
        org.springframework.security.ldap.LdapUtils.closeEnumeration(resultsEnum);
        logger.info("Ignoring PartialResultException");
    }

    return authorities;
}
@spring-issuemaster

Maxime Noirjean said:

Yeah, I have same problem.
Your code works perfectly but your code should be integrated in "ActiveDirectoryLdapAuthenticationProvider" class.

After more 6 month, this should already be done.

@spring-issuemaster

Marcel Stör said:

Out customer has the same requirement as far as nested groups are concerned. However, we're not using the AD authentication provider as all requests are pre-authenticated (SSO). We only make calls to AD/LDAP to retrieve user details and authorities using the DefaultLdapAuthoritiesPopulator. Wondering if the approach detailed in this issue can somehow be integrated into DefaultLdapAuthoritiesPopulator...

@spring-issuemaster

Rick Jensen said:

@Maxime - I haven't had time yet, but I intend to push a patch upstream for this at some point.

@Marcel - Yes, covering the scenario where users have already been authenticated and only need permissions loaded is important. I believe this covers that, but I'm open to suggestions if it doesn't.

@spring-issuemaster

Marcel Stör said:

@Rick - not sure if we have a misunderstanding or not ;-)
My point was that in our scenario we're not using the ActiveDirectoryLdapAuthenticationProvider that you patched above. We're using our own PreAuthenticationProvider because, much to our surprise, we weren't able to configure what we need with standard SS classes. I outlined the solution here: http://forum.springsource.org/showthread.php?115826-Combine-pre-authentication-with-LDAP-for-user-details-and-authorities. Hence, if I understand your proposal correctly we'd need to patch the DefaultLdapAuthoritiesPopulator to make it work for us (didn't have time yet to really dig into this)?

@spring-issuemaster

Yann Nicolas said:

This issue has 2 years but I have this exactly same problem. Is it planned to resolve it?
Thanks

@spring-issuemaster

Benne Otten said:

I've stumbled upon thesame problem as outlined in this issue, but considering this issue has been open for almost three years now I am not hopefull it has been resolved yet and might not be resolved anytime soon.
Does anyone have a workaround for this? Any help is greatly appreciated.

@spring-issuemaster

Jean-Pierre Bergamin said:

I use this class here: https://gist.github.com/ractive/258dd06c99d2939781c0
Put it in the package org.springframework.security.ldap.authentication.ad and you should be ready to go...

@spring-issuemaster

Benne Otten said:

@Jean-Pierre, I'm now using the class you mentioned, and it looks promising.
It works, but I haven't had the chance yet to test it thouroughly.

Thanks for the input. ;)

@fpmoles

what is the status on this issue (and the solutions documented) becoming part of the main line. We too have had to implement a work around for what is normal behavior in AD (nesting groups) and would prefer to use "official" code.

@rwinch rwinch added the help-wanted label Aug 10, 2016
@rwinch
Spring member

@fpmoles Thanks for reaching out.

The proposed changes cannot be made in Spring Security because they are not passive. Ideally, we would use a strategy pattern to obtain the roles with a default implementation using the same behavior of the existing method. An additional implementation could then be provided that supports nested groups.

NOTE Simply setting a flag is not a good approach. This makes the code more complex and less flexible than using a strategy pattern.

One interface to look at is the 'LdapAuthoritiesPopulator`. This API might not work entirely due to the fact it doesn't expose the password. However, I haven't looked into this in any details.

If we get a Pull Request that:

  • Is passive
  • Documented
  • Tested
  • Uses a strategy pattern

I would be glad to consider this for Spring Security 4.2.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment