-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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
Conversation
May be used e.g. if DN does not contain username.
ping @csarrazi |
In any case, the file permission changes must be reverted. |
I'm -1 on this one. The problem here is that the The logic should be moved instead in the |
…ider functionality
@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.. |
…thenticationProvider
Sorry, my bad, it should indeed be in the |
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; |
There was a problem hiding this comment.
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.
Also, this is a new feature. It should not be made from the 2.8 branch, but from the |
@@ -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) |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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
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 If you wish to make a search prior to running a Ldap bind(), the only class that needs refactoring is the Also, as mentioned before, this is a new feature and, therefore, it should not be based (and merged) on the What we can do, though, since |
This is to be used inside a Symfony application, using the Security component. 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. |
@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 |
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. |
Did you try using |
Regardless, this is indeed a new feature. We could implement the bind using different strategies:
@fabpot Should we create a new issue for this, and close this PR? |
Sure, I'm closing this PR and I let you create the issue. |
@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... |
What about 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. |
This is really really strange, but you made my day! |
You're welcome! I figured it was really strange, that nothing worked! 😃 |
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