omitting HKDF salt is hard to get right

[warner]

We've seen one (brief) bug in https://github.com/warner/magic-wormhole.rs in which our protocol requires calling HKDF without a salt, and the initial code used Hkdf::<Sha256>::extract(&[], key) to express this. But the HKDF definition (RFC 5869) says that when the salt is omitted, "it is set to a string of HashLen zeros". Passing an empty salt string is not the same as omitting the salt (i.e. passing 32 zero bytes), causing a protocol mismatch.

To properly omit the salt, you must use something like extract(&[0; Sha256::OutputSize], key) or something that I'm not even sure would compile. Manually setting the salt size feels error-prone.

The python HKDF library lets you pass None as the salt value, and it will do the right thing.

Should we consider changing the extract() API to take an Option<&[u8]> for the salt value?

Can Rust do some kind of polymorphic thing that might let us have two different extract() methods, with different type signatures (one with &[u8], the other with Option) to enable backwards compability?

[timvisee]

Would checking whether the given salt is empty be good enough (which would then automatically create a zeroed salt under the hood)? Although I think an Option would be nicer, this solution would preserve the backwards compatability.

No, function overloading is not part of Rust. Differently named methods can be created, but I don't think that is a nice solution.

I'm not part of this project, I'm just thinking out loud :)

[newpavlov]

I like Option<&[u8]> solution. Maybe we could undeprecate new(ikm: &[u8], salt: &[u8]) -> Hkdf<D>method and change signature of extract to extract(salt: Option<&[u8]>, ikm: &[u8]) -> Hkdf<D>? Also it would be nice to change order of ikm and salt arguments, because extract(key, None) looks nicer than extract(None, key).

[vladikoff]

Should we consider changing the extract() API to take an Option<&[u8]> for the salt value?

This sounds good 👍

Also it would be nice to change order of ikm and salt arguments, because extract(key, None) looks nicer than extract(None, key).

I Think the reason for this is to follow the spec per: https://tools.ietf.org/html/rfc5869#section-2.2

[vladikoff]

Would checking whether the given salt is empty be good enough (which would then automatically create a zeroed salt under the hood)? Although I think an Option would be nicer, this solution would preserve the backwards compatability.

Thanks for the suggestion, I'm thinking because we are still in 0.x version ranges we have the ability to be more brave changing these things. /me shrugs