Support `CryptoRng` in `RngCore` trait objects

[aticu]

Summary

Change CryptoRng such that it can be used as a random number generator through a trait object, by making it a subtrait of RngCore.
This would break existing CryptoRng implementations, which were previously not implementing RngCore, but those were likely of limited use.

Details

Change the CryptoRng trait from this

pub trait CryptoRng {}

to this

pub trait CryptoRng: RngCore {}

Motivation

Currently the use of RngCore is supported in trait objects, however because of how trait objects are implemented, it is not possible to use CryptoRng in addition in the trait object, thus making it impossible to actually use a CryptoRng as a random number generator in trait objects.

This change is really small and likely has very little negative impact on users who don't need this change, but it can enable another way to use the API for users who do need it.

Alternatives

• Add a CryptoRngCore trait to the rand_core crate:

  trait CryptoRngCore: RngCore + CryptoRng {}
  impl<T: RngCore + CryptoRng> CryptoRngCore for T {}

  This would allow users to use RngCore + CryptoRng types through trait objects as well, but at the cost of an additional trait in the (I presume intentionally) small rand_core crate.

• Do nothing and let users implement the CryptoRngCore trait above if they need it.

  This allows them to use RngCore + CryptoRng types through trait objects, but since it isn't a unified solution, multiple equivalent traits with the exact same purpose and definition might pop up over time.

  Optionally this pattern could also be documented in the CryptoRng trait.

[dhardy]

Thanks for the clear RFC. I agree that the initial proposal makes sense, though it will likely to a while to land (or at least to affect downstream crates like rand) since it is a breaking change.

Either way, we should allow some time for comments before making a decision.

[newpavlov]

I wonder why we haven't done it initially... I vaguely remember there was kind of a reason for the current design, but I can't remember what was it exactly. If there are no significant obstacles to it, I also would prefer the subtrait approach.

[vks]

Would it be possible to cast a dyn RngCore to a dyn CryptoRng?

[aticu]

Would it be possible to cast a dyn RngCore to a dyn CryptoRng?

I don't think that would be possible even with unsafe code. At least not without undefined behavior, since you'd need have a valid dyn CryptoRng vtable somewhere, which is different from the vtable of dyn RngCore. Even if their layout is likely to be identical, this isn't guaranteed (unless I'm missing something here).

There would also not be any guarentee that the original RngCore object also implements the CryptoRng trait, making it UB to cast it without checking that some other way, even if such a cast was possible safely.

[burdges]

I'd expect casts from dyn RngCore to dyn CryptoRng would never work, but more importantly rust has no roadmap for casts from dyn CryptoRng to dyn RngCore, making them incompatible for the foreseeable future. ATCs make casting among trait objects possible but complex, ala https://internals.rust-lang.org/t/casting-families-for-fast-dynamic-trait-object-casts-and-multi-trait-objects/12195

We never knew if CryptoRng makes much sense: We've so many like ChaChaRng where security depends upon the seed, so usually one wants a random oracle here. We never knew if associated constants would eventually do this better or if more detailed information helps.

pub enum RngSecurity {
    Garbage,
    NumericalMethods,
    ???,
    RandomOracle,
    RandomnessSource,
}
pub trait RngCore {
    const SECURITY: RngSecurity = RngSecurity::RandomOracle;
    fn next_u32(&mut self) -> u32;
    fn next_u64(&mut self) -> u64;
    fn fill_bytes(&mut self, dest: &mut [u8]);
    fn try_fill_bytes(&mut self, dest: &mut [u8]) -> Result<(), Error>;
}

I suppose one solution would be to deprecate CryptoRng but encourage people say what they want:

pub trait RngCore {
    fn information() { eprint!(::std::any::type_name()) }
    fn contract(msg: &str) {
        #[cfg(feature = "contractreport")]
        {  information();  eprintln!("RngCore contract: {}", msg);  }
    }
    fn next_u32(&mut self) -> u32;
    fn next_u64(&mut self) -> u64;
    fn fill_bytes(&mut self, dest: &mut [u8]);
    fn try_fill_bytes(&mut self, dest: &mut [u8]) -> Result<(), Error>;
}

pub trait SeedableRng: Sized {
    fn information() { eprint!(::std::any::type_name()) }
    fn contract(msg: &str) {
        #[cfg(feature = "contractreport")]
        {  information();  eprintln!("SeedableRng contract: {}", msg);  }
    }
    type Seed: Sized + Default + AsMut<[u8]>;
    fn from_seed(seed: Self::Seed) -> Self;
    fn seed_from_u64(state: u64) -> Self { ... }
    fn from_rng<R: RngCore>(rng: R) -> Result<Self, Error> { ... }
    fn from_entropy() -> Self { ... }
}

so folks could insert any warnings the like via say

fn information() { eprint!("Non-RandomOracle {}", ::std::any::type_name()) }

and if you ask then you get messages like "Non-RandomOracle XorShiftRng contract: Random Oracle".

Ideally formal methods and design by contract folks would spend more time developing rust tooling outside rustc itself.

[dhardy]

After sleeping on it I remember there is a reason we can't just implement this: CryptoRng is implemented on non-RNGs such as ChaChaXCore so that CryptoRng is implemented on the derived RNG.

So maybe the CryptoRngCore alternative is a better idea.

[newpavlov]

CryptoRng is implemented on non-RNGs such as ChaChaXCore so that CryptoRng is implemented on the derived RNG.

Ah, yes! It was exactly that. I think that instead of CryptoRngCore a better alternative would be to introduce CryptoBlockRng:

trait CryptoBlockRng: BlockRngCore {}
trait CryptoRng: RngCore {}

impl<T: BlockRngCore> RngCore for BlockRng<T> { .. }
impl<T: CryptoBlockRng> CryptoRng for BlockRng<T> {}

[aticu]

I think that instead of CryptoRngCore a better alternative would be to introduce CryptoBlockRng:

While I do think that would be a better solution when designing a new API, it likely isn't feasible in this situation, since does have a much bigger impact on downstream users.

CryptoRng + BlockRngCore would go from semantically meaning "this is a block RNG suitable for cryptographic use" to meaning "this is an RNG suitable for cryptographic use that also happens to be a block RNG not necessarily suitable for cryptographic use".

Completely changing the meaning of traits (especially "marker" traits like CryptoRng) is a change that may even silently break things in some situations.

Seeing how CryptoRng is also used for BlockRngCore, I also think that a CryptoRngCore trait (and CryptoBlockRngCore) would be the better solution here.

[dhardy]

@aticu I agree that the less breaking change is desirable. The naming of your suggestions in unfortunate though:

• RngCore is what RNGs implement; Rng is the automatically impl'd user trait for all RngCore
• CryptoRng is a pure marker trait; the proposed CryptoRngCore is that marker + an extension over RngCore

[aticu]

@dhardy Adding a new trait wouldn't be a breaking change at all.

Naming is (as always) tough to get right.
I think you parsed the name CryptoRngCore differently.
I read it as Crypto(RngCore), whereas I think you read it as (CryptoRng)Core.
I do agree that this ambiguity is a problem.

Here are some alternative naming ideas, thought I'm not a huge fan of any one of them:

• CryptoCompatibleRngCore & CryptoCompatibleBlockRngCore
• RngCoreWithCrypto & BlockRngCoreWithCrypto
• RngCoreWithCryptoRng & BlockRngCoreWithCryptoRng
• RngCryptoCore & BlockRngCryptoCore

I personally don't really care a lot what the name would be if this were added, but I still think a good name is important, so if you have any better ideas, I'd be open to them.

[burdges]

We should seriously consider deprecating CryptoRng entirely because honestly dyn RngCore vs dyn CryptoRngCore sounds like a Hobson's choice or worse. It's possible this problem simply lies out of scope for rust's type system.

Aside from the fn contract(msg: &str) and associated const tricks I mentioned upthread, we might ask @RalfJung if he foresees any interesting tricks for enforcing this sort of contract with miri runs not traits, and maybe the hacspec authors @denismerigoux and @franziskuskiefer if they see anything.

An RFC for permitting dyn RealTrait + CustomMarkerTrait may already exists too.

[newpavlov]

@aticu

CryptoRng + BlockRngCore would go from semantically meaning "this is a block RNG suitable for cryptographic use" to meaning "this is an RNG suitable for cryptographic use that also happens to be a block RNG not necessarily suitable for cryptographic use".

