-
-
Notifications
You must be signed in to change notification settings - Fork 473
Adapt to chacha20 #1642
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
base: master
Are you sure you want to change the base?
Adapt to chacha20 #1642
Changes from all commits
20e7ec9
92e094d
3adf5e6
e3c1701
8dc528a
09befe3
296ae2b
0fbb567
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,7 +14,7 @@ keywords = ["random", "rng"] | |
categories = ["algorithms", "no-std"] | ||
autobenches = true | ||
edition = "2021" | ||
rust-version = "1.63" | ||
rust-version = "1.85" | ||
include = ["src/", "LICENSE-*", "README.md", "CHANGELOG.md", "COPYRIGHT"] | ||
|
||
[package.metadata.docs.rs] | ||
|
@@ -34,7 +34,7 @@ serde = ["dep:serde", "rand_core/serde"] | |
|
||
# Option (enabled by default): without "std" rand uses libcore; this option | ||
# enables functionality expected to be available on a standard platform. | ||
std = ["rand_core/std", "rand_chacha?/std", "alloc"] | ||
std = ["rand_core/std", "alloc"] | ||
|
||
# Option: "alloc" enables support for Vec and Box when not using "std" | ||
alloc = [] | ||
|
@@ -46,7 +46,7 @@ os_rng = ["rand_core/os_rng"] | |
simd_support = [] | ||
|
||
# Option (enabled by default): enable StdRng | ||
std_rng = ["dep:rand_chacha"] | ||
std_rng = ["dep:chacha20"] | ||
|
||
# Option: enable SmallRng | ||
small_rng = [] | ||
|
@@ -70,11 +70,16 @@ members = [ | |
] | ||
exclude = ["benches", "distr_test"] | ||
|
||
# Force chacha20 to use the local rand_code to avoid compiling two different "rand_core"s. | ||
# This is necessary even if the specified versions are the same. | ||
[patch.crates-io] | ||
rand_core = { path = "rand_core" } | ||
Comment on lines
+73
to
+76
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is going to make it impossible to make releases. Ugh. I think there are only three options here:
This is why There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think crate releases ignore these directives and try to build directly against crates.io? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There could be a 4th option: Depend on chacha20 but without the "rng" feature, then implement the required traits for chacha inside the rand crate instead. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does it? I wish their documentation would clarify.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But even assuming this does work, how are we supposed to handle breaking changes to I don't think that's viable. (And the only reason it's not definitely unviable is that I don't expect much in the way of breaking changes to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@tarcieri @newpavlov do you have a policy on breaking changes to affected crates (e.g. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, often we'll have a PR to the other repo ready to go. That said we'll hopefully be wrapping up breaking changes soon and be stable for awhile. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@dhardy Isn't that a deadlock situation? With the "patch"-solution, if you make changes to rand_core so that local builds fail (due to chacha being forced to use the newer version that it is not compatible with), then you cannot publish a new version of rand_core, which means that chacha20 cannot be updated to support the new rand_core version either, because there is no such version published. Can that deadlock be broken in any acceptable way? If I implement the newtype/adapter workaround above, then we can use the patch solution 99% of the time to ensure that rand_core is only built once, but if you get into this potential deadlock situation then you can remove the patch temporarily to resolve it (everything will work, but you get rand_core built twice) and put it back again when both rand_core and chacha20 has been updated. You will get to have your cake and eat it too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @hpenne publishing a new version of I guess the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Thanks for clearing that up for me. I suppose that the problem of rand itself potentially not building in that situation due to breaking changes in rand_core can be resolved by modifying rand to fetch rand_core from crates.io for a while (using the previous release of rand_core), until chacha20 is updated to use the new rand_core and everything is in sync again so that it all builds with the local current rand_core from the rand git repo. Or perhaps there are even better ways, these kinds of sort-of-circular dependencies are a little new to me.
Noted. It probably won't be needed as long as the situation can be resolved by publishing any new breaking version rand_core separately. |
||
|
||
[dependencies] | ||
rand_core = { path = "rand_core", version = "0.9.0", default-features = false } | ||
log = { version = "0.4.4", optional = true } | ||
serde = { version = "1.0.103", features = ["derive"], optional = true } | ||
rand_chacha = { path = "rand_chacha", version = "0.9.0", default-features = false, optional = true } | ||
chacha20 = { version = "=0.10.0-rc.0", default-features = false, features = ["rng"], optional = true } | ||
|
||
[dev-dependencies] | ||
rand_pcg = { path = "rand_pcg", version = "0.9.0" } | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some equivalence proptests which (in addition to the existing unit tests) ensure the two implementations generate equivalent outputs: RustCrypto/stream-ciphers#435
However, in doing so I did notice
rand_chacha
uses the legacy 64-bit nonce forset_stream
, which currently isn't supported bychacha20
. However, it's already a generic parameterStreamId
which supports a variety of types, so I think it should be fairly straightforward to make it backwards compatible withu64
too.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tarcieri there's a bigger problem:
chacha20
RNGs only have a period of 2**36 words.https://rust-random.github.io/book/guide-rngs.html#period
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current implementation in
chacha20
has a data volume limit of approximately 256GB, constrained by the use of a 32-bit counter. There is a PR to add 64-bit counter support: RustCrypto/stream-ciphers#399That would also be a notable difference from
rand_chacha
, which supports a 64-bit counter, and something we can potentially address before a release.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to see some form of longer counters before release, yes.
This could also be viewed as a difference between the requirements of a cipher and of a PRNG: one has a testable data length, the other does not.