RngCore + CryptoRng + Default semantics

[burdges]

Should we express cryptographically secure random number generators at the type level? I n particular, should we say CryptoRng types should not implement Default unless their default is cryptographically secure system randomness?

In practice this means types like ChaChaRng should not implement Default, already the case, but maybe not the case for other's code.

pub struct SecretKey<R: RngCore + CryptoRng + Default> {
    scalar: [u8; 32],
    public: PublicKey,
    _rng: PhandomData<R>,
}

impl<R: RngCore + CryptoRng + Default> SecretKey<R> {
    pub fn sign(&self) -> Signaure {
        let mut nonce = [0u8; 32];
        R::default().fill_bytes(&mut nonce);
    }
}

This code would not be good practice as written anyways of course.

[newpavlov]

Yes, I think it makes sense that CryptoRng + Default implies either seeding or direct reading from a "true" entropy source. We even could imagine a default impl for Default for SeedableRng in rand_core when getrandom feature is enabled.

But I think a better approach will be to discourage implementation of Default for PRNG implementations in the first place because of its ambiguity.

[burdges]

I'd leave SeedableRng alone, but CryptoRng could contain this text:

Avoid implementing both CryptoRng and Default unless Default::default() returns a secure PRNG, typically meaning good system randomness. As examples, OsRng and ThreadRng implement CryptoRng+Default, but ChaChaRng implements only CryptoRng, but not Default.

But I think a better approach will be to discourage implementation of Default for PRNG implementations in the first place because of its ambiguity.

It's fine for OsRng and ThreadRng so imho nothing is wrong right now. It's also unclear if folks even want type level PRNGs like this, but figured I'd ask.

[dhardy]

So... nothing to change, then? (Maybe just some doc PR tweak?)

impl<R: RngCore + CryptoRng + Default> SecretKey<R> {
    pub fn sign(&self) -> Signaure {
        let mut nonce = [0u8; 32];
        R::default().fill_bytes(&mut nonce);
    }
}

This seems like bad practice to me:

1. It is incompatible with a local RNG (in fact, it is compatible with so few RNGs I'm not even sure why it's generic; might as well just use rand::thread_rng())
2. The struct has a bound on the RNG, yet only uses it within one method

I would recommend one of the following instead:

• fn sign<R: CryptoRng>(&self, rng: &mut R) -> Signature
• rng: R as a struct field, where R: CryptoRng
• just use rand::thread_rng()

(Note: in the next release we will have CryptoRng: RngCore.)

[dhardy]

We even could imagine a default impl for Default for SeedableRng in rand_core when getrandom feature is enabled.

We could (advantages):

• RNG becomes usable in generics using Default
• Makes it clear that Default shouldn't be implemented on the RNG directly

... but (disadvantages):

• construction via default() hides a non-deterministic operation
• construction via sys-call (fresh entropy) might cause perf issues in a few cases (e.g. if operations like the above impl of sign are very frequent)

[newpavlov]

but ChaChaRng implements only CryptoRng, but not Default.

My point is that we probably should discourage implementation of Default for PRNGs regardless whether they CryptoRng or not, i.e. it applies to small PRNGs like Xoroshiro as well. This discouragment should probably go to the SeedableRng docs.

OsRng and ThreadRng are semantically "true" entropy sources, so it does not apply to them.

[burdges]

I'll maybe just close this since we all agreed anyways about the overall picture, nothing really needs to be done except maybe warn others off using Default, and the type level PRNG thing does not necessarily make to much sense.