Adapt to chacha20 #1642
Adapt to chacha20 #1642
Conversation
| # 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" } |
There was a problem hiding this comment.
This is going to make it impossible to make releases.
Ugh. I think there are only three options here:
- Do this, but remove the patch when publishing. Undesirable because then the source does not correspond to the published versions.
- Move
chacha20source into this repo (likely undesirable; CC @tarcieri) - Remove
rand_corefrom the workspace. This will require using a patch similar to this for development wheneverrand_corehas breaking changes, but at least allows clean compiling otherwise.
This is why rand_chacha is co-located in this repo with external dependencies.
There was a problem hiding this comment.
I think crate releases ignore these directives and try to build directly against crates.io?
There was a problem hiding this comment.
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.
Does it? I wish their documentation would clarify.
cargo publish --dry-run looks okay, so this might work.
There was a problem hiding this comment.
But even assuming this does work, how are we supposed to handle breaking changes to rand_core in the future? If we ever make any changes incompatible with chacha20 our local builds will simply not work until upstream chacha20 gets fixed.
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 rand_core in the future.)
There was a problem hiding this comment.
@hpenne I think the
patch.crates-ioapproach should be fine. We use it frequently in @RustCrypto repos where we have external interdependencies
@tarcieri @newpavlov do you have a policy on breaking changes to affected crates (e.g. digest, signature)? Do you just simultaneously commit a fix to other affected repos and pin the version? Or do you never really get breaking changes which affect the build in practice?
There was a problem hiding this comment.
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.
But even assuming this does work, how are we supposed to handle breaking changes to
rand_corein the future? If we ever make any changes incompatible withchacha20our local builds will simply not work until upstreamchacha20gets fixed.
@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.
@hpenne publishing a new version of rand_core only requires that the rand_core build succeeds, not the whole workspace, so breaking changes don't cause a deadlock, no.
I guess the patch solution is tolerable. If you do try implementing the newtype/adapter workaround, please push as a new PR.
There was a problem hiding this comment.
@hpenne publishing a new version of
rand_coreonly requires that therand_corebuild succeeds, not the whole workspace, so breaking changes don't cause a deadlock, no.
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.
I guess the
patchsolution is tolerable. If you do try implementing the newtype/adapter workaround, please push as a new PR.
Noted. It probably won't be needed as long as the situation can be resolved by publishing any new breaking version rand_core separately.
|
I opened #1643. This PR is useful as a draft but won't be merged in its current form (likely we'll want the MSRV/edition bump first as its own PR). I'm not certain on the timeline yet; the main blocker is the |
CHANGELOG.mdentrySummary
Replaced the dependency on
rand_chachawith one onchacha20. Added some tests tostd.rsto ensure that the output ofStdRngdid not change.Fixes #934.
Motivation
Reduces total code size and the total amount of unsafe code.
Details
Changes to config.toml and some replacement of
rand_chacha::withchacha20::.Added three new unit tests to std.rs. These were based on tests of IETF test vectors from
rand_chacha, but the actualexpectedvalues had to be replaced, as the IETF test vectors are for ChaCha20 whileranduses ChaCha12. Theexpectedvalues were generated by usingrand_chacha(beforechacha20was used) to verify that the algorithm change did not affect the output.