Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

aead: factor apart AeadInPlace/*Detached #1714

Merged
merged 1 commit into from
Feb 28, 2025
Merged

aead: factor apart AeadInPlace/*Detached #1714

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 48 additions & 23 deletions aead/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ pub trait Aead: AeadCore {
) -> Result<Vec<u8>>;
}

/// In-place stateless AEAD trait.
/// In-place AEAD trait.
///
/// This trait is both object safe and has no dependencies on `alloc` or `std`.
pub trait AeadInPlace: AeadCore {
Expand All @@ -224,25 +224,61 @@ pub trait AeadInPlace: AeadCore {
nonce: &Nonce<Self>,
associated_data: &[u8],
buffer: &mut dyn Buffer,
) -> Result<()> {
let tag = self.encrypt_in_place_detached(nonce, associated_data, buffer.as_mut())?;
buffer.extend_from_slice(tag.as_slice())?;
Ok(())
}
) -> Result<()>;

/// Decrypt the message in-place, returning an error in the event the
/// provided authentication tag does not match the given ciphertext.
///
/// The buffer will be truncated to the length of the original plaintext
/// message upon success.
fn decrypt_in_place(
&self,
nonce: &Nonce<Self>,
associated_data: &[u8],
buffer: &mut dyn Buffer,
) -> Result<()>;
}

/// Encrypt the data in-place, returning the authentication tag
/// In-place AEAD trait which handles the authentication tag as a return value/separate parameter.
pub trait AeadInPlaceDetached: AeadCore {
/// Encrypt the data in-place, returning the authentication tag.
fn encrypt_in_place_detached(
&self,
nonce: &Nonce<Self>,
associated_data: &[u8],
buffer: &mut [u8],
) -> Result<Tag<Self>>;

/// Decrypt the message in-place, returning an error in the event the
/// provided authentication tag does not match the given ciphertext.
///
/// The buffer will be truncated to the length of the original plaintext
/// message upon success.
/// Decrypt the message in-place, returning an error in the event the provided
/// authentication tag does not match the given ciphertext (i.e. ciphertext
/// is modified/unauthentic)
fn decrypt_in_place_detached(
&self,
nonce: &Nonce<Self>,
associated_data: &[u8],
buffer: &mut [u8],
tag: &Tag<Self>,
) -> Result<()>;
}

/// Marker trait for AEAD algorithms which append the authentication tag to the end of the
/// ciphertext message.
///
/// This is the common convention for AEAD algorithms.
pub trait PostfixTagged {}
Copy link
Member

@newpavlov newpavlov Feb 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't think that the trait is better than the IS_POSTFIX: bool associated constant.

Considering that we do not have specialization, it would be hard to implement a generic method encrypt_to_buffer<B: Buffer>(&self, nonce: &Nonce<Self>, aad: &[u8], data: &[u8]) -> Result<B>.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A generic implementation of a prefix tag will always be suboptimal due to copying. It requires special features to implement efficiently. It's also an edge case.

Copy link
Member

@newpavlov newpavlov Feb 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the method works buffer-to-buffer, so there is no additional suboptimality in it for the prefixed case.

With the constant we can simply write this:

fn encrypt_to_buffer<B: Buffer>(&self, nonce: &Nonce<Self>, aad: &[u8], data: &[u8]) -> Result<B> {
    if Self::IS_POSTFIX {
        // postfix impl
    } else {
        // prefix impl
    }
}

And the compiler will trivially remove the unused branch. It's exactly what I did here (UPD: fixed the link).

Meanwhile, with the trait it's impossible to write such implementation without relying on specialization. Alternatively, you need to introduce an additional hacky PrefixTagged trait.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want a PrefixTagged trait.

Personally I am having trouble reading your code. Where is your solution for:

  • Accepting the input as a Buffer which contains the plaintext input message
  • Resizing the buffer to make room for the tag
  • Encrypting the plaintext to ciphertext in-place while shifting the ciphertext forward in the buffer to make room for the prefix tag
  • Adding the tag to the beginning of the message

Copy link
Member

@newpavlov newpavlov Feb 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accepting the input as a Buffer which contains the plaintext input message

Here is your fundamental misunderstanding of the proposed method. It accepts data: &[u8] and writes encrypted data into impl Buffer (e.g. Vec). In other words, it fundamentally does not support in-place operation.

The method which implements in-place postfix encryption of a Buffer with resizing for the tag is here. For obvious reasons, we do not have a prefix variant for this.

Copy link
Member

@newpavlov newpavlov Feb 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

encrypt_to_buffer is the method for "don't care" users. It works with both prefix and postfix AEAD constructions. Yes, it has the inefficiency of writing encrypted data into a separate buffer, but I believe it's a fine tradeoff for "don't care" users.

We could implement a generic in-place encrypt_buffer(&self, aad: &[u8], data: &mut impl Buffer) method in a similar fashion, but it would inevitably involve an overlapping copy inside the buffer in the prefix branch.

Copy link
Member Author

@tarcieri tarcieri Feb 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this is exactly what the solution in this PR solves:

  • There is only one API for end users. The prefix/postfix distinction is not exposed in the methods. Everything Just Works(tm). Exposing it to users just invites them to use the wrong tag order, which would produce AEAD messages incompatible with other implementations, violating standards.
  • The postfix case is handled generically
  • For the minuscule number of constructions (AES-CMAC-SIV, XSalsa20Poly1305) which do use a prefix construction, those implementations can call directly into an efficient encryption/decryption operation which shifts the output (ring provides such a feature, we don't, but probably should), which could avoid a copy

The latter is what I was talking about at the start of this thread:

A generic implementation of a prefix tag will always be suboptimal due to copying. It requires special features to implement efficiently.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is only one API for end users. The prefix/postfix distinction is not exposed in the methods. Everything Just Works(tm).

And how is it different from the API proposed by me? It also has high level methods which "just work" generically for any construction. Meanwhile with the current design, Aead may not be available for an AEAD implementation since we have a blanket impl only for postfix constructions. It's less "just works" in my opinion compared to my design in which you always have the high-level methods.

Exposing it to users just invites them to use the wrong tag order, which would produce AEAD messages incompatible with other implementations, violating standards.

I disagree with this. We provide a plenty of "use with caution" APIs. The method names literally contain "prefix" and "postfix" in their names and warnings about potential incompatibility with standards in their docs. But I digress, we could gate those methods on IS_POSTFIX, if you really want it that strongly.

For the minuscule number of constructions (AES-CMAC-SIV, XSalsa20Poly1305) which do use a prefix construction, those implementations can call directly into an efficient encryption/decryption operation which shifts the output (ring provides such a feature, we don't, but probably should), which could avoid a copy

Nothing prevents you from overwriting the default method impls in my PR with a more efficient implementation when (if?) we implement the offsetting API.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And how is it different from the API proposed by me?

We just went over this: you have no in-place API which abstracts over the prefix/postfix distinction. You instead make users have to care about the distinction.

I don't think we should expose the prefix/postfix distinction to users at all, or at the very least try to make the degree to which we expose it as minimal as possbile.

I disagree with this. We provide a plenty of "use with caution" APIs.

If we have the choice of an API with footguns versus a footgun-free API, we should absolutely choose the footgun-free API.

We sometimes expose APIs with footguns to end users, but with great care (placed under hazmat features/modules), and with clean separation between the two, not forced into the same trait.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you have no in-place API which abstracts over the prefix/postfix distinction

And I explicitly wrote that we could easily implement it using overlapping copies for the blanket impl while reserving the ability to overwrite it in future with a more efficient impl.

I would like to return to the PrefixTagged vs IS_POSTFIX discussion, but I guess it's better to open a separate issue for it.


impl<T: AeadInPlaceDetached + PostfixTagged> AeadInPlace for T {
fn encrypt_in_place(
&self,
nonce: &Nonce<Self>,
associated_data: &[u8],
buffer: &mut dyn Buffer,
) -> Result<()> {
let tag = self.encrypt_in_place_detached(nonce, associated_data, buffer.as_mut())?;
buffer.extend_from_slice(tag.as_slice())?;
Ok(())
}

fn decrypt_in_place(
&self,
nonce: &Nonce<Self>,
Expand All @@ -261,17 +297,6 @@ pub trait AeadInPlace: AeadCore {
buffer.truncate(tag_pos);
Ok(())
}

/// Decrypt the message in-place, returning an error in the event the provided
/// authentication tag does not match the given ciphertext (i.e. ciphertext
/// is modified/unauthentic)
fn decrypt_in_place_detached(
&self,
nonce: &Nonce<Self>,
associated_data: &[u8],
buffer: &mut [u8],
tag: &Tag<Self>,
) -> Result<()>;
}

#[cfg(feature = "alloc")]
Expand Down