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

Import offset-cookbook-mode #550

Closed
wants to merge 7 commits into from
Closed

Conversation

sgmenda
Copy link
Contributor

@sgmenda sgmenda commented Sep 16, 2023

Re sgmenda/offset-cookbook-mode#1

I slightly cleaned up the code, made the implementation generic over Aes, added more KATs, and tried to match the repo's style.

Comment on lines +27 to +32
/// Number of L values to be precomputed. Precomputing m values, allows
/// processing inputs of length up to 2^m blocks (2^m * 16 bytes) without
/// needing to calculate L values at runtime.
///
/// By setting this to 32, we can process inputs of length up to 1 terabyte.
const L_TABLE_SIZE: usize = 32;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we support inputs longer than one terabyte?

@tarcieri tarcieri mentioned this pull request Sep 18, 2023
8 tasks
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.

You should add a CI config for this crate.

Note: I haven't looked deep into the implementation yet.

ocb3/README.md Outdated Show resolved Hide resolved
associated_data: hex!("").to_vec(),
plaintext: hex!("").to_vec(),
ciphertext: hex!("785407BFFFC8AD9EDCC5520AC9111EE6").to_vec(),
},
Copy link
Member

Choose a reason for hiding this comment

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

These tests are better implemented using the new_test macro. If you want, I can generate blobby file for these tests a bit later.

ocb3/Cargo.toml Show resolved Hide resolved
ocb3/src/util.rs Outdated Show resolved Hide resolved
ocb3/src/lib.rs Outdated Show resolved Hide resolved
ocb3/src/lib.rs Outdated Show resolved Hide resolved
ocb3/src/lib.rs Outdated Show resolved Hide resolved
@@ -0,0 +1,602 @@
#![no_std]
#![cfg_attr(docsrs, feature(doc_cfg))]
#![doc = include_str!("../README.md")]
Copy link
Member

Choose a reason for hiding this comment

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

If you include readme, it would be nice to add a simple example or two into it.

pub type Tag<TagSize> = GenericArray<u8, TagSize>;

/// AES-OCB3 with a 128-bit key, 96-bit nonce, and 128-bit tag.
pub type Aes128Ocb3 = AesOcb3<Aes128, U12>;
Copy link
Member

Choose a reason for hiding this comment

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

You don't feature gate these aliases properly. Personally, I would prefer for this crate to be cipher-agnostic, maybe later we could introduce aes-ocb3 as a helper combination crate.

@sgmenda
Copy link
Contributor Author

sgmenda commented Oct 8, 2023

Thank you for the detailed comments.

I'd like to push back gently against decoupling OCB3 from AES. This implementation of OCB3 assumes a 128-bit blockcipher, which limits its utility with other blockciphers.

@tarcieri
Copy link
Member

tarcieri commented Oct 8, 2023

@sgmenda you can express the block size requirement generically by bounding on the BlockCipher trait, e.g.

Cipher: BlockCipher<BlockSize = U16>

One of the advantages of making the underlying cipher generic is that it becomes possible to replace the AES implementation with e.g. a hardware cryptographic accelerator. Embedded platforms which use such accelerators can benefit greatly from OCB because it cuts the number of cipher invocations in half versus e.g. the more commonly used CCM mode.

@sgmenda
Copy link
Contributor Author

sgmenda commented Oct 8, 2023

@tarcieri Ah, TIL. I will make the change then.

@dignifiedquire
Copy link
Member

applied the missing CR in here: #587

@tarcieri
Copy link
Member

Merged in #587

Thanks @sgmenda! Sorry it took so long!

@tarcieri tarcieri closed this Mar 25, 2024
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.

None yet

4 participants