Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DiffieHellman exchange as AEAD key #61

Open
troublescooter opened this issue Nov 3, 2020 · 11 comments
Open

DiffieHellman exchange as AEAD key #61

troublescooter opened this issue Nov 3, 2020 · 11 comments

Comments

@troublescooter
Copy link
Contributor

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
Copy link
Collaborator

burdges commented Nov 3, 2020

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
Copy link
Contributor Author

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
Copy link
Collaborator

burdges commented Nov 3, 2020

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
Copy link
Collaborator

burdges commented Nov 4, 2020

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

@troublescooter
Copy link
Contributor Author

troublescooter commented Nov 4, 2020

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
Copy link
Collaborator

burdges commented Nov 4, 2020

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
Copy link
Collaborator

burdges commented Nov 5, 2020

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

@burdges
Copy link
Collaborator

burdges commented Nov 30, 2020

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
Copy link
Contributor Author

troublescooter commented Jun 30, 2021

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
Copy link
Collaborator

burdges commented Jun 30, 2021

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
Copy link
Collaborator

burdges commented Nov 28, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants