aead: Add detached in-place encrypt/decrypt API; make alloc optional

[tarcieri]

This commit adds encrypt_in_place_detached() and decrypt_in_place_detached() methods to Aead and AeadMut, along with default implementations of encrypt() and decrypt() which assume a postfix authentication tag.

Presently all of the AEAD implementations in RustCrypto/AEADs use this pattern. This PR exposes the detached interface as a public API:

RustCrypto/AEADs#21

Note that this need not be the only in-place API (hence the long name with _detached on the end. I plan on doing a follow-up PR for adding an in-place API which does not depend on alloc but also handles ciphertext message assembly/parsing, which would be more useful for end
users.

That said, this API isn't just useful for AEAD implementers: there are genuine use cases for a detached API from an end user perspective, e.g encrypted filesystems.

With the addition of this API, we can also make alloc an optional (but
enabled-by-default) feature, allowing use of AEADs on truly #![no_std]
targets.

[tarcieri]

cc @newpavlov

[jcape]

Could you help me understand why you'd want to return the tag here, rather than doing that in-place as well? Other than requiring split_at_mut() in the macro it seems like it would otherwise be superior...

[tarcieri]

That's actually what I originally suggested here: #40 (comment)

@newpavlov suggested returning the tag instead here: #40 (comment)

I ended up liking that suggestion. I think it's cleaner because it's one less side effect, one less mutable argument, and actually leverages the return value.

Every time you rely on a side effect there's a risk it won't happen. That risk is unavoidable with an in-place API: mutation-by-side-effect is the name of the game. But we don't need to rely on it happening it twice, and can instead have the underlying implementation construct the tag by value, instead of it being one more thing for the caller to initialize with the hope the backing implementation actually does its job.

[jcape]

I dunno, if the cipher implementation is doing all this in-place, and it has trouble mutating slices, my intuition is that you've probably got bigger problems than worrying about the tag getting written.

[akhilles]

I agree with @jcape on doing the tag in-place. Typically, the tag will be prefixed/suffixed to the ciphertext anyways. From a performance standpoint, it would be great if this didn't require a memcpy.

IMO, the risk of the backing implementation not mutating the tag is roughly the same as it returning a bad tag.

[akhilles]

This might be a bit extreme, but I would be in favor of having 0 visibility into the tag for all APIs. It should be treated as implicit ciphertext expansion.

[jcape]

This might be a bit extreme, but I would be in favor of having 0 visibility into the tag for all APIs. It should be treated as implicit ciphertext expansion.

There's an extensive discussion on the ciphertext layout in #40 :-) I'm generally in agreement with you (RFC and all), but if it prevents divergent implementations for weird non-RFC-compliant schemes like the encrypted filesystem thing where the tag lives in some separate metadata structure, I'm OK with exposing this API.

[akhilles]

I was exploring an uglier version of the API that you're proposing:

fn encrypt_in_place<'a>(
  &self,
  nonce: &GenericArray<u8, Self::NonceSize>,
  buffer: &'a mut [u8],
  offset: usize,
  len: usize,
  associated_data: &[u8],
) -> Result<&'a [u8], Error>;

It attempts to maximize flexibility at the cost of certain assumptions that need to be made:

• plaintext fits in buffer: buffer.len() >= offset + len
• ciphertext fits in buffer: buffer.len() >= len + tag_len

Is this something that can coexist with encrypt_in_place_detached() and decrypt_in_place_detached(). Maybe it can be part of an UnsafeAEAD trait?

[akhilles]

Alternative using range bounds:

fn encrypt_in_place<'a>(
  &self,
  nonce: &GenericArray<u8, Self::NonceSize>,
  buffer: &'a mut [u8],
  plaintext: RangeFrom<usize>,
  associated_data: &[u8],
) -> Result<&'a [u8], Error>;

fn decrypt_in_place<'a>(
  &self,
  nonce: &GenericArray<u8, Self::NonceSize>,
  buffer: &'a mut [u8],
  ciphertext_tag: RangeFrom<usize>,
  associated_data: &[u8],
) -> Result<&'a [u8], Error>;

[tarcieri]

@akhilles see #59 for my take on an in-place API based on a Buffer trait, based loosely on some of the things @newpavlov was proposing back here (albeit using a trait instead of a callback function): #40 (comment)

Is this something that can coexist with encrypt_in_place_detached() and decrypt_in_place_detached()

Yup, that was the goal of having *_in_place_detached vs *_in_place and composing them

[akhilles]

Since the _in_place_detached API is the implementation layer, can the offset param be added to it? It's very useful for processing messages/packets with headers, and it cannot be efficiently incorporated into a higher level API.

[tarcieri]

@akhilles I don't understand what you're trying to accomplish which can't be accomplished by slicing the buffer alone...

[akhilles]

@tarcieri, borrowing from ring's documentation:

                Packet 1                  Packet 2                 Packet 3
Input:  [Header][Ciphertext][Tag][Header][Ciphertext][Tag][Header][Ciphertext][Tag]
                     |         +--------------+                        |
              +------+   +-----+    +----------------------------------+
              v          v          v
Output: [Plaintext][Plaintext][Plaintext]
       “Split stream reassembled in place”

this can't be done with slicing AFAIK

[akhilles]

I guess I should have been more specific: offset is only used for the input buffer. The output starts at index 0 and overwrites the offset bytes.

I encountered this use case a lot in applications using mbedtls. It's typically solved by offsetting the raw pointers for input and output.

[tarcieri]

If I understand correctly, you have multiple AEAD messages in a single buffer, and want the resulting plaintexts to be written to an earlier position in the same buffer?

[akhilles]

@tarcieri yup!

[tarcieri]

Ok, interesting. That seems worth supporting, but also note that to really be helpful we'd need to implement offsets throughout the whole "stack", e.g. in SyncStreamCipher.

Without that the only thing the current AEAD implementations can do is copy the plaintext after decryption.

[akhilles]

Understood, it's probably a bit out of scope for this PR. I'll start with some of the lower level components and try to introduce offset where it makes sense.

[tarcieri]

@akhilles we can go ahead and add it to the API now, it's just it will have to be implemented entirely in terms of something like copy_within until an apply_keystream_with_offset method or thereabouts is added to SyncStreamCipher

[tarcieri]

@newpavlov any chance you have some time to look at this?

[newpavlov]

Sorry for the delay!

I am not 100% happy with this API (ideally I would like to do unification with block-modes), but it should be a fine starting point. The request raised by @akhilles is an interesting one and we should not forget about it during discussions of the next version.

[newpavlov]

In the implementation crates don't forget to write:

aead = { version = "0.1.2", default-features=false }
[features]
default = ["alloc"]
alloc = ["aead/alloc"]

And in the next minor version we should not forget to clean it up.

[tarcieri]

@newpavlov awesome, thank you! Does this mean you're going to cut a v0.1.2 with this in it?