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

Generic CTR #195

Merged
merged 7 commits into from Dec 4, 2020
Merged

Generic CTR #195

merged 7 commits into from Dec 4, 2020

Conversation

newpavlov
Copy link
Member

@newpavlov newpavlov commented Dec 2, 2020

An attempt to define CTR mode generically. The approach is quite flexible and I think it can be even used with the __m128i type (i.e. we will be able to reduce amount of copied code in the aes crate).

But I get a weird compilation error when I try to define implementations of generate_block and load methods. Even though the default implementation works without problems, when I copy it to the concrete type (Ctr128BE) impl, I get an error which asks me to further restrict N with Div<U16> bound, even though it's already restricted by Div<Self::Size>. Even copying the suggested bound does not resolve the issue, compiler suggests to add already existing bound. Looks like a bug in the compiler to me.

UPD: The implementation got restricted back to 128-bit ciphers, probably until landing of const generics.

ctr/src/ctr32.rs Outdated Show resolved Hide resolved
Comment on lines -99 to -89
pub fn seek_ctr(&mut self, pos: u32) {
self.ctr.seek(pos);
}

/// Get the current NIST SP800-38D counter value.
// TODO(tarcieri): implement `SyncStreamCipherSeek`
#[inline]
pub fn current_ctr(&self) -> u32 {
self.ctr.current_pos()
}
Copy link
Member

Choose a reason for hiding this comment

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

aes-gcm (the crate) presently relies on these APIs, although they could probably be replaced with SyncStreamCipherSeek

Copy link
Member Author

@newpavlov newpavlov Dec 2, 2020

Choose a reason for hiding this comment

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

If it's block position, then it can be simply computed by dividing byte position by block size. Though we probably can extend the seek trait with a method which would return this number right away. I also thought about exposing the block nature of stream ciphers, but I haven't found a good AP for it which would fit different backends and runtime detection for ChaCha.

Copy link
Member

Choose a reason for hiding this comment

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

I'd previously opened RustCrypto/traits#336 to discuss this

@newpavlov
Copy link
Member Author

I am not sure how to work around the compiler error issue. Making the CtrFlavor trait generic over block size instead of the method results in a similar error. I guess we can continue to support only 128-bit ciphers and wait for const generics to land, which it looks like do not have this issue.

ctr/src/lib.rs Outdated Show resolved Hide resolved
@newpavlov newpavlov marked this pull request as ready for review December 4, 2020 09:04
Copy link
Member

@tarcieri tarcieri left a comment

Choose a reason for hiding this comment

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

Nice! A little curious about the impact on performance, but otherwise looks fantastic.

@newpavlov
Copy link
Member Author

Well, I took all measures I can think of to allow compiler to properly optimize the code, but we can't know for sure without properly measuring it.

@newpavlov newpavlov merged commit 7ddcab1 into master Dec 4, 2020
@newpavlov newpavlov deleted the ctr_rework branch December 4, 2020 13:33
This was referenced Apr 29, 2021
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

2 participants