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.

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

universal-hash v0.5 #965

Closed
wants to merge 6 commits into from
Closed

universal-hash v0.5 #965

wants to merge 6 commits into from

Conversation

newpavlov
Copy link
Member

The new API uses approach of emulating rank-2 closures previously introduced in cipher v0.4.

Also moves ParBlocksSizeUser and ParBlocks from cipher to crypto-common.

use subtle::ConstantTimeEq;
use typenum::Unsigned;

/// Trait implemented by UHF backends.
Copy link

Choose a reason for hiding this comment

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

Document how this trait is intended to be used.

/// Size of the inputs to and outputs from the universal hash function
type BlockSize: ArrayLength<u8>;
pub trait UniversalHash: BlockSizeUser + Sized {
/// Update hash function state using the provided rank-2 closure.
Copy link

Choose a reason for hiding this comment

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

Document how this method is intended to be implemented (calling out to target-specific backends). If you don't want that documentation to live on the trait that end users will be using, then instead add a reference here to UhfBackend and document the implementor notes there.

Copy link

Choose a reason for hiding this comment

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

This is also where the note about user trait usage should go, for e.g. AEADs (RustCrypto/AEADs#414 (comment)).

@tarcieri
Copy link
Member

@newpavlov can you add the docs @str4d requested and get this merged so we can make progress on RustCrypto/universal-hashes#153 ?

@newpavlov
Copy link
Member Author

I plan to improve the docs, but they do not block the work on RustCrypto/universal-hashes#153 and update of the AEAD crates. Before working on docs I plan to properly review the @str4d's PR first.

@tarcieri
Copy link
Member

Okay, well there's also RustCrypto/AEADs#414 and RustCrypto/AEADs#415, which means we have a lot of PRs that are interdependent on other PRs.

And I'd like to start work on updating aes-gcm and aes-gcm-siv but it doesn't make sense to me to start that until some of these interdependencies are unblocked.

/// Trait implemented by UHF backends.
pub trait UhfBackend: ParBlocksSizeUser {
/// Process single block.
fn proc_block(&mut self, block: &Block<Self>);
Copy link

@str4d str4d Apr 24, 2022

Choose a reason for hiding this comment

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

Per RustCrypto/universal-hashes#153 (comment), we are currently lacking a way to inform the consumer of the UhfBackend that the backend is "unaligned" and cannot consume ParBlocks efficiently (with impls either panicking, or always falling back to a simple loop over UhfBackend::proc_block which causes an extra copy of each block and I suspect breaks inlining). One way to avoid this would be for the backend to expose the misalignment:

Suggested change
fn proc_block(&mut self, block: &Block<Self>);
fn proc_block(&mut self, block: &Block<Self>);
/// Returns the number of blocks that should be passed to `Self::proc_block` before
/// `Self::proc_par_blocks` can be used efficiently. This is always less than
/// `Self::ParBlocksSize`.
fn blocks_needed_to_align(&self) -> usize {
0
}

@tarcieri
Copy link
Member

In the interest of moving things along and getting additional -pre the AEAD crates out (it's now been 5 months since cipher v0.4.0 was released, FWIW), I'd suggest merging this and releasing universal-hash v0.5.0-pre, and then merging the respective PRs on https://github.com/rustcrypto/universal-hashes and releasing prereleases of those crates.

We can open a general tracking issue for crates which are in need of more documentation.

@tarcieri tarcieri added univeral-hash Universal Hash Function (UHF) crate breaking Proposed breaking changes labels Jun 28, 2022
@str4d
Copy link

str4d commented Jul 3, 2022

Looks like this PR needs a rebase to address merge conflicts? Also I'd like feedback on my suggestion above (as currently I don't think the API changes here can ever help ChaCha20Poly1305 performance due to the immediate 1-block misalignment when initializing the AEAD).

@tarcieri
Copy link
Member

tarcieri commented Jul 9, 2022

@str4d it seems like adding a "flush" API which consumes the internal buffer and returns it to an aligned state could be added to address that problem? Then AEADs could just flush the UHF after inputting the AAD.

Regardless, releases of the AEAD crates seem stalled on this PR, and the only way we could experiment with the necessary API changes to address issues like buffer misalignment without merging this is as a series of PRs to AEADs which point to PRs to this branch of universal-hash which seems somewhat untenable.

Getting this merged (and ideally an associated prerelease cut) means we can more easily make incremental progress towards a final release.

tarcieri added a commit that referenced this pull request Jul 16, 2022
Moves `ParBlocks`/`ParBlocksSizeUser` from the `cipher` crate so it can
be reused in the `universal-hash` crate (see #965, #1051).

The `cipher` crate re-exports them in an API-compatible way, so this is
not a breaking change.
tarcieri added a commit that referenced this pull request Jul 16, 2022
Moves `ParBlocks`/`ParBlocksSizeUser` from the `cipher` crate so it can
be reused in the `universal-hash` crate (see #965, #1051).

The `cipher` crate re-exports them in an API-compatible way, so this is
not a breaking change.
@tarcieri
Copy link
Member

Rebased/squashed this PR and opened a new one: #1051

@tarcieri
Copy link
Member

Merged in #1051

@tarcieri tarcieri closed this Jul 17, 2022
@tarcieri tarcieri deleted the uhf/v0.5 branch July 17, 2022 14:27
tarcieri added a commit that referenced this pull request Jul 24, 2022
Originally suggested by @str4d here:

#965 (comment)

The use case is ensuring unbuffered processing of the input AEAD message
body when the AAD does not line up with the number of blocks it's
processing in parallel.

This number of blocks can be input from the start of the message body to
finish completely filling the buffer, ensuring all subsequent processing
occurs at the start of the buffer.
tarcieri added a commit that referenced this pull request Jul 24, 2022
Originally suggested by @str4d here:

#965 (comment)

The use case is ensuring unbuffered processing of the input AEAD message
body when the AAD does not line up with the number of blocks it's
processing in parallel.

This number of blocks can be input from the start of the message body to
finish completely filling the buffer, ensuring all subsequent processing
occurs at the start of the buffer.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Proposed breaking changes univeral-hash Universal Hash Function (UHF) crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants