8000 [ldap][2.8] Optional search for primary DN before bind by markussc · Pull Request #17813 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[ldap][2.8] Optional search for primary DN before bind #17813

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

Closed
wants to merge 7 commits into from

Conversation

markussc
Copy link
Q A
Bug fix? [no]
New feature? [no]
BC breaks? [no]
Deprecations? [no]
Tests pass? [yes]
Fixed tickets []
License MIT
Doc PR []

Problem

LDAP bind operation allows only primary Distinguished Name (DN). If this is set to the username (UID), then it is easy to compose the full DN. If on the other hand, the LDAP admin decided to use e.g. the display name as part of the primary DN, then it is not possible to compose the full DN by just having the username and password.

Solution

Retrieve the full DN from the LDAP directory using the information we have and then use this for the bind operation.

Implementation

Search query is separated by ";" from the rest of the dn.
Example:
Primary DN with a priori unknown CN: CN=Harry Potter,DC=example,DC=com
Query DN with known user id: UID={username};DC=example,DC=com

May be used e.g. if DN does not contain username.
@fabpot
Copy link
Member
fabpot commented Feb 15, 2016

ping @csarrazi

@xabbuh
Copy link
Member
xabbuh commented Feb 15, 2016

In any case, the file permission changes must be reverted.

@csarrazi
Copy link
Contributor

I'm -1 on this one.

The problem here is that the bind() method will break if the admin did not enable anonymous search.

The logic should be moved instead in the LdapUserProvider, whose logic is actually to search for users before actually binding to the Ldap server.

< 10000 div class="d-flex">

@markussc
Copy link
Author

@csarrazi : Understand your point. But what should we do if e.g. FOSUserBundle is used as user provider and ldap is only needed for authentication? This is exacly my situation..
It seems to me that the "search DN before bind" logic should therefore be placed inside the LdapBindAuthenticationProvider.

@csarrazi
Copy link
Contributor

Sorry, my bad, it should indeed be in the LdapBindAuthenticationProvider.

@markussc
Copy link
Author

Just moved it in there with latest commit b79e663. This now requires that the LdapClient service gets some additional parameters passed in:

app.ldap:
        class: Symfony\Component\Ldap\LdapClient
        arguments: [ "%ldap_host%", "%ldap_port%", "%ldap_version%", "%ldap_use_ssl%", "%ldap_use_start_tls%", "%ldap_opt_referrals%", "%ldap_base_dn%", "%ldap_search_dn%", "%ldap_search_password%", "%ldap_uid_key%", "%ldap_filter%" ]

@@ -30,6 +31,7 @@ class LdapClient implements LdapClientInterface
private $useStartTls;
private $optReferrals;
private $connection;
private $charmaps;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this field. It is not used.

@csarrazi
Copy link
Contributor

Also, this is a new feature. It should not be made from the 2.8 branch, but from the master branch.

