[WIP][Security] Added Ldap provider #5189

lyrixx · 2012-08-06T09:50:38Z

Bug fix: no, but may be
Feature addition: yes
Backwards compatibility break: no, but may be
Symfony2 tests pass: no, not yet
Fixes the following tickets: -
Todo: Finish implementation, Add more tests. Fix \DependencyInjection\Security\Factory*`
License of the code: MIT
Documentation PR: not available yet

Hello.

First, it will be useful to have this provider into the security component. Thanks to this, the feature will be available into silex application, and also drupal 8 and ezpublish 5.

I open a PR, but my work is not finished. It could be great to have an early feedback (not about CS, just about architecture)

I read the codebase of bundles available on http://knpbundles.com/search?q=ldap but there is some issues with these bundles:

Dependency on Zend\Ldap (opensky)
A global username / password account is not needed to check is a user is able to connect to ldap. (all bundle)
The factory are no enough flexible.

Of course, I will rebase / squash all my commits before final merge (if happen).

I am really not sure about f40c85b

I tried to keep the default workflow of the user authentification
But with ldap, we can not retrieve user password. The only way to check if a user is in a ldap server, and if he is able to connect to the ldap, it is to try to connect the user against the ldap.

Then, we have an issue in src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/UserProvider/*Factory.php.
For exemple in https://github.com/lyrixx/symfony/blob/1834aaa11a9342455eaf30f1c6cd7ef69f951bdb/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/FormLoginFactory.php#L65 the $provider (AuthentificationProvider) is hardcoded. It should not be.
So, just for my test, i changed this value (this change breaks tests).
But we have to find a way to do this cleanly. How can we do that ?

(A) Duplicate the factory (Not ok with that)
(B) Add a new argument createAuthProvider
??

With A option, there will be lot of code.
With B option, we could add BC break.

lyrixx · 2012-08-06T09:51:13Z

src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/UserProvider/LdapFactory.php

+                ->scalarNode('host')->isRequired()->cannotBeEmpty()->end()
+                ->scalarNode('port')->defaultValue(389)->end()
+                ->scalarNode('dn')->isRequired()->cannotBeEmpty()->end()
+                ->scalarNode('username')->isRequired()->cannotBeEmpty()->end()


Not needed anymore.

travisbot · 2012-08-06T09:57:03Z

This pull request fails (merged 1834aaa1 into 0ccda38).

stof · 2012-08-06T10:11:00Z

src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/FormLoginFactory.php

        $container
-            ->setDefinition($provider, new DefinitionDecorator('security.authentication.provider.dao'))
+            // @todo : Fix that hack


This is really important as you are currently breaking everything for people not using ldap.

Yes of course. We have to find a clever way to fix that.

IMO, you should add a separate factory for now instead of hacking the form_login one.

Ok.
This point is my real issue.

I can add another factory, but it will a copy / paste of FormLoginFactory.
Moreover, a start to work with FormLoginFactory, but I will have to duplicate HttpBasicFactory. And maybe others ...

So I don't think it's a good idea.

We need feedback here.

Well, as I said during our IRC discussion last week, we indeed need to find a better way. But currently, your PR is breaking existing functionalities, so I don't think it is a good idea to keep it as is. Either refactor the factories to provide a clean way (which requires some work and is likely not so easy) or create another factory for now (which could extend from the form_login one to avoid duplicating everything)

And btw, you are talking about duplicating the HttpBasicFactory. but your current PR does not support http_basic. And if you go the current way, you would be hacking the existing factory, thus breaking it too.

I totally agree with you. Of course I will not let this code in place.

So for now, i will duplicate the factory ..

Added 2 new factories.

stof · 2012-08-06T10:24:59Z

Btw, @JMSBot found a few other issues as the symfony PRs are simulated in its test repository to test the code-review tool. You may want to take a look at it: JMSBot/test-repository#240 (note that it will not update the simulated PR for your new changes)

travisbot · 2012-08-06T13:28:52Z

This pull request fails (merged dc9211eb into 0ccda38).

travisbot · 2012-08-06T13:40:16Z

This pull request fails (merged 610e03ca into 0ccda38).

travisbot · 2012-08-06T15:02:25Z

This pull request passes (merged 7a7dba1c into 0ccda38).

travisbot · 2012-08-06T15:11:38Z

This pull request passes (merged 981f3b46 into 0ccda38).

stof · 2012-08-06T15:58:05Z

src/Symfony/Component/Security/Core/Authentication/Provider/LdapAuthenticationProvider.php

+
+        // @todo : how to manage roles ?
+        return new User($username, $token->getCredentials(), array('ROLE_USER'));
+    }


why are you not using the user provider to retrieve the user ?

Because I need to store the password in the User.

but this means that it is impossible to use a user provider, which seems quite strange

We can use a UserProvider, be we will have to add another User class which will support setPassword(). I think it's useless to add an overhead here.

UserProvider::loadUserByUsername is not possible due to ldap limitation. Thats why, IMHO it's easier to no use this method.

BTW : It's possible to check if a user exists in the ldap. But the LDAP will not return the password (plain text or not)...
Then, the query to find the user is very difficult to find.

travisbot · 2012-08-06T16:06:01Z

This pull request fails (merged bb975f75 into 0ccda38).

travisbot · 2012-08-06T16:25:51Z

This pull request fails (merged 2b67f3ea into 0ccda38).

travisbot · 2012-08-06T16:32:02Z

This pull request fails (merged 0d15ef14 into 842b599).

travisbot · 2012-08-06T16:35:02Z

This pull request passes (merged e548552c into 842b599).

travisbot · 2012-08-06T17:31:49Z

This pull request passes (merged d9ab53ca into 842b599).

stof · 2012-08-07T09:23:29Z

src/Symfony/Component/Security/Core/User/LdapUserProvider.php

+     */
+    public function loadUserByUsername($username)
+    {
+        throw new \RuntimeException(sprintf('You sould not call the method "%s"', __METHOD__));


this should be a BadMethodCallException

travisbot · 2012-08-07T09:26:37Z

This pull request passes (merged f77f02e4 into 842b599).

lyrixx · 2012-08-07T09:28:15Z

At this point we have 3 issues :

(A) The username / password is stored in the session. I have to do that to make UserProvider::refreshUser works.
(B) The roles are hardcoded.
(C) Some factories are duplicated

(A) I don't know how to avoid that.
(B) We could add to LdapUserProviderInterface a new method : getRoles. This method could fetch into the ldap some information about roles. What do you think ?

travisbot · 2012-08-07T10:15:16Z

This pull request fails (merged 8e65021b into 842b599).

ylynfatt · 2014-02-21T12:43:44Z

Has this officially been added to Symfony yet?

cordoval · 2014-02-21T12:47:17Z

it obviously has not @ylynfatt because it is not merged, it is not a finished PR either

ylynfatt · 2014-02-24T16:11:04Z

Hi @lyrixx Are the outstanding issues the same as those you mentioned in your initial pull request comment? is there anything else the community can do to help? Or is there anything from @relwell's DapsBundle that can be added to your code?

Renrhaf · 2014-05-03T15:13:57Z

Hello, is the goal of this PR to add an authentication provider against a simple LDAP bind ?

ylynfatt · 2014-05-08T20:39:47Z

@Renrhaf I believe so.

linaori · 2014-07-31T07:47:44Z

Any updates on this PR? I'm quite interested to see this implemented by default.

ChadSikorra · 2015-02-17T02:22:13Z

src/Symfony/Component/Security/Ldap/Ldap.php

+                ldap_start_tls($this->connection);
+            }
+
+            $this->connection = ldap_connect($host, $this->getPort());


You should do the ldap_connect() first, then do the ldap_set_option() and ldap_start_tls().

jakzal · 2015-02-17T09:20:51Z

@lyrixx if you don't have time to work on this, or there's no one to take over, I think it's better to close this PR.

I'm not fully convinced we need this in the core by the way. If the existing bundles are no good, perhaps its easier to fix them, or implement a new bundle?

lyrixx · 2015-02-17T09:42:38Z

@jakzal Actually, I don't need this PR anymore. So it's a bit hard for me to find time and motivation to finish it.

But I'm not sure closing this PR is our best option. I know Everybody want less open issue / PR, but This PR is almost complete. I (really) will try to finish it ! ;)

fabpot · 2015-02-17T10:04:53Z

... and I and it looks like a lot of people are looking for a good LDAP support in the Symfony Security Component.

jakzal · 2015-02-17T10:06:08Z

@lyrixx let me know if I can help with finishing it off in any way.

csarrazi · 2015-02-17T13:31:02Z

No personal feelings, here, but two things are wrong in this PR:

Why would we need to keep the password in the session?
Why would we need to call the ldap_bind() method for each request?

We only need to

either call ldap_bind() in the authenticator
or compare the provided password with the LDAP attributes

and do this once. The only case where we would need to do this for every request is if we are using HTTP Basic as the authentication mechanism.

Binding to an LDAP means initiating a session on the server. However, there is no support in PHP for keeping alive the LDAP session (the connection is not persistent). Thus, using the ldap_bind() for every subsequent request also means potentially creating hundreds of LDAP sessions on the directory service. Using ldap_bind() is also not possible without:

either using HTTP Basic as an authentication mechanism (which sends the password for each request)
or keeping the user's password in the session (which would induce a security flaw)

This also means that apart from HTTP Basic, stateless authentication mechanisms won't work with the current implementation, which basically makes the user provider useless.

Until we can actually use persistent LDAP connections, or until we have a broad usage of PHP applications with WSGI-like capability, we should not call the ldap_bind() method in the UserProvider.

Until then, for a correct implementation, we would need:

An LDAP user provider which simply fetches the DN from the LDAP directory, which involves a search in the directory, unless the exact mapping is already known.
Two LDAP authentication providers, which either:
- binds the user to the LDAP connection on first request (authentication)
- compares the encrypted password from the LDAP information
Loading the list of roles of the user from the LDAP attributes.

This also means that we don't need to create a new method (loadUserByUsernameAndPassword) in the new user provider (which currently does way more it is supposed to do).

amenophis · 2015-04-22T10:46:28Z

@lyrixx Should we wait for something new about this PR ?

csarrazi · 2015-04-22T11:31:07Z

Actually, I'm working on this PR :)

I'll give more info next week, as the code has received a major overhaul. :)

amenophis · 2015-04-22T11:32:50Z

Really a good news to hear this !
If i can help you, don't hesitate !

csarrazi · 2015-04-22T13:46:30Z

Here is the current status:

LDAP Authentication providers
- LDAP HTTP Basic authentication provider
- LDAP Form login authentication provider
LDAP User provider

In the updated PR, the bind() method is only used when we want to actually check the user's credentials. This means:

For each request, if we are using the LDAP HTTP Basic authentication provider
When checking the username and password, during login check, for the LDAP Form login authentication provider
When the LDAP User provider actually needs to authenticate to the LDAP to issue queries

Regardless, as seen with @fabpot last day, I should be able to issue a WIP PR against the 2.8 branch by sunday (tests won't be working yet, though).

amenophis · 2015-04-28T18:41:40Z

@csarrazi Do you have news on this PR ?

csarrazi · 2015-04-29T01:34:59Z

Yes. The groundwork is basically done, but needs some cleanup.

Also, I'm working on an edge case, where a developer would like to use both the LDAP user provider and an LDAP authentication provider.

In the current implementation, each use a different instance of the LDAP service.
To support this case, we would need to either

duplicate the configuration of the LDAP services (one in the authentication provider's factory, another in the user provider's factory), and have two different instances of LDAP services (and connections),
or use the same LDAP service, but this means that the common LDAP configuration would need to be set neither in the providers node nor the firewalls section.

In short, it's basically something like:

security:
    providers:
        ldap_users:
            ldap:
                host: ldap.mydomain.tld
                uid_key: sAMAccountName
                base_dn: my_base_dn
                bind_dn: default_bind_dn
                password: P4$$w0rd
                filter: my_filter_string
    firewalls:
        demo_secured_area:
            stateless: true
            provider: ldap_users
            pattern:    ^/
            http_basic_ldap:
                host: ldap.mydomain.tld
                base: dc=mydomain,dc=local
                username_suffix: "@MYDOMAIN"

vs

services:
    my_ldap_service_id:
        arguments: [ ldap.mydomain.tld, my_base_dn, default_bind_dn, my_filter_string, p4$$w0rd ]

security:
    providers:
        ldap_users:
            ldap:
                id: my_ldap_service_id
                filter: my_filter_string
                uid_key: sAMAccountName
    firewalls:
        demo_secured_area:
            stateless: true
            provider: ldap_users
            pattern:    ^/
            http_basic_ldap:
                id: my_ldap_service_id
                username_suffix: "@MYDOMAIN"

Of course, we support both ways: if a service is provided, then we use a pre-configured service. Otherwise, we create a new service instance.

Regardless of whether we chose the a common service or two separate services, I would actually make another change, in the authentication providers: currently, all authentication providers start by fetching the user (from the user provider), before authenticating the user (call the bind() method). But to support this specific case, inverting the logic would prevent us to make a double bind() (one for the search user, another authenticating the actual user).

Does this seem okay for everyone?

linaori · 2015-04-29T06:01:38Z

In my opinion the configuration is supposed to be done in your user-provider and not your firewall. LDAP configuration is not related to your firewall, but just how you load the users. I'm not sure why it's even required to create "http_basic_ldap" and the form variant, what's wrong with the current implementations (or the Simple* variants for that matter), where you have to implement more than a user provider?

csarrazi · 2015-04-29T07:22:08Z

You don't necessarily have to use the http_basic_ldap or form_login_ldap.

These are only for authenticating against LDAP (i.e. authenticating using ldap_bind(), instead of comparing password hashes as, depending on your LDAP server's configuration, you may not receive the password hash in the LDAP info.

Thus, a different password validation strategy must be used with LDAP.

The LDAP user provider's goal is to actually fetch the user's information from the LDAP.

If your LDAP server returns the password hash, then you're good to go simply with the LDAP user provider (using either anonymous search, or a search user's credentials) + any "normal" firewall configuration.
However, if you don't have the password hash in the info (which is the case with most LDAP implementations), the only way to validate your credentials is by using the ldap_bind() PHP function.

For this, you either need:

A new authentication provider
implement a strategy to switch between password comparison and ldap_bind() in the DAO authentication provider

Also, setting the authentication provider needs to be done either by overriding HttpBasicFactory::create() and FormLoginFactory:createAuthProvider() using class inheritance, or by introducing a new option in both factories, which can be used to switch between the DAO authentication provider and LDAPBindAuthenticationProvider, and setting the default to the DAO authentication provider.

Creating new firewall factories is not necessary. But my work is based on the original PR, in which both factories were introduced (HttpBasicLdapFactory and FormLoginLdapFactory). Currently, I feel that those are not necessary, and I would actually prefer the using an option to switch between the dao or ldap_bind authentication providers.

The reason behind separating the LDAP user provider from the authentication provider is that you may use ldap_bind() to authenticate your users, while fetching them from another data source (entity, in_memory, etc.), or use password comparison while fetching users from an LDAP server.

linaori · 2015-04-29T07:46:29Z

I propose to use an implementation SimpleAuthenticatorInterface instead of the DAO.

csarrazi · 2015-04-29T08:54:15Z

Point taken. The base PR is quite old (from 2012), and the SimpleAuthenticator didn't exist at the time. But we're getting there! ;)

csarrazi · 2015-05-11T08:49:35Z

I've released my PR for this issue. I would be glad to receive some feedback on this.

lyrixx · 2015-05-11T12:13:09Z

Closing in favor of #14602

…ntegration in the Security Component). (csarrazi, lyrixx) This PR was merged into the 2.8 branch. Discussion ---------- [2.8] [Ldap] Added support for LDAP (New Component + integration in the Security Component). | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | not yet | Fixed tickets | - | License | MIT | Doc PR | not yet Current state: - [x] Implement logic - [x] Post-review tuning and stabilization - [x] Fix tests This PR is a follow-up to #5189, which was in a stand-still for a few years now. It tries to fix the remaining issues which were mentioned in the discussion. There are still a few issues with the PR, as it is. For example, it introduces two new firewall factories, whereas the base factories (`form_login` and `http_basic`) could simply introduce new configuration options. Also, for a user to use an LDAP server as an authentication provider, he first needs to define a service which should be an instance of `Symfony\Component\Security\Ldap\Ldap`. For example: ```yml services: my_ldap: class: Symfony\Component\Security\Ldap\Ldap arguments: [ "ldap.mydomain.tld" ] ``` Then, in `security.yml`, this service can be used in both the user provider and the firewalls: ```yml security: encoders: Symfony\Component\Security\Core\User\User: plaintext role_hierarchy: ROLE_ADMIN: ROLE_USER ROLE_SUPER_ADMIN: [ROLE_USER, ROLE_ADMIN, ROLE_ALLOWED_TO_SWITCH] providers: ldap_users: ldap: service: my_ldap base_dn: dc=MyDomain,dc=tld search_dn: CN=My User,OU=Users,DC=MyDomain,DC=tld search_password: p455w0rd filter: (sAMAccountName={username}) default_roles: ROLE_USER firewalls: dev: pattern: ^/(_(profiler|wdt)|css|images|js)/ security: false demo_login: pattern: ^/login$ security: false api: provider: ldap_users stateless: true pattern: ^/api http_basic_ldap: service: my_ldap dn_string: "{username}@mydomain" demo_secured_area: provider: ldap_users pattern: ^/ logout: path: logout target: login form_login_ldap: service: my_ldap dn_string: CN={username},OU=Users,DC=MyDomain,DC=tld check_path: login_check login_path: login ``` Commits ------- 60b9f2e Implemented LDAP authentication and LDAP user provider 1c964b9 Introducing the LDAP component

lyrixx reviewed Aug 6, 2012
View reviewed changes

stof reviewed Aug 6, 2012
View reviewed changes

stof reviewed Aug 7, 2012
View reviewed changes

javiereguiluz mentioned this pull request Apr 10, 2014

Voting on some of the oldest and most relevant issues #10684

Closed

phalcon mentioned this pull request Jan 19, 2015

The brand-new Phalcon/Zephir development workflow phalcon/cphalcon#3246

Closed

ChadSikorra reviewed Feb 17, 2015
View reviewed changes

csarrazi mentioned this pull request May 10, 2015

[2.8] [Ldap] Added support for LDAP (New Component + integration in the Security Component). #14602

Merged

3 tasks

lyrixx closed this May 11, 2015

lyrixx deleted the feat-security-ldap branch December 8, 2015 15:21

[WIP][Security] Added Ldap provider #5189

[WIP][Security] Added Ldap provider #5189

Conversation

lyrixx commented Aug 6, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

travisbot commented Aug 6, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stof commented Aug 6, 2012

travisbot commented Aug 6, 2012

travisbot commented Aug 6, 2012

travisbot commented Aug 6, 2012

travisbot commented Aug 6, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

travisbot commented Aug 6, 2012

travisbot commented Aug 6, 2012

travisbot commented Aug 6, 2012

travisbot commented Aug 6, 2012

travisbot commented Aug 6, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

travisbot commented Aug 7, 2012

lyrixx commented Aug 7, 2012

travisbot commented Aug 7, 2012

ylynfatt commented Feb 21, 2014

cordoval commented Feb 21, 2014

ylynfatt commented Feb 24, 2014

Renrhaf commented May 3, 2014

ylynfatt commented May 8, 2014

linaori commented Jul 31, 2014

Choose a reason for hiding this comment

jakzal commented Feb 17, 2015

lyrixx commented Feb 17, 2015

fabpot commented Feb 17, 2015

jakzal commented Feb 17, 2015

csarrazi commented Feb 17, 2015

amenophis commented Apr 22, 2015

csarrazi commented Apr 22, 2015

amenophis commented Apr 22, 2015

csarrazi commented Apr 22, 2015

amenophis commented Apr 28, 2015

csarrazi commented Apr 29, 2015

linaori commented Apr 29, 2015

csarrazi commented Apr 29, 2015

linaori commented Apr 29, 2015

csarrazi commented Apr 29, 2015

csarrazi commented May 11, 2015

lyrixx commented May 11, 2015