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

Move thread RNG into a separate crate #1520

Closed
wants to merge 1 commit into from
Closed

Move thread RNG into a separate crate #1520

wants to merge 1 commit into from

Conversation

newpavlov
Copy link
Member

@newpavlov newpavlov commented Oct 28, 2024

This was previously discussed in #716 and recently raised in #1514.

The main disadvantage of moving thread RNG into its own crates is that users of rand (with default crate features) will have one additional dependency in their dependency tree. On the other hand, ThreadRng can be now used without the rest of rand (e.g. in cryptographic applications). It also results in simpler configuration flags and thread RNG code, since we don't need to deal with the generality of ReseedingRng.

If ReseedingRng is not used in practice (cursory search on GitHub shows mostly vendored rand code), we probably can remove it completely or move into a separate crate.

@newpavlov newpavlov requested a review from dhardy October 28, 2024 18:23
Copy link
Member

@dhardy dhardy left a comment

Choose a reason for hiding this comment

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

Do we want this? I'm still not convinced.

If we do:

@newpavlov
Copy link
Member Author

Probably should be rand_thread_rng

How about rand_trng?

@newpavlov newpavlov marked this pull request as ready for review October 30, 2024 11:50
@newpavlov
Copy link
Member Author

This PR is mostly ready for review. I will work on docs and readme after it's decided that we will move forward with it.

Do we want this?

I do. :) My main motivation is to be able to use and recommend rand's ThreadRng for uses in cryptographic applications.

@dhardy
Copy link
Member

dhardy commented Oct 30, 2024

My main motivation is to be able to use and recommend rand's ThreadRng for uses in cryptographic applications.

You can already. And if you're talking about the disclaimer, it applies to this just as much as it does to rand.

@newpavlov
Copy link
Member Author

I can not fully recommend it now because it pulls the whole rand, which is not a small dependency. Everything else except ThreadRng does not have any uses in this context as was mentioned in #1514.

@dhardy
Copy link
Member

dhardy commented Nov 8, 2024

@dhardy
Copy link
Member

dhardy commented Nov 12, 2024

@newpavlov you were the only proponent in #716 while several people were against, and I haven't seen any change there.

@dhardy dhardy closed this Nov 12, 2024
@newpavlov
Copy link
Member Author

@vks @tarcieri
WDYT?

@tarcieri
Copy link

Having ThreadRng in the rand crate and having it marked as CryptoRng seems somewhat at odds with #1514.

I guess the actual wording in the changes in #1514 is perhaps not as strong as "rand is not a crypto library":

Rand is not: [...] A cryptography library. Rand provides functionality for generating unpredictable random data (potentially applicable depending on requirements) but does not provide high-level cryptography functionality.

I would argue a CSRNG is high-level cryptographic functionality. It is not uncommon for cryptography libraries to suggest the use of thread_rng in end user-facing documentation for the purpose of creating cryptographic secrets including keys.

@dhardy
Copy link
Member

dhardy commented Nov 13, 2024

My POV is that #1514 is mostly a disclaimer: rand is a community project which cannot provide binding guarantees. Meanwhile, rand has basically always tried to provide a fast and unpredictable randomness source, though IIUC the original motivation had less emphasis on absolute security and more on avoiding a DoS attack through hash keys.

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