w3f / schnorrkel

Schnorr VRFs and signatures on the Ristretto group
BSD 3-Clause "New" or "Revised" License
306 stars 92 forks source link

DiffieHellman exchange as AEAD key #61

Open troublescooter opened 3 years ago

troublescooter commented 3 years ago

The output of the raw diffie-hellman is used as the aead key after compression. Afaik standard practice is to use a hkdf over diffie-hellman to get symmetric keys. Is there a reason why this isn't needed/recommended here?

burdges commented 3 years ago

Afaik, there is nobody using these functions in deployment, so we could change them.

The hkdf is make_aead although it works via merlin and may not correspond exactly with how you'd use strobe directly for a kdf: https://strobe.sourceforge.io/specs/#ops.bare.prf A question is if we should make it correspond any better, presumably by forking merlin. Also if b"" should be b"KDF" in https://github.com/w3f/schnorrkel/blob/master/src/aead.rs#L47

If I understand, you're suggesting we remove the *aead32* methods, or feature gate them, or put big scary warning in their doc comments.

I felt the *aead32* made sense because its they remain safe if used with chacha or aes while using the merlin based SigningTranscript elsewhere, because keccak never gets used for encryption. You'd should evaluate things more closely if you say used chacha but also switched your SigningTranscript to blake2x. This is not explained in the doc comments though.

troublescooter commented 3 years ago

I'm not sure I got everything from your last paragraph, but I would make a PR where aead32_unauthenticated would more closely model aead_unauthenticated , initialising a new Transcript and hashing the DH in.

Also, I've noticed that the api would look a bit more consistent, arguably more idiomatic, if commit_* methods operated on a transcript instead of having to hand it down as &mut. Is this something I can mix in the pr?

burdges commented 3 years ago

I've put far too little thought into the aead module, so sure it could be improved in many ways, but that said..

Actually *aead* methods already do what you propose, so altering *aead32* to do the same makes little sense. The choices are either (a) remove *aead32*, or else (b) explain when and why *aead32* is safe, or else (c) pursue some middle path where we roadblock to using *aead32* unwisely.

A feature gate is a questionable example of (c). I suppose *aead32* could become pub(crate) and instead provide a aead_chacha_poly1305 convenience method that skips the kdf. meh.. As a (d) option, one could run some 256 bit permutation in aead32 but actually nothing comes to mind.

We've no "inherent" trait methods, and no arbitrary self types here, so you're suggesting making the aead commit_* methods into trait methods. We should probably keep traits simple unless we've some good reason.

burdges commented 3 years ago

We could place deprecation warnings on *aead32* and think about it longer term. I donno..

troublescooter commented 3 years ago

Actually *aead* methods already do what you propose, so altering *aead32* to do the same makes little sense.

I was unaware that this difference was on purpose. Given the similarity in names I was thinking *aead32* would be roughly what *aead* does on a Keypair. Apparently you disagree, but the naming would suggest they are somewhat similar in operation. I wouldn't expect such a difference based on the appended 32. My thinking is thus to either (a) make *aead32* a rough convenience method for *aead* on SecretKey, (b) deprecate and rename with added documentation on its behaviour or (c) deprecate/remove altogether without replacement.

We've no "inherent" trait methods, and no arbitrary self types here, so you're suggesting making the aead commit_* methods into trait methods. We should probably keep traits simple unless we've some good reason.

The SigningTranscript trait currently adds default methods that operate on typed data, so I think this would lead to a more coherent api. I'm also simply stating how I think merlin's documentation is to be read.

burdges commented 3 years ago

I kinda think separate Keypair and SecretKey types were a mistake actually, so maybe de/serialization becomes to/from _ secret_key_bytes/keypair_bytes, or maybe disallow keypair serialization entirely, or provide non-cannonical serialization like zexe does. I'm always okay providing methods only for keypairs though.

It's actually common that ad hoc niche protocols skip the KDF like *aead32* does, but they know their AEAD and know doing so works okay. Arguably a generic AEAD like here without a KDF is not miss-use resistant enough.

It's true I made commit_key_exchange public so not sure.

burdges commented 3 years ago

I've mostly convinced myself we should deprecate aead32 now. :)

burdges commented 3 years ago

There is an interest in key committing AEADs which I suspect poses minimal risk here but still..

https://eprint.iacr.org/2017/664.pdf https://eprint.iacr.org/2020/1491.pdf https://eprint.iacr.org/2020/1153.pdf

troublescooter commented 3 years ago

I felt the *aead32* made sense because its they remain safe if used with chacha or aes while using the merlin based SigningTranscript elsewhere, because keccak never gets used for encryption. You'd should evaluate things more closely if you say used chacha but also switched your SigningTranscript to blake2x. This is not explained in the doc comments though.

I never understood that last paragraph, can you elaborate on it? Are there differences between Blake2x and Keccak that are relevant, or are you referring to how Keccak is used internally in Merlin? I could (maybe) dig through it, but a tl;dr would be appreciated.

burdges commented 3 years ago

I suspect no, and my words there make little sense, but.. blake2 is based on chacha so one asks does it do much over chacha alone, ala aerad32? The answer is that it brings in some context.

burdges commented 1 year ago

Appears the age/rage symmetric crypto part lives externally at https://github.com/str4d/rage/tree/main/age-core/src so this key exchange could be used with it, similar to the code at https://github.com/str4d/rage/blob/main/age/src/x25519.rs#L211

We should ensure nothing from age-core is really missing here I guess.