SEC-1979: Constants for ShaPasswordEncoder

[spring-projects-issues]

Daniel Holmes (Migrated from SEC-1979) said:

Would be nice to have this class provide public constants (or enum) so that
client code could have a more readable name. As an enum also would be more specific to allowed values as well.

example
new ShaPasswordEncoder(256);
could be
new ShaPasswordEncoder(SHA256);

[spring-projects-issues]

Luke Taylor said:

I don't think this is really worthwhile. It would break the existing API, which is normally used through dependency injection anyway. The current API also maps directly to the SHA strengths supported by the JCA provider.

I'd regard this as being legacy code. Nobody should really be using SHA hashes for password encoding unless they are forced to do so for an existing system. And if they are they should be looking to upgrade. Even then, an existing system will hopefully at least be using salt (unlike linkedin), and the hashes are unlikely to be compatible with this class.

I do think we should probably add some notes to the docs and Javadoc to explain that people shouldn't be actively choosing these classes for password hashing, but should use something like BCrypt instead.

[spring-projects-issues]

Daniel Holmes said:

I'm not sure I understand how adding

public static in SHA-256 = 256;

to the class would break the interface. An enum would require another overloaded constructor, but still not sure how that would break the interfce.

Also not sure I understand why you say this should not be used for password hashing. This is the current NIST recommendation (which we are having to follow) http://csrc.nist.gov/groups/ST/toolkit/secure_hashing.html#Approved%20Algorithms Where does the bcrypt recommendation come from?

[spring-projects-issues]

Luke Taylor said:

Having an additional constructor which takes an enum would then mean there are two single-argument constructors, which would cause problems when using dependency injection in XML, for example, which is how an instance will usually be configured.

That NIST document you've linked to is a recommendation for hashing algorithms in general, not for storing passwords. The requirements are not the same and in some ways are completely orthogonal - the properties you want for calculating a digital signature are not at all the same as those for an algorithm designed to hinder a password cracker.

There's no question about it - using a simple unsalted SHA hash to store passwords is a very bad choice. The recent linkedin fiasco should be enough to remove any trace of doubt about that. I'd really recommend you do some additional research. The choice should probably be between BCrypt, PBKDF2 and scrypt, as explained here:

http://security.stackexchange.com/questions/4781/do-any-security-experts-recommend-bcrypt-for-password-storage

[spring-projects-issues]

Daniel Holmes said:

Thanks for the link. I'll have a discussion with the security guy here that is providing the requirements. We are salting.

Yea, understood on the enum aspect. Static constant ints would be nice, but if these APIs are really lesser used at this point I guess it is no big deal. You can either close this or make it low priority.

[spring-projects-issues]

Rob Winch said:

To add a bit to the discussion...

I agree with Luke that adding this is not really worthwhile. Adding the constants seems to provide little value since the constants that are available are dependent on what is provided by the JDK. Additionally, the crypto PasswordEncoder should be used rather than the core PasswordEncoder's. A few good reasons to use the crypto module:

As Luke pointed out BCrypt is quickly emerging as a replacement for SHA. I realize this is not reflected on NIST, but you can find many examples of others recommending it and providing sound reasoning behind their recommendations.

When using SHA it is recommended to provide multiple rounds of hashing rather than a single round that is done with the core libraries

It is best to use a random salt rather than the old approach of deriving the salt from a user property.

[spring-projects-issues]

Rob Winch said:

Resolving as Won't Fix per the discussion. Note that I have created SEC-1982 to update the documentation as discussed on the comments.

[spring-projects-issues]

This issue relates to #2206