@@ -41,7 +43,7 @@ class LdapClient implements LdapClientInterface
* @param bool $useStartTls
* @param bool $optReferrals
*/
public function __construct($host = null, $port = 389, $version = 3, $useSsl = false, $useStartTls = false, $optReferrals = false)
public function __construct($host = null, $port = 389, $version = 3, $useSsl = false, $useStartTls = false, $optReferrals = false, $ldapBaseDn = null, $ldapSearchDn = null, $ldapSearchPassword = null, $ldapUidKey = null, $ldapFilter = null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you add these fields? They are not even used...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@csarrazi : these fields are passed to the LdapUserProvider inside the LdapBindAuthenticationProvider. This is required in the cases where another than the LdapUserProvider is used for all other purposes than authentication (as described above). Refer to line 87 in LdapBindAuthenticationProvider.php

@csarrazi
Copy link
Contributor

Just a little question. Are you trying to use the Ldap component as a standalone library? Or from a Symfony application, using the Security component?

If your intent is to implement this on the Ldap component (standalone), then the code is wrong, as the Ldap component should not rely on the Security component.

Also, I'm against implementing this feature directly in the Ldap/LdapClient class, as this may break many Ldap configurations. Indeed, one may not have a server authorizing for anonymous bind, simple bind (using UID), or the ability to search entries without password. So in this case, this PR will break the Ldap component. In this case, you first need to bind() to the server using the search_dn and search_password, running the search, and then bind() once again, with the correct DN.

If you wish to make a search prior to running a Ldap bind(), the only class that needs refactoring is the LdapAuthenticationProvider class. However, this means that you will need to duplicate the search logic from the LdapUserProvider, which is also wrong.

Also, as mentioned before, this is a new feature and, therefore, it should not be based (and merged) on the 2.8 branch, but the master (3.1) branch instead.

What we can do, though, since 3.1, is to use an Ldap Query (Symfony\Component\Ldap\Adapter\QueryInterface) in order to share the Ldap search, and use the same result. Thus, we could reuse the same query for both the user provider and the authentication provider.

@markussc
Copy link
Author
markussc commented Mar 1, 2016

This is to be used inside a Symfony application, using the Security component.
Actually the implementation is not inside the LdapClient class, but in the LdapBindAuthenticationProvider (lines 82-93). Only the parameters are passed in via the LdapClient, which is not very nice but I don't see an alternative for it.

From my point of view this is compatible both with Ldap configurations using anonymous / non-anonymous bind, since if search_dn and search_password have been specified the search is made using the search-credentials. Otherwise, direct bind with credentials from the login form is made.

Since in the 3.1 branch it seems to be possible to use another aproach (QueryInterface), I think we should maybe close this discussion. Since I am still bound to the 2.8 branch (large project, migration takes a lot of time) I will not be able to implement this feature for 3.1 in the near future. However, since this is really required for most Ldap setups with standard Windows Active Directory servers, I would really appreciate if it could be included in the roadmap.

@csarrazi
Copy link
Contributor
csarrazi commented Mar 4, 2016

@markussc It could be included in the roadmap, but for this, we need to be sure that this is a standard case for Active Directory, and not some configuration, specific to your own servers.

Just saying, as I could successfully authenticate on an AD server using the code as is, simply by overriding the dn_string parameter from the authentication provider for a form like: {username}@MYDOMAIN, which works perfectly.

@markussc
Copy link
Author
markussc commented Mar 7, 2016
8000

I do not know how the "standard" DN scheme looks when configuring an AD server (I never did that myself). I just found that the example configuration by Microsofts uses exact the scheme which is used in the customer system I am supposed to bind to (refer to https://msdn.microsoft.com/en-us/library/windows/desktop/aa366101%28v=vs.85%29.aspx) : DN="CN={{displayName}}, ..." Unfortunately, this makes it impossible to use the {username}@mydomain scheme. My assumption (and the answer from the AD responsible regarding the composition of DN names ) is, that by default, Microsoft uses the display name in the DN.
For my own test-system, I setup a openLdap server, where I used the username in the DN. There, your suggestion works perfectly.

@csarrazi
Copy link
Contributor
csarrazi commented Mar 9, 2016

Did you try using UID={username} in the dn_string? Or something like UID={username},DC=MYDOMAIN,DC=COM?

@csarrazi
Copy link
Contributor
csarrazi commented Mar 9, 2016

Regardless, this is indeed a new feature. We could implement the bind using different strategies:

  • Simple bind
  • Search bind
  • etc.

@fabpot Should we create a new issue for this, and close this PR?

@fabpot
Copy link
Member
fabpot commented Mar 9, 2016

Sure, I'm closing this PR and I let you create the issue.

@fabpot fabpot closed this Mar 9, 2016
@markussc
Copy link
Author
markussc commented Mar 9, 2016

@csarrazi : yes, tried all the combinations. The point is, that for a bind, the DN used at creation of the entry MUST be used. Windows AD denies all alternative DNs. Since they used the "displayName" in the DN, I am stuck...
Thanks for taking this feature to the agenda! I'll be happy to assist, at least with testing!

@csarrazi
Copy link
Contributor
csarrazi commented Mar 9, 2016

What about samAccountName={username}?

It is weird that you still have an authentication issue, as from the official Microsoft documentation, AD tries to find the user using different strategies, one after the other: https://msdn.microsoft.com/en-us/library/cc223499.aspx

Regardless, this will be added in a future iteration.

@markussc
Copy link
Author
markussc commented Mar 9, 2016

This is really really strange, but you made my day!
The following patterns work:
<domain>\{username} and <email-address>@FQDN . I can use the first one.
With the sAMAccountName, it didn't work...
As said before, the feature will be a good add-on for the future anyway! Thanks a lot for helping!

@csarrazi
Copy link
Contributor
csarrazi commented Mar 9, 2016

You're welcome! I figured it was really strange, that nothing worked! 😃

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

Successfully merging this pull request may close these issues.

6 participants
0