On Oct 23, 2010, at 11:59 AM, Les Hazlewood wrote:
> 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.
My response here is that we should have.
try {
Hasher hasher = new MessageDigestHasher("SHA-512");
HashedCredentialsMatcher matcher = new HashedCredentialsMatcher(hasher);
} catch(NoSuchAlgorithmException nsae)
{
}
or if they are wild and crazy
Hasher liveLifeForTheMoment = MessageDigestHasher.noCheck("DUDE");
HashedCredentialsMatcher willMeetYouInTheNextLife = new
HashedCredentialsMatcher(liveLifeForTheMoment);
;)
Strings specification for algorithms are there for a reason, pluggable
algorithms into the JVM. If it's known, a priori, that the algorithm is in
every JVM, e.g. SHA-512, then one can use a helper method.
Now, with that said. Look at all the classes we can remove. Quite a lot.
>
> 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.
What I have an issue with here is that you've also bundled in the encoding.
That should be decoupled. CodecSupport is the ancestor of too many classes.
Once you have removed all that, what do you have? Classes that hold byte
arrays to associate the algorithm that was used to hash it are marginally
useful. At best one class is needed where there's the byte array and the
algorithm used to generate it.
Regards,
Alan
