search

dhardy / rand

http://doc.rust-lang.org/rand
Other
2 stars 2 forks source link

Make from_rng require CryptoRng #78

Closed dhardy closed 6 years ago

dhardy commented 6 years ago

This is a concept implementation with issues; see the FIXMEs.

I'm not convinced that we should do this since it's more restrictive than required to achieve the desired effects (prevent accidental clone of XorShiftRng and prevent seeding a strong PRNG from a weak PRNG).

The sham implementation for ThreadRng shows how easy it is to mis-use CryptoRng; I wonder if we should just rename it StrongRng or NonTrivialRng and let ISAAC implement it.

pitdicker commented 6 years ago

My thoughts about requiring CryptoRng for from_rng have now changed three times.

My first thought was no, because it is an unneccesary heavy requirement in some cases. From https://github.com/dhardy/rand/issues/18#issuecomment-345293603:

For RNG's that are itself CryptoRng it should. For simple PRNG's something that is statistically strong is already good enough.

On second thought yes, because I don't see all that many situations where you would choose to seed from a simple RNG.

It prevents some legal uses of seeding from a weaker RNG, like splitting streams. But then it is still possible call from_seed directly, and an indication you are doing something to think twice about. For cryptographic RNG's it prevents a small chance of misuse.

Now I am not sure anymore.

We have a couple of CryptoRng-quality sources: OsRng, with JitterRng as fallback in NewSeeded. And I suppose soon thread_rng. Then seeding from a good source can be ergonomic and/or fast. So requiring CryptoRng should hinder almost no-one. But if the natural candidates for initializing an RNG already are CryptoRng, is there much use in enforcing it?

But do we really expect misuse? I do not really consider the Xorshift seeding Xorshift problem here, that should not be interesting anymore when we have better small RNG's. Do we expect someone to seed something like ChaCha with PCG? Sounds like a solution looking for a problem...

I can't call requiring CryptoRng a good or a bad thing now. I see little benefit now, but also little negative consequences.

pitdicker commented 6 years ago

Some of the changes here are good even if we don't want to require CryptoRng

dhardy commented 6 years ago

Basically, agreed. There's not much for or against, but there are a couple of annoying things making it harder to merge this. So maybe I'll just drop it.

I did once see a colleague creating an XorShiftRng from another and had to point out the clone. But if the default fast RNG is something else, it's not really a problem.

And yes, there are a couple of lines that should have been committed before (impl CryptoRng for &mut CryptoRng).