Use SHA instead of Blake2 for MACs? about quinn

[Ralith]

Blake2b was selected as a convenient high-performance modern cryptographic hash function. It's fine to swap it out with another HMAC scheme (it's not even a breaking change) if you think increasing our use of ring is worth it.

[djc]

I'd actually also have picked Blake2b if all other things were equal. But having one less dependency means less maintenance (and less compile time), and I don't think this particular area of the code is in a particularly hot loop that the difference between Blake2b and HMAC-SHA256 will be noticeable?

[Ralith]

Yeah, should be fine. It's ran on small amounts of data for every packet that was successfully parsed but not associated with a connection, and (formerly) for every handshake when using stateless retries. No real hot loop.

[kylegalloway]

I would like to take a look at this issue. What crate does the HMAC-SHA256 algorithm come from?

Edit: found it in the ring crate.

Should it be used like this:

 let mut mac = SigningKey::new(&digest::SHA256, key.as_ref());

or this:

 let mut mac = hmac::SigningContext::with_key(key.as_ref()).sign();

[djc]

@kylegalloway great, thanks for working on this! The function that needs changing is here:

https://github.com/djc/quinn/blob/master/quinn-proto/src/crypto.rs#L110

Since (on quick skimming) it seems we're reusing the same key over time, it would probably good to store the SigningKey in the ListenKeys and work from that, but @Ralith knows more about this code.

[Ralith]

@djc is correct; in particular, the reset member of ListenKeys should be replaced with a SigningKey. Let's use SHA-512/256 rather than SHA-256 for the hash algorithm to take better advantage of current hardware, too.

[djc]

@kylegalloway are you still interested in working on this? If not, someone else might want to take a swing.

[kylegalloway]

@djc I have gotten really busy recently. Thanks for the opportunity, but if someone else wants to take a swing, please let them.

[djc]

@kylegalloway no problem, thanks for the quick response.

[stammw]

I'd like to work on this one.

[djc]

Great! Let me know if you have any questions.

[stammw]

SHA512/256 hash seem to have a size of 32 bytes, but the RESET_TOKEN_SIZE is 16, the same as stateless_reset_token field of tls's TransportParameters. Should the digest be truncated?

[stammw]

OK, so I found in the ietf draft that it is the expected behavior:

The output of this function is truncated to 16 bytes to produce the Stateless Reset Token for that connection.

[djc]

This is done, thanks again to @stammw.