spring-projects / spring-security

Spring Security
http://spring.io/projects/spring-security
Apache License 2.0
8.77k stars 5.89k forks source link

ActiveDirectoryLdapAuthenticationProvider not working with custom searchFilter since domain is added to username #3960

Open maecki-maecki opened 8 years ago

maecki-maecki commented 8 years ago

Summary

Since SEC-1915 the ActiveDirectoryLdapAuthenticationProvider provides means to insert a custom searchFilter to select which user is taken from Active Directory.

The example given is searching by 'sAMAccountName' instead of 'userPrincipalName' but even this example is not working in real Active Directories.

Actual Behavior

There are two steps involved: 1.) Binding to the Active Directory Server: Here the domain Part is added to the given Username to bind. This works like a charm 2.) Finding the User in the Directory to Fill in all userDetails. This does not work at the moment, since the domain Part is also added to the username before feeding it to the given searchFilter (e.g. (sAMAccountName={0}). The User is not found since sAMAccountnames do not contain domain information. The authentication fails.

There is a test within the project, but it uses a mock that is not modeled to copy real AD behaviour: it always finds the sAMAccountName whether provided with or without the domain part ...

Expected Behavior

The ActiveDirectoryLdapAuthenticationProvider should use the domain Part ONLY for binding to the directory, for queryiing the username should be used AS-IS.

Expected behaviour is that (sAMAccountName={0}) searchFilter works against real Active Directory

Configuration

AD Server Windows 2013

Version

Verified in 3.2.9.RELEASE, Code looks the same in Current 4.x versions

Sample

AbstractLdapAuthenticationProvider ad = AbstractLdapAuthenticationProvider('domain.com','ldaps://active-directory');
ad.setSearchFilter("(sAMAccountName={0})");
m1ndwalker commented 7 years ago

I don't think commit 69ec327 from @timkingman fixes this particular issue.

In the topic created by the OP you don't want to append the domain, but rather remove it. The sAMAccountName (pre Windows 2000) attribute in the AD does not contain the domain

Let's say you have a pre-Windows 2000 domain: MYDOMAIN The modern domain is: mydomain.com

When you authenticate with the pre-Windows 2000 username token: MYDOMAIN\someuser and you've specified the search filter (sAMAccountName={0}) you want to match someuser and not MYDOMAIN\someuser for the sAMAccountName

thcase commented 7 years ago

Has there been any effort on this issue. I am running into the same issue with the automatic appending of the domain to the username. In reviewing the closed issues, issue 3114 (https://github.com/spring-projects/spring-security/issues/3114) appears to have changed the behavior to its current state. In that issue, a user states some assumptions that aren't always true. One is that the sAMAccountName equals the first part of the UserPrincipalName. This doesn't have to be true. In my organization's case, the first part of the UserPrincipalName matches the first part of the Email Address. The sAMAccountName is a shortername that has no relationship to email first part. The second assumption is that the second part of the UserPrincipalName matches the Active Directory Domain. This is also not the case in my organization. The second part of the User Principal name matches the second part of the email address, but the actual Active Directory Name is a three part internal network name (i.e., subdomain with the last two parts being an alternative domain that we use for internal network resources which is different than email domain).

Therefore, to properly connect to active directory by either sAMAccountName or UserPrincipalName, one should do the following: (a) if format of username is without @domainname part, assume it is sAMAccountName and strip domain\ from username, if present, or (b) if @domainnamepart is present assume it is UserPrincipalName and use as is. The searchFilter should default to objectClass=user and either UserPrincipalName={username} or sAMAccountName={username}. There should be no automatic appending of @domainname to username. That should be user provided. One has to provide that when authenticating to Azure AD (Microsoft's OATH service), which uses the UserPrincipalName. So Windows users should be getting used to providing.

A different option is simply to remove the automatic appending of the @domainname to the username (i.e., correct the misunderstanding presented in Issue 3114 that caused the change in the first place). One can always adjust the SearchFilter through a property set at runtime and in it auto-append the domain name.

irmac commented 7 years ago

Hi @thcase - I hit this as an issue as well.

I'm happy to contribute a fix, have asked on the spring-security gitter if anyone is actively working on this but haven't got a response yet. I'll hold off if you are planning on working on it, but if you're not, I'll start working on it..let me know what you think.

thcase commented 6 years ago

@irmac Sorry for delay in getting back to you. I haven't had the time to look into a fix yet. If you wanted to tackle, please feel free to pursue. I notice one additional problems with the assumed behavior. That is in our domain the suffix after @ part of UPN is not consistent. Sometimes it is the actual Active Directory domain, other times it is the Email Suffix, which follows our external domain name. So, I think the fix is to include a flag or not to append the domain name. One can already override the search filter and include looking for sAMAccount instead that way.

codeislyric commented 6 years ago

@irmac Is there any update on this issue?

dopsun commented 6 years ago

I am facing same issue. The assumption that userPrincialName always containing full domain name breaks in AD enterprise setup I'm dealing now.

In my case, my domain is int.example.com, but userPricinalName in AD is user@example.com (without int). And since ActiveDirectoryLdapAuthenticationProvider always assume full domain (in createBindPrincipal), then the search always fail.

If a set method putting a value to overwrite behavior of createBindPrincipal would work for us.

For time being, I'm copying ActiveDirectoryLdapAuthenticationProvider and make my own version with filters and bind principal as needed. Would be great if this can be enhanced.

irmac commented 6 years ago

@thcase @codeislyric Apologies have had no time to look at this issue, but going to attempt to get a fix out now as it still appears to be a problem.

taasjord commented 6 years ago

See if this works for you: https://github.com/spring-projects/spring-security/pull/5271

konopka commented 6 months ago

Hi, The current implementation makes wrong assumptions that sAMAccountName and userPrincipal are similar as stated above by other people, and I have encountered this as well ... It all works only if the provided username (with optionally appended domain) matches userPrincipalName in AD, because current implementation tries to immediately login with provided credentials, before searching for the user.

Universal way to make this work requires "admin" account that can read from AD, and this approach should look something like this:

    // Login using "admin" credentials
    adminCtx = bindAsUser(adminDn, adminPassword);
    // Search for user with provided filter
    DirContextOperations user = searchForUser(adminCtx, username);
    // Now login as user using distinguishedName of the user found in the previous step
    userCtx = bindAsUser(user.getNameInNamespace(), password);

I can make a PR, but I need to know whether it is desired to keep current behavior, or it can be completely replaced with this new one.

BTW. this issue looks like a possible duplicate of https://github.com/spring-projects/spring-security/issues/3204