Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SEC-2224: ActiveDirectoryLdapAuthenticationProvider throws BadCredentialsException if userPrincipalName not equal to sAMAccountName + @domain #2448

Closed
spring-issuemaster opened this issue Jul 23, 2013 · 19 comments
Assignees
Milestone

Comments

@spring-issuemaster
Copy link

@spring-issuemaster spring-issuemaster commented Jul 23, 2013

Michael Solano (Migrated from SEC-2224) said:

When using the sAMAccountName for authentication via ActiveDirectoryLdapAuthenticationProvider, a BadCredentialsException will be thrown if the userPrincipalName is not the sAMAccountName with @Domain post-fixed.

For example, if the sAMAccountName is "bwayne" but the userPrincipalName is "bruce.wayne@batcave.net", authentication will fail. The createBindPrincipal method assumes the userPrincipalName will be "bwayne@batcave.net" and not "bruce.wayne@batcave.net".

The code below shows the details of that method:

    String createBindPrincipal(String username) {
        if (domain == null || username.toLowerCase().endsWith(domain)) {
            return username;
        }

        return username + "@" + domain;
    }
@spring-issuemaster
Copy link
Author

@spring-issuemaster spring-issuemaster commented Jul 25, 2013

Vladimir Terzic said:

below is my "fix" for this issue. Changing the searchFilter variable to include sAMAccountName (as an OR option) and passing plain username as an additional search parms is all that needs to change.

private DirContextOperations searchForUser(DirContext ctx, String username) throws NamingException {
        SearchControls searchCtls = new SearchControls();
        searchCtls.setSearchScope(SearchControls.SUBTREE_SCOPE);

        //have to use plain username for sAMAccountName since bind principal adds domain
        String searchFilter = "(&(objectClass=user)(|(userPrincipalName={0})(sAMAccountName={1})))";

        final String bindPrincipal = createBindPrincipal(username);

        String searchRoot = rootDn != null ? rootDn : searchRootFromPrincipal(bindPrincipal);

        return SpringSecurityLdapTemplate.searchForSingleEntryInternal(ctx, searchCtls, searchRoot, searchFilter,
                new Object[]{bindPrincipal, username});
    }
@spring-issuemaster
Copy link
Author

@spring-issuemaster spring-issuemaster commented Jul 26, 2013

Michael Solano said:

Hi Vladimir,

I did something similar as a temporary fix that got it working. I modified the search filter to only use sAMAccounName but had to code in some domain specific logic that, even if generalized, probably wouldn't work for everyone. I appreciate your feedback though!

@spring-issuemaster
Copy link
Author

@spring-issuemaster spring-issuemaster commented Apr 3, 2014

Robert Whitcomb said:

This is still an issue in Spring Security 3.2.3. Is there any chance this issue will be fixed in upcoming releases?

@spring-issuemaster
Copy link
Author

@spring-issuemaster spring-issuemaster commented May 9, 2014

Michael Solano said:

Hi Robert,

