Add trait AeadInto so callers can provide separate input and output buffers

[Cel-Service]

And identical counterpart AeadIntoMut for completeness' sake.

Resolves issue #1311.

Rationale

trait Aead allocates a new Vec every time a message is encrypted or decrypted.

trait AeadInPlace forces the caller to use one buffer for input and output.

It's not currently possible to write an optimal and specification-compliant implementation of the Noise Protocol Framework using either of the existing traits.

[newpavlov]

I think a better path forward is to use inout types in the AeadInPlace trait. cipher already uses it, so it should be relatively easy to do.

[tarcieri]

It might make sense to change the existing detached APIs to use inout types, rather than adding yet more traits. We already have a confusingly large variety of traits as-is.

[tarcieri]

It's not currently possible to write an optimal and specification-compliant implementation of the Noise Protocol Framework using either of the existing traits.

Having implemented parts of the Noise CipherState protocol using these crates, I don't see the issue here. Noise should work fine with in-place encryption.

That said, I do think there would be value in exposing APIs based on inout.

[Cel-Service]

It might make sense to change the existing detached APIs to use inout types, rather than adding yet more traits. We already have a confusingly large variety of traits as-is.

To clarify -- are you proposing a backwards-incompatible change to the signature of AeadInPlace::encrypt_in_place_detached and AeadInPlace::decrypt_in_place_detached to accept InOutBuf instead of &mut [u8] and &mut dyn Buffer?

I haven't yet had coffee, so take this with a grain of salt, but I believe merging the in-place and in-out APIs (in a way that would be ergonomic for developers who need either one) might be troublesome in Rust; I don't believe it's possible to actively use both a mutable and immutable reference to overlapping memory (at the same time) without some unsafe getting involved.

That said, I'm personally fine with looking into this; I just didn't want to destabilize your API out of the blue.

Having implemented parts of the Noise CipherState protocol using these crates, I don't see the issue here. Noise should work fine with in-place encryption.

It probably would have been more accurate for me to phrase my last point as "an optimal, specification-compliant, and pedantic implementation of the Noise Protocol Framework" -- at least according to my own personal reading of the spec, which is obviously not the be-all, end-all of interpretations.

Of course, from a cryptographic perspective, there's no meaningful difference between an encryption function which reuses one buffer versus one which uses separate buffers.

[newpavlov]

are you proposing a backwards-incompatible change

Yes. We are in the process of preparing the next release cycle, so it's a good time to introduce it.

merging the in-place and in-out APIs might be troublesome in Rust

See the inout crate docs. It provides types which allow construction from &mut T and (&T, &mut T) (i.e. in-place and buffer-to-buffer modes). Under the hood they use raw pointers and the crate exposes safe API to work with them. Ergonomic issues are usually solved by introducing separate methods for "in-place" and "buffer-to-buffer" applications with blanket impls based on methods which operate over inout types. For example, see the BlockEncrypt trait.

[Cel-Service]

I've taken a closer look at InOutBuf and I now see that (unless I'm mistaken) it can be created from either a pair of buffers or a single buffer. I can work on making that change when I have some spare time.

That said -- if breaking API changes are on the table, perhaps we should rename AeadInPlace to AeadInOut? We could then convert encrypt_into and decrypt_into into provided methods over the inout base method, as you suggested.

Edit: Wow, that was a fast reply! :)

[tarcieri]

AeadInPlace should stay the same (it’s generic over a Buffer trait which provides an abstraction over various buffer types for adding/removing the tag in-place)

Edit: sorry, that is to say, there are two sets of methods, and only the _detached methods should be updated.

Separately I thought we split AeadInPlace into an AeadDetached or thereabouts trait already, but apparently not.

[Cel-Service]

AeadInPlace should stay the same (it’s generic over a Buffer trait which provides an abstraction over various buffer types for adding/removing the tag in-place)

Edit: sorry, that is to say, there are two sets of methods, and only the _detached methods should be updated.

Separately I thought we split AeadInPlace into an AeadDetached or thereabouts trait already, but apparently not.

