aead: Proposal for revision to API surface for trait AeadInPlace

[Cel-Service]

Migrated from PR #1663 for discussion.

@tarcieri -- following on from where we left off: I've been ruminating on the issue since my last message yesterday.

As @newpavlov proposed, I've been working on modifying the algorithm crates so that we can revise the API surface of AeadInPlace (or whatever we might rename or split it out to) to be implemented in terms of methods which consume InOutBuf.

---

Fully separating authentication tag placement from the algorithm itself

As you already know, currently, the AeadInPlace API is generally implemented in terms of self.encrypt_in_place_detached and self.decrypt_in_place_detached. If we want to change this so that the API is implemented in terms of methods consuming InOutBuf, then we need to make some surrounding changes to make that work.

Implementing in-place, tag-position-agnostic decryption in terms of a call to self.decrypt_inout_detached (or whatever we call it) would be very simple: the API could be revised to fn decrypt_in_place<'a>(&self, nonce: &Nonce<Self>, ad: &[u8], buffer: &'a mut [u8]) -> Result<&'a mut [u8], Error>, very similar to plain fn decrypt.

The provided implementation would split the provided buffer, converting part of it into an InOutBuf<'_, '_, u8> and part of it into an &Tag<Self> in order to pass it to self.decrypt_inout_detached; it would then return the subslice containing just the decrypted plaintext to the caller (thereby retaining all of the convenience that current API consumers enjoy).

The trouble is, in order to implement its counterpart, encrypt_in_place, in terms of encrypt_inout_detached specifically for tag-prepending AEADs, something needs to give:

• If we were to just convert a subslice of the buffer into an InOutBuf (as above), the caller would need to put their plaintext at an algorithm-dependent offset -- this would be VERY surprising and strange breaking behavior, so it's not an option.

• If we want to sand down the above rough edge but keep the concept, InOutBuf would need to support an offset (so that .get_in() would return just the plaintext region, and .get_out() would return just the ciphertext region).

• If we just throw our hands up and accept the status quo, then the aes-siv crate (and any implementation of AeadInPlace which prepends its authentication tag) needs to implement BOTH an in-place, authentication-tag-packing-aware encryption method (one which has to read plaintext from the start and write ciphertext at an offset) AND in-out, authentication-tag-packing-unaware encryption method (one which simply reads plaintext from the in-buffer and write ciphertext to the out-buffer at the same position).

Honestly, I'm personally of the mind that the last option might be the most sensible one in terms of time spent, but I haven't yet dug deep into the aes-siv crate to gauge how much work would be required and whether I'm confident enough to implement the necessary changes optimally. That said, I think that adjusting InOutBuf to suit this use case might end up being the more flexible, elegant, and above all maintainable option.

What do you think? Am I overlooking something?

Simplifying wrappers

On a lesser note, as I was mentioning last night in the above PR, I'd very much like to see the specialist buffer types entirely abstracted away from callers in wrapper methods where possible.

As above, whatever we end up doing to encrypt_in_place, it would be ideal if we could also have an in-place encryption method that foregoes specialist types in exchange for slightly more complex semantics (needing to provide at least Algorithm::TagSize::to_usize() additional bytes of unused space).

Once we've resolved the conundrum in the section above, so long as adding a separate method is acceptable, it should be a fairly straightforward implementation.

[tarcieri]

As above, whatever we end up doing to encrypt_in_place, it would be ideal if we could also have an in-place encryption method that foregoes specialist types in exchange for slightly more complex semantics (needing to provide at least Algorithm::TagSize::to_usize() additional bytes of unused space).

I'll reiterate my previous comments that, based on my firsthand experience with APIs like this, I find them to be very clunky and annoying from an API ergonomics perspective:

• When encrypting, it punts the work of adding the tag overhead space to the caller
• It doesn't abstract over prefix versus postfix tags, leaving that as a caller concern
• It punts all of the above complexity to the caller, and in that regard I don't consider it a "simplification": it makes the code the caller must write more complex by removing the existing abstractions

I would very much consider removing Buffer in favor of such an API to be a significant regression over the current API ergonomics, and would be very, very strongly opposed to such a change.

I'm also confused why you're bringing it up again when yesterday you said "clearly it should stay in". I thought we had actually settled this matter?

[newpavlov]

I haven't looked deeply into the AEAD APIs for some time, so here some surface-level ideas and preferences which may change depending on discussion.

