ActiveDirectoryLdapAuthenticationProvider does not implement support for multiple urls

[setu9760]

Summary

The javadoc for ActiveDirectoryLdapAuthenticationProvider constructors says the param url supports multiple URLs. The javadoc however does not define how the multiple URLs needs to be supplied (i.e delimiter specification). Upon trying to supply this param as multiple URLs as a comma or pipe separated list this string is used as is during the ldap binding

Actual Behavior

By looking at the code, specifically the constructors of ActiveDirectoryLdapAuthenticationProvider and method bindAsUser it uses the url provided in the constructor as is without checking if multiple URLs are present. Due to this the the line env.put(Context.PROVIDER_URL, bindUrl); in bindAsUser method will inject the list of URLs without splitting in the env which will be incorrect and the ldap bind does not work.

Expected Behavior

The code should check if the supplied param url is a delimited list or URLs and then store it as such. During the authentication if an ldap server is unavailable the remaining URLs should be tried. The javadoc/code should also make it clear that if multiple URLs are to be supplied what format they should be in or there should be an additional constructor/setter that accepts list instead of a string.

Configuration

@Configuration
public class WebSecurityConfig extends WebSecurityConfigurerAdapter {

@Value("${ldap.url:ldap://localhost:389}") private String url;
@Value("${ldap.domain:domain}") private String domain;

@Override
protected void configure(HttpSecurity http) throws Exception
{
    http.authorizeRequests()
          .anyRequest().fullyAuthenticated()
          .httpBasic();
}

@Override
public void configure(AuthenticationManagerBuilder auth) throws Exception
{
    ActiveDirectoryLdapAuthenticationProvider adProvider = 
                new ActiveDirectoryLdapAuthenticationProvider(domain,url);
    adProvider.setConvertSubErrorCodesToExceptions(true);
    adProvider.setUseAuthenticationRequestCredentials(true);
    auth.authenticationProvider(adProvider);
    auth.eraseCredentials(false);
 }
}

Version

spring-security-ldap 4.2.10 and 5.1.5.RELEASE

Sample

[setu9760]

Happy to open a PR for this if someone from spring team can confirm how they would like this to be fixed? Below are the three suggestions I could think of;

• Keep the current constructor as is and provide and additional constructor which takes list of strings for url field. This maintains backwards compatibility.
• Only keep the current constructor but split the url parameter value by a delimiter probably comma or pipe character. This also maintains backwards compatibility however need to be careful when choosing delimiter character and make sure the url themselves do not contain the delimiter character. Otherwise this could result in invalid urls being split
• Replace current constructor with the one that takes list of string for url field. This breaks backwards compatibility.

In all of the three suggestions the field url type will be changed from String to list of String. The bindUser method will also be changed loop through the list of urls if contextFactory.createContext(env) throws javax.naming.CommunicationException. If all the urls from the list are tried and all of them throws javax.naming.CommunicationException then the doAuthentication method will throw BadCredentialsException. For any other exception the current behaviour will remain.

[tvirtualw]

@setu9760 Did you try to pass in a space-separated list, e.g. "ldaps://dc1.mycompany.com:636 ldaps://dc2.mycompany.com:636"?
This worked for me. At least I did not get any error during startup and usage.

Based on https://docs.oracle.com/javase/jndi/tutorial/ldap/misc/url.html:

Instead of just one URL, you can also supply a space-separated list of URLs. In this case, the LDAP provider will attempt to use each URL in turn until it is able to create a successful connection. The LDAP provider will then set the Context.PROVIDER_URL property to the successful URL, so that the application can determine which URL is being used.

[Avec112]

I can confirm that @tvirtualw is correct regarding space-separated list. I have just now tested it. I configured two working ldaps urls, and then started to block them out in my hosts file both at the same time and one at the time. It all worked as expected. Note! there is a little lag time when blocking addresses in hosts file on Windows. So LDAP might be reached/not reached if you do not give it some time to "settle".

However this should probably be better documented and maybe comma separated list aka array/list is a more natural way to handle multiple values than space-separated seen from/with "Spring eyes".

[jzheaux]

The Javadoc today says:

@param url an LDAP url (or multiple URLs)

This could change to "(or multiple space-delimited URLs)". The Javadoc could also link to https://docs.oracle.com/javase/jndi/tutorial/ldap/misc/url.html to clarify where the contract comes from.

I hesitate to take a collection as that would bump the number of constructors from 2 to 4. It also deviates from the other APIs in Spring Security LDAP that take a space-delimited set of URLs as a parameter. Either way, that should be a discussion for another ticket since it will probably involve more components than just the authentication provider.

Can someone submit a PR that updates the Javadoc?

[Haarolean]

Got a bit puzzled by the fact that AbstractContextSource, on the other hand, has both setUrls and setUrl methods.

Raised a PR #15409 to update the javadocs.