I updated the affected versions to include 3.2.3 as well as 3.2.4 (which I'm using now).

I plan on upgrading to version 4 fairly soon and will test it out there as well.

Thanks,
Mike

Edit: Scratch that, I'm still using 3.1.4 (the rest of my spring stuff is on 3.2.4). So, I removed 3.2.4 from the affected versions and kept your 3.2.3.

@spring-issuemaster
Copy link
Author

@spring-issuemaster spring-issuemaster commented Mar 12, 2015

Ryan J. McDonough said:

I have gotten around the issue in 3.2.6 by doing the following:

 ActiveDirectoryLdapAuthenticationProvider provider =
                new ActiveDirectoryLdapAuthenticationProvider(ldapProperties.getDomain(),
                                                              ldapProperties.getUrl());
          provider.setConvertSubErrorCodesToExceptions(true);
          provider.setUseAuthenticationRequestCredentials(true);
          provider.setSearchFilter("(sAMAccountName={0})");
          return provider;

The addition of the filter fixes the issue. Prior to 3.2.6, Spring Security LDAP did something else and upgrading to Spring Boot 1.2.2 (which includes Spring Security 3.2.6) caused all kinds of grief.

@spring-issuemaster
Copy link
Author

@spring-issuemaster spring-issuemaster commented Jul 17, 2015

Rob Winch said:

damnhandy Thanks for the response. Injecting the search filter is the preferred way to handle this as of Spring Security 3.2.7+. I'm closing this issue issue as being superseded by SEC-1915

@gkibilov
Copy link

@gkibilov gkibilov commented Apr 15, 2016

Still an issue using Spring 4.0.3, injecting sAMAccountName filter has no effect as searchForUser still uses only bindPrincipal when calling searchForSingleEntryInternal!

My filter property looks like this:
<property name="searchFilter" value="(&amp;(objectClass=user)(sAMAccountName={0}))" />
and searchForSingleEntryInternal throws IncorrectResultSizeDataAccessException.

Why not pass user name too? Then I could use a filter like this:
<property name="searchFilter" value="(&amp;(objectClass=user)(sAMAccountName={1}))" />

    private DirContextOperations searchForUser(DirContext context, String username)
            throws NamingException {
        SearchControls searchControls = new SearchControls();
        searchControls.setSearchScope(SearchControls.SUBTREE_SCOPE);

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

        try {
            return SpringSecurityLdapTemplate.searchForSingleEntryInternal(context,
                    searchControls, searchRoot, searchFilter,
                    new Object[] { bindPrincipal, userName});
        }

@taasjord
Copy link
Contributor

@taasjord taasjord commented May 18, 2017

This is still an issue. In my case the userPrincipalName has a different suffix than domain, ie. while bindPrincipal is "bwayne@batcave.net", the userPrincipalName is "bwayne@gotham.com" and sAMAccountName is "bwayne".

So I need to pass username to match sAMAccountName instead. My solution was just to copy the entire source of ActiveDirectoryLdapAuthenticationProvider (along with a package private exception) and change bindPrincipal to username.

@adamjhamer
Copy link

@adamjhamer adamjhamer commented Jun 26, 2017

Hi,

I've been looking at the issues of sAMAccountName in search filters and the history of requests and changes to ActiveDirectoryLdapAuthenticationProvider, and I believe this issue can now be closed in favour of using the new rootDn and leaving the domain blank.

Flexibility was added to allow the use of a custom search filters, such as sAMAccountName, through #SEC-1915. A change was also made, #SEC-2987, to change the username to the bindPrincipal which then presented the search with username@domain and not just username. I understand the Pre-Win2k domain name is not part of the sAMAccountName attribute, so it is not correct to supply a domain in the search filter (we also have different domain names on the userPrincipalName to the sAMAccountName as @taasjord above). However a domain is required for the searchRoot (error thrown in ActiveDirectoryLdapAuthenticationProvider.searchRootFromPrincipal) - so we end up with a catch-22 -
hence the workaround @gkibilov provides above.

The latest code though now allows rootDN to be specified for the searchRoot whilst leaving the domain empty. This means we can keep the domain blank which satisfies the filter when using sAMAccountName, but satisfies searchRoot being provided with a rootDn. We copied the latest class in (fbb902c), and specified the rootDn, left the domain blank and its working.

Therefore this issue can be closed again, but I do feel that this position is fragile - so it would be good to allow the username to be also passed in to match the searchFilter as above by @gkibilov:
return SpringSecurityLdapTemplate.searchForSingleEntryInternal(context, searchControls, searchRoot, searchFilter, new Object[] { bindPrincipal, userName});

Thanks
Adam

@adamjhamer
Copy link

@adamjhamer adamjhamer commented Jun 27, 2017

Further to my above comment, it was necessary for us to use a domain in the end because some AD accounts differed between their sAMAccountName and their 'User DN' name (not just the domain!). This meant the username passed in (expecting to match sAMAccountName) wasn't finding them since the user's DN was different. By providing the domain allowed both accounts to be found - a separate rootDn context was not enough.

Being required to provide the domain meant we were back with the problem of the searchFilter using the user@domain format, when sAMAccountName must only have the username. So we modified the latest code to add new Object[] { bindPrincipal, userName}) (as above) which allows the searchFilter to reference the second argument, the username, with (&(objectClass=user)(sAMAccountName={1})).

So whether this is considered a separate issue I'm not sure! Its not a domain mismatch (which is now catered for in the latest code) but a username DN mismatch which was solved by the change of code to new Object[] { bindPrincipal, userName})!

Thanks
Adam

@taasjord
Copy link
Contributor

@taasjord taasjord commented Jul 18, 2017

I just want to note that I did try to use rootDN and blank domain, but that doesn't work for me, because the domain is needed in bindAsUser (but cannot be in searchForUser).

The proposed change to add the necessary flexibility is very small and I don't see what harm it could make.

taasjord pushed a commit to taasjord/spring-security that referenced this issue Jul 18, 2017
Allows the username only (without domain) to be used in custom search filter like "sAMAccountName={1}",
in eg. situations where the userPrincipalName has a different suffix than domain.

Thanks to contributors in issue.

fixes spring-projectsgh-2448
taasjord added a commit to taasjord/spring-security that referenced this issue Jul 18, 2017
Allows the username only (without domain) to be used in custom search filter like "sAMAccountName={1}",
in eg. situations where the userPrincipalName has a different suffix than domain.

Thanks to contributors in issue.

fixes spring-projectsgh-2448
taasjord added a commit to taasjord/spring-security that referenced this issue Jul 18, 2017
Allows the username only (without domain) to be used in custom search filter like "sAMAccountName={1}",
in eg. situations where the userPrincipalName has a different suffix than domain.

Thanks to contributors in issue.

fixes spring-projectsgh-2448
@sidamos
Copy link

@sidamos sidamos commented Aug 25, 2017

I strongly vote for accepting this pull request. We have exactly the same problem.

If at least ActiveDirectoryLdapAuthenticationProvider was not final and the affected method was not private...

@mdeterman
Copy link

@mdeterman mdeterman commented Aug 26, 2017

+1
I'm having the same issue.

@rehmatt
Copy link

@rehmatt rehmatt commented Sep 6, 2017

+1
Same here

@rwinch rwinch self-assigned this Oct 30, 2017
@rwinch rwinch added this to the 5.0.0.RC1 milestone Oct 30, 2017
@rwinch rwinch closed this in 8d717c6 Oct 30, 2017
@shafsongithub
Copy link

@shafsongithub shafsongithub commented Nov 24, 2017

This issue still persists even after new code checked-in in RC.

@taasjord
Copy link
Contributor

@taasjord taasjord commented Nov 27, 2017

@shafsongithub, did you set the search filter to use it?
Eg: provider.setSearchFilter("sAMAccountName={1}");

@shafsongithub
Copy link

@shafsongithub shafsongithub commented Nov 28, 2017

No.. sounds like this will resolve my issue. I will try and let you guys know. BTW When this fix is coming in Release?

@shafsongithub
Copy link

@shafsongithub shafsongithub commented Nov 28, 2017

It worked. Thanks for your timely comment.

thomasdarimont added a commit to thomasdarimont/spring-security that referenced this issue Apr 25, 2018
Allows the username only (without domain) to be used in custom search filter like "sAMAccountName={1}",
in eg. situations where the userPrincipalName has a different suffix than domain.

Thanks to contributors in issue.

fixes spring-projectsgh-2448
@aivantsov
Copy link

@aivantsov aivantsov commented Apr 27, 2018

Would you please integrate/cherry-pick the fix into the 4.2.x branch.
Thanks in advance.

N.B. Currently cannot switch to Spring Boot 2.x which would solve the issue due to a third party lib limitation.

taasjord added a commit to taasjord/spring-security that referenced this issue Apr 29, 2018
Allows the username only (without domain) to be used in custom search filter like "sAMAccountName={1}",
in eg. situations where the userPrincipalName has a different suffix than domain.

Thanks to contributors in issue.

fixes spring-projectsgh-2448

(cherry picked from commit 8d717c6)
rwinch added a commit that referenced this issue May 4, 2018
Allows the username only (without domain) to be used in custom search filter like "sAMAccountName={1}",
in eg. situations where the userPrincipalName has a different suffix than domain.

Thanks to contributors in issue.

fixes gh-2448

(cherry picked from commit 8d717c6)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

10 participants
You can’t perform that action at this time.