[WIP][Security] Added Ldap provider #5189
Conversation
| ->scalarNode('host')->isRequired()->cannotBeEmpty()->end() | ||
| ->scalarNode('port')->defaultValue(389)->end() | ||
| ->scalarNode('dn')->isRequired()->cannotBeEmpty()->end() | ||
| ->scalarNode('username')->isRequired()->cannotBeEmpty()->end() |
| $container | ||
| ->setDefinition($provider, new DefinitionDecorator('security.authentication.provider.dao')) | ||
| // @todo : Fix that hack |
There was a problem hiding this comment.
This is really important as you are currently breaking everything for people not using ldap.
There was a problem hiding this comment.
Yes of course. We have to find a clever way to fix that.
There was a problem hiding this comment.
IMO, you should add a separate factory for now instead of hacking the form_login one.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I totally agree with you. Of course I will not let this code in place.
So for now, i will duplicate the factory ..
|
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) |
|
|
||
| // @todo : how to manage roles ? | ||
| return new User($username, $token->getCredentials(), array('ROLE_USER')); | ||
| } |
There was a problem hiding this comment.
why are you not using the user provider to retrieve the user ?
There was a problem hiding this comment.
Because I need to store the password in the User.
There was a problem hiding this comment.
but this means that it is impossible to use a user provider, which seems quite strange
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| */ | ||
| public function loadUserByUsername($username) | ||
| { | ||
| throw new \RuntimeException(sprintf('You sould not call the method "%s"', __METHOD__)); |
There was a problem hiding this comment.
this should be a BadMethodCallException
|
At this point we have 3 issues :
(A) I don't know how to avoid that. |
|
Has this officially been added to Symfony yet? |
|
it obviously has not @ylynfatt because it is not merged, it is not a finished PR either |
|
Hello, is the goal of this PR to add an authentication provider against a simple LDAP bind ? |
|
@Renrhaf I believe so. |
|
Any updates on this PR? I'm quite interested to see this implemented by default. |
| ldap_start_tls($this->connection); | ||
| } | ||
|
|
||
| $this->connection = ldap_connect($host, $this->getPort()); |
There was a problem hiding this comment.
You should do the ldap_connect() first, then do the ldap_set_option() and ldap_start_tls().
|
@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? |
|
@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 ! ;) |
|
... and I and it looks like a lot of people are looking for a good LDAP support in the Symfony Security Component. |
|
@lyrixx let me know if I can help with finishing it off in any way. |
|
No personal feelings, here, but two things are wrong in this PR:
We only need to
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
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 Until then, for a correct implementation, we would need:
This also means that we don't need to create a new method ( |
|
@lyrixx Should we wait for something new about this PR ? |
|
Actually, I'm working on this PR :) I'll give more info next week, as the code has received a major overhaul. :) |
|
Really a good news to hear this ! |
|
Here is the current status:
In the updated PR, the
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). |
|
@csarrazi Do you have news on this PR ? |
|
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.
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 Does this seem okay for everyone? |
|
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? |
|
You don't necessarily have to use the These are only for authenticating against LDAP (i.e. authenticating using 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. For this, you either need:
Also, setting the authentication provider needs to be done either by overriding Creating new firewall factories is not necessary. But my work is based on the original PR, in which both factories were introduced ( The reason behind separating the LDAP user provider from the authentication provider is that you may use |
|
I propose to use an implementation |
|
Point taken. The base PR is quite old (from 2012), and the SimpleAuthenticator didn't exist at the time. But we're getting there! ;) |
|
I've released my PR for this issue. I would be glad to receive some feedback on this. |
|
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
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:
Zend\Ldap(opensky)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 ?
With A option, there will be lot of code.
With B option, we could add BC break.