[Ldap] Use discovered DNs for the authentication bind #16823
Comments
|
PR, in-progress, still needs work #16827 |
|
As mentioned in your PR, I would rather have a dedicated LdapUser class, in which the logic would be encapsulated. For your information, LDAP implementations in the framework are independent from each other. This means that you can use the Ldap user provider with a different check mechanism (password comparison, and not necessarily ldap_bind), or the ldap_bind-based authentication mechanisms with a third-party user storage (database, or other). |
|
I'm mentioning PR #17813 as another way to improve DN management using a previous search. I will start from your current PR (#16827), improving it and implement both approaches as a single feature. Unfortunately, this will have to wait until next week, as I first intend to work on the documentation for the Ldap component, and a cookbook explaining how to use the Ldap component. |
|
I ran into this problem myself while debugging why I was getting a Moreover the EDIT SOLVED: Thank you for your linked PR #17813 comment, from there I was able to know that I could change the EDIT 2: For those trying to troubleshoot Active Directory problems, I found this extra debugging information invaluable. I editing the vendor code and added this information (per this php comment) That gives you extra information on AD errors, so I was able to get |
|
Wouldn't it be better to extend LdapBindAuthenticationProvider the way how LdapUserProvider is configured already - so it accepts searchDn with the password, and then filter to find user's DN based on condition like (uid={username})? If you think that would be reasonable I can implement that. |
|
@csarrazi I assume you are not actively working on this topic after all? |
|
@lsmith77 Unfortunately, not on this specific topic. The component is only in its infancy, so there's still a lot of work to do, and I do not have much time to work on this. Heck, I don't even have time to finalise the docs PR for 3.1! So, contributions are always welcome! :) |
|
In case someone is interested: I have an OpenLDAP instance configured with the meta backend as a proxy for two ldap directories (with distinct uid's!). As such, users have two different dn ( I extended my custom User model with a getter & setter for the dn. In my overriden LdapUserProvider the dn gets set in namespace AppBundle\Security\Core\User;
use AppBundle\Model\User;
use Symfony\Component\Security\Core\User\LdapUserProvider as BaseProvider;
use Symfony\Component\Security\Core\User\UserInterface;
use Symfony\Component\Security\Core\Exception\UnsupportedUserException;
use Symfony\Component\Ldap\Entry;
use Symfony\Component\Ldap\LdapInterface;
class LdapUserProvider extends BaseProvider
{
private $defaultRoles;
public function __construct(LdapInterface $ldap, $baseDn, array $defaultRoles = array())
{
$this->defaultRoles = $defaultRoles;
parent::__construct($ldap, $baseDn, null, null, $defaultRoles, 'uid');
}
/**
* {@inheritdoc}
*/
public function refreshUser(UserInterface $user)
{
if (!$user instanceof User) {
throw new UnsupportedUserException(sprintf('Instances of "%s" are not supported.', get_class($user)));
}
return new User($user->getUsername(), null, $user->getRoles());
}
/**
* {@inheritdoc}
*/
public function supportsClass($class)
{
return $class === User::class;
}
protected function loadUser($username, Entry $entry)
{
$user = new User($username, null, $this->defaultRoles);
$user->setDn($entry->getDn());
return $user;
}
}The stored dn is then retrieved by the registered authenticator: namespace AppBundle\Security\Http\Authentication;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\Ldap\Ldap;
use Symfony\Component\Ldap\Exception\BadCredentialsException;
use Symfony\Component\Ldap\Exception\ConnectionException;
use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
use Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken;
use Symfony\Component\Security\Core\Exception\CustomUserMessageAuthenticationException;
use Symfony\Component\Security\Core\Exception\UsernameNotFoundException;
use Symfony\Component\Security\Core\User\UserProviderInterface;
use Symfony\Component\Security\Http\Authentication\SimpleFormAuthenticatorInterface;
class LdapAuthenticator implements SimpleFormAuthenticatorInterface
{
private $ldap;
public function __construct(Ldap $ldap)
{
$this->ldap = $ldap;
}
public function authenticateToken(TokenInterface $token, UserProviderInterface $userProvider, $providerKey)
{
try {
$user = $userProvider->loadUserByUsername($token->getUsername());
} catch (UsernameNotFoundException $e) {
throw new CustomUserMessageAuthenticationException('Invalid username or password');
}
try {
$this->ldap->bind(); // or $ldap->bind($searchDn, $searchPassword);
} catch (ConnectionException $e) {
throw new BadCredentialsException('Could not connect to server');
}
try {
$this->ldap->bind($user->getDn(), $token->getCredentials());
} catch (ConnectionException $e) {
throw new BadCredentialsException('Bad password');
} catch (LdapException $e) {
throw new BadCredentialsException('Bad query');
}
return new UsernamePasswordToken(
$user,
$user->getPassword(),
$providerKey,
$user->getRoles()
);
}
public function supportsToken(TokenInterface $token, $providerKey)
{
return $token instanceof UsernamePasswordToken
&& $token->getProviderKey() === $providerKey;
}
public function createToken(Request $request, $username, $password, $providerKey)
{
return new UsernamePasswordToken($username, $password, $providerKey);
}
}The authenticator is slightly different than suggested by @csarrazi in #20905, as I don't see the point in a repeated query if I already have everything I need as the authenticator would have to be configured just like the providers to be able to find the users. And that shouldn't be necessary imho. |
|
@nietonfir As mentioned before, a repeated query should not be necessary in all cases. However, keep in mind that you may have access to different attributes depending on the user, or search may actually be disabled for users other than the search user, based on the access controls enforced by the server. Indeed, in many setups, human users may not have the ability to search other users' information in the LDAP tree. Likewise, the search user may not have the ability to see certain attributes which can be seen by a user of the application (an email, for example, may only be visible to the user, and not the search user). In the longer term, I think that we should have a configuration option which will define the behaviour of the user provider, as well as the authenticator. A.k.a:
By the way @nietonfir, you'll be happy to know that I'm currently working on providing a means to use multiple DNs for users. Currently, the design is still in WIP, but it could both replace the search before bind in a a lot of cases, or the use of a chain user provider. |
|
@csarrazi You're totally right, I didn't think about that use case. Can't wait to see your solution. ;-) Is there anything one could help with? |
|
see #21402 |
… of searching for the DN (lsmith77, nietonfir) This PR was merged into the 3.3-dev branch. Discussion ---------- [Security] make LdapBindAuthenticationProvider capable of searching for the DN | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #16823, #20905 | License | MIT | Doc PR | symfony/symfony-docs#7420 I guess due to the separation between the user and auth provider something like the following isn't ok (note: the following works just fine for me but if course in the end the username is the DN and not the user provided shorter username): ```diff diff --git a/src/Symfony/Component/Security/Core/Authentication/Provider/LdapBindAuthenticationProvider.php b/src/Symfony/Component/Security/Core/Authentication/Provider/LdapBindAuthenticationProvider.php index 5ebb09a..18d7825 100644 --- a/src/Symfony/Component/Security/Core/Authentication/Provider/LdapBindAuthenticationProvider.php +++ b/src/Symfony/Component/Security/Core/Authentication/Provider/LdapBindAuthenticationProvider.php @@ -70,7 +70,7 @@ class LdapBindAuthenticationProvider extends UserAuthenticationProvider */ protected function checkAuthentication(UserInterface $user, UsernamePasswordToken $token) { - $username = $token->getUsername(); + $username = $user->getUsername(); $password = $token->getCredentials(); if ('' === $password) { @@ -78,10 +78,7 @@ class LdapBindAuthenticationProvider extends UserAuthenticationProvider } try { - $username = $this->ldap->escape($username, '', LdapInterface::ESCAPE_DN); - $dn = str_replace('{username}', $username, $this->dnString); - - $this->ldap->bind($dn, $password); + $this->ldap->bind($username, $password); } catch (ConnectionException $e) { throw new BadCredentialsException('The presented password is invalid.'); } diff --git a/src/Symfony/Component/Security/Core/User/LdapUserProvider.php b/src/Symfony/Component/Security/Core/User/LdapUserProvider.php index fc42419..8194c4c 100644 --- a/src/Symfony/Component/Security/Core/User/LdapUserProvider.php +++ b/src/Symfony/Component/Security/Core/User/LdapUserProvider.php @@ -115,7 +115,7 @@ class LdapUserProvider implements UserProviderInterface { $password = $this->getPassword($entry); - return new User($username, $password, $this->defaultRoles); + return new User($entry->getDn(), $password, $this->defaultRoles); } /** ``` Therefore I created an entire new auth provider. Commits ------- 8ddd533 Merge pull request #1 from nietonfir/http_basic_ldap a783e5c Update HttpBasicLdapFactory a30191f make LdapBindAuthenticationProvider capable of searching for the DN
…capable of searching for the DN (lsmith77, nietonfir) This PR was merged into the 3.3-dev branch. Discussion ---------- [Security] make LdapBindAuthenticationProvider capable of searching for the DN | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony#16823, symfony#20905 | License | MIT | Doc PR | symfony/symfony-docs#7420 I guess due to the separation between the user and auth provider something like the following isn't ok (note: the following works just fine for me but if course in the end the username is the DN and not the user provided shorter username): ```diff diff --git a/src/Symfony/Component/Security/Core/Authentication/Provider/LdapBindAuthenticationProvider.php b/src/Symfony/Component/Security/Core/Authentication/Provider/LdapBindAuthenticationProvider.php index 5ebb09a..18d7825 100644 --- a/src/Symfony/Component/Security/Core/Authentication/Provider/LdapBindAuthenticationProvider.php +++ b/src/Symfony/Component/Security/Core/Authentication/Provider/LdapBindAuthenticationProvider.php @@ -70,7 +70,7 @@ class LdapBindAuthenticationProvider extends UserAuthenticationProvider */ protected function checkAuthentication(UserInterface $user, UsernamePasswordToken $token) { - $username = $token->getUsername(); + $username = $user->getUsername(); $password = $token->getCredentials(); if ('' === $password) { @@ -78,10 +78,7 @@ class LdapBindAuthenticationProvider extends UserAuthenticationProvider } try { - $username = $this->ldap->escape($username, '', LdapInterface::ESCAPE_DN); - $dn = str_replace('{username}', $username, $this->dnString); - - $this->ldap->bind($dn, $password); + $this->ldap->bind($username, $password); } catch (ConnectionException $e) { throw new BadCredentialsException('The presented password is invalid.'); } diff --git a/src/Symfony/Component/Security/Core/User/LdapUserProvider.php b/src/Symfony/Component/Security/Core/User/LdapUserProvider.php index fc42419..8194c4c 100644 --- a/src/Symfony/Component/Security/Core/User/LdapUserProvider.php +++ b/src/Symfony/Component/Security/Core/User/LdapUserProvider.php @@ -115,7 +115,7 @@ class LdapUserProvider implements UserProviderInterface { $password = $this->getPassword($entry); - return new User($username, $password, $this->defaultRoles); + return new User($entry->getDn(), $password, $this->defaultRoles); } /** ``` Therefore I created an entire new auth provider. Commits ------- 8ddd533 Merge pull request #1 from nietonfir/http_basic_ldap a783e5c Update HttpBasicLdapFactory a30191f make LdapBindAuthenticationProvider capable of searching for the DN
In implementing LDAP authentication in 2.8, we have found that authentication is limited to only users whose DNs fit the configured dn_string pattern. Let us consider this configuration:
In this case, a user with DN "uid=foo,ou=people,dc=example,dc=com" can authenticate but one with DN "uid=foo,ou=something,ou=people,dc=example,dc=com" cannot, even though both accounts are in the configured search base.
The immediate reason is that after discovering information from LDAP with a call to ->find() in the LdapUserProvider::loadUserByUsername() method, the DN is discarded when LdapUserProvider::loadUser() creates the User instance.
Because the DN is discarded, later LdapBindAuthenticationProvider::checkAuthentication() recombines the basic username with what is configured in dn_string:
$dn = str_replace('{username}', $username, $this->dnString);
It would seem if User were extended to store the DN (perhaps by creating LdapUser with a setDn() method), it would not be necessary to set dn_string, and users in various OU's could authenticate.
The text was updated successfully, but these errors were encountered: