Salt for UserPassword in Seam 2.1

[kapitanpetko]

I've been looking into Seam 2.1 security, and here's a feature I think would be useful.

If we want to change the password hashing algorithm, we have to override PasswordHash (btw, SHA-1 should probably the default), that's easy enough. But if we want to use a salt value different from the username (the default), we need to override JpaIdentityStore, and it seems one should not generally need to do this. The default implementation just uses the username string, but in most cases we'd probably like to use a dedicated random value and save it in the UserPrincipal entity. In addition, one would generally loop a number of times (makes generating hashes harder), when generating the hash (as per PKCS#5), so the inputs for generating the password hash become: password, salt and iteration count. It would be nice if one could have all of these in one place, the UserPrincipal entity.

To support this we need one new annotation and a few params to @UserPassword. Something like:

@UserPasswordSalt
String getSalt()

@UserPassword(hash="sha1", saltLength=64, iterationCount=1000)
String getPasswordHash()

Salt length is in bits and is randomly generated by createUser, iteration count would be passed to PasswordHash.generatedSaltedHash.
This more general implementation should cover the most common cases, I think, and one would rarely need to override PasswordHash or JpaIdentityStore.

How does this sound?

[joblini]

Added to your suggestions, I would like add the ability to specify any algorithm, via EL, and not be limited to SHA1 or MD5.  Just pass the string specifying the algorithm as is to MessageDigest.

[shane.bryzak]

This solution is not abstract enough to cater for all the other possible salt values that can be dreamt up.  The getUserAccountSalt() method was absolutely designed to be easily overridable if you want to use an alternative salt to the username.

[shane.bryzak]

The algorithm string is already passed as is, you're not limited to md5 or sha.

[kapitanpetko]

Shane Bryzak wrote on Nov 26, 2008 01:52:


This solution is not abstract enough to cater for all the other possible salt values that can be dreamt up. 

No, it is not. But this being related to security, 'dreaming up' a solution is usually not a good idea.
It is generally accepted that the salt should be a long enough random value, and it should be easy enough to support this.
Besides, you have to store the salt somewhere, so having a UserPasswordSalt on you principal entity makes sense, I think.

Other major frameworks support random salt by default: ASP.NET's SqlMembershipProvider has a PasswordSalt column in the
user table and Spring Security has a configurable salt source. If we want to further abstract salt generation/selection, we
could have something similar to Spring's salt source, but having a UserPasswordSalt should be sufficient for most cases,
I think. Here is how you configure salt in Spring Security:

<password-encoder hash="sha">
  <salt-source user-property="username"/>
</password-encoder>

The getUserAccountSalt() method was absolutely designed to be easily overridable if you want to use an alternative salt to the username.

Yes, it is easy enough to override, but what I am saying is that one should not need to override the whole IdentityStore component
just to select the salt.

[shane.bryzak]

Ok I guess this feature would be useful, and if people don't want to use it then there's always the alternative.  Please raise a feature request in JIRA.

[joblini]

Shane Bryzak wrote on Nov 26, 2008 01:53:


The algorithm string is already passed as is, you're not limited to md5 or sha.

Ok, thanks for clearing that up, the Seam ref doc page 254 says

Possible values for hash are md5, sha and none.

Would it be possible to support EL so we could do something like:

@UserPassword(hash = "#{applicationProperties.get('security.password.digestAlgorithm')}")

[shane.bryzak]

Would it be possible to support EL so we could do something like:

@UserPassword(hash = "#{applicationProperties.get('security.password.digestAlgorithm')}")

I guess so - raise this in JIRA as a feature request.

[joblini]

OK, thanks.  JBSEAM-3761

[kapitanpetko]

Shane Bryzak wrote on Nov 26, 2008 04:01:


Ok I guess this feature would be useful, and if people don't want to use it then there's always the alternative.  Please raise a feature request in JIRA.

Done: JBSEAM-3762

Since iteration count is actually a different issue (Cf. my first post), I have created a separte FR: JBSEAM-3763