aead: rework traits API#2427
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
|
Overall this doesn't feel like an improvement to me. Several things have changed but I don't think they've changed in ways that make more sense. |
|
IMO variable nonce support, merging |
| /// | ||
| /// # Errors | ||
| /// AEAD algorithm implementations may return an error if the plaintext or AAD are too long. | ||
| fn encrypt_into_vec(&self, nonce: &[u8], aad: &[u8], plaintext: &[u8]) -> Result<Vec<u8>>; |
There was a problem hiding this comment.
I removed the Payload type for now. I think we either need a more general solution which would help with other methods as well (e.g. Nonce, Aad, Plaintext, etc. wrappers), or we could just tolerate potential incorrect reorderings of the arguments, which are hopefully somewhat unlikely in modern conditions (with IDEs and stuff).
| ) -> Result<()>; | ||
| aad: &[u8], | ||
| plaintext: &[u8], | ||
| allocate: impl FnOnce(usize) -> B, |
There was a problem hiding this comment.
Should we handle potential fallibility here and in extend/truncate?
| buffer: &mut dyn Buffer, | ||
| aad: &[u8], | ||
| buf: &mut B, | ||
| extend: impl FnOnce(&mut B, usize), |
There was a problem hiding this comment.
I thought about using a pass-through API:
fn encrypt_within<B: AsMut<[u8]>>(
&self,
nonce: &Nonce<Self>,
aad: &[u8],
buf: B,
extend: impl FnOnce(&mut B, usize),
) -> Result<B> { ... }But I decided against it since it may be inefficient for some buffer types (e.g. ArrayVec).
| /// Functionality of Authenticated Encryption with Associated Data (AEAD) algorithms | ||
| /// with variable nonce and tag size support. | ||
| /// | ||
| /// <div class="warning"> | ||
| /// Some algorithms support very short nonce and tag sizes. Users should exercise extreme caution | ||
| /// while using this trait since incorrect handling of nonces and tags may defeat security | ||
| /// provided by the algorithm. | ||
| /// </div> | ||
| pub trait VariableAead: AeadCore { |
There was a problem hiding this comment.
This seems like a purely additive change which could be added and discussed in its own PR
There was a problem hiding this comment.
I also don't understand the supertrait bounds here. It seems like it should be the most flexible, fundamental trait which isn't constrained by knowledge of particular sizes, and support different sizes at runtime.
There was a problem hiding this comment.
As I wrote above, I am not sure about what would be the best implementation relationships between the traits, so I am open to alternatives. I got to the current solution from the following factors:
Aeadshould support variable nonces, so it's blanket implementation should be based onVariableAead.- We want to automatically (or at least easily) implement
VariableAeadif type implementsAeadCore. - We do not have specialization in stable Rust.
- We want to guide users towards "default" sizes even if they rely on
VariableAeadand, as discussed above, we are likely to have separate "variable" types in implementation crates.
We could make VariableAead more "fundamental" than AeadCore, but it would look strange from the naming perspective ("core" becomes a mid-level layer) and it would mean that implementing both VariableAead and AeadCore would be slightly more annoying.
There was a problem hiding this comment.
VariableAead is more fundamental in that it can do things an AeadCore cannot, namely implement multiple sizes simultaneously.
An AeadCore bound means a type that impls VariableAead will have an associated set of fixed sizes for things like nonces and tags, which isn't a property a type which supports multiple sizes at runtime can have.
There was a problem hiding this comment.
We also could view VariableAead as an "extension" of AeadCore for additional support of nonce sizes at runtime, but I understand your position. I will try to play with it later.
Am I correct, that you prefer for AeadCore implementers to not implement VariableAead? And that Aead should be implemented in terms of AeadCore and not VariableAead? Note that we can not have two blanket impls without support of mutually exclusive traits in the language.
There was a problem hiding this comment.
I think in cases where there is actually a legitimate need to support multiple nonces and tag sizes at runtime, as with AES-GCM, that it can have a VariableAead core which is untyped in regard to nonce and tag sizes, and separately have well-typed wrappers which are newtypes composed in terms of the VariableAead core and delegate to that implementation, rather than using a blanket impl.
The only way such a blanket impl could even work, IMO, is if you kept AeadCore as-is where it only provides the size information, and the blanket impl could use that size information to provide a well-typed API. But I think that makes less sense than just having those types impl the relevant traits directly with a blanket impl that goes the other direction, providing VariableAead for AeadCore implementations.
|
I really think, at the very least, this should be broken down into multiple, smaller PRs. You are trying to rush through a large number of unrelated changes in a single PR shortly before a release. I would like to see at least two PRs:
Trying to make this many changes so very late in the crate's development lifecycle right before a stable release is highly problematic, IMO, and having them all in a single PR means discussion is unfocused as we're jumping around between all of the different changes rather than focusing on a set of them in isolation. |
|
Ok, I will remove |
|
Can we start with |
|
I would strongly prefer to start with the I will ignore your "last minute" remarks, otherwise the discussion could get pointlessly heated again and we will get nowhere. |
Where are the actual interdependencies which make that preferable? |
|
The structure of methods. |
TODO:
Unresolved questions:
nonce,aad,plaintext/ciphertext) with the same type (&[u8]).allocate,extend, andtruncateclosures?