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. :)

[burdges]

As an aside, there are also almost perfectly deterministic computing environments, like the "execute block functions" in most blockchains, which afaik ban randomness, floating point arithmetic, etc. I suppose getrandom might exist but panic in such environments?

[newpavlov]

I suppose getrandom might exist but panic in such environments?

Right now getrandom uses a dummy implementation which always returns error on unsupported targets. So from_entropy would always panic in such environments. There is a proposal to use a compile-time error on unsupported platforms, but we have not decided what to do yet.

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

We don't have ReseedingRng in rand_core. Did you mean SeedableRng? We had it from the earliest versions of rand_core, so I don't see why people would complain about it. Previously we had rand::FromEntropy, which got superseded by a new feature-gated SeedableRng::from_entropy method, which is really simple, so again I don't see why would it cause any issues. Almost all of the complexity is handled by getrandom, which can be removed by disabling getrandom feature.

@dhardy

rand_core must depend on getrandom anyway (error type compatibility)

Right now getrandom is an optional dependency of rand_core. So we have to re-define error type in rand_core either way. Plus rand_core::Error is more complex compared to the getrandom::Error. So I am not sure if it's correct to use "must" here.

[burdges]

Right now getrandom is an optional dependency of rand_core

Oh? Are all the concerns about features being additive addressed?

[newpavlov]

AFAIK, yes. getrandom feature only enables SeedableRng::from_entropy method and From<getrandom::Error> for Error impl.

[dhardy]