Completely changing the meaning of traits (especially "marker" traits like CryptoRng) is a change that may even silently break things in some situations.

I should've been more clear: the proposal was for the next breaking release of rand_core. Of course, in the described form it's a clear breaking change which should not be shipped in a patch release. Assuming it will be done in rand_core v0.7, in the worst case scenario developer of an implementation crate during migration to v0.7 may forget to change impl from CryptoRng to CryptoBlockRng for its block RNG. Users of this crate may then encounter a compilation error, which should be fixable quick enough by reporting a bug in the implementation crate. Yes, it could be somewhat annoying, but nothing close to the silent breakage mentioned in your post.

I think that the CryptoRngCore trait could be a fine stop-gap solution for rand_core v0.6. Another possible alternative is to introduce the two sub-traits without removing the existing CryptoRng:

// will be removed in rand_core v0.7
trait CryptoRng {}

trait CryptoBlockRngCore: BlockRngCore {}
trait CryptoRngCore: RngCore {}

impl<T: BlockRngCore> RngCore for BlockRng<T> { .. }
impl<T: CryptoBlockRngCore> CryptoRngCore for BlockRng<T> {}

impl<T: CryptoRng + BlockRngCore> CryptoBlockRngCore for T {}
impl<T: CryptoRng + RngCore> CryptoRngCore for T {}

// alternatively we can add impls in different direction:
impl<T: CryptoBlockRngCore> CryptoRng for T {}
impl<T: CryptoRngCore> CryptoRng for T {}

It will cause a bit of churn in downstream crates, but I think replacing impl CryptoRng + RngCore with impl CryptoRngCore.

@burdges

We should seriously consider deprecating CryptoRng entirely

As one of the RustCrypto maintainers I am strongly against that. I do not want for our users to accidentally plug clearly insecure RNGs into cryptographic code. Yes, the CryptoRng trait is not an absolute protection, but it's significantly better than nothing.

[dhardy]

@burdges are you referring to Rust's (current) lack of trait-object upcast (i.e. inability to convert dyn CryptoRngCore to RngCore)? We can work around this by adding an fn as_rng_core(&mut self) -> &mut dyn RngCore method.

Overall I am in favour of @newpavlov's suggestion (CryptoBlockRng and change CryptoRng in the next breaking release).

@aticu is there any urgency to support CryptoRng trait objects within rand_core on your part? I presume not since you mentioned a work-around in downstream crates.

[burdges]

Yes fn as_rng_core(&mut self) -> &mut dyn RngCore works, but overall a naive marker trait does not fit the goals of CryptoRng terribly well. We should explore less naive alternatives, which capture more nuance, some of which I listed above.

If all you wanted was a CryptoRng-style marker then this achieves the same goals without all the crap

pub trait RngCore {
    /// Panic in test builds unless this `RngCore` is a CSPRNG
    ///
    /// Any `impl RngCore for ..` for a CSPRNG should add `fn assert_cryptorng(&self) { }` so that 
    /// this assertion always passes.  
    ///
    /// If you want a CSPRNG is desired then invoke `rng.assert_cryptorng("reasons")`, which does nothing in release builds, but non-CSPRNGs panic in tests. 
    fn assert_cryptorng(&self, msg: &str) {
        #[cfg(test)]
        panic!("{} is not a cryptographic secure PRNG!  {}", ::std::any::type_name(), msg);
    }
    fn next_u32(&mut self) -> u32;
    fn next_u64(&mut self) -> u64;
    fn fill_bytes(&mut self, dest: &mut [u8]);
    fn try_fill_bytes(&mut self, dest: &mut [u8]) -> Result<(), Error>;
}

I suggest we do something similarly unorthodox but that captures CSPRG better than CryptoRng does.

[vks]

@burdges You are using strong words against CryptoRng, but I still don't understand your argument. What exactly is the problem with the extension to CryptoRng discussed here? As @dhardy mentioned, the lack of casting dyn CryptoRng to dyn RngCore can be worked around. As far as I can see, supporting upcasting like this in the language is desired, but not yet implemented.

The alternatives you suggested move the problem from compile time to run time. I don't understand why this would be preferable, or more nuanced.