I think that "fundamental" AEAD API should be a detached-style trait on top of InOutBuf<u8>. We probably can drop support for modes based on padded block modes, since it makes API more complex for an almost nonexistent gain. The main disadvantage of this approach is lack of uninitialized buffer support (e.g. we will not be able to write encrypted data into buffer allocated with Vec::reserve), but we probably should leave it for a future breaking release.

Next, we should provide extension traits which implement prepending and appending modes of operation. In other words, even if an algorithm specification states that tag should be prepended, we should allow users to append tags while using it. Depending on whether tag is appended or prepended, the trait methods may provide slightly different APIs (e.g. for appending methods we can support in-place encryption over Buffer, but not for prepending ones). These traits may also provide methods to prepend/append not only tag, but also nonce+tag or tag+nonce.

Finally, about abstracting over prefix versus postfix tags in a high-level API. I think the easiest way to support this will be to provide only slice-to-Buffer encrypt API, i.e. it could look something like this: fn encrypt<B: Buffer>(nonce: &Nonce<Self>, payload: impl Into<Payload>) -> Result<B, Error>. This trait would be blanket implemented on top of the fundamental trait and a marker trait with single associated bool constant specifying tag position.

[Cel-Service]

I would very much consider removing Buffer in favor of such an API to be a significant regression over the current API ergonomics, and would be very, very strongly opposed to such a change.

I'm also confused why you're bringing it up again when yesterday you said "clearly it should stay in". I thought we had actually settled this matter?

Apologies; if you came away with the understanding that I think Buffer should be removed, I think I may have miscommunicated! In that section, I'm speaking in reference to this comment:

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.

The section "Simplifying wrappers" was a direct response to this section of that comment. Apologies that I hadn't made that clearer.

My position is only that it would be ideal for there to be an additional alternative to the APIs that require the caller to use specialist types (e.g. InOutBuf, Buffer, ArrayVec, BytesMut) when such an alternative is practical -- I hold no opinions that such types are inherently bad!

As a practical example, think of a situation where one receives a pair of file descriptors pointing to some mmappable read-only and read-write shared memory, into which some data must be efficiently decrypted. It's simply not an option to change how one allocates their buffer and swap in e.g. an ArrayVec -- the memory is already allocated -- thus such callers would have to go and figure out how to either allocate a Buffer-compatible struct inside of the shared memory buffer, use something like bytemuck to try to force the conversion by reference (as neither ArrayVec nor BytesMut offer a zero-copy conversion from &mut [u8]), or use the detached method and reimplement the boilerplate to copy out the tag from the return value into the correct, algorithm-dependent position manually.

In that situation, it would certainly be preferable to have an alternative available to forego that trouble!

---

I believe some of that confusion may also have been introduced by what I suggested in the prior section, so I wanted to make clear my understanding.

As I understand it, trait Buffer accomplishes two separate goals that make the API more ergonomic for callers:

• During encryption, it allows the caller to pass in just their plaintext, and expect their buffer to be automatically grown to accommodate the authentication tag (wherever and whatever size it may be).

• During decryption, it allows the caller to pass in their combined ciphertext and authentication tag (i.e. an opaque encrypted blob with the two combined in an algorithm-dependent order), and expect their buffer to be automatically truncated to contain just the resulting plaintext.

These goals make perfect sense -- both in the current API and any other.

My suggestion in the first section (specified below for clarity):

Implementing in-place, tag-position-agnostic decryption in terms of a call to self.decrypt_inout_detached (or whatever we call it) would be very simple: the API could be revised to fn decrypt_in_place<'a>(&self, nonce: &Nonce, ad: &[u8], buffer: &'a mut [u8]) -> Result<&'a mut [u8], Error>, very similar to plain fn decrypt.

...is only in relation to in-place decryption; if we want to revise the API to be entirely based on just methods (implemented by each algorithm crate) which consume InOutBuf, this alternative makes that goal easier while preserving the convenience that Buffer provides (by not expecting the caller to know how the ciphertext and authentication tag will be laid out).

The fact that the suggested in-place decryption API doesn't use Buffer to accomplish that goal is only a side-effect, not one of the goals of my suggestion.

---

I hope that clarification was able to resolve your concerns!

If so, what do you think of the conundrum I brought up regarding how best to adapt in-place, tag-prepended encryption to an InOutBuf-centric API?

Do you feel that adapting InOutBuf to better suit the prepended-authentication-tag use case is the better solution, that simply treating aes-siv as an outlier (as it currently more-or-less is) is more practical, or do you perhaps see an alternative or additional problems that I'm missing?

