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
signature.asc
Description: OpenPGP digital signature
