rand_os crate has no purpose now

[burdges]

If I understand, rand_core now depends upon getrandom for SeedableRng::from_entropy, which makes the rand_os crate pointless.

Ideally, I'd think OsRng should be merged into rand_core, rand_os should be deprecated, and the SeedableRng::from_entropy method removed.

Alternatively, you might remove both SeedableRng::from_entropy method and the rand_core dependency on getrandom, making rand_os the only crate that ever depends upon getrandom, so getrandom could be folded into rand_os, but nobody cares.

In all cases, there should be no crates anywhere besides the one containing OsRng that depend upon getrandom, but some crate called getrandom or rand_os separates out the OS specific code.

In all cases, RandCore v0.4 and v0.6? agree, so you can yank this whole RandCore v0.5 breakage mess.

[dhardy]

But why? from_entropy was added as a convenience method then promoted as the default way to get a fresh seed because it makes sense. (Except when you actually want a seed in byte form — then you can use getrandom directly.)

I agree, rand_os is basically useless. We left it because... there might be people who want it but don't want the whole of rand — at any rate, it shouldn't need much maintenance. But OsRng is copied into rand for a reason.

What's your goal? Some people are complaining about the semver breakage caused by our usage of the 0.3.1 → 0.4 shim (#852), but you're complaining about the lack of a shim. I don't think we can win here, but the fewer new versions of rand_core we need the better. And version 0.5 already counts for about 1/4 of downloads so yanking that now is a pretty drastic idea.

[newpavlov]

For example I plan to use OsRng in RustCrypto crates as a default entropy source and I don't want to bring the whole rand into dependency tree by default. And I don't think that rand_core will be a correct place for OsRng.

[burdges]

Yes, I'd want a shim so the merlin crate would work with rand_core 0.5, but not sure if that's even possible. I've crates now stuck at rand 0.6.? for the foreseeable future.

I actually think SeedableRng::from_entropy() should not be considered a breaking change, but..

I still think SeedableRng::from_entropy() appears poorly thought out. It'll never be used by numerical users, who want Rng not RngCore.

Also SeedableRng::from_entropy() should not be used for cryptography because it only makes auditing harder: You cannot discharge responsibility to the OS or to ThreadRng, which make real CryptoRng promises, but you must instead (a) consider if the PRNG actually suffices, and (b) reread the impl SeedableRng to check that it breaks nothing.

It'd appear less silly if fn from_entropy() -> ReseedingRng<Self, OsRng> maybe, so like maybe a methods on ReseedingRng or preferable a lone fn reseeding_rng<R: ReseedableRng>() -> ReseedingRng<Self, OsRng> in rand like thread_rng().

I think a lone fn in rand_os works fine for @newpavlov case, but a method makes auditing needlessly hard.

[vks]

I still think SeedableRng::from_entropy() appears poorly thought out. It'll never be used by numerical users, who want Rng not RngCore.

How are Rng and RngCore related to SeedableRng? They are independent traits. Also, I think all numerical users who want to initialize an RNG with random data want to use from_entropy.

[newpavlov]

I think a lone fn in rand_os works fine for @newpavlov case, but a method makes auditing needlessly hard.

Probably I wasn't explicit enough: I plan to use types generic over R: RngCore + CryptoRng, and "default" methods like new would return Self<OsRng>. So users will be able to easily replace OsRng with ThreadRng or something entirely different. Yes, those types could be generic over FnMut, but I believe such approach will be less ergonomic and harder to understand.

[burdges]

You cannot make that work because OsRng and ThreadRng must not be SeedableRng.

Above, I'm saying you're plans audit poorly even if you pass ChaChaRng, and call SeedableRng::from_entropy, because no individual crate can enforce the contract that SeedableRng::from_entropy() calls getrandom. ReseedingRng audits better and already exists.

It still appears SeedableRng::from_entropy() was poorly thought out and seemingly serves no useful purpose.

[dhardy]

You cannot make that work because OsRng and ThreadRng must not be SeedableRng.

Both ThreadRng and OsRng can be constructed via Default, or you can simply pass an instance (if on the same thread).

So your complaint is that now auditors must examine both the SeedableRng trait and the RNG implementing it? Isn't that necessary anyway?

[burdges]

Just to clarify: I do not claim from_entropy is super dangerous. I claim from_entropy is slightly dangerous and totally useless.

Both ThreadRng and OsRng can be constructed via Default

Any generic code must choose if it constructs an RngCore by Default::default or SeedableRng::from_entropy.

If you employ SeedableRng::from_entropy then you'll never make OsRng and ThreadRng work because they cannot satisfy SeedableRng. It follows that R:from_rng(OsRng::new()) achieves exactly the same thing in all cases, and makes from_entropy useless.

If you employ Default::default then you cannot pass any SeedableRng because they cannot satisfy Default with the same contract. I'd actually ban Default+RngCore myself, but that's another topic, and rustc lacks the negative reasoning required.

Isn't that necessary anyway?

Yes and no, from_entropy is easy to audit: git pull ../somerng && grep -r 'fn from_entropy' somerng must return nothing.

Yet, audits agree on what parts are covered and not covered beforehand, which saves considerable time and money. It's perfectly reasonable that stream cipher implementation should not be covered by an audit, so auditors would not necessarily do this test. And somerng can change in future without the caller knowing.