[tarcieri]

I would really prefer to initially focus exclusively on adapting the detached APIs to use InOutBuf, rather than making any simultaneous changes to the in-place APIs, and if we want to do that, consider it separately in a different issue. I think this discussion is already all over the place which is making it somewhat frustrating. There are many possible directions we can go and I feel like we're trying to explore too many of them at once, which is complicating actually making progress on any of them.

Doing that will also make PRs easier to review, and avoids unnecessarily breaking any existing APIs and code using them.

As a practical example, think of a situation where one receives a pair of file descriptors pointing to some mmappable read-only and read-write shared memory, into which some data must be efficiently decrypted.

Use cases like this that need very explicit control of buffering should be using the InOutBuf APIs.

If so, what do you think of the conundrum I brought up regarding how best to adapt in-place, tag-prepended encryption to an InOutBuf-centric API?

Adapting all of the existing code should be straightforward: the impls of AeadInPlace all use InOutBuf behind the scenes anyway.

Note: we can consider adding new slice-based APIs which abstract over InOutBuf, e.g. making it easier to use separate input and output slices, or a detached that calls .into() for you similar to how encrypt_in_place_detached/decrypt_in_place_detached work today.

[tarcieri]

Finally, about abstracting over prefix versus postfix tags in a high-level API. I think the easiest way to support this will be to provide only slice-to-Buffer encrypt API, i.e. it could look something like this: fn encrypt<B: Buffer>(nonce: &Nonce, payload: impl Into) -> Result<B, Error>. This trait would be blanket implemented on top of the fundamental trait and a marker trait with single associated bool constant specifying tag position.

This sounds good to me, although I'd probably suggest a marker trait with an associated enum const rather than a bool to specify the tag position, e.g. TagPosition::Prefix / TagPosition::Postfix to improve readability.

This could potentially improve the efficiency of the blanket impl of Aead (currently for AeadInPlace) which currently makes a needless copy of the input buffer so it can leverage *_in_place, rather than being able to use InOutBuf to use a separate output buffer for the Vec, which would allow the compiler to optimize away the initialization.

[Cel-Service]

Addressing your input, @newpavlov:

I think that "fundamental" AEAD API should be a detached-style trait on top of InOutBuf. We probably can drop support for modes based on padded block modes, since it makes API more complex for an almost nonexistent gain. The main disadvantage of this approach is lack of uninitialized buffer support (e.g. we will not be able to write encrypted data into buffer allocated with Vec::reserve), but we probably should leave it for a future breaking release.

I agree -- when you suggested this, I started work revising things to accommodate this. It makes a lot of sense -- implementing every other use case in terms of a wrapper around a method which consumes InOutBuf -- so I had (and have) no trouble with this approach.

Next, we should provide extension traits which implement prepending and appending modes of operation. In other words, even if an algorithm specification states that tag should be prepended, we should allow users to append tags while using it. Depending on whether tag is appended or prepended, the trait methods may provide slightly different APIs (e.g. for appending methods we can support in-place encryption over Buffer, but not for prepending ones). These traits may also provide methods to prepend/append not only tag, but also nonce+tag or tag+nonce.

I have no personal objection to this idea, but it might be wise to gate the alternative behind an explicit gesture to indicate that it's not the normal use case -- perhaps with something like this:

pub trait TagDetachedAead: AeadCore {/* ... */}
pub trait AppendsAuthenticationTag {} // Marker trait
pub trait PrependsAuthenticationTag {} // Marker trait
pub trait TagAppendingAead {/* ... */}
pub trait TagPrependingAead {/* ... */}

impl<Algorithm: TagDetachedAead + AppendsAuthenticationTag> TagAppendingAead for Algorithm {/* ... */}
impl<Algorithm: TagDetachedAead + PrependsAuthenticationTag> TagPrependingAead for Algorithm {/* ... */}

#[repr(transparent)
pub struct TagReversed<Algorithm: TagDetachedAead>(pub Algorithm);

impl<Algorithm: TagDetachedAead + AppendsAuthenticationTag> TagPrependingAead for TagReversed<Algorithm> {/* ... */}
impl<Algorithm: TagDetachedAead + PrependsAuthenticationTag> TagAppendingAead for TagReversed<Algorithm> {/* ... */}

(Of course, all above names are placeholders -- I hold no attachment to any of them.)

