Skip to content

Conversation

@tarcieri
Copy link
Member

@tarcieri tarcieri commented Nov 1, 2020

This PR is based on #352, which should get merged first.

Adds traits explicitly for parallel encryption/decryption, and blanket impls of the Encrypt/Decrypt traits for EncryptPar/DecryptPar.

This makes the slice-based methods the primary user-facing API, and with it eliminating the need to worry about the ParBlocks associated type where it doesn't matter.

Closes #349

Adds `Encrypt` and `Decrypt` traits which can be used in cases where
e.g. only the encryption portion of a block cipher is used, as in
CTR mode.

This PR does not otherwise attempt to add things like a blanket impl.
@tarcieri tarcieri marked this pull request as draft November 1, 2020 17:10
@tarcieri tarcieri requested a review from newpavlov November 1, 2020 17:10
Adds traits explicitly for parallel encryption/decryption, and blanket
impls of the `Encrypt`/`Decrypt` traits for `EncryptPar`/`DecryptPar`.

This makes the slice-based methods the primary user-facing API, and with
it eliminating the need to worry about the `ParBlocks` associated type
where it doesn't matter.
Copy link
Member

@newpavlov newpavlov left a comment

Choose a reason for hiding this comment

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

I am not sure about usefulness of such split. If the main concern is a possible runtime variability of ParBlocks, I think it will be better to move from the associated constant. One option is to provide a method which would return a recommended number of parallel blocks as was proposed earlier. Another one is to introduce methods which could like this:

fn encrypt_block_chunk<'a>(&self, blocks: &'a mut [Block]) -> (&'a [Block], &'a mut [Block]);

The idea is that it will return processed blocks on the left and unprocessed leftovers on the right. For example it could be helpful for one-pass implementations of AEAD construct. For efficiency sake we could mandate that it will do parallel processing only, so if the left slice is empty, users should use encrypt_block(s) on leftovers. I am not sure how well it will work in practice so some tweaks may be needed.

@tarcieri
Copy link
Member Author

tarcieri commented Nov 1, 2020

The main goal is to encapsulate parallelism as an implementation detail and avoid dealing with it where it's irrelevant.

Most (12 out of 16) of the crates in https://github.com/rustcrypto/block-ciphers declare ParBlocks = U1. With this approach, they wouldn't even need to do that.

For similar reasons we omitted ParBlocks from BlockCipherMut, namely that it's targeting embedded accelerators which are most often not going to operate in parallel. But some do, so there's something of an API hole there. It'd be nice to have symmetry with the *Mut APIs, but also avoid dealing with parallelism when we don't have to.

Also now that you mention it, it does also simplify making runtime adapters to different backing cipher implementations.

I think it will be better to move from the associated constant.

I think it's nice to have an associated parallel buffer type accessible when possible, especially for things like interleaving encryption and universal hash operations in parallel. It might make sense to make that into a more fully fledged associated type which could e.g. wrap core::simd (or rather, packed_simd_2) or other XMM register vector type and keep data in XMM registers between AES-NI operations and POLYVAL/GHASH operations. (Edit: left some notes about this in #289)

Those are things we can't explore without some kind of associated type, though.

@newpavlov
Copy link
Member

Most (12 out of 16) of the crates in https://github.com/rustcrypto/block-ciphers declare ParBlocks = U1. With this approach, they wouldn't even need to do that.

Saving one line per crate does not look like a good motivation for introducing two traits. Also note that we have to specify ParBlocks = U1 only because associated type defaults are unstable. With const generics we will be able to write const PAR_BLOCK_SIZE: usize = 1; in trait definitions, so non-parallel BlockEncrypt/BlockDecrypt implementors would have to implement only one method per trait without any associated constants or types.

Making it easier to keep block data in registers is a separate issue, which is probably better discussed in the linked issue.

@tarcieri
Copy link
Member Author

tarcieri commented Nov 5, 2020

Ok, will close this for now

@tarcieri tarcieri closed this Nov 5, 2020
@tarcieri tarcieri deleted the cipher/split-par-blocks-trait branch November 5, 2020 16:34
@tarcieri
Copy link
Member Author

@newpavlov I encountered a reason why I think it might be nice to revive this, or at least a problem I'd like to generally solve one way or another.

The problem occurs when trying to write a bound like this:

where B: BlockCipher<BlockSize = U16>

These exist in several places, including aes-gcm, aes-gcm-siv, ccm, ctr, eax, and mgm.

In each of these cases, ParBlocks "leaks" out from the trait bounds:

error[E0277]: the trait bound `<Aes as BlockCipher>::ParBlocks: ArrayLength<GenericArray<u8, UInt<UInt<UInt<UInt<UInt<UTerm, B1>, B0>, B0>, B0>, B0>>>` is not satisfied
   --> aes-gcm/src/lib.rs:189:39
    |
189 |     Aes: NewBlockCipher + BlockCipher<BlockSize = U16> + BlockEncrypt,
    |                                       ^^^^^^^^^^^^^^^ the trait `ArrayLength<GenericArray<u8, UInt<UInt<UInt<UInt<UInt<UTerm, B1>, B0>, B0>, B0>, B0>>>` is not implemented for `<Aes as BlockCipher>::ParBlocks`
    |
   ::: /Users/bascule/.cargo/registry/src/github.com-1ecc6299db9ec823/cipher-0.3.0-pre.4/src/block.rs:53:21
    |
53  |     type ParBlocks: ArrayLength<Block<Self>>;
    |                     ------------------------ required by this bound in `BlockCipher`

So in each case, to bound on the block size, you actually have to write:

where
    B: BlockCipher<BlockSize = U16>,
    B::ParBlocks: ArrayLength<GenericArray<u8, U16>>,

This feels like an implementation detail leaking out which could otherwise be hidden.

@tarcieri tarcieri restored the cipher/split-par-blocks-trait branch December 30, 2020 18:45
@tarcieri tarcieri reopened this Dec 30, 2020
@newpavlov
Copy link
Member

Yes, it's quite unfortunate, but I don't think it's a good motivation for introducing those traits. I would like to keep API which will require minimal changes when migrated to const generics and AFAIK with them the problematic bounds will not be longer needed.

@tarcieri
Copy link
Member Author

Closing this out in favor of a more general and ambitious proposal: #444

@tarcieri tarcieri closed this Dec 30, 2020
@tarcieri tarcieri deleted the cipher/split-par-blocks-trait branch December 30, 2020 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants