SEC-1510: BindAuthenticator should work with less user privileges

[spring-projects-issues]

Steffen Ryll (Migrated from SEC-1510) said:

The current implementation of BindAuthenticator implicitly expects that the user it binds with is allowed to read its own user object. In some environments this is not true, e.g. Microsoft's ADAM does by default not allow normal users (or user proxies) to read their own directory objects. Typically only dedicated 'manager' users are allowed to read all user objects.

In such an environment, BindAuthenticator binds to the server with the user credentials and succeeds in the first step (this is the first line of the try block in bindWithDn() ). Next, it tries to retrieve all attributes on the user object, but fails with LDAP error code 32. This is because that normal user account must not descend that far into the directory tree. This problem was described similarly also in http://forum.springsource.org/showthread.php?t=53273

What I want to suggest is that the user attributes are not retrieved through the specific user's context, but rather through the general context source (which is the manager user with the appropriate permissions).

There's one disadvantage with my approach, as an additional bind must be performed to the LDAP server (one user bind becomes one user bind + one manager bind). The advantage is that less directory privileges are required for the normal users - the bind for password checking only, and all supplemental user attributes are retrieved by the manager user.

I have a rough patch against Spring Security 2.0.5 already that I would like to discuss. I will post it shortly, after porting it to Spring Security 3.0.3.

[spring-projects-issues]

Luke Taylor said:

I don't think this is a good idea as a general strategy. It's also very common for companies to only allow binding and reading data as the user themselves and not allow the application to have a privileged "manager" user at all (see SEC-962, for example).

[spring-projects-issues]

Steffen Ryll said:

I agree with you, this does not fit for every organization. Basically, there seem to be two typical strategies how companies set up their directories. One is, like you said, that privileged "manager users" are frowned upon and normal users can read their attributes. The second is that normal users have very restricted rights and only dedicated "manager users" are granted read access to all user objects.

At the moment, Spring Security offers a solution for the first strategy (if you can get away without using FilterBasedLdapUserSearch), but always fails in the second situation. So I'd argue that Spring Security should ship a solution for the second case as well. It's not that uncommon, after all.

Right now, I copied most of the code in BindAuthenticator to a new class (see attachment), also extending AbstractLdapAuthenticator. This classes uses two different LDAP contexts in the bindWithDn() method. I have to admit that this is definitely not perfect, but I'd like to hear you opinion whether this fits into the framework and how it should be improved. Following ideas come to my mind:

• Instead of having a second class hanging around, BindAuthenticator could also be amended with a boolean property and some if-else-logic, in order to address both directory strategies.
• If you prefer to have two separate classes, I'd suggest to make bindWithDn() a protected method in BindAuthenticator. The second class can then extend BindAuthenticator and override binWithDn(), instead of duplicating all the code.
• Going further, I could even carve out some logic from the try-catch-finally block into separate methods that can be overridden in subclasses, further reducing code duplication.

Does that make sense?

[spring-projects-issues]

Steffen Ryll said:

Just to clarify about the attached class -- I'm aware it doesn't fit into Spring Security in the current form, but I'm definitely willing to bring it into the right shape and make it easy to include. I'm only not sure what approach you would see as most helpful.

Or just turn this ticket down, that would be ok with me as well.

[spring-projects-issues]

Luke Taylor said:

I don't think it's even the case that there are only two main patterms of LDAP usage. In my experience there are many different setups. It's rare that an application will be assigned any user identity which gives it any more privileges than necessary (similar to JDBC) and so a "manager" user is often one which is allowed to perform searches but doesn't have the ability to read user data. I'd prefer not to add more complexity to the existing classes. These days, when the out-of-the-box functionality doesn't fit a particular use case, we tend to encourage integration at the interface level rather than the use of more complex inheritance-based solutions and the exposure of more class internals. I think this is one of these cases. Implementing AuthenticationProvider directly is often the cleanest option for some use cases.

[spring-projects-issues]

Steffen Ryll said:

Alright, I understand your motivation and your advice works for me. Nevertheless, thanks for considering my idea.
PS: I had to smile when I read that you explicitly included this advice in your recent article about authentication in GAE ;-)

[spring-projects-issues]

Adam Lewandowski said:

Sorry for commenting on and old closed issue, but I have run in to variations of this problem several times and wanted to post an alternate solution here in case anyone else has a need for it. I've created a version of BindAuthenticator that allows you to specify (via configuration) whether user attributes should be retrieved at all as part of the user bind, and if so, to allow those attributes to be retrieved using the 'manager' context source instead of the user's context. I've encountered LDAP configurations that needed these options at clients multiple times and have found this custom implementation useful.

I guess JIRA isn't going to let me attach a file to a closed issue, but the relevant code was submitted as a pull request on github: #9