That said, if we want this kind of swappable behavior, I believe we will need to go with the option of adding an optional offset value to InOutBuf in order to facilitate the in-place, attached-tag wrapper methods.

[newpavlov]

a marker trait with an associated enum const rather than a bool to specify the tag position, e.g. TagPosition::Prefix / TagPosition::Postfix to improve readability

Yes, I agree. I had in mind the const generics restrictions, which do not apply in this case.

That said, if we want this kind of swappable behavior, I believe we will need to go with the option of adding an optional offset value to InOutBuf in order to facilitate the in-place, attached-tag wrapper methods.

Yes, we may need to introduce something similar to InOutBufReserved, but for prepended tags.

[tarcieri]

Next, we should provide extension traits which implement prepending and appending modes of operation. In other words, even if an algorithm specification states that tag should be prepended, we should allow users to append tags while using it.

I don't think this makes sense.

Most AEADs use a postfix mode. Why would we allow users to use a prefix tag with these modes? It's not how they're specified. It introduces needless user choice which allows for non-standard variant of something which is standardized, and won't be compatible with other implementations.

[newpavlov]

Most AEADs use a postfix mode. Why would we allow users to use a prefix tag with these modes?

I think that at the very least we should allow users to use prefix constructions in the postifx mode. I agree that the reverse is less important, but it may make sense from completeness point of view and in some niche applications. Bounding prefix methods on the associated constant is likely to result in a more convoluted API overall.

[tarcieri]

Using prefix constructions in a postfix mode would again create non-standard variants of algorithms which aren't compatible with any other implementation.

I think we should implement the algorithms as specified and not introduce arbitrary knobs that allow users to create nonstandard variants.

I can imagine that leading to a nasty surprise when users attempt to interop only to discover that their messages aren't formatted in the standard way.

[Cel-Service]

I would really prefer to initially focus exclusively on adapting the detached APIs to use InOutBuf, rather than making any simultaneous changes to the in-place APIs, and if we want to do that, consider it separately in a different issue.

Part of the trouble is that this is perfectly doable for most of the API's use cases, but falls apart for in-place, tag-attached, tag-prepended encryption and decryption, as I specified before (this is the reason I created this issue rather than setting my nose to the grindstone and (eventually) submitting a PR for exactly that).

I'll go over the issues I've encountered while updating aead and its algorithm crates, and hopefully that will better explain why I've gone to all of this effort.

Problem 1: In-place, tag-attached, tag-prepended operations

We have a number of constraints for in-place encryption and decryption, all of which need to be met:

• Callers want to pass in their plaintext (i.e. at the start of their buffer) and when their method returns, have their opaque "encrypted blob" (i.e. the combined ciphertext and authentication tag) at the start of whatever they get back (and vice versa). This holds true for all of the APIs: Vec-based, in-out, and in-place; tag-detached, tag-appended, and tag-prepended.

• A small number of algorithms (like AES-SIV) expect to put their authentication tag before the plaintext, and we want to accomodate that (i.e. we can't just assume that the ciphertext will be in the same place as the plaintext -- it might be written at an offset).

• InOutBuf only accepts either one single &mut [u8] or a non-overlapping &[u8] and &mut [u8] pair.

• We ideally want to write only one implementation that covers every use case -- so at the root of everything, we want a fn encrypt_inout_detached(&self, nonce: &Nonce<Self>, ad: &[u8], buffer: InOutBuf<'_, '_, u8>) -> Result<Tag<Self>> and fn decrypt_inout_detached(&self, nonce: &Nonce<Self>, ad: &[u8], buffer: InOutBuf<'_, '_, u8>, tag: &Tag<Self>) -> Result<...> (or whatever we name it) to be implemented by each algorithm and called by every provided method in our API.

• We ideally want neither the callers nor the implementers of these traits to have to think about how to pack or unpack the authentication tag, nor where the authentication tag is or will be, as reiterated; i.e. ideally, our provided methods should (at least eventually) take care of this if possible.

Everything works fine in-place while tags are detached because the plaintext is going to be the same length and position as the ciphertext -- we can create an InOutBuf from that.

Everything works fine in-place while tags are appended because the plaintext is still going to be the same length and position as the ciphertext .

Everything works fine while prepending in-out because the in-buffer and out-buffer will never overlap.

But everything breaks down when tags are prepended in-place because the plaintext and ciphertext (i.e. in-buffer and out-buffer) necessarily overlap at an offset.

Thus, something needs to give: either we need to revise the API to accommodate our constraints, or we need to implement offset-aware in-place, tag-prepended overrides in crates like aes-siv, or we need to improve InOutBuf to enable this use case.

That is why I put my thoughts as to various options in the original post -- we don't have to change the surrounding APIs, but we do have to change something if we want to base this API on InOutBuf at its core.

Problem 2: Algorithm crate compatibility with InOutBuf

Adapting all of the existing code should be straightforward: the impls of AeadInPlace all use InOutBuf behind the scenes anyway.

This is actually, unfortunately, not the case for every crate in AEADs. It is for most of them, but not for all of them. Going from most to least time-intensive:

• ascon-aead's control flow had to be reworked, and I don't believe it uses any of the underlying cipher crate traits, anyway. (I did this a week and a half ago, after receiving the suggestion to use InOutBuf.)

• ocb3 is in a similar boat -- it does use encrypt_block_inout, but it implements its own chunking control flow very similar to ascon-aead. Either I have to continue digging through it to make sure I completely understand what I'm changing, or someone already familiar with it would need to update it. (I'm midway through this, but paused my work to wait and see how our solution to Problem 1 might affect the API surface.)

