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

xaes: initial implementation #612

Closed
wants to merge 4 commits into from

Conversation

SergioBenitez
Copy link
Contributor

This is an initial implementation of XAES-256-GCM (re: #1) which passes the test vectors.

Would love a review, especially as it pertains to constant-time and zeroing (why isn't zeroize used to zero IVs?). I don't see an obvious constant-time byte-slice XOR in use elsewhere in Rust-Crypto, but please point to a canonical reference if possible. I also have not placed this behind any feature flags, yet. Finally, the primary structure XaesGcm256 is not parameterized in any way. If it's desirable to parameterize it in a similar fashion to AesGcm, please let me know.

@SergioBenitez SergioBenitez force-pushed the xaes branch 3 times, most recently from 90897dd to 8c08e80 Compare June 29, 2024 05:41
aes-gcm/src/xaes.rs Outdated Show resolved Hide resolved
aes-gcm/src/lib.rs Outdated Show resolved Hide resolved
use common::TestVector;
use hex_literal::hex;

/// C2SP XAES-256-GCM test vectors
Copy link

@pinkforest pinkforest Jun 29, 2024

Choose a reason for hiding this comment

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

sidebar - would love if C2SP test vectors would be embedded in crate a-like of wycheproof-rs so test vectors can be updated independently to all associated impl's and no messing updating multiple places

aes-gcm/src/xaes.rs Outdated Show resolved Hide resolved
aes-gcm/src/xaes.rs Outdated Show resolved Hide resolved
@pinkforest
Copy link

gave you few nits - take em if you like ☺️

if you feel like it and if worried about non-ct anywhere:

aes-gcm/src/xaes.rs Outdated Show resolved Hide resolved
aes-gcm/src/xaes.rs Outdated Show resolved Hide resolved
aes-gcm/src/xaes.rs Outdated Show resolved Hide resolved
@SergioBenitez
Copy link
Contributor Author

@newpavlov @tarcieri Any chance for a review here?

@tarcieri
Copy link
Member

tarcieri commented Jul 8, 2024

@SergioBenitez sorry, I've been on vacation. I'll look at this soon.

@SergioBenitez
Copy link
Contributor Author

Checking in. Any chance to push this forward?

@tarcieri
Copy link
Member

@SergioBenitez haven't had a whole lot of free time lately for code review but I still hope to review it soon

@tarcieri
Copy link
Member

tarcieri commented Aug 1, 2024

Sorry for the belated review.

On #1 we had discussed an xaes-gcm crate, but I now see there is already an xaes-gcm crate which implements the Derive-Key-AES-GCM construction, and I have not carefully looked at the differences between that and this construction.

I am a bit wary including the construction in the aes-gcm crate itself, which otherwise implements NIST standard constructions.

However, I'd also note the construction in the spec is called XAES-256-GCM, so how about an xaes-256-gcm crate instead, which is currently available?

@SergioBenitez SergioBenitez force-pushed the xaes branch 7 times, most recently from 5908f6a to d3141c6 Compare August 9, 2024 10:47
@SergioBenitez
Copy link
Contributor Author

Sure! Went ahead and published a -pre version to reserve the name. I also added you as an owner; feel free to use that as you wish. Looking forward to a review of the crate.

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.

Sorry for the long delay in review! This looks good now.

@tarcieri
Copy link
Member

@SergioBenitez attempted to do a Cargo.lock merge but it seems it didn't work. Can you take a look?

@tarcieri
Copy link
Member

Merged in #642

@tarcieri tarcieri closed this Oct 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.

3 participants