You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by Christopher Schultz <ch...@christopherschultz.net> on 2014/09/16 17:20:13 UTC
Discussion of pluggable password-derivation in Realms [Bug 56403]
All,
In reference to bug 56403
(https://issues.apache.org/bugzilla/show_bug.cgi?id=56403) and
specifically markt's proposed patch
(http://people.apache.org/~markt/patches/2014-09-16-bug56403-tc8-v1.patch),
I have the following comments.
I'm interested in what others have to say.
1. In terms of limiting converting to String values, we could base
everything on byte[] instead of String. That also makes it possible to
blank-out the credential arrays when we are done with them, slightly
improving security; as long as the j_password is never retrieved using
HttpServletRequest.getParameter, we can avoid String values entirely and
purge the characters from memory as soon as we are done. Nothing that I
know of can stop the memory manager from re-locating byte arrays in
memory (short of using native code to temporarily pin them, but that
gets very dangerous and messy).
2. I don't like CredentialHandler.mutate(String input, byte[] salt, int
iterations). I think it ties the method signature to the implementation
of the mutation algorithm. PBKDF2 for instance has both "salt" and
"iterations", but straight-up MD5 (e.g. our existing implementation)
does not. Also, non-hashing algorithms like any symmetric encryption
algorithm (e.g. AES, Blowfish, etc.) does not have such parameters, and
would require instead an encryption key. I think that mutate() should
only take a single argument but also have setters that can be called
from configuration, like this:
<Realm>
<CredentialHandler class="PBKDF2" saltSize="8" iterations="50000" />
</Realm>
In this case, the PBKDF2 class would have a setIterations(int) method
and a setSaltSize(int) method to set the number of salt bytes to use.
If we want the implementations for MessageDigest-based handlers as well
as things like PBKDF2 to extend the same base class that provides lots
of utility methods, that's find. I just don't like tying the interface
itself for the matches() method to a particular style of implementation.
3. MessageDigestCredentialHandler is good, but I think we ought to
push-down the iteration and salt members and methods from
BaseCredentialHandler into the subclass.
4. In MessageDigestCredentialHandler.matches, there are two uses of
StandardCharsets.ISO_8859_1 directly instead of using the client's
preferred character encoding. Is this a requirement of {MD5|SHA|SSHA}
formatting or an oversight?
5. matchesSaltIterationsEncoded method does not check for unexpected
formatting. Specifically, the values for sep1 and sep2 are never checked
to see if they are positive or that sep1 < sep2. I'm not sure which
would be better: StringArrayIndexOutOfBoundsException from the JVM or
something a little more nuanced from the CredentialHandler itself. The
JVM is going to perform the checks anyway... should we waste a few
cycles to check these values ourselves and throw a different kind of
exception, or is it unnecessary? (I suppose try/catch could be used to
convert SAIOOBE into something else with a better error message, but
try/catch has its own overhead as well).
6. Performance. There are places where we know the sizes of things
before they are allocated but don't use that to our advantage. An
example from CredentialHandlerBase.generate:
return HexUtils.toHexString(salt) + "$" + iterations + "$" +
serverCredential;
We know in advance the exact length of the string we need, but the
compiler will use a StringBuilder with a default capacity (16 characters
in Oracle Java 7). It would be better to pre-allocate the buffer we need:
StringBuilder credential = new StringBuilder(saltLength << 1 +
serverCredential.length() + 10 + 2);
credential.append(HexUtils.toString(salt))
.append('$')
.append(iterations)
.append('$')
.append(serverCredential);
return credential.toString();
I'm not sure if it matters too much, but a bit of space could be saved
by storing the iterations in hex encoding as well (max 8 characters
instead of 10).
7. Thread safety. Is it safe to check/set the "random" reference in
multiple threads? I suppose the worse thing that could happen would be
to create more SecureRandom objects than strictly necessary.
8. RealmBase.main checks all known CredentialHandlers for matching
algorithms (a bit inefficient, but its offline so who cares), but it
doesn't know about anyone's custom CH classes. We should allow the
caller to specify the CH class if they want, so they can use the
RealmBase driver with their own CredentiaHandler implementation.
I'm sure I'll come up with other comments later, but this ought to get
the ball rolling.
Thanks,
-chris
Re: Discussion of pluggable password-derivation in Realms [Bug 56403]
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Mark,
On 9/16/14 12:46 PM, Mark Thomas wrote:
> On 16/09/2014 17:17, Mark Thomas wrote:
>> On 16/09/2014 16:20, Christopher Schultz wrote:
>
>>> 2. I don't like CredentialHandler.mutate(String input, byte[] salt, int
>>> iterations). I think it ties the method signature to the implementation
>>> of the mutation algorithm. PBKDF2 for instance has both "salt" and
>>> "iterations", but straight-up MD5 (e.g. our existing implementation)
>>> does not. Also, non-hashing algorithms like any symmetric encryption
>>> algorithm (e.g. AES, Blowfish, etc.) does not have such parameters, and
>>> would require instead an encryption key. I think that mutate() should
>>> only take a single argument but also have setters that can be called
>>> from configuration, like this:
>>>
>>> <Realm>
>>> <CredentialHandler class="PBKDF2" saltSize="8" iterations="50000" />
>>> </Realm>
>>>
>>> In this case, the PBKDF2 class would have a setIterations(int) method
>>> and a setSaltSize(int) method to set the number of salt bytes to use.
>>>
>>> If we want the implementations for MessageDigest-based handlers as well
>>> as things like PBKDF2 to extend the same base class that provides lots
>>> of utility methods, that's find. I just don't like tying the interface
>>> itself for the matches() method to a particular style of implementation.
>>
>> I take the point but having fixed values for saltSize and iterations
>> then limits you to using the same values for every entry in the Realm
>> which creates a different set of issues. I'm not sure there is an API
>> that can meet all those requirements but it is worth spending some time
>> thinking about it.
>
> Hmm. mutate() needs the actual salt, not the salt size, and that is
> going to be different for every entry. I don't see a viable option apart
> from passing it in as a parameter that doesn't involve some form of ugly
> hack that defeats the benefit of a single parameter mutate method.
>
> I have some ideas for handling a variety of salt sizes and iteration
> counts but I don't yet see a way around mutate() needing the actual salt.
The "matches" method should not have to call the public one-arg "mutate"
and should therefore not need the salt at all: it will unpack the stored
credential which contains the salt and iteration count.
The salt and iteration count is only required when generating a /new/
password to be stored. Otherwise, all that information it packed into
the existing (stored) credential and can be checked.
Specifying the salt size and iteration count in the configuration will
only affect credentials that are created /after/ that node is launched.
Any credentials created in the past will use whatever salt length and
iteration count that were used when the credential was created.
I see you have written a number of responses. I'll try to consolidate my
responses to those instead of replying individually.
-chris
Re: Discussion of pluggable password-derivation in Realms [Bug 56403]
Posted by Mark Thomas <ma...@apache.org>.
On 16/09/2014 17:17, Mark Thomas wrote:
> On 16/09/2014 16:20, Christopher Schultz wrote:
>> 2. I don't like CredentialHandler.mutate(String input, byte[] salt, int
>> iterations). I think it ties the method signature to the implementation
>> of the mutation algorithm. PBKDF2 for instance has both "salt" and
>> "iterations", but straight-up MD5 (e.g. our existing implementation)
>> does not. Also, non-hashing algorithms like any symmetric encryption
>> algorithm (e.g. AES, Blowfish, etc.) does not have such parameters, and
>> would require instead an encryption key. I think that mutate() should
>> only take a single argument but also have setters that can be called
>> from configuration, like this:
>>
>> <Realm>
>> <CredentialHandler class="PBKDF2" saltSize="8" iterations="50000" />
>> </Realm>
>>
>> In this case, the PBKDF2 class would have a setIterations(int) method
>> and a setSaltSize(int) method to set the number of salt bytes to use.
>>
>> If we want the implementations for MessageDigest-based handlers as well
>> as things like PBKDF2 to extend the same base class that provides lots
>> of utility methods, that's find. I just don't like tying the interface
>> itself for the matches() method to a particular style of implementation.
>
> I take the point but having fixed values for saltSize and iterations
> then limits you to using the same values for every entry in the Realm
> which creates a different set of issues. I'm not sure there is an API
> that can meet all those requirements but it is worth spending some time
> thinking about it.
Hmm. mutate() needs the actual salt, not the salt size, and that is
going to be different for every entry. I don't see a viable option apart
from passing it in as a parameter that doesn't involve some form of ugly
hack that defeats the benefit of a single parameter mutate method.
I have some ideas for handling a variety of salt sizes and iteration
counts but I don't yet see a way around mutate() needing the actual salt.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Discussion of pluggable password-derivation in Realms [Bug 56403]
Posted by Mark Thomas <ma...@apache.org>.
On 16/09/2014 16:20, Christopher Schultz wrote:
> All,
>
> In reference to bug 56403
> (https://issues.apache.org/bugzilla/show_bug.cgi?id=56403) and
> specifically markt's proposed patch
> (http://people.apache.org/~markt/patches/2014-09-16-bug56403-tc8-v1.patch),
> I have the following comments.
>
> I'm interested in what others have to say.
>
> 1. In terms of limiting converting to String values, we could base
> everything on byte[] instead of String. That also makes it possible to
> blank-out the credential arrays when we are done with them, slightly
> improving security; as long as the j_password is never retrieved using
> HttpServletRequest.getParameter, we can avoid String values entirely and
> purge the characters from memory as soon as we are done. Nothing that I
> know of can stop the memory manager from re-locating byte arrays in
> memory (short of using native code to temporarily pin them, but that
> gets very dangerous and messy).
No objections to this if it is doable without too much impact to the
existing code but:
a) I'm not convinced it is worth it
b) I want to look at when the String -> bytes conversions would need to
take place.
> 2. I don't like CredentialHandler.mutate(String input, byte[] salt, int
> iterations). I think it ties the method signature to the implementation
> of the mutation algorithm. PBKDF2 for instance has both "salt" and
> "iterations", but straight-up MD5 (e.g. our existing implementation)
> does not. Also, non-hashing algorithms like any symmetric encryption
> algorithm (e.g. AES, Blowfish, etc.) does not have such parameters, and
> would require instead an encryption key. I think that mutate() should
> only take a single argument but also have setters that can be called
> from configuration, like this:
>
> <Realm>
> <CredentialHandler class="PBKDF2" saltSize="8" iterations="50000" />
> </Realm>
>
> In this case, the PBKDF2 class would have a setIterations(int) method
> and a setSaltSize(int) method to set the number of salt bytes to use.
>
> If we want the implementations for MessageDigest-based handlers as well
> as things like PBKDF2 to extend the same base class that provides lots
> of utility methods, that's find. I just don't like tying the interface
> itself for the matches() method to a particular style of implementation.
I take the point but having fixed values for saltSize and iterations
then limits you to using the same values for every entry in the Realm
which creates a different set of issues. I'm not sure there is an API
that can meet all those requirements but it is worth spending some time
thinking about it.
> 3. MessageDigestCredentialHandler is good, but I think we ought to
> push-down the iteration and salt members and methods from
> BaseCredentialHandler into the subclass.
Fair point if we start having CredentialHandlers based on symmetric
encryption. I'm not convinced that is a good idea but I take the point.
> 4. In MessageDigestCredentialHandler.matches, there are two uses of
> StandardCharsets.ISO_8859_1 directly instead of using the client's
> preferred character encoding. Is this a requirement of {MD5|SHA|SSHA}
> formatting or an oversight?
Don't know. It is what the current code does so I am reluctant to change
it without a report that something is broken.
> 5. matchesSaltIterationsEncoded method does not check for unexpected
> formatting. Specifically, the values for sep1 and sep2 are never checked
> to see if they are positive or that sep1 < sep2. I'm not sure which
> would be better: StringArrayIndexOutOfBoundsException from the JVM or
> something a little more nuanced from the CredentialHandler itself. The
> JVM is going to perform the checks anyway... should we waste a few
> cycles to check these values ourselves and throw a different kind of
> exception, or is it unnecessary? (I suppose try/catch could be used to
> convert SAIOOBE into something else with a better error message, but
> try/catch has its own overhead as well).
We should probably log a warning message for the sys admin.
> 6. Performance. There are places where we know the sizes of things
> before they are allocated but don't use that to our advantage. An
> example from CredentialHandlerBase.generate:
>
> return HexUtils.toHexString(salt) + "$" + iterations + "$" +
> serverCredential;
>
> We know in advance the exact length of the string we need, but the
> compiler will use a StringBuilder with a default capacity (16 characters
> in Oracle Java 7). It would be better to pre-allocate the buffer we need:
>
> StringBuilder credential = new StringBuilder(saltLength << 1 +
> serverCredential.length() + 10 + 2);
> credential.append(HexUtils.toString(salt))
> .append('$')
> .append(iterations)
> .append('$')
> .append(serverCredential);
>
> return credential.toString();
Fair enough.
> I'm not sure if it matters too much, but a bit of space could be saved
> by storing the iterations in hex encoding as well (max 8 characters
> instead of 10).
No strong opinion.
> 7. Thread safety. Is it safe to check/set the "random" reference in
> multiple threads? I suppose the worse thing that could happen would be
> to create more SecureRandom objects than strictly necessary.
It is only used in off-line mode so I think this is safe.
> 8. RealmBase.main checks all known CredentialHandlers for matching
> algorithms (a bit inefficient, but its offline so who cares), but it
> doesn't know about anyone's custom CH classes. We should allow the
> caller to specify the CH class if they want, so they can use the
> RealmBase driver with their own CredentiaHandler implementation.
That is doable but it raises some of the issues you've highlighted in 2)
and 3) above.
> I'm sure I'll come up with other comments later, but this ought to get
> the ball rolling.
>
> Thanks,
> -chris
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Discussion of pluggable password-derivation in Realms [Bug 56403]
Posted by Mark Thomas <ma...@apache.org>.
On 16/09/2014 16:20, Christopher Schultz wrote:
> StringBuilder credential = new StringBuilder(saltLength << 1 +
> serverCredential.length() + 10 + 2);
> credential.append(HexUtils.toString(salt))
> .append('$')
> .append(iterations)
> .append('$')
> .append(serverCredential);
You might want to remind yourself of the rules regarding operator
precedence. Particularly '<<' vs. '+'
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Discussion of pluggable password-derivation in Realms [Bug 56403]
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Gabriel,
On 9/22/14 7:56 PM, "Gabriel E. Sánchez Martínez" wrote:
>
> 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.
I think it makes sense to have a single CredentialHandler at the Realm
level. We can nest them if necessary.
> 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.
An old credential would always be matched by the old algorithm. No
auto-upgrade would occur or anything like that -- that's up to the
application to handle.
>>> 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?
While timing attacks are certainly possible, String.equals() is only
used at the very end of the operation and should be completely dominated
by the iterated hashing algorithm which happens regardless of the
correctness of the password.
What might contribute to a timing attack is avoiding the iterated
hashing operation under certain circumstances. For instance, in our own
implementation of DataSourceRealm, if the user does not exist, we waste
a bit of time hashing garbage and discard the result to avoid being able
to determine whether a user exists simply by attempting a login and
timing the response. We might want to have a "wasteTime" method added to
the CredentialHandler interface to allow the Realm to ask the CH to just
do a bunch of useless work to simulate a password match. Lazy
implementations could simply do nothing.
-chris
Re: Re: Discussion of pluggable password-derivation in Realms [Bug
56403]
Posted by "Gabriel E. Sánchez Martínez" <ga...@gmail.com>.
On 09/24/2014 12:27 PM, Mark Thomas wrote:
> On 24/09/2014 16:59, Christopher Schultz wrote:
>> Mark,
>>
>> On 9/24/14 5:00 AM, Mark Thomas wrote:
>>> On 23/09/2014 10:49, Mark Thomas wrote:
>>>> On 23/09/2014 00:56, "Gabriel E. Sánchez Martínez" wrote:
>>>>> 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
I like it. There is a typo on line 1589 of the patch (passwwords).
>>>>>>>>
>>>>> It's looking good!
>>>> I have an updated version I need to upload that addresses the remaining
>>>> issues.
>>> Version 3:
>>> http://people.apache.org/~markt/patches/2014-09-24-bug56403-tc8-v3.patch
>> Looks good.
>>
>> I'm just curious: why did you call the class that does PBKDF2
>> PBECredentialFilter? Does that stand for "Password-based
>> encryption/encoding"?
> It does.
>
>> PBE is often used for "password-based encryption" but here we aren't
>> actually doing any encryption; we're just doing the password part.
>> Naming this class is tough because technically it can use any algorithm
>> that works with Java's SecretKey API.
> SecretKeyCredentialHandler?
>
>> Also, why does ConcurrentMessageDigest.digest have a varargs byte[]
>> parameter? Is it useful to be able to accept more than one byte array to
>> that method?
> Yes. You want to be able to pass either just the password or the salt
> and the password.
>
> Mark
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Discussion of pluggable password-derivation in Realms [Bug 56403]
Posted by Gabriel Sánchez Martínez <ga...@gmail.com>.
> On Sep 26, 2014, at 17:03, Mark Thomas <ma...@apache.org> wrote:
>
> On 26/09/2014 16:45, Christopher Schultz wrote:
>
>>> +1 for commit.
>>
>> Are you up for back-porting this to Tomcat 7?
>
> Hmm. Not sure at this point. I'd like to give it sometime to settle in
> to 8.0.x first.
I understand wanting to wait a bit, but at the same time I am eager to see this in tomcat7, because that's what is available in Ubuntu repositories for 14.04 LTS. Pluggable password derivation is a great feature and a security enhancement that allows admins to follow best practice, so we should make it easy for those who stick with what's available in the repository to enjoy the benefits.
>
>> I noticed that you
>> committed to trunk in smaller pieces rather than a single commit. Was
>> that to make it easier to back-out certain items if necessary?
>
> More so folks could see how the solution evolved.
>
>> Finally, I'd like to write an implementation for bcrypt which is quite
>> popular, but we have already discussed not wanting to have a build-time
>> dependency on anything we don't absolutely need (a policy with which I
>> totally agree).
>
> Can you point me to that discussion (where no doubt I will have taken
> completely the opposite position to the one I am about to take).
>
> Build time dependencies don't bother me as long as:
> - they are required for release (so we don't break things)
> - they are optional for other build targets (so folks can still build a
> working instance without them)
>
> So I'd be +1 for BcryptCredentialHandler that shipped with Tomcat but
> required the user to manually add one or more JARs to make it work.
>
>> Where would be the best place to put a bcrypt implementation? Source
>> code on the wiki? There's the possibility of writing an implementation
>> using reflection, but that prospect is quite horrifying to me. :)
>
> I'm leaning towards "in the source tree" but if you wanted to put it
> somewhere else, the wiki is as good as anywhere.
>
> Mark
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Discussion of pluggable password-derivation in Realms [Bug 56403]
Posted by Mark Thomas <ma...@apache.org>.
On 26/09/2014 16:45, Christopher Schultz wrote:
>> +1 for commit.
>
> Are you up for back-porting this to Tomcat 7?
Hmm. Not sure at this point. I'd like to give it sometime to settle in
to 8.0.x first.
> I noticed that you
> committed to trunk in smaller pieces rather than a single commit. Was
> that to make it easier to back-out certain items if necessary?
More so folks could see how the solution evolved.
> Finally, I'd like to write an implementation for bcrypt which is quite
> popular, but we have already discussed not wanting to have a build-time
> dependency on anything we don't absolutely need (a policy with which I
> totally agree).
Can you point me to that discussion (where no doubt I will have taken
completely the opposite position to the one I am about to take).
Build time dependencies don't bother me as long as:
- they are required for release (so we don't break things)
- they are optional for other build targets (so folks can still build a
working instance without them)
So I'd be +1 for BcryptCredentialHandler that shipped with Tomcat but
required the user to manually add one or more JARs to make it work.
> Where would be the best place to put a bcrypt implementation? Source
> code on the wiki? There's the possibility of writing an implementation
> using reflection, but that prospect is quite horrifying to me. :)
I'm leaning towards "in the source tree" but if you wanted to put it
somewhere else, the wiki is as good as anywhere.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Discussion of pluggable password-derivation in Realms [Bug 56403]
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Mark,
On 9/25/14 9:18 AM, Christopher Schultz wrote:
> Mark,
>
> On 9/24/14 12:27 PM, Mark Thomas wrote:
>> On 24/09/2014 16:59, Christopher Schultz wrote:
>>> Mark,
>>>
>>> On 9/24/14 5:00 AM, Mark Thomas wrote:
>>>> On 23/09/2014 10:49, Mark Thomas wrote:
>>>>> On 23/09/2014 00:56, "Gabriel E. Sánchez Martínez" wrote:
>>>>>>
>>>>>> 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!
>>>>>
>>>>> I have an updated version I need to upload that addresses the remaining
>>>>> issues.
>>>>
>>>> Version 3:
>>>> http://people.apache.org/~markt/patches/2014-09-24-bug56403-tc8-v3.patch
>>>
>>> Looks good.
>>>
>>> I'm just curious: why did you call the class that does PBKDF2
>>> PBECredentialFilter? Does that stand for "Password-based
>>> encryption/encoding"?
>>
>> It does.
>>
>>> PBE is often used for "password-based encryption" but here we aren't
>>> actually doing any encryption; we're just doing the password part.
>>> Naming this class is tough because technically it can use any algorithm
>>> that works with Java's SecretKey API.
>>
>> SecretKeyCredentialHandler?
>>
>>> Also, why does ConcurrentMessageDigest.digest have a varargs byte[]
>>> parameter? Is it useful to be able to accept more than one byte array to
>>> that method?
>>
>> Yes. You want to be able to pass either just the password or the salt
>> and the password.
>
> Gotcha.
>
> +1 for commit.
Are you up for back-porting this to Tomcat 7? I noticed that you
committed to trunk in smaller pieces rather than a single commit. Was
that to make it easier to back-out certain items if necessary?
Finally, I'd like to write an implementation for bcrypt which is quite
popular, but we have already discussed not wanting to have a build-time
dependency on anything we don't absolutely need (a policy with which I
totally agree).
Where would be the best place to put a bcrypt implementation? Source
code on the wiki? There's the possibility of writing an implementation
using reflection, but that prospect is quite horrifying to me. :)
-chris
Re: Discussion of pluggable password-derivation in Realms [Bug 56403]
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Mark,
On 9/24/14 12:27 PM, Mark Thomas wrote:
> On 24/09/2014 16:59, Christopher Schultz wrote:
>> Mark,
>>
>> On 9/24/14 5:00 AM, Mark Thomas wrote:
>>> On 23/09/2014 10:49, Mark Thomas wrote:
>>>> On 23/09/2014 00:56, "Gabriel E. Sánchez Martínez" wrote:
>>>>>
>>>>> 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!
>>>>
>>>> I have an updated version I need to upload that addresses the remaining
>>>> issues.
>>>
>>> Version 3:
>>> http://people.apache.org/~markt/patches/2014-09-24-bug56403-tc8-v3.patch
>>
>> Looks good.
>>
>> I'm just curious: why did you call the class that does PBKDF2
>> PBECredentialFilter? Does that stand for "Password-based
>> encryption/encoding"?
>
> It does.
>
>> PBE is often used for "password-based encryption" but here we aren't
>> actually doing any encryption; we're just doing the password part.
>> Naming this class is tough because technically it can use any algorithm
>> that works with Java's SecretKey API.
>
> SecretKeyCredentialHandler?
>
>> Also, why does ConcurrentMessageDigest.digest have a varargs byte[]
>> parameter? Is it useful to be able to accept more than one byte array to
>> that method?
>
> Yes. You want to be able to pass either just the password or the salt
> and the password.
Gotcha.
+1 for commit.
-chris
Re: Discussion of pluggable password-derivation in Realms [Bug 56403]
Posted by Mark Thomas <ma...@apache.org>.
On 24/09/2014 16:59, Christopher Schultz wrote:
> Mark,
>
> On 9/24/14 5:00 AM, Mark Thomas wrote:
>> On 23/09/2014 10:49, Mark Thomas wrote:
>>> On 23/09/2014 00:56, "Gabriel E. Sánchez Martínez" wrote:
>>>>
>>>> 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!
>>>
>>> I have an updated version I need to upload that addresses the remaining
>>> issues.
>>
>> Version 3:
>> http://people.apache.org/~markt/patches/2014-09-24-bug56403-tc8-v3.patch
>
> Looks good.
>
> I'm just curious: why did you call the class that does PBKDF2
> PBECredentialFilter? Does that stand for "Password-based
> encryption/encoding"?
It does.
> PBE is often used for "password-based encryption" but here we aren't
> actually doing any encryption; we're just doing the password part.
> Naming this class is tough because technically it can use any algorithm
> that works with Java's SecretKey API.
SecretKeyCredentialHandler?
> Also, why does ConcurrentMessageDigest.digest have a varargs byte[]
> parameter? Is it useful to be able to accept more than one byte array to
> that method?
Yes. You want to be able to pass either just the password or the salt
and the password.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Discussion of pluggable password-derivation in Realms [Bug 56403]
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Mark,
On 9/24/14 5:00 AM, Mark Thomas wrote:
> On 23/09/2014 10:49, Mark Thomas wrote:
>> On 23/09/2014 00:56, "Gabriel E. Sánchez Martínez" wrote:
>>>
>>> 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!
>>
>> I have an updated version I need to upload that addresses the remaining
>> issues.
>
> Version 3:
> http://people.apache.org/~markt/patches/2014-09-24-bug56403-tc8-v3.patch
Looks good.
I'm just curious: why did you call the class that does PBKDF2
PBECredentialFilter? Does that stand for "Password-based
encryption/encoding"?
PBE is often used for "password-based encryption" but here we aren't
actually doing any encryption; we're just doing the password part.
Naming this class is tough because technically it can use any algorithm
that works with Java's SecretKey API.
Also, why does ConcurrentMessageDigest.digest have a varargs byte[]
parameter? Is it useful to be able to accept more than one byte array to
that method?
Thanks,
-chris
Re: Discussion of pluggable password-derivation in Realms [Bug 56403]
Posted by Mark Thomas <ma...@apache.org>.
On 23/09/2014 10:49, Mark Thomas wrote:
> On 23/09/2014 00:56, "Gabriel E. Sánchez Martínez" wrote:
>>
>> 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!
>
> I have an updated version I need to upload that addresses the remaining
> issues.
Version 3:
http://people.apache.org/~markt/patches/2014-09-24-bug56403-tc8-v3.patch
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Discussion of pluggable password-derivation in Realms [Bug 56403]
Posted by Mark Thomas <ma...@apache.org>.
On 23/09/2014 00:56, "Gabriel E. Sánchez Martínez" wrote:
>
> 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!
I have an updated version I need to upload that addresses the remaining
issues.
<snip/>
> 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.
The Realm API has no mechanism up writing to storage. That doesn't stop
a custom realm doing this but it isn't (currently) part of Tomcat's API.
<snip/>
> 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?
It is a real concern in that such an attack is possible. However the
number of requests necessary for such an attack to be successful are far
higher than the limits imposed by the LockOutRealm so Tomcat should be
protected against this attack.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Discussion of pluggable password-derivation in Realms [Bug 56403]
Posted by Felix Schumacher <fe...@internetallee.de>.
Am 23.09.2014 um 19:35 schrieb Felix Schumacher:
> Am 23.09.2014 um 01:56 schrieb "Gabriel E. Sánchez Martínez":
>>
>> 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.
> Django is doing that. I think we could extend the CredentialHandler
> (or a subclass) to take a CredentialUpdater which then could be fed
> with the new stored credential. That way each Realm could implement
> its own CredentialUpdater and set it on its main CredentialHandler.
I was just looking at implementing it and found, that CredentialHandler
is user agnostic, so putting the updater into CredentialHandler is not
working, as long as it doesn't know which user credentials it should update.
To get around that we could either give the user information to
CredentialHandler, or let CredentialHandler give back the best (newly
hashed) credentials, which could then be stored, if different to to the
old stored credentials.
Felix
>
> Felix
>>>>
>>>> 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-unsubscribe@tomcat.apache.org
>> For additional commands, e-mail: dev-help@tomcat.apache.org
>>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Discussion of pluggable password-derivation in Realms [Bug 56403]
Posted by Felix Schumacher <fe...@internetallee.de>.
Am 23.09.2014 um 01:56 schrieb "Gabriel E. Sánchez Martínez":
>
> 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.
Django is doing that. I think we could extend the CredentialHandler (or
a subclass) to take a CredentialUpdater which then could be fed with the
new stored credential. That way each Realm could implement its own
CredentialUpdater and set it on its main CredentialHandler.
Felix
>>>
>>> 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-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Discussion of pluggable password-derivation in Realms [Bug 56403]
Posted by "Gabriel E. Sánchez Martínez" <ga...@gmail.com>.
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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Discussion of pluggable password-derivation in Realms [Bug 56403]
Posted by Felix Schumacher <fe...@internetallee.de>.
Am 17.09.2014 um 10:36 schrieb Mark Thomas:
> 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
>> 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.
If the mutate method was removed from CredentialHandler we could
implement a CombinedCredentialHandler, which holds a list of
CredentialHandlers. Those could be asked to handle the credentials in
order to support more than one derivation type.
The mutate method could be placed in an interface like
MutatingCredentialHandler which then could be used as the basis for the
abstract class CredentialHandlerBase.
I wonder if it would be a good idea to introduce a CredentialExtractor
interface, which would extract the salt, algorithm, iteration,
password-hash from a stored credential.
>
>> 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.
>>
>> 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.
>
>> 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.
>
> 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
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Discussion of pluggable password-derivation in Realms [Bug 56403]
Posted by Mark Thomas <ma...@apache.org>.
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
>
> 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.
>
> 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.
> 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.
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Discussion of pluggable password-derivation in Realms [Bug 56403]
Posted by Christopher Schultz <ch...@christopherschultz.net>.
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
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.
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.
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.
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?
I'd like to get some other opinions on the public mutate() interface. I
think we might not be able to convince each other ;)
-chris
Re: Discussion of pluggable password-derivation in Realms [Bug 56403]
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Mark,
On 9/16/14 3:39 PM, Mark Thomas wrote:
> On 16/09/2014 16:20, Christopher Schultz wrote:
>> 1. In terms of limiting converting to String values, we could base
>> everything on byte[] instead of String.
>
> Having looked at the current code and the Servlet API I don't believe
> that this is practical.
>
>> 2. I don't like CredentialHandler.mutate(String input, byte[] salt, int
>> iterations).
>
> Having considered the options this looks like the least bad approach. A
> symmetric encryption impl can just ignore the salt and the iterations.
I stand by my previous statements: the public mutate method does not
need the salt and iteration count. A protected/private method could
certainly use them to help verify the credential, but the salt and
iteration count come not from the client but from the stored credential.
Is there a particular advantage to having a public method that takes
those arguments? I don't see any advantage at all; it's an
implementation detail that I think should be left out of the public
interface.
>> 3. MessageDigestCredentialHandler is good, but I think we ought to
>> push-down the iteration and salt members and methods from
>> BaseCredentialHandler into the subclass.
>
> Given my view of 2, I don't see a need for this.
>
>> 4. In MessageDigestCredentialHandler.matches, there are two uses of
>> StandardCharsets.ISO_8859_1 directly instead of using the client's
>> preferred character encoding. Is this a requirement of {MD5|SHA|SSHA}
>> formatting or an oversight?
>
> As per previous e-mail, I'm not sure but it is what the current code
> does so I plan to leave it alone for this work.
Ok.
>> 5. matchesSaltIterationsEncoded method does not check for unexpected
>> formatting.
>
> Fixed in updated patch.
Great.
>> 6. Performance. There are places where we know the sizes of things
>> before they are allocated but don't use that to our advantage.
>
> Fixed in updated patch.
Great.
>> 7. Thread safety. Is it safe to check/set the "random" reference in
>> multiple threads?
>
> I believe it is since it is only used via command line tool which will
> be single threaded.
Good point. We use our Realm in production to generate new credentials
online, so we have to ensure thread safety. Depending upon how these
CredentialHandlers are ultimately used (why /not/ use the CH to create
new passwords?), we may need a synchronization review. I didn't see any
immediate issues other than the SecureRandom-creation.
>> 8. RealmBase.main checks all known CredentialHandlers for matching
>> algorithms (a bit inefficient, but its offline so who cares), but it
>> doesn't know about anyone's custom CH classes. We should allow the
>> caller to specify the CH class if they want, so they can use the
>> RealmBase driver with their own CredentiaHandler implementation.
>
> Fixed in updated patch.
>
> Updated patch:
> http://people.apache.org/~markt/patches/2014-09-16-bug56403-tc8-v2.patch
I'll take a look.
Thanks,
-chris
Re: Discussion of pluggable password-derivation in Realms [Bug 56403]
Posted by Mark Thomas <ma...@apache.org>.
On 16/09/2014 16:20, Christopher Schultz wrote:
> 1. In terms of limiting converting to String values, we could base
> everything on byte[] instead of String.
Having looked at the current code and the Servlet API I don't believe
that this is practical.
> 2. I don't like CredentialHandler.mutate(String input, byte[] salt, int
> iterations).
Having considered the options this looks like the least bad approach. A
symmetric encryption impl can just ignore the salt and the iterations.
> 3. MessageDigestCredentialHandler is good, but I think we ought to
> push-down the iteration and salt members and methods from
> BaseCredentialHandler into the subclass.
Given my view of 2, I don't see a need for this.
> 4. In MessageDigestCredentialHandler.matches, there are two uses of
> StandardCharsets.ISO_8859_1 directly instead of using the client's
> preferred character encoding. Is this a requirement of {MD5|SHA|SSHA}
> formatting or an oversight?
As per previous e-mail, I'm not sure but it is what the current code
does so I plan to leave it alone for this work.
> 5. matchesSaltIterationsEncoded method does not check for unexpected
> formatting.
Fixed in updated patch.
> 6. Performance. There are places where we know the sizes of things
> before they are allocated but don't use that to our advantage.
Fixed in updated patch.
> 7. Thread safety. Is it safe to check/set the "random" reference in
> multiple threads?
I believe it is since it is only used via command line tool which will
be single threaded.
> 8. RealmBase.main checks all known CredentialHandlers for matching
> algorithms (a bit inefficient, but its offline so who cares), but it
> doesn't know about anyone's custom CH classes. We should allow the
> caller to specify the CH class if they want, so they can use the
> RealmBase driver with their own CredentiaHandler implementation.
Fixed in updated patch.
Updated patch:
http://people.apache.org/~markt/patches/2014-09-16-bug56403-tc8-v2.patch
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org