• I haven't looked deep into aes-siv yet (I put that off until we have a solution to Problem 1) but the amount of effort involved will vary depending on our chosen solution.

• deoxys uses AES hazmat directly -- I'm not brave enough to mess around inside of it more than implementing *_inout_detached in terms of *_in_place_detached, copying the contents of the the input buffer to the output buffer in order to be processed. (I've done this already as well.)

• mgm seems to be unmaintained -- I believe it's still on traits 0.4, if I'm not mistaken.

Of course, these things also take longer for me than they would someone already deeply familiar with these algorithms' internals as I want to be completely certain I'm understanding what I'm changing -- I don't simply want to dump an API change proposal and expect someone else to implement those changes, nor do I want to unintentionally contribute half-baked, poorly-thought-out implementations to your project!

[tarcieri]

But everything breaks down when tags are prepended in-place because the plaintext and ciphertext (i.e. in-buffer and out-buffer) necessarily overlap at an offset.

Supporting this properly would need support in the underlying cipher for encrypting/decrypting data at an offset. There's an issue tracking support for that here: #764

In absence of that, we work around this issue for aes-siv by moving the data within the buffer first in order to make room for the tag: https://github.com/RustCrypto/AEADs/blob/7fc675e/aes-siv/src/siv.rs#L278-L279

That isn't ideal since it's incurring an additional copy, but it's the best we can do without underlying support from the cipher itself.

In the meantime I would suggest continuing to use the copy_within approach, which should still work fine with InOutBuf. This problem only impacts aes-siv so I'm not terribly worried about trying to find a solution with micro-optimizes this use case.

[Cel-Service]

Understood. So just to be clear, we'll treat aes-siv as an outlier and implement adaptations to accomodate its quirks as overrides to the provided methods?

I'm not a complete Rust expert, so please do bear with me in asking this question, as I will admit this solution has grown on me since having posed it: would it not be possible to adapt any algorithm which is implemented in terms of InOutBuf to support an offset if InOutBuf itself supports an offset?

That is to say -- would there be any fundamental problem in adding an offset: isize, field to InOutBuf (or its internals) that affects which region of the in-place buffer is returned when calling .get_in() and .get_out()?

If not, I'm more than willing to go the route of simply adapting aes-siv as an edge case so long as performing that copy is an acceptable compromise -- my only reason for starting all of this discussion was to ensure I wouldn't be submitting a PR that would be rejected. I only ask as I would like to contribute a more optimal implementation if the effort would be comparable.

[tarcieri]

That is to say -- would there be any fundamental problem in adding an offset: isize, field to InOutBuf (or its internals) that affects which region of the in-place buffer is returned when calling .get_in() and .get_out()?

That sounds like a potentially interesting solution if it can be made to work

[newpavlov]

Using prefix constructions in a postfix mode would again create non-standard variants of algorithms which aren't compatible with any other implementation.

Honestly, I am not sure about importance of prefix/postfix mode specified by an AEAD algorithm in practice. In my experience, tag/nonce handling is usually defined by a file format or protocol, not by an AEAD algorithm.

Moreover, as discussed above, SIV is a clear outlier in this regard, so I question the effort we spend on ergonomically supporting it. The copy_within approach is clearly inefficient, so it should be avoided if possible.