@burdges rand_core has been since its creation a "minimal API crate"... yet it seems to be growing. If we were to add ReseedingRng there the next question would be ThreadRng... no thank you (to both). (Arguably all the block stuff doesn't belong in rand_core either, but in that case the only option would be yet another crate.)

@newpavlov impl From<getrandom::Error> for rand_core::Error is very much worth having IMO, and can't live anywhere else.

[burdges]

I'd love some clarification on two points:

Should getrandom/OsRng work on no_std targets? If so, how should one learn if OsRng exists? Is there some trick for doing roughly this?

#[cfg(exists = rand_os::OsRng)]

We need the error type from getrandom but if getrandom cannot work then rand_core could break the build with an informative error message whenever someone sets some special feature like rand_os_required or whatever?

Should crates support both ThreadRng and OsRng? If so, how?

I tried doing this by copying the features from https://docs.rs/crate/rand/0.6.5/source/Cargo.toml in
w3f/schnorrkel@2d4c2ef but that sounds pretty iffy and fragile.

[newpavlov]

Should getrandom/OsRng work on no_std targets? If so, how should one learn if OsRng exists?

Not on all, but on some yes (e.g. SGX). If getrandom supports the given target, then OsRng will work as well. After rust-random/getrandom#71 lands users will get a compilation error on unsupported targets. In application they can replace getrandom with a custom crate supporting their target.

I think you ask a wrong question. If application or library requires system entropy it has to use getrandom. You may feature-gate some functionality as we do in rand_core (and library crates may have to cascade this feature), but IIRC right now we don't have better tools to make it more convenient or less fragile.

Should crates support both ThreadRng and OsRng? If so, how?

I wouldn't say they "should", but they certainly can by being generic over RngCore + CryptoRng (and depending on situation maybe also Default).

[burdges]

Should getrandom/OsRng work on no_std targets? If so, how should one learn if OsRng exists?

Not on all, but on some yes (e.g. SGX). If getrandom supports the given target, then OsRng will work as well.

I read https://github.com/rust-random/rand/blob/master/rand_os/Cargo.toml#L25 as activating std for rand_core whenever rand_os gets used, maybe causing w3f/schnorrkel#31 (comment) And rand_os never says default_features = false

After rust-random/getrandom#71 lands users will get a compilation error on unsupported targets.

Cute trick

Should crates support both ThreadRng and OsRng? If so, how?

I wouldn't say they "should", but they certainly can by being generic over RngCore + CryptoRng (and depending on situation maybe also Default).

Yes, crates should be "generic" over RngCore + CryptoRng for various reasons, like test vectors, but..

Any "user facing" crates should provide safe defaults without the users specifying anything.

I'm asking if such crates should operate with ThreadRng when linked with rand but use OnRng but when not linked with rand. I attempted this with https://github.com/w3f/schnorrkel/blob/master/src/lib.rs#L232 and copying rand's feature delegations in https://github.com/w3f/schnorrkel/blob/master/Cargo.toml#L96-L97

[newpavlov]

I read https://github.com/rust-random/rand/blob/master/rand_os/Cargo.toml#L25 as activating std for rand_core whenever rand_os gets used

Oh, good catch! This is indeed an oversight. rand_os should only activate getrandom feature (for converting getrandom::Error to rand_core::Error). I think it's a leftover from times when getrandom was mostly an std crate. It will be fixed in #859.

Any "user facing" crates should provide safe defaults without the users specifying anything.

You can write code like this:

// `thread_rng` here is a crate feature
#[cfg(not(thread_rng))]
type DefaultRng = OsRng;
#[cfg(thread_rng)]
type DefaultRng = ThreadRng;

struct Foo<R: CoreRng + CryptoRng=DefaultRng> { .. }

Unfortunately defaults for type parameters don't work with functions, see rust-lang/rust#36887.

BTW cases like this were one of the reasons why I advocated that we need a stand-alone crate for ThreadRng.

[burdges]

Yes struct Foo<R: Copy=DefaultRng> { .. } helps. I think the thread_rng feature gets messy, but not sure if my rand and rand_os features improve upon that.

Are there build.rs tricks with which DefaultRng could be defined by rand_os/rand_core only when rand does not exist?

[newpavlov]

Are there build.rs tricks with which DefaultRng could be defined by rand_os/rand_core only when rand does not exist?

AFAIK no. It's essentially boils down to a problem of detecting if current build uses std or not. So std feature is our main tool right now. I guess we could add a DefaultRng alias to rand which will handle this switching depending on std feature (although personally I probably would not use it, since even with a disabled std feature rand is not a small dependency).

[burdges]

Cool. We've two approaches for inclusion code from lib.rs then, either

use rand_core::{RngCore,CryptoRng};

#[cfg(all(feature = "rand_os", feature = "rand"))] 
fn default_rng() -> impl RngCore+CryptoRng { ::rand::thread_rng()  }

#[cfg(all(feature = "rand_os", not(feature = "rand")))] 
fn default_rng() -> impl RngCore+CryptoRng {  ::rand_os::OsRng::new().unwrap(). }

#[cfg(not(feature = "rand_os"))]
fn default_rng() -> impl RngCore+CryptoRng {
    const PRM : &'static str = "Attempted to use functionality that requires system randomness!!";

    struct PanicRng;
    impl ::rand_core::RngCore for PanicRng {
        fn next_u32(&mut self) -> u32 {  panic!(&PRM)  }
        fn next_u64(&mut self) -> u64 {  panic!(&PRM)  }
        fn fill_bytes(&mut self, _dest: &mut [u8]) {  panic!(&PRM)  }
        fn try_fill_bytes(&mut self, _dest: &mut [u8]) -> Result<(), ::rand_core::Error> {  panic!(&PRM)  }
    }
    impl ::rand_core::CryptoRng for PanicRng {}

    PanicRng
}

or else

#[cfg(not(rand))]
type DefaultRng = ::rand_os::OsRng;
#[cfg(rand)]
type DefaultRng = ::rand::ThreadRng;

which you invoke via DefaultRng::default().

[dhardy]

I don't think this is the place to discuss your library's API. I personally have no idea what your requirements are, and it sounds like you also have little idea (e.g. whether it should compile with neither rand_os nor rand, or whether performance of DefaultRng for repeated calls is important).

---

I think the only action item from this issue is the question of moving OsRng into rand_core?

[burdges]

Yes, sorry for the derail. :)

[burdges]

We can close this issue in favor of further discussion on #863 since the issues got kinda confused here mostly due to my miss understandings. I've made comments elsewhere that reopenned and/or summarized any remaining discussion from here.