Upgrade hmac, sha2 and generic_array #313
Conversation
| fn hi(str: &[u8], salt: &[u8], i: u32) -> GenericArray<u8, U32> { | ||
| let mut hmac = Hmac::<Sha256>::new(str); | ||
| fn hi(str: &[u8], salt: &[u8], i: u32) -> io::Result<GenericArray<u8, U32>> { | ||
| let mut hmac = Hmac::<Sha256>::new(str) |
There was a problem hiding this comment.
Under what conditions will Hmac fail to be constructed?
There was a problem hiding this comment.
The documentation implies it fails if it is unhappy with the key length, and, looking at the rust-postgres code, I decided that the user had sufficient control over the key to trigger this. As there's already a Result being returned from the public API, propagating the error would be the right thing to do.
https://docs.rs/hmac/0.5.0/hmac/trait.Mac.html#tymethod.new
However, the current hmac crate implementation does not actually validate the key length. It's unclear to me from the code why they've made the change to return a Result; perhaps to make the interface consistent with other crypto apis (e.g. cmac), which do validate the key length?
While (as far as I'm aware) it's not technically necessary to have "long" (>= digest size) keys in theory, it's always what you want to do in practice. As such, it's not inconceivable that the hmac crate would be updated to validate the key length is not 1 byte? Hence unnecessarily unwrap()ing seemed risky.
There was a problem hiding this comment.
Rejecting 1 byte keys wouldn't be a valid HMAC implementation IMO.
@newpavlov is Hmac::new actually fallible?
There was a problem hiding this comment.
No, Hmac::new is infallible and will accept even zero sized keys.
There was a problem hiding this comment.
Thanks! Let's remove the error propagation here then, and just .expect("BUG") or whatever.
There was a problem hiding this comment.
I think something like .expect("HMAC is able to accept all key sizes") will be better. :)
About why Result is used: Hmac is just one of implementations of Mac trait and other algorithms can have restrictions on key sizes (e.g. Cmac works only with fixed size keys). I hope one day with stabilized never_type and associated type defaults we will be able to express infallibility better.
|
Thanks! |
hmachas had a couple of changes:::new, which I've handled by propagating the only case as anio::Error(InvalidInput, ...).code()has become a move, so lift that out of some loops, etc.Tests are green. Note to self: To run the tests, first you must run
docker-compose up.The only slightly scary change is the changes around referencing in
*hi ^= prev;, which is something that's easy to get wrong, at least in my experience as a zero-star programmer.