Spring Security
  1. Spring Security
  2. SEC-1205

Add support for salted LDAP passwords to PasswordComparisonAuthenticator

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Won't Fix
    • Affects Version/s: None
    • Fix Version/s: 3.0.0 M2
    • Component/s: LDAP
    • Labels:
      None

      Description

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

      1. ssha_check_patch.diff
        3 kB
        Michael Laccetti
      2. ssha_check_test_patch.diff
        2 kB
        Michael Laccetti

        Activity

        Hide
        Luke Taylor added a comment -

        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.

        Show
        Luke Taylor added a comment - 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.
        Hide
        Michael Laccetti added a comment -

        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.

        Show
        Michael Laccetti added a comment - 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.
        Hide
        Luke Taylor added a comment -

        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).

        Show
        Luke Taylor added a comment - 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).
        Hide
        Luke Taylor added a comment -

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

        Show
        Luke Taylor added a comment - I've added a comment to PasswordComparisonAuthenticator's javadoc to clarify that it won't work with SSHA passwords in the directory.
        Hide
        Michael Laccetti added a comment -

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

        Show
        Michael Laccetti added a comment - Righty-o, I will just create a custom implementation and wait for v3 so I can use the BindAuthenticator again.

          People

          • Assignee:
            Luke Taylor
            Reporter:
            Michael Laccetti
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: