On 09/17/2014 04:36 AM, Mark Thomas wrote:
On 16/09/2014 22:14, Christopher Schultz wrote:
Mark,
On 9/16/14 3:39 PM, Mark Thomas wrote:
Updated patch:
http://people.apache.org/~markt/patches/2014-09-16-bug56403-tc8-v2.patch
It's looking good!
Looks good, but its missing a configuration for the digester to actually
read the configuration and set-up the CredentialHandler objects at
runtime. Existing MessageDigest-based configs will work, but explicit
class references won't.
Ack. The docs need updating as well.
Speaking of which, I'd like to be able to nest CredentialHandler
instances. The use case is when switching from one type of
password-derivation method to another. We have done this at $work twice
and being able to handle more than one kind of valid credential in the
database is essential.
OK. That seems like a reasonable requirement.
Given that we are giving better options to users than standard
single-pass MessageDigest password-mutators, we should help them
migrate. The only way to do that would be something like
CombinedCredentialHandler analogous to the CombinedRealm: you will
accept either MessageDigestCredentiaHandler{SHA1} /or/, say, PBKDF,
bcrypt, etc., by checking one CredentialHandler and then the second (or
third?) if the first one fails.
This would be good. One could have an array of CredentialHandler to
check in order. Is the idea that a password stored in an old format
would be matched using the old CredentialHandler and (upon first match)
stored in the upgraded format (the first CredentialHandler)? I assume
the same idea goes for when the same CredentialHandler is used but the
number of iterations has changed.
Use of a CombinedCredentialHandler might result in a lot of spurious
warnings in the log about invalid credentials. Maybe the
CombinedCredentialHandler could tell the individual child
CredentialHandlers that they should not log invalid credentials?
Yes, we'll need to make sure the logs don't fill up with false positives.
+1
I'd like to get some other opinions on the public mutate() interface. I
think we might not be able to convince each other ;)
You might be surprised. I was looking at using mutate() from match to
reduce code duplication but if you limit mutate() to just generating new
passwords then I agree there is no need for any other parameters. A
protected method used by both mutate() and match() should work. I'll
take another look.
Without giving an opinion about what method should be called when, I
agree that salt size and iteration count should be taken from the stored
credential when matching. If the latest configuration asks for a
different algorithm, salt size, or iteration count, then the supplied
cleartext password should be re-hashed to the latest specs.
Regarding thread safety for the SecureRandom, we can do that now. It
doesn't cost us very much and it prevents it tripping us up in the future.
Mark
I saw that String.equals(String s) is being used. I'm not familiar with
the implementation but I imagine that if the string lengths differ or if
the first characters don't match, for example, the method returns false
without checking the rest of the characters. Perhaps that could lead to
a small vulnerability in which through many attempts and timing an an
attacker can infer whether the password length, etc. is right. I've
seen some implementations use a SecureEquals that tries to take
approximately the same time by comparing all characters of the strings
even if the lengths or first characters don't match. Is this a real
concern, or only theory?
Gabriel
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