I think anything else in SeedableRng should reasonably be covered by test vectors, so they should behave mostly correctly, and hence auditors not looking. Yet, any from_entropy could do absolutely anything it likes with minimal chances that anyone notices.

ReseedingRng<R,Rsdr> does not have this problem. Also, some SeedOnce<R,Rsdr> or fn seed_once<R: RngCore+SeedableRng>() -> R do not have this problem either, although imho we should favor ReseedingRng by not implementing such options.

There is a case along the lines @newpavlov argued to move ReseedingRng into rand_os, but not sure if that actually works

[dhardy]

Any generic code must choose if it constructs an RngCore by Default::default or SeedableRng::from_entropy.

But of course. This is the case anyway unless you propose that OsRng should implement SeedableRng just so that generic code can do OsRng::from_rng(other_rng), but I'm not sure that's useful? Code generic over the RNG it uses should also allow use of an existing RNG, which usually means accepting that by value or &mut (or could mean using a lambda to get the RNG).

Actually, just about the whole purpose of from_entropy now is because from_rng(OsRng).unwrap() is longer, so in that sense you are right.

Yet, any from_entropy could do absolutely anything it likes with minimal chances that anyone notices.

This is a good point. In Rand 0.6 this wasn't a problem because it was part of an auto-implemented extension trait which could not be manually implemented. We are now allowing PRNGs to implement a non-deterministic function in any way they like.

There is still the point that a PRNG could implement any logic it likes in from_rng: it should be deterministic but it's completely up to the PRNG implementation how it tests this since there isn't a specification for what it should do. Doesn't this mean an audit of the PRNG is still required? (In this case unique implementations are sometimes useful, e.g. what XorShiftRng does.)

I'd actually ban Default+RngCore myself

Of course seedable PRNGs should not do this, but for OsRng and ThreadRng it's harmless.

---

Personally I am in favour of moving OsRng into rand_core since it is now only a small shim and rand_core must depend on getrandom anyway (error type compatibility), but we did already have a discussion about that (somewhere), with arguments against (I believe mostly minimalism + unclear crate naming — both weak arguments).

[vks]

Personally I am in favour of moving OsRng into rand_core since it is now only a small shim and rand_core must depend on getrandom anyway (error type compatibility), but we did already have a discussion about that (somewhere), with arguments against (I believe mostly minimalism + unclear crate naming — both weak arguments).

I agree that it makes sense to move OsRng to rand_core and deprecate rand_os. I think the "unclear crate naming" argument is weak, because from_entropy is already providing very similar functionality. Regarding the minimalism argument: OsRng would still depend on the getrandom flag, so people who don't like it can easily disable it.

[dhardy]

That we can do with minimal chance of breakage (including a new release of rand_os which is deprecated). However, I think we should wait a while for further comment before committing ourselves.

The follow-up question is whether we should deprecate from_entropy (which would affect many of our benchmarks and I think many users). @burdges provides a reasonable rationale for doing so, but it must be weighed up against convenience (small bonus) and breakage (it seems quite a few users have been annoyed by breaking changes).

[newpavlov]

Well, we can mark from_entropy as deprecated and remove it only in rand_core v0.6. But I disagree with the argument that it's an audit hazard, as @dhardy said earlier SeedableRng implementation can do anything it likes, including returning zeros instead of the random data in addition to eating your laundry, so full review of the PRNG implementation is still required and unreasonable re-implementation of from_entropy will be a noticeable red flag.

I guess we could deprecate from_entropy in favor of from_rng(OsRng), but from_rng(OsRng).unwrap() is not very convenient, so I don't think we should do it in the current rand_core version. We could change it in rand_core v0.6 after introduction of try_from_rng method, but I still think that from_entropy is a good convenience method.

Regarding moving OsRng into rand_core, personally I don't like it from aesthetic PoV, but I will not be against doing so (I guess it will also help with the duplication of OsRng in rand).

[dhardy]

Yes, de-duplication is one of my rationales.

We could add a function like follows instead of from_entropy, but personally I'd rather just keep from_entropy:

fn make_rng<R: SeedableRng, S: Default+RngCore = OsRng>() {
    R::from_rng(S).unwrap()
}

[burdges]

There is still the point that a PRNG could implement any logic it likes in from_rng

In principle from_rng could also have a test vector, unlike from_entropy. Yet, from_rng would not often become subject to other crate's test vectors, unlike from_seed, so yes.

Personally I am in favour of moving OsRng into rand_core since it is now only a small shim
and rand_core must depend on getrandom anyway (error type compatibility)

Ahh, I now understand, thank you !!! :)

In my mind, we were keeping ReseedingRng out of rand_core mostly to satisfy cryptography folk who dislike anything except the OS's CSPRNG. And from_entropy is worse than ReseedingRng, so they'd dislike it too. Yet..

It's possible ReseedingRng exists outside rand_core for another reason however?

I'm not doing the cryptography folk's prejudice justice: It's largely the complexity of ReseedingRng they dislike, even though this complexity is purely type-level.

I'm therefore partly wrong about ReseedingRng being better in that it'll create unnecessary contention. I do think from_entropy would cause contention too, but only similar not more.

I still think from_entropy is niche enough that a free standing fn would've make more sense:

#[inline(always)]
make_rng<R: RngCore+SeedableRng>() -> R {
    R::from_rng(OsRng::new()).unwrap()
}

Yet, I'm no longer confident this warrants the breakage. :)