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

Implement xchacha reduced-round variants #355

Merged
merged 2 commits into from Aug 24, 2021
Merged

Implement xchacha reduced-round variants #355

merged 2 commits into from Aug 24, 2021

Conversation

lain-dono
Copy link
Contributor

XChaCha8Poly1305 and XChaCha12Poly1305 via ChaChaPoly1305 generalisation.

Comment on lines +36 to +40
xchacha20 = ["chacha20/xchacha"]
xchacha20poly1305 = ["xchacha20"] # alias
reduced-round = ["chacha20-reduced-round", "xchacha20-reduced-round"]
chacha20-reduced-round = ["chacha20"]
xchacha20-reduced-round = ["xchacha20"]
Copy link
Contributor Author

@lain-dono lain-dono Aug 24, 2021

Choose a reason for hiding this comment

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

I left xchacha20poly1305. You may want to remove this in the future.


/// Generic ChaCha+Poly1305 Authenticated Encryption with Additional Data (AEAD) construction.
///
/// See the [toplevel documentation](index.html) for a usage example.
pub struct ChaChaPoly1305<C>
pub struct ChaChaPoly1305<C, N: ArrayLength<u8> = U12>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default option is used here, so this has no effect on compatibility.

stream = ["aead/stream"]
xchacha20poly1305 = ["chacha20/xchacha"]
xchacha20 = ["chacha20/xchacha"]
Copy link
Member

@tarcieri tarcieri Aug 24, 2021

Choose a reason for hiding this comment

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

I feel a bit of explanation is needed as to why there's a chacha20 feature in the first place, as I feel there doesn't need to be an xchacha20 feature.

This feature activates the chacha20 crate.

The reason why it's optional is because the c2-chacha crate provided a faster implementation and also impl'd the RustCrypto traits, so people who really cared about performance could swap it in instead, and disable the chacha20 crate.

With RustCrypto/stream-ciphers#261 the gap has narrowed considerably (ty @str4d).

I think in the next breaking release we can make chacha20 a mandatory dependency again, which would eliminate the need for this feature.

Anyway this is all a long winded way of saying it feels like there are too many features here and I take responsibility for at least part of that. It seems like xchacha20poly1305 and xchacha20-reduced-round could both activate chacha20/xchacha.

Longer term perhaps it would also make sense for simplicity's sake to get rid of chacha20/xchacha and always include the XChaCha family as the code size really isn't that big.

@tarcieri
Copy link
Member

Overall this looks great, although it seems the tests are failing due to unused import warnings under --no-default-features.

I also left one nit about a (cargo) "feature explosion" and am thinking it'd be good to simplify the overall feature set.

@lain-dono
Copy link
Contributor Author

I have tried to leave all changes within the PATCH change only. Not breaking change was the goal.

It also makes the documentation more beautiful. One of the rust features that we rarely notice, but love.

@lain-dono
Copy link
Contributor Author

It might make sense to remove the reduced-round. In that case, only chacha20 and xchacha20 will remain.
Of course, the aliases must remain in cargo.toml. Otherwise a new minor version is required.

@tarcieri
Copy link
Member

Ok, let's start with this, and I can circle back on slimming down features when I can make breaking changes to the chacha20 crate in the next non-patch release.

@codecov-commenter
Copy link

Codecov Report

Merging #355 (48a41bb) into master (9f8a010) will increase coverage by 0.14%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #355      +/-   ##
==========================================
+ Coverage   89.05%   89.19%   +0.14%     
==========================================
  Files          40       39       -1     
  Lines        1909     1897      -12     
==========================================
- Hits         1700     1692       -8     
+ Misses        209      205       -4     
Impacted Files Coverage Δ
chacha20poly1305/src/lib.rs 76.92% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a287c3b...48a41bb. Read the comment docs.

@tarcieri tarcieri merged commit 5983bc0 into RustCrypto:master Aug 24, 2021
@tarcieri
Copy link
Member

Thank you!

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

3 participants