On Oct 25, 2010, at 12:08 PM, Les Hazlewood wrote:
> Just to clarify, the 'crypto hierarchy' that Kalle and I were talking
> about at the time was regarding the CipherService implementations, not
> the Hash implementations.
Ok.
>> My response here is that we should have.
>>
>> try {
>> Hasher hasher = new MessageDigestHasher("SHA-512");
>>
>> HashedCredentialsMatcher matcher = new HashedCredentialsMatcher(hasher);
>> } catch(NoSuchAlgorithmException nsae)
>> {
>> }
>
> There is no reason for this IMO - this is essentially no different to
> the end-user than using the MessageDigest class directly. Shiro uses
> unchecked exceptions for everything as a design tenet. The end-user
> has the choice to catch a CryptoException if they want to react to a
> potentially erroneous algorithm name.
Ok. This makes sense to me then. :)
>> Now, with that said. Look at all the classes we can remove. Quite a lot.
>
> We can remove the concrete Hash implementations, but I don't agree
> that we should. Without question, the number one design goal of
> Shiro is ease-of-use, even if it means maintaining a little more code
> by the dev team. A lot of people are using code like this:
>
> new Sha1Hash(blah).toBase64()
> new Md5Hash(foo).toHex()
>
> which is a just a joy to use as an end-user and more readable compared
> to anything the JDK provides.
>
> So, even if we can remove the Hash implementations and ask everyone to
> move to using SimpleHash("SHA-1", blah).toBase64() instead, I don't
> know that we should.
or
String hash = Base64.encode(Hash.hash(Hash.SHA-1, blah));
With static imports one gets
String hash = Base64.encode(hash(SHA-1, blah));
All the type safety with none of the class baggage.
But there are two separate points going on here.
1) Whether every hash algorithm deserves its own class.
2) Should every instance of hash include all the various mechanisms for
encoding?
For 1 I would say that it depends on the concrete use cases. AFAICS there are
none that require N classes.
For 2 I think that it's a bad practice.
> The additional feature of type-safety (which
> also eliminates erroneous input: "is it SHA1? or SHA-1? or sha-1?")
> can be used for reflection and other configuration mechanisms is a
> _nice_ thing. Keeping 6 or 7 or so Hash implementations that never
> change for this level of convenience is a suitable concession to me.
> And we retain backwards compatibility, which is ideal if we can do it,
> even when moving to 2.0.
Yeah, this is all just a chat. May never see the light of day. Just
interested in knowing what other people think.
Concrete use cases that people have actually run into would help here I think.
>>> 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
>
>> What I have an issue with here is that you've also bundled in the encoding.
>> That should be decoupled.
>
> I agree with you on principle and try to do this almost always -
> separate orthogonal concerns and not combine them. However, in this
> particular case, I think practicality wins out. Shiro is not a
> framework 'on a hill' that mandates perfect architectural design. We
> strive for it when possible, but when in conflict with practicality,
> practicality should 'win out'.
>
> When working with hashes and output from Ciphers, the need to
> hex-encode or base64-encode the resulting data is so strong, they
> should be immediately accessible for the end-user. This is a
> real-world use-case where I think architectural philosophy takes a
> back seat.
>
> Without the toBase64() and toHex() methods, now and end-user has to go
> find out how to do this themselves. They could search for a while in
> Shiro's APIs or go read the documentation, or resort unnecessarily to
> commons-codec because they didn't know they didn't have to - or they
> could just use their IDE's auto-complete feature and have a working
> solution in seconds.
Hey, how about
String hash = Hash.hash(Hash.SHA-1, blah)).encode(Encoder.Base64);
or with static imports
String hash = hash(SHA-1, blah)).encode(Base64);
where
class Hash {
private final String algorithm;
private final String hash;
public static Hash hash(Hash.Algorithm, bytes[] data) { }
public String encode(Encoder.enum) { }
public enum Algorithm {
SHA-1,
MD5,
...
}
}
Regards,
Alan