Can do. I haven't had much time to spare on this, but I've been in progress on an updated proposal based on the above conversation. I'm almost done; I've just been trying to provide a higher quality implementation for the aes-siv and ocb3 crates. (The quick option would be to just copy the input buffer to the output buffer, but I'd like to avoid that where possible.)

One thing I'd like to ask: I found it quite surprising that there's no Buffer implementation for &mut [u8]; I would imagine others would as well.

It appears that the primary purpose of trait Buffer (as used in the AEADs repo) is to abstract over extending and truncating the provided buffer (in other words, offering Vec-like functionality to other types); however, only aes-siv makes use of this functionality.

The order (ciphertext then tag or tag then ciphertext) is already algorithm-dependent; it doesn't look like trait Buffer helps with the caller-side ergonomics.

Would it not make more sense to simply accept an &mut [u8] in its place, and explicitly specify that callers of AeadInPlace::encrypt_in_place should provide AeadCore::TagSize additional bytes of space beyond their plaintext, while callers of AeadInPlace::decrypt_in_place should ignore AeadCore::TagSize bytes at the end of their buffer?

[tarcieri]

One thing I'd like to ask: I found it quite surprising that there's no Buffer implementation for &mut [u8]; I would imagine others would as well.

&mut [u8] can't impl Buffer because it can't impl Buffer::extend_from_slice, i.e. it isn't growable, as you identified (seems like you answered your own question?)

however, only aes-siv makes use of this functionality.

Every AEAD makes use of it via the default provided implementations of AeadInPlace::{encrypt_in_place, decrypt_in_place}, which implement the far more common postfix-appended tags, which is why you're seeing little custom logic dealing with Buffer:

https://docs.rs/aead/0.5.2/src/aead/lib.rs.html#287

It's also used by the blanket impl of Aead for AeadInPlace to handle populating a returned Vec: https://docs.rs/aead/0.5.2/src/aead/lib.rs.html#384-407

These are the primary user-facing APIs for these crates, so yes, they're very much used!

The order (ciphertext then tag or tag then ciphertext) is already algorithm-dependent; it doesn't look like trait Buffer helps with the caller-side ergonomics.

Would it not make more sense to simply accept an &mut [u8] in its place

Actually, Buffer and its usage in AeadInPlace::{encrypt_in_place, decrypt_in_place} makes it possible to abstract over the algorithm-specific details like whether or not the tag is appended or prepended.

Someone using the current API doesn't have to worry about such details, because the current API abstracts over them, allowing truly generic usage over AEAD modes regardless of how they structure the AEAD message.

Just accepting a slice as input means that the user needs to include additional space for the AEAD tag, and its position may vary depending on the algorithm. This means the end user needs to have to have some awareness of how the AEAD message is formatted when constructing the buffer, i.e. algorithm-specific details leak out, and it's just onerous.

AeadInPlace::{encrypt_in_place, decrypt_in_place} and Buffer trait are very much based on first-hand experience with the bad API ergonomics of such an approach.

[Cel-Service]

After some rumination, I think I understand what you're trying to convey.

decrypt_in_place and encrypt_in_place being generic over Buffer means that callers can plug any implementation into &dyn AeadInPlace (or impl AeadInPlace, or so on) and expect it to Just Work, so long as they provide a growable type -- Vec for convenience, or ArrayVec (with adequate space) for control.

From that perspective, I can see how this would be very convenient in a trait that (for example) a TLS library would want to consume -- clearly, it should stay in!

---

That said, if we're open to further API changes, it would be ideal if we could also make the "I have a simple &mut [u8] and I, too, don't care where the authentication tag goes, I just want to encrypt/decrypt this data in place" use case more ergonomic, as well.

From that perspective -- the one I was seeing from before the above realization -- it seems quite surprising that AeadInPlace::encrypt_in_place_detached accepts &mut [u8], but AeadInPlace::encrypt_in_place does not. As a caller, I would just want my AEAD library to take care of packing (and unpacking) the authentication tag for me -- it makes little sense, from that caller's perspective, to preclude them from using Rust-standard byte slices if they want that boilerplate taken care of.

From the caller's perspective, the simplest option I could see would likely be to add a separate pair of methods (e.g. encrypt_slice_in_place and decrypt_slice_in_place) with explicitly-defined semantics as to where the plaintext is expected to be before calling encrypt, and where it will be after calling decrypt. The old methods keep their semantics, so there are still no annoying surprises for API consumers after updating their package list.

Alternatively, one could include an ergonomic route by which a &mut [u8] can implement Buffer (or be wrapped) in order to be passed directly into the existing methods. The semantics would necessarily be different, so I'm not sure this is a great option, but I figured I would mention it as an option.

[tarcieri]

it seems quite surprising that AeadInPlace::encrypt_in_place_detached accepts &mut [u8], but AeadInPlace::encrypt_in_place does not.

If the *detached methods are changed to use InOut they should probably be renamed to encrypt_detached and decrypt_detached, since InOut supports separate input and output buffers and is therefore no longer necessarily "in place".

We can potentially add some explicit methods that work directly on slices too but the InOut APIs can also operate on slices by using slice.into() to convert to InOut. Documenting that conversion on each of the methods might be a good place to start.

Note that there are two cases for slice-based APIs to consider: in-place and working over separate input and output buffers. InOut abstracts over these two cases, but in considering a slice-based design you'll need to decide which to support or both.

Also it would probably be good to open a new issue for this.