SEC-1205: Add support for salted LDAP passwords to PasswordComparisonAuthenticator #1455

Closed
spring-issuemaster opened this Issue Jul 21, 2009 · 13 comments

1 participant

@spring-issuemaster

Michael Laccetti (Migrated from SEC-1205) said:

Only SHA encoding is supported. Support for SSHA etc would be nice.

@spring-issuemaster

Michael Laccetti said:

This is a re-opened SEC-241 - the original implementation didn't support SSHA properly, that I've found. Will add a patch in a moment.

@spring-issuemaster

Michael Laccetti said:

Patch to the PasswordComparisonAuthenticator and mock test to support SSHA password validation. For reals!

@spring-issuemaster

Michael Laccetti said:

The words to describe the fix: because SSHA stuff is a SALTED hash, you need to pass in a salt! Calling passwordEncoder.encode(rawPass, null) means that SSHA will never be validated. I'm not sure how this snuck through? So, the fix is to get the password from LDAP, and use passwordEncoder.isPasswordValid(...) instead. I had to modify the mock test as well, since we do a lookup instead of a search.

@spring-issuemaster

Michael Laccetti said:

Also, please note that I worked off the 2.0.5 release tag, not the 2.x branch, nor do I know if this is an issue with the 3.x trunk.

@spring-issuemaster

Luke Taylor said:

You can't use SSHA with a compare operation. The salt is typically a random sequence of bytes over which you have no control. You don't pass it in. So unless you read the hashed password from the directory you can't know the salt value. For example running slappasswd twice with "password" as the argument

{SSHA}m0kvRry+8sP8fvU4QdduDL8isulpKFdc
{SSHA}BGT2MrwOQ2LRtfbYfMU9lXs67mhESZb9

If either of these had been generated within the directory, how would you know what the salt was?

@spring-issuemaster

Luke Taylor said:

Looking at your patch, it seems you have removed the compare operation and assumed that the password can be read from the directory. This may not be a valid assumption (and usually isn't for passwords in any secure system). The point of an LDAP "compare" operation is that it sends the value to the directory and the directory lets you know if it matches. That's what PasswordComparisonAuthenticator is for.

@spring-issuemaster

Michael Laccetti said:

Given that the passwords are hashed, I'm not sure there is a huge security risk in doing so. However, if the idea is to let the directory do the comparison, then the original statement that SSHA is supported is invalid, and should be marked as such.

@spring-issuemaster

Michael Laccetti said:

Also, the only reason that I ran into this issue is because the BindAuthenticator requires a Spring Security context, which can't be instantiated with a PropertyOverrideConfigurer, and the PasswordComparisonAuthenticator can (in 2.0.5). If 3 were out, I would happily go back to using the BindAuthenticator, and you could tell me to stuff my ideas. :)

@spring-issuemaster

Luke Taylor said:

Hashing passwords doesn't mean that it's OK to make them accessible and there are plenty of organizations which would not accept this. The best place for them is still in the directory and it's most common to restrict access to bind (or compare) operations and writing by the user.

I'm not sure what you mean by "the original statement that SSHA is supported is invalid". LdapShaPasswordEncoder supports SSHA for both encoding (if you supply a salt) and validation of a supplied password.

@spring-issuemaster

Michael Laccetti said:

I'm not sure why you, as a developer, are trying to force security procedures on organizations - for the ones who don't provide access to read, great, they don't use it. For those that can, then why not let them?

SEC-241: "Add support for salted LDAP passwords to PasswordComparisonAuthenticator" - not true, since you call passwordEncoder.encodePassword(password, null) - null as a salt means SSHA will never work. I'm glad you added SSHA encoding to LdapShaPasswordEncoder, and you'll see that I do use it (via the patch's call to isPasswordValid), but I was talking about the authenticator itself.

@spring-issuemaster

Luke Taylor said:

We aren't forcing a procedure on any organization. Changing the behaviour to rely on being able to read the password, as this patch does, would break the implementation for existing users. Anyone can still implement the LdapAuthenticator interface differently (as you have effectively done). We just support the two most common approaches - using LDAP "bind" and "compare".

SEC-241 is an Acegi Security issue from over 3 years ago (and hence not a good source of documentation for the current version of the framework :) ). In fact in those days the code did attempt to make a local comparison of the password. That behaviour was later removed (see SEC-560). Authenticating against the loaded LDAP password means you are not really using LDAP authentication at all - just using the stored data. The LdapuserDetailsService can be used to achieve the same thing (with DaoAuthenticationProvider and an LdapShaPasswordEncoder).

@spring-issuemaster

Luke Taylor said:

I've added a comment to PasswordComparisonAuthenticator's javadoc to clarify that it won't work with SSHA passwords in the directory.

@spring-issuemaster

Michael Laccetti said:

Righty-o, I will just create a custom implementation and wait for v3 so I can use the BindAuthenticator again. :)

@spring-issuemaster spring-issuemaster added this to the 3.0.0 M2 milestone Feb 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment