shiro-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Les Hazlewood <lhazlew...@apache.org>
Subject Re: Password hashing - getting the salt from AuthenticationInfo
Date Wed, 20 Oct 2010 08:20:34 GMT
On Tue, Oct 19, 2010 at 9:47 PM, Kalle Korhonen
<kalle.o.korhonen@gmail.com> wrote:
> The new SaltedAuthenticationInfo interface is an interesting take on
> this. With it, we are rolling the responsibility of storing and
> retrieving the salt on users. The implementation I'm currently using
> was based on adding getSalt(AuthenticationInfo token) to a
> HashedCredentialsMatcher and let it decode the salt from the
> credentials. The advantage is that there's no need for new
> AuthenticationInfo interface and that is easy to provide a default
> implementation. Obviously the new interface is more flexible but I
> wonder whether anybody ever needs that flexibility. Since you always
> need the salt and the hashed password together, you can just as well
> store them together (like Unix-style passwords). Take it as a general
> comment  - the differences are minor, I'm not too particular on any
> given approach and we can still provide a default implementation that
> for example assumes a fixed-with salt stored with the hash.

This is great feedback - thanks.  I never thought of bundling the salt
and the password together and representing them both as the return
value from the AuthenticationInfo.getCredentials() method.  It is
interesting to see how it could be done alternatively.

My thought process for the current implementation was that I see salts
and passwords as two different things and I thought our API should
reflect that. In my own experience at least, I typically store salts
as a separate field in a relational database or LDAP server, so it
made sense to me to have a 1-to-1 mapping with an AuthenticationInfo
property.  Either approach will work of course - this is just the one
I landed with :)

One of the benefits of keeping the existing mechanism though is that I
think it will be easier on end-users:  They already have to look up
account data and represent it as an AuthenticationInfo, so it's no
extra work to have them include a salt at that time.  If we didn't
represent this concretely in an AuthenticationInfo interface, they
would have to do as you suggested and additionally create a custom
HashedCredentialsMatcher implementation to extract the salt.  At least
with the current implementation, that's one less custom class they'd
have to learn about and create, and that seems like a pretty nice
plus.

> salt is better than no salt - but since random per-user-salt doesn't
> cost any extra and is much more secure, it doesn't make sense to use
> anything else.

Agreed - and at least as is demonstrated by the test case, it is
really easy to have this out of the box now.  Hopefully people will be
happy with it.

> Perhaps just for symmetry, SimpleAuthenticationInfo.setSalt() should
> be renamed to setCredentialsSalt(). Obviously it's not in the
> interface, but the getter is called getCredentialsSalt().

Excellent catch and I definitely agree.  I've fixed that and just committed it.

Thanks for the review - I'm always grateful for it!

Les

Mime
View raw message