aead: rework traits

[newpavlov]

This PR introduced the unified Aead trait which replaces the old Aead* traits. It adds inout support and helper methods for buffer-to-buffer encryption and decryption. The trait also provides a set of methods for postfix encryption/decryption.

Tag position in ciphertext is defined by the IS_POSTFIX associated constant used by the en/decrypt_to_vec and en/decrypt_to_buffer methods.

The Buffer trait is tweaked slightly in accordance to operations which are used in the Aead trait methods.

Closes #1672

[tarcieri]

FWIW I have an incomplete local branch trying to do similar things (mostly implementing the ideas I talked about in #1672)

[newpavlov]

@tarcieri WDYT about usefulness of the arrayvec, bytes, and heapless bridges based on Buffer? I don't know how widely these bridges are used in practice, but to me it seems like the new APIs can be easily used in embedded contexts on top of those crates with a small amount of glue code. So I am somewhat inclined towards completely removing the Buffer-based methods.

[tarcieri]

@newpavlov as I said in #1672, I am a user of those APIs and know of other embedded users of those APIs. The "small amount of glue code" involves length calculations on slices which those APIs abstract away for users of e.g. arrayvec. Dealing with the fallout of removing them would be rather annoying for me personally.

I also don't think we should capriciously remove features and break downstream users' code. Features should only be removed if they're a misfeature or they don't have any users, neither of which is the case here. And I especially don't think we should be comingling the removal of major user-facing features in large PRs which are making a large number of unrelated changes such as this one.

[tarcieri]

FWIW I have an incomplete local branch trying to do similar things

I pushed this up here: #1714

[tarcieri]

As a general statement, this PR contains a lot of breaking and a lot of irrelevant, unrelated changes, especially with its stated goal of addressing #1672.

I would generally prefer to see smaller, more focused PRs that try to solve one-problem-at-a-time, and avoid making unnecessary breaking changes.

[newpavlov]

Please consider it a ground up redesign of the crate. I was not happy with the existing API for quite some time and it's a good opportunity to work on it.

If we are to exclude the DynAead trait, this PR contains less than 400 lines of changes. I don't think it makes sense to split it into separate PRs since the changes are connected to each other.

[newpavlov]

Should we add something for automatically prepending/appending nonces to ciphertexts?

[tarcieri]

Please consider it a ground up redesign of the crate. I was not happy with the existing API for quite some time and it's a good opportunity to work on it.

I don't think "ground up redesigns" make sense at this point in the project: the aead crate has 50 million downloads. Completely redesigning the API and asking all the existing users to switch to it will break everyone's code. To justify that, there must be something very fundamentally wrong with the current design.

I don't think that's the case here and I think the proposed new API is very much a regression.

The current design is based on being simple to understand and use. The Aead trait is targeted at the crate's foremost users: those with a working alloc implementation. The AeadInPlace trait is targeted at embedded/power users. These traits each have a minimal number of methods: just encrypt and decrypt for Aead, which follows the design for an AEAD interface you will see in most other programming language environments.

You have combined these into a "god object" trait with dozens of methods. In doing so you removed the blanket impls that allow the previous traits to abstractly build on one another and replaced them with copy-and-paste code. The crate was previously forbid(unsafe_code) and now contains unsafe code for reasons I'm uncertain of, but which feels like mixing abstraction levels. There seem to be many methods defined on the trait that aren't intended for end-users but are internal implementation details.

[tarcieri]

Should we add something for automatically prepending/appending nonces to ciphertexts?

I think a higher-level API that wraps up calling generate_nonce and prepending a random nonce would be good, as commonly seen in other AEAD APIs (e.g. Tink). A random nonce is the foremost use case for a prepended nonce. See #1666.

[newpavlov]

Completely redesigning the API and asking all the existing users to switch to it will break everyone's code.

No, it will not break user's code without manually updating dependencies. Changes required in user code will be quite small, far smaller than some of API changes in our other popular crates. Arguably, migration to hybrid-array will cause users more annoyance than changes in this PR.

We are still far from 1.0 release and I don't think it's a right time to go full ossification route.

there must be something very fundamentally wrong with the current design

There was nothing "fundamentally" wrong with generic-array and yet here we are migrating all our crates to hybrid-array.

The question is whether the new API is better long-term or not. I believe the answer is yes, it has one clear AEAD trait instead of splitting functionality into several different traits. It's more consistent with the cipher APIs. It does not rely on dynamic dispatch for the Buffer support.

You call the new Aead trait a "god object", but it's no more godly than for example Iterator. One trait is easier for users to understand and browse docs for than several smaller traits. You need to provide arguments for why a split could be beneficial. I think the dynamic dispatch use case is better served by a different trait with &[u8] nonces or with methods which opaquely add nonce to ciphertext. For example, in the digest crate I would've liked to have less traits, but their existence was dictated by difference in functionality between hash algorithms.

One potential split which could be beneficial is between a "core" trait (AeadCore) and the main sealed "extension" trait (Aead) containing all helper methods. The seal will be used to prevent users from potentially overwriting methods with blanket implementations.

We can have a "minimal" number of methods by removing most helper methods, but I doubt users will thank us for that.

There seem to be many methods defined on the trait that aren't intended for end-users but are internal implementation details.

All methods in the PR are intended for potential use by users. The trait just has a trifecta of inout, inplace, and _to_buf methods, which creates an initial impression of API complexity. All 3 can be useful in different contexts. What methods do you think can be removed?

I guess we could remove the to_vec methods and refer users to the to_buffer methods, but I think it's a nice quality-of-life API.

The crate was previously forbid(unsafe_code) and now contains unsafe code for reasons I'm uncertain of, but which feels like mixing abstraction levels.

As mentioned in the TODO comment it's a temporary change until the required methods will be added to the inout crate.

[tarcieri]

The question is whether the new API is better long-term or not. I believe the answer is yes, it has one clear AEAD trait instead of splitting functionality into several different traits. It's more consistent with the cipher APIs.

It's less consistent with the digest APIs which are decomposed into multiple traits linked with blanket impls.

And I think there are problematic aspects to the cipher APIs at well: look at the trouble it's causing in #177 trying to add tweakable block cipher support. That seems to be a similar case of methods which are closer to "internals" being combined into a single "god object" trait, versus the traits merely expressing the user-facing concerns as they did in previous releases of the crate.

[newpavlov]

look at the trouble it's causing in #177 trying to add tweakable block cipher support

It's a bit off-topic, but that exact trouble are you talking about? We discussed a potential bridge between tweakable and non-tweakable traits. You argued that my bridge proposals are overengineered and I mostly agree. I don't see where the design of cipher traits by itself causes issues in this particular case. We can't move helper methods from BlockCipherEnc/Dec to some other trait which would work for both tweakable and non-tweakable ciphers simply because the latter require an additional tweak argument on each block cipher invocation.

[tarcieri]

Arguably, migration to hybrid-array will cause users more annoyance than changes in this PR.

hybrid-array has a full migration guide, which is something that's been lacking from similar changes like this in the past: https://docs.rs/hybrid-array/latest/hybrid_array/#migrating-from-generic-array

It's a bit off-topic, but that exact trouble are you talking about?

If you have separate traits for standard block ciphers versus tweakable block ciphers, under the current design you would also have to duplicate all of the methods for using backends as well, which didn't exist in previous versions of the trait:

https://github.com/RustCrypto/traits/issues/177#issuecomment-2442162213

[newpavlov]

hybrid-array has a full migration guide, which is something that's been lacking from similar changes like this in the past

It still does not mean that migration will not be annoying. Users still have to manually fix compilation errors and to read the docs. Same for changes in this PR.

I agree that documenting migration process for these changes could be helpful.

If you have separate traits for standard block ciphers versus tweakable block ciphers, under the current design you would also have to duplicate all of the methods for using backends as well, which didn't exist in previous versions of the trait:

I wrote a bit about it in an update to my previous comment, but I don't see what can be done differently here. Tweakable and non-tweakable ciphers simply have a different number of arguments. Unless we want to pile even more complexity with generics (e.g. by considering non-tweakable ciphers as tweakable with zero sized tweak), I don't see a good way towards potential unification.

[tarcieri]

The current design of the aead crate was the result of one of the most commented on PRs in the history of this project: #40

This PR is re-litigating literally everything in the current design 5 years later, in absence of that previous discussion.

There was no planning or discussion, or even a prose description of what's wrong with the current design and what the goals of a rewrite are. So far you haven't even identified those things except to say you were "not happy with the existing API". What was wrong? Why does it warrant a ground up rewrite? What is being done differently here to address those issues?

I think if you took a step back and tried to actually plan things out by writing down goals, you would discover many of the changes you want don't require a ground-up rewrite but can be done incrementally, which would make them much easier to understand than a single PR that changes everything, especially for anyone reviewing the source history.

Perhaps in the process you could avoid capricious breaking changes which don't add new functionality or other improvements but merely generate churn for users of the library.

[newpavlov]

As I've written above, I do not consider the ossification arguments to be valid. We are right in the middle of a major breaking release cycle, so it's a good time to reconsider the existing APIs without being tied to backward compatibility. Therefore, I believe the APIs should be evaluated on their own merits, regardless of how long they have existed.

5 years ago we did not have the inout crate, we made some wrong assumptions like CiphertextOverhead and using the *Mut traits as HAL (remember, earlier we removed it from cipher). Requiring to fix tag and nonce size for object safety is also quite inconvenient and inflexible. It also introduces a small inefficiency around &mut dyn Buffer which is not present with impl Buffer. The split between AeadCore and AeadInPlace looks simply redundant. What types implement the former, but not the latter? IIUC it was intended for "opaque" hardware encryptors. Correct me if I'm wrong, but I don't think aead is used like this in practice.

I have mostly left the aead crate to you and did not participate much in its development outside of discussions. While these changes may seem sudden, I hope you will review them with an open mind.

Let's review the new methods step-by-step to find where exactly our disagreements start (let's consider only their essence, we can bikeshed the names and traits in which they should be defined separately): 1) The fundamental methods: detached_encrypt_inout, detached_decrypt_inout. You may have some different ideas about "inout integration which should only take a few dozen lines of code", but I don't see how you can get more fundamental than these methods and you haven't provided a draft of your ideas yet. Note that an inout support on top of &mut [u8] methods will be simply less efficient. 2) The helper detached methods: detached_encrypt_inplace, detached_decrypt_inplace, detached_decrypt_inplace, detached_decrypt_to_buf. These methods could be removed, but they are consistent with the cipher traits and quite helpful in practice, especially the inplace methods. 3) The basic postifx methods: postfix_encrypt_inout, postfix_decrypt_inout. These methods could be used even for AEADs which specify the prefix mode of operation. You've argued that this may result in "non-standard" algorithms, but I think the method name clearly indicates what happens, so users are unlikely to use it accidentally. We can add a big fat warning about it to the trait docs to reduce this probability even further. 4) The helper postfix methods: detached_encrypt_inplace, detached_decrypt_inplace, detached_decrypt_inplace, detached_decrypt_to_buf. These methods are not controversial iff you agree with the steps 2 and 3. 5) The postfix Buffer methods: postfix_encrypt_buffer, postfix_decrypt_buffer. As discussed in #1672, we can provide an efficient in-place implementation only for the postfix mode. And since we already defined Buffer for methods in the next step, I think it's worth to use it for postfix methods as well. 6) High-level Buffer methods: encrypt_to_buffer, decrypt_to_buffer. Straightforward use of Buffer which is useful in practice as argued by you above. 7) High-level Vec specialization: encrypt_to_vec, decrypt_to_vec. I think these methods are useful for clarity and easier to use, since most users will encrypt message to vectors instead of other Buffer implementers.

I also have some ideas for an inout type which reserves a fixed amount from head and tail, which would allow us to implement an efficient set of prefix methods, but I probably will postpone their implementation until we finish discussing the existing methods.

[tarcieri]

As I've written above, I do not consider the ossification arguments to be valid.

It's not an "ossification argument", it's asking you to justify why your changes are an improvement over the old code.

If breaking changes aren't actually improvements, if they're just moving things around, if they're not better but just different, then it would be better to leave things as they are, because otherwise you're just making chores for everyone and wasting their time without actually improving the library.

I have mostly left the aead crate to you and did not participate much in its development outside of discussions. While these changes may seem sudden, I hope you will review them with an open mind.

It feels as though your first major contribution to the library is to steamroll over everything, changing literally every method name.

5 years ago we did not have the inout crate, we made some wrong assumptions like CiphertextOverhead and using the *Mut traits as HAL (remember, earlier we removed it from cipher).

These all seem like things that can be addressed in self-contained PRs, rather than having to rewrite everything at once.

Let's review the new methods step-by-step to find where exactly our disagreements start

Our disagreements start with how to proceed with these changes.

There's an inherent lack of focus in trying to change literally everything at once. I don't think we can drill down on that list productively in this issue. It's too much going on at once. I think it would be better to make separate issues/PRs to discuss each of those changes.

You haven't even enumerated everything yet. Just as one example, you've removed the generate_nonce method and all of its associated documentation. I mean, really you've removed all of the documentation.

Documentation, or lack thereof, is one of the biggest complaints about this project. I really, really hope this PR doesn't turn into a huge documentation regression.

[newpavlov]

If breaking changes aren't actually improvements, if they're just moving things around

You haven't addressed what exactly you consider "moving around" and what not. I've compiled the list specifically for that purpose. The main change in this PR is an inout support. Do you consider the first step in the list "moving things around" or not? You have mentioned an alternative proposal for it, but you still haven't provided a draft for me to work with.

It feels as though your first major contribution to the library is to steamroll over everything, changing literally every method name.

I do not care about methods names, only about their functionality. I will gladly use any naming scheme you want as long as you take a proper look at the PR without being so dismissive just because I apparently "steamroll" over your previous work.

It's too much going on at once. I think it would be better to make separate issues/PRs to discuss each of those changes.

The changes in this PR are connected to each other. The Aead trait provides one coherent API surface. In my opinion, doing the same changes piece-by-piece is an unnecessary busywork which only makes it harder to track changes in the crate source code. It's far easier to view a diff for one PR/commit than to dig 5-10 different commits potentially separated by commits which affect other crates.

You haven't even enumerated everything yet.

I have started from the most important part. We can list and discuss the remaining items after we come to some kind of consensus about desired method signatures. Discussing docs, splitting methods into different traits, allowing or disallowing the postfix methods for SIV-like algorithms, etc., comes after that.

Just as one example, you've removed the generate_nonce method and all of its associated documentation. I mean, really you've removed all of the documentation.

Sorry, but are you serious?! I've moved the docs to README, which is included as the crate-level docs. The change has IMPROVED visibility of the nonce docs. I even provided an explicit reference in the generate_nonce docs: "See the crate-level documentation for requirements for random nonces.". I haven't figured how to properly make a link to the section, if you know how, I will gladly add it.