Uh oh!
There was an error while loading. Please reload this page.
ChaCha20Poly1305 AEAD - #3
Conversation
98ec721 to
7925b28Comparetarcieri
commented
Aug 19, 2019
Removed the draft PR status on this as it's functionally complete and passing tests, but leaving cc @jcape |
602b190 to
013035aCompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Implements the ChaCha20Poly1305 AEAD described in RFC 8439 using the `chacha20` and `poly1305` crates.
013035a to
d90303dCompareUh oh!
There was an error while loading. Please reload this page.
tarcieri
commented
Aug 21, 2019
Note: I'd like to do XChaCha20Poly1305 as well, which is a simple extension of this, but I figured I'd start with this as a first pass. |
| aead = "0.1" | ||
| chacha20 = { version = "0.2.1", features = ["zeroize"] } | ||
| poly1305 = "0.2" | ||
| zeroize = { version = "0.9", default-features = false } |
There was a problem hiding this comment.
I thought in other crates zeroize was optional?
There was a problem hiding this comment.
Happy to keep it that way if you want. The other crates have a lower MSRV than zeroize, so it had to be optional there. Could always be optional but on-by-default as well.
There was a problem hiding this comment.
Hm, I guess making it optional and enabling it by default will not make substantial difference compared to the non-optional approach, so we can leave it as-is.
BTW I wonder how zeroize will work in case like this:
structFoo{ .. }structBar{foo:Foo,baz:Baz}if both Foo and Bar implement Zeroize. Will it zeroize Foo space twice?
There was a problem hiding this comment.
@newpavlov I've been using explicit Drop handlers, and generally trying to push those down to the "leaf" data owner. If there are two Drop handlers like in your example, they will zero foo twice. I'd suggest only Foo impl Drop in the example above.
Uh oh!
There was an error while loading. Please reload this page.
| struct CipherInstance { | ||
| chacha20: ChaCha20, | ||
| poly1305: Poly1305, | ||
| } |
There was a problem hiding this comment.
Looks like it can be made generic over the StreamCipher trait? I wonder if it should not go to the poly1305 crate... Or do you plan to make it even more generic?
There was a problem hiding this comment.
This is a first cut 😉But yeah, good point.
It would definitely be nice to reuse the same generic "Poly1305 plus a Salsa20 family stream cipher" core to implement Salsa20Poly1305, ChaCha20Poly1305, and XChaCha20Poly1305.
The poly1305 crate might make sense as it's the eponymous algorithm's primary use case. I could also see this further justifying the existence of salsa20-core versus trying to abstract it away into a more generic ctr.
newpavlov
commented
Aug 21, 2019
BTW I wonder if we should not modify the AEAD trait constructors to |
jcape
commented
Aug 21, 2019
That would break things like STREAM that need to pass specific nonce values, wouldn't it? |
@jcape correct. Particularly for AES-GCM(-SIV) it'd be nice to do key expansion once for a particular key, and reuse that across nonces, especially when encrypting several AEAD messages in a row as in STREAM (or a transport encryption protocol like TLS or Noise). It helps less for Salsa20 family ciphers, where you do want to buffer/cache the initial block state (RustCrypto/stream-ciphers#50), but particularly for ChaCha20 w\ 96-bit nonce much of the complexity of computing it comes from the nonce. |
The internal `CipherInstance` type is the actual holder of mutable state, allowing the `ChaCha20Poly1305` type to be a `StatelessAead`.
tarcieri
commented
Aug 21, 2019
@newpavlov aside from making the implementation generic, look good for a first cut? |
newpavlov
commented
Aug 21, 2019
Yeap, you can merge it at will. |
tarcieri
commented
Aug 21, 2019
Alright! Now I guess we just gotta figure out what to do about AAD in the |
newpavlov
commented
Aug 21, 2019
BTW another idea which I had is to introduce a trait for AEAD based on stream ciphers, it would allow us to simplify detached interface. maybe we could even add an associated constant of type |
tarcieri
commented
Aug 21, 2019
@newpavlov yeah definitely, almost every AEAD is based on a stream cipher, so it'd be nice to simplify the common case for those |
Opened an issue to discuss stream cipher-based AEADs: #45 |
Implements the ChaCha20Poly1305 AEAD described in RFC 8439 using the
chacha20andpoly1305crates.