You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@shiro.apache.org by Les Hazlewood <lh...@apache.org> on 2010/10/20 05:17:15 UTC
Password hashing - getting the salt from AuthenticationInfo
Hi folks,
I've committed a good bit of code to trunk that allows for safe
acquisition of a salt from AuthenticationInfo when hashing passwords.
I also created a nice little RandomNumberGenerator abstraction which
can be used for generating random (and secure) salts, initialization
vectors, or any other type of cryptographic seed data. Shiro
end-users can use this in their own apps for user-account salt
creation. This is _much_ better and safer than using any
account-related data (e.g. username or something else) as the salt.
Also, existing AuthenticationInfo implementations have been updated to
support this.
I've written a few test cases to verify that the new behavior works
and that I've retained backwards compatibility (although it is highly
recommended to use the new approach since user-submission-derived
salts are dangerous). See the HashedCredentialsMatcherTest
'testSaltedAuthenticationInfo' test case to see a good/common example
of how the new salt support would work in a typical application.
Anyway, feedback is welcome. If I don't hear anything, I'll resolve
the issue and consider it finished for 1.1 (issue: Here's the issue:
https://issues.apache.org/jira/browse/SHIRO-186)
Cheers,
--
Les Hazlewood
Founder, Katasoft, Inc.
Application Security Products & Professional Apache Shiro Support and Training:
http://www.katasoft.com
Re: Password hashing - getting the salt from AuthenticationInfo
Posted by Les Hazlewood <lh...@apache.org>.
>> Of all 30+ files that were changed for this, there was only one method
>> that ended up being backwards incompatible. That doesn't sound very
>> brittle to me ;)
>
> You changed RedirectView's signature. That's not backward compatible. Again, the reason for this problem is because of the extensive use of inheritance for complex and brittle interactions with their implementations.
My comment was an illustration at how most of that code used
composition (instantiating an object - StringBuilder - instead of
receiving it from a protected method through inheritance). Only one
method didn't do this, and I already stated it wasn't backwards
compatible.
While there are areas of the code that would do well to favor
composition over inheritance, the large majority of the code base
already does this, so I think it might be a bit misleading for you to
represent us as "'deprecation thrashing' because of the extensive use
of inheritance". It doesn't happen very often, and where it has, it
has been localized to a few things that aren't that critical in the
grand scheme of the project.
But I'm pretty sure we do agree in principle :).
Les
Re: Password hashing - getting the salt from AuthenticationInfo
Posted by "Alan D. Cabrera" <li...@toolazydogs.com>.
On Oct 23, 2010, at 1:08 PM, Les Hazlewood wrote:
>> Customization is fine. Delegation is fine. But these capabilities don't require that the implementation be based on abstract classes with complex and brittle interactions with their implementations. I might even go so far as to say that extensive use of delegation via inheritance is an anti-pattern.
>
> I'll wait for the other threads, but please explain how you might do
> this otherwise using the SecurityManager as an example. Maintaining a
> 3000-line class is not a good option IMO (I'm sure you have great
> ideas, I would just like us to not have to maintain that type of
> file).
>
>> Also, having all those protected methods out there makes the API very brittle. We saw this when we converted the code to start using StringBuilder.
>
> Of all 30+ files that were changed for this, there was only one method
> that ended up being backwards incompatible. That doesn't sound very
> brittle to me ;)
You changed RedirectView's signature. That's not backward compatible. Again, the reason for this problem is because of the extensive use of inheritance for complex and brittle interactions with their implementations.
>
> But I understand your point - it's good to lock down where you can.
> Most of the protected methods exist from the (very old) JSecurity days
> and could use a bit of review.
>
>> I think Kalle and I are talking about a new architecture based on what we've learned so far. I'm sure 2.0 is a long way off but it doesn't hurt to chat about it.
>
> I'm all for it - I think talking about 2.x now is a good idea. One of
> the big things I want to do for 2.x is split the shiro-core module
> into different modules (authc, authz, etc). This will probably
> involve moving some classes and packages around in order to make all
> those new modules OSGi-compliant - something we can only do in a new
> major release.
Good stuff!
Regards,
Alan
Re: Password hashing - getting the salt from AuthenticationInfo
Posted by "Alan D. Cabrera" <li...@toolazydogs.com>.
On Oct 23, 2010, at 2:15 PM, Les Hazlewood wrote:
> I just committed and resolved https://issues.apache.org/jira/browse/SHIRO-204
>
> Please check it out and review. Feel free to re-open the issue if
> I've missed anything. All changes were 100% backwards compatible.
> Here's the commit log:
>
> "committed implementation. HashedCredentialsMatcher is now no longer
> abstract and all of its subclasses have been deprecated. Introduced
> new SimpleHash implementation to allow ad-hoc hash algorithms to be
> used and all existing AbstractHash subclasses (Md5Hash, etc) have been
> updated to subclass SimpleHash instead of AbstractHash directly.
> AbstractHash has been marked deprecated."
So, all this "deprecation thrashing" is because of the extensive use of inheritance.
SimpleHash still suffers from the swiss army knife syndrome but the above work is a definite improvement. Maybe there's a reason for doing it this way, bundling the and I don't see it.
Regards,
Alan
Re: Password hashing - getting the salt from AuthenticationInfo
Posted by Les Hazlewood <lh...@apache.org>.
I just committed and resolved https://issues.apache.org/jira/browse/SHIRO-204
Please check it out and review. Feel free to re-open the issue if
I've missed anything. All changes were 100% backwards compatible.
Here's the commit log:
"committed implementation. HashedCredentialsMatcher is now no longer
abstract and all of its subclasses have been deprecated. Introduced
new SimpleHash implementation to allow ad-hoc hash algorithms to be
used and all existing AbstractHash subclasses (Md5Hash, etc) have been
updated to subclass SimpleHash instead of AbstractHash directly.
AbstractHash has been marked deprecated."
Cheers,
Les
Re: Password hashing - getting the salt from AuthenticationInfo
Posted by Les Hazlewood <lh...@apache.org>.
> Customization is fine. Delegation is fine. But these capabilities don't require that the implementation be based on abstract classes with complex and brittle interactions with their implementations. I might even go so far as to say that extensive use of delegation via inheritance is an anti-pattern.
I'll wait for the other threads, but please explain how you might do
this otherwise using the SecurityManager as an example. Maintaining a
3000-line class is not a good option IMO (I'm sure you have great
ideas, I would just like us to not have to maintain that type of
file).
> Also, having all those protected methods out there makes the API very brittle. We saw this when we converted the code to start using StringBuilder.
Of all 30+ files that were changed for this, there was only one method
that ended up being backwards incompatible. That doesn't sound very
brittle to me ;)
But I understand your point - it's good to lock down where you can.
Most of the protected methods exist from the (very old) JSecurity days
and could use a bit of review.
> I think Kalle and I are talking about a new architecture based on what we've learned so far. I'm sure 2.0 is a long way off but it doesn't hurt to chat about it.
I'm all for it - I think talking about 2.x now is a good idea. One of
the big things I want to do for 2.x is split the shiro-core module
into different modules (authc, authz, etc). This will probably
involve moving some classes and packages around in order to make all
those new modules OSGi-compliant - something we can only do in a new
major release.
Great stuff - I look forward to the other threads!
Les
Re: Password hashing - getting the salt from AuthenticationInfo
Posted by "Alan D. Cabrera" <li...@toolazydogs.com>.
On Oct 23, 2010, at 11:59 AM, Les Hazlewood wrote:
> Hi Alan and Kalle,
>
> Thanks for chiming in on this.
>
> But I'm not sure that inheritance is used too often. Sure, it is used
> in some places where it shouldn't be and that there are definitely
> places where we can and should eliminate it. For example, the
> CodecSupport class and the HashedCredentialsMatcher hierarchy can be
> deprecated now and then removed in a 2.0 release (to retain backwards
> compatibility).
>
> But if you look at pretty much all of Shiro's inheritance mechanisms
> (SecurityManager and its subclasses, Realm and its subclasses,
> SessionManager and its subclasses, etc), almost _all_ of those things
> are purely wrapper classes that delegate to internal pluggable
> components. Almost all of it is composition.
I snipped most of this email because there are a number of good things discussed here. Using a single thread to discuss them all would be confusing and inefficient. I'll post a reply to them all in separate emails. :)
I will add a few comments below to give a taste of my thinking.
> Anyway, I'm all for cleaning up things where we can, and making it
> even more pluggable - but the level of customization you can do today
> due to almost everything relying on delegation is *huge* compared to
> most frameworks.
Customization is fine. Delegation is fine. But these capabilities don't require that the implementation be based on abstract classes with complex and brittle interactions with their implementations. I might even go so far as to say that extensive use of delegation via inheritance is an anti-pattern.
Also, having all those protected methods out there makes the API very brittle. We saw this when we converted the code to start using StringBuilder.
> About versions: I think we should do as much of this type of cleanup
> as is possible for 1.2 where it is %100 backwards-compatible based on
> APR versioning guidelines. We're a TLP project, and a lot more people
> are using our software today - I would like to ensure that upgrades
> don't cause people API grief (not including the very occasional change
> of internal code that is backwards compatible, but wasn't expected to
> be used by end-users).
Yep, hence my point in discussing "2.0".
> In fact, I think the HashedCredentialsMatcher changes can be done now,
> for 1.1. I was in the process of making similar changes when working
> on the Salt support, but kept those changes in a separate change list
> and didn't commit them. I can probably have that stuff in in an hour
> or two for your review.
Minor releases for backward compatible cleanups are fine.
I think Kalle and I are talking about a new architecture based on what we've learned so far. I'm sure 2.0 is a long way off but it doesn't hurt to chat about it.
Regards,
Alan
Re: Password hashing - getting the salt from AuthenticationInfo
Posted by Les Hazlewood <lh...@apache.org>.
Hi Alan and Kalle,
Thanks for chiming in on this.
But I'm not sure that inheritance is used too often. Sure, it is used
in some places where it shouldn't be and that there are definitely
places where we can and should eliminate it. For example, the
CodecSupport class and the HashedCredentialsMatcher hierarchy can be
deprecated now and then removed in a 2.0 release (to retain backwards
compatibility).
But if you look at pretty much all of Shiro's inheritance mechanisms
(SecurityManager and its subclasses, Realm and its subclasses,
SessionManager and its subclasses, etc), almost _all_ of those things
are purely wrapper classes that delegate to internal pluggable
components. Almost all of it is composition.
These class hierarchies for these things exist purely to logically
separate behavior associated with respective state (the internal
delegates). If we didn't do this for example, the SecurityManager
(which mostly uses delegation for 95% of its logic) would be a _huge_
class - probably well over 3000 lines long.
The crypto hierarchy is a *good thing* to me actually, and I often
talk about this as a benefit in presentations and people (at least
from what I'm told afterwards) really appreciate it. There is a very
clear separation of behavior specific to certain types of ciphers, and
this classification most definitely follows a single-inheritance class
hierarchy in real life. Look at BouncyCastle's source code and you'll
see the exact same thing (for good reason). The JDK JCE mechanism,
that represents _all_ Ciphers possible with a single abstract class,
is absolute garbage and is a very large reason why people are so darn
confused by it ('transformation strings' as factory arguments and
procedural methods abound - yuck!).
As far as HashedCredentialsMatcher, I agree that this can be a single
class and all of its subclasses should be deprecated and removed
later. The alternative should be that HashedCredentialsMatcher
instead reflect the Hash algorithm that it will use for its matching,
e.g.:
HashedCredentialsMatcher matcher = new HashedCredentialsMatcher();
matcher.setHashAlgorithm(Sha256Hash.class); //retains type-safety
or
matcher.setHashAlgorithm("SHA-512"); //not type safe and potentially error-prone
the end-user could choose what they want based on their needs.
As for the Hash class hierarchy, I'm not sure if we should do the same
thing. It exists because of type safety, non-forced exceptions, and
to make end-users lives noticeably simpler: Calling
new Sha512Hash(aFile).toHex()
is _much_ nicer for end users than using the JDK's crappy
MessageDigest class. It is also type safe. Using a string to
represent the algorithm (e.g. new SimpleHash("SHA-512", aFile).toHex()
is less type-safe than the first example - you don't know if the
string is validly typed and you can't perform type-safe
algorithm-specific logic (e.g maybe enforcing a certain algorithm is
used). Now if we want to add a SimpleHash that takes in a String
argument as an alternative in the case where we don't represent a
common algorithm already, I think that's a smart idea. In fact, the
existing concrete Hash implementations could probably just subclass
that one for simplicity. Then end-users can choose whatever approach
they want.
Anyway, I'm all for cleaning up things where we can, and making it
even more pluggable - but the level of customization you can do today
due to almost everything relying on delegation is *huge* compared to
most frameworks.
About versions: I think we should do as much of this type of cleanup
as is possible for 1.2 where it is %100 backwards-compatible based on
APR versioning guidelines. We're a TLP project, and a lot more people
are using our software today - I would like to ensure that upgrades
don't cause people API grief (not including the very occasional change
of internal code that is backwards compatible, but wasn't expected to
be used by end-users).
In fact, I think the HashedCredentialsMatcher changes can be done now,
for 1.1. I was in the process of making similar changes when working
on the Salt support, but kept those changes in a separate change list
and didn't commit them. I can probably have that stuff in in an hour
or two for your review.
If there are any other areas where you guys think this stuff can and
should be changed, please bring it up! Better to get this stuff
addressed early and make issues for them.
Best,
Les
Re: Password hashing - getting the salt from AuthenticationInfo
Posted by "Alan D. Cabrera" <li...@toolazydogs.com>.
On Oct 23, 2010, at 10:29 AM, Kalle Korhonen wrote:
> On Sat, Oct 23, 2010 at 8:24 AM, Alan D. Cabrera <li...@toolazydogs.com> wrote:
>> Some comments. IMO, Shiro uses inheritance too much. It's a brittle practice that I think we're now starting to see the cracks; e.g. the conversion to use StringBuilder.
>> It seems that are some hashing implementation details that have leaked into the abstract methods, i.e. hashing iterations.
>> I'm not a fan of mixing data with code that manipulates it, e.g. the Hash hierarchy. Just a personal preference. Things start to end up looking like swiss army knives and Hash seems to be a good example.
>>
>
> Agree - that's one of my pet peeves with Shiro as well, and especially
> visible in the crypto/hash packages. Most of the code there is not
> really required. Question is, when would be a good time to clean it
> up? Perhaps for 1.2.
IMO, such a reorg should be 2.0. I would love to see a set of simple generics based interfaces with a modicum of POJOs fleshed out first and then implementations filled in afterward.
Regards,
Alan
Re: Password hashing - getting the salt from AuthenticationInfo
Posted by Kalle Korhonen <ka...@gmail.com>.
On Sat, Oct 23, 2010 at 8:24 AM, Alan D. Cabrera <li...@toolazydogs.com> wrote:
> Some comments. IMO, Shiro uses inheritance too much. It's a brittle practice that I think we're now starting to see the cracks; e.g. the conversion to use StringBuilder.
> It seems that are some hashing implementation details that have leaked into the abstract methods, i.e. hashing iterations.
> I'm not a fan of mixing data with code that manipulates it, e.g. the Hash hierarchy. Just a personal preference. Things start to end up looking like swiss army knives and Hash seems to be a good example.
>
Agree - that's one of my pet peeves with Shiro as well, and especially
visible in the crypto/hash packages. Most of the code there is not
really required. Question is, when would be a good time to clean it
up? Perhaps for 1.2.
Kalle
Re: Password hashing - getting the salt from AuthenticationInfo
Posted by "Alan D. Cabrera" <li...@toolazydogs.com>.
On Oct 19, 2010, at 8:17 PM, Les Hazlewood wrote:
> Hi folks,
>
> I've committed a good bit of code to trunk that allows for safe
> acquisition of a salt from AuthenticationInfo when hashing passwords.
> I also created a nice little RandomNumberGenerator abstraction which
> can be used for generating random (and secure) salts, initialization
> vectors, or any other type of cryptographic seed data. Shiro
> end-users can use this in their own apps for user-account salt
> creation. This is _much_ better and safer than using any
> account-related data (e.g. username or something else) as the salt.
>
> Also, existing AuthenticationInfo implementations have been updated to
> support this.
>
> I've written a few test cases to verify that the new behavior works
> and that I've retained backwards compatibility (although it is highly
> recommended to use the new approach since user-submission-derived
> salts are dangerous). See the HashedCredentialsMatcherTest
> 'testSaltedAuthenticationInfo' test case to see a good/common example
> of how the new salt support would work in a typical application.
>
> Anyway, feedback is welcome. If I don't hear anything, I'll resolve
> the issue and consider it finished for 1.1 (issue: Here's the issue:
> https://issues.apache.org/jira/browse/SHIRO-186)
This is great. I also needed this functionality.
Some comments. IMO, Shiro uses inheritance too much. It's a brittle practice that I think we're now starting to see the cracks; e.g. the conversion to use StringBuilder.
It seems that are some hashing implementation details that have leaked into the abstract methods, i.e. hashing iterations.
I'm not a fan of mixing data with code that manipulates it, e.g. the Hash hierarchy. Just a personal preference. Things start to end up looking like swiss army knives and Hash seems to be a good example.
Just my 2 cents. Thanks for getting this set up!
Regards,
Alan
Re: Password hashing - getting the salt from AuthenticationInfo
Posted by Kalle Korhonen <ka...@gmail.com>.
Don't see anything wrong. Use this thread for discussing the new salt
implementation.
Kalle
On Thu, Oct 21, 2010 at 4:00 PM, Mike K <mk...@semanticresearch.com> wrote:
>
> Sorry, posted too much info. Yes this has nothing to do with salting, but
> having grabbed the latest source whee Les posted the changes and it resulted
> in the JSESSION cookie:
> JSESSIONID=61112b06-04f5-4e7c-af87-b84c3901d4af; Path=/service; Max-Age=-1;
> which is causing it to be deleted by the browser.
> I can create a JIRA for this, but was not sure if this is intentional for
> 1.1-SNAPSHOT
> --
> View this message in context: http://shiro-developer.582600.n2.nabble.com/Password-hashing-getting-the-salt-from-AuthenticationInfo-tp5653460p5660714.html
> Sent from the Shiro Developer mailing list archive at Nabble.com.
>
Re: Password hashing - getting the salt from AuthenticationInfo
Posted by Mike K <mk...@semanticresearch.com>.
Sorry, posted too much info. Yes this has nothing to do with salting, but
having grabbed the latest source whee Les posted the changes and it resulted
in the JSESSION cookie:
JSESSIONID=61112b06-04f5-4e7c-af87-b84c3901d4af; Path=/service; Max-Age=-1;
which is causing it to be deleted by the browser.
I can create a JIRA for this, but was not sure if this is intentional for
1.1-SNAPSHOT
--
View this message in context: http://shiro-developer.582600.n2.nabble.com/Password-hashing-getting-the-salt-from-AuthenticationInfo-tp5653460p5660714.html
Sent from the Shiro Developer mailing list archive at Nabble.com.
Re: Password hashing - getting the salt from AuthenticationInfo
Posted by Kalle Korhonen <ka...@gmail.com>.
Hmm.. that's the rememberMe cookie - shouldn't have anything to do
with password salting?
Kalle
On Thu, Oct 21, 2010 at 3:38 PM, Mike K <mk...@semanticresearch.com> wrote:
>
> Les, it is not yet exactly working. I am seeing a problem with the source I
> pulled yesterday - the cookie expires immediately:
>
> SESSIONID=61112b06-04f5-4e7c-af87-b84c3901d4af; Path=/service; Max-Age=-1;
> HttpOnly rememberMe=deleteMe; Path=/service; Max-Age=0; Expires=Wed,
> 20-Oct-2010 22:30:21 GMT
>
> Is there new configuration that I am missing or is this a bug?
>
> Another question, how do you ensure that the matcher knows that the string
> representing the salt is base64?
>
>
>
> --
> View this message in context: http://shiro-developer.582600.n2.nabble.com/Password-hashing-getting-the-salt-from-AuthenticationInfo-tp5653460p5660670.html
> Sent from the Shiro Developer mailing list archive at Nabble.com.
>
Re: Password hashing - getting the salt from AuthenticationInfo
Posted by Les Hazlewood <lh...@apache.org>.
Not sure what is going on w/ the rememberme cookie, but as to your
hashing/base64 question, that should be covered in the realm page
(http://shiro.apache.org/realm.html). If it's cached, you'll need to
hit refresh.
Anyway, here's how to enable that (in INI at least):
# base64 encoding, not hex in this example:
credentialsMatcher.storedCredentialsHexEncoded = false
This boolean flag is only evaluated if the credentials returned from
the AuthenticationInfo is a char[] or String. Otherwise, it is
assumed to be 'byte source compatible' already. (aside: I'm thinking
we should make base64 the assumed default - pretty much most of the
rest of the framework assumes that already).
Since AuthenticationInfo already returns a ByteSource for the salt,
there shouldn't be any problems there *unless* you base64-encoded your
salt before storing it in the database. If so, you need to base64
decode it before wrapping it in a SimpleByteSource (if you put the
base64-encoded string directly in SimpleByteSource, this will only get
the string's bytes - not the original salt bytes).
Perhaps that is what was causing you problems?
The realm page will have more additional useful info. If you feel
anything relevant is missing regarding salting/hashing, please let me
know and I'll be happy to update it.
Cheers,
--
Les Hazlewood
Founder, Katasoft, Inc.
Application Security Products & Professional Apache Shiro Support and Training:
http://www.katasoft.com
Re: Password hashing - getting the salt from AuthenticationInfo
Posted by Mike K <mk...@semanticresearch.com>.
Les, it is not yet exactly working. I am seeing a problem with the source I
pulled yesterday - the cookie expires immediately:
SESSIONID=61112b06-04f5-4e7c-af87-b84c3901d4af; Path=/service; Max-Age=-1;
HttpOnly rememberMe=deleteMe; Path=/service; Max-Age=0; Expires=Wed,
20-Oct-2010 22:30:21 GMT
Is there new configuration that I am missing or is this a bug?
Another question, how do you ensure that the matcher knows that the string
representing the salt is base64?
--
View this message in context: http://shiro-developer.582600.n2.nabble.com/Password-hashing-getting-the-salt-from-AuthenticationInfo-tp5653460p5660670.html
Sent from the Shiro Developer mailing list archive at Nabble.com.
Re: Password hashing - getting the salt from AuthenticationInfo
Posted by Les Hazlewood <lh...@apache.org>.
Just a quick update - I've updated the Realm documentation to reflect
this. It still could use some more organization and cleanup, but it's
a start:
http://shiro.apache.org/realm.html
Cheers,
Les
On Wed, Oct 20, 2010 at 10:37 AM, Les Hazlewood <lh...@apache.org> wrote:
> Glad to hear it will work for you Mike. Thanks for the feedback!
>
> On Wed, Oct 20, 2010 at 9:35 AM, Mike K <mk...@semanticresearch.com> wrote:
>>
>> Looks good Les.
>> I have thought about the Unix password-like implementation Kalle mentioned,
>> but that seems more of a storage schema issue. In our case we are storing
>> the salt as a separate field in our HBase store.
>> I do prefer the API to have separate values the way you have implemented it.
>> --
>> View this message in context: http://shiro-developer.582600.n2.nabble.com/Password-hashing-getting-the-salt-from-AuthenticationInfo-tp5653460p5655666.html
>> Sent from the Shiro Developer mailing list archive at Nabble.com.
>>
>
Re: Password hashing - getting the salt from AuthenticationInfo
Posted by Les Hazlewood <lh...@apache.org>.
Glad to hear it will work for you Mike. Thanks for the feedback!
On Wed, Oct 20, 2010 at 9:35 AM, Mike K <mk...@semanticresearch.com> wrote:
>
> Looks good Les.
> I have thought about the Unix password-like implementation Kalle mentioned,
> but that seems more of a storage schema issue. In our case we are storing
> the salt as a separate field in our HBase store.
> I do prefer the API to have separate values the way you have implemented it.
> --
> View this message in context: http://shiro-developer.582600.n2.nabble.com/Password-hashing-getting-the-salt-from-AuthenticationInfo-tp5653460p5655666.html
> Sent from the Shiro Developer mailing list archive at Nabble.com.
>
Re: Password hashing - getting the salt from AuthenticationInfo
Posted by Mike K <mk...@semanticresearch.com>.
Looks good Les.
I have thought about the Unix password-like implementation Kalle mentioned,
but that seems more of a storage schema issue. In our case we are storing
the salt as a separate field in our HBase store.
I do prefer the API to have separate values the way you have implemented it.
--
View this message in context: http://shiro-developer.582600.n2.nabble.com/Password-hashing-getting-the-salt-from-AuthenticationInfo-tp5653460p5655666.html
Sent from the Shiro Developer mailing list archive at Nabble.com.
Re: Password hashing - getting the salt from AuthenticationInfo
Posted by Les Hazlewood <lh...@apache.org>.
On Tue, Oct 19, 2010 at 9:47 PM, Kalle Korhonen
<ka...@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
Re: Password hashing - getting the salt from AuthenticationInfo
Posted by Kalle Korhonen <ka...@gmail.com>.
On Tue, Oct 19, 2010 at 8:17 PM, Les Hazlewood <lh...@apache.org> wrote:
> I've committed a good bit of code to trunk that allows for safe
> acquisition of a salt from AuthenticationInfo when hashing passwords.
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.
> recommended to use the new approach since user-submission-derived
> salts are dangerous). See the HashedCredentialsMatcherTest
I wouldn't go as far as saying that static salt is dangerous - any
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.
> Anyway, feedback is welcome. If I don't hear anything, I'll resolve
> the issue and consider it finished for 1.1 (issue: Here's the issue:
> https://issues.apache.org/jira/browse/SHIRO-186)
Perhaps just for symmetry, SimpleAuthenticationInfo.setSalt() should
be renamed to setCredentialsSalt(). Obviously it's not in the
interface, but the getter is called getCredentialsSalt().
Kalle