Make RngCore infallible?

[dhardy]

Summary

Remove RngCore::try_fill_bytes and recommend that the trait only be implemented for infallible RNGs.

Details

While, in theory, RNGs like ChaCha or even PCG could detect the end of a cycle and report an error, our current RNGs don't do this. BlockRng also does not have any consideration for RNG errors. Meanwhile, OsRng is our only remaining fallible RNG.

A potential external fallible RNG is a hardware RNG token. Plugging one of these into code expecting an RngCore would therefore be more difficult, requiring a fallback, hang or panic-on-error. It is, however, questionable if such a generator should ever implement RngCore.

Users of try_fill_bytes:

• RngReadAdapter
• Fill::try_fill; this is only to push error handling up to the caller, and arguably should be replaced with an infallible variant

Hence, for the rand library, the only real loss would be OsRng.

What this doesn't solve

Cryptographic applications requiring a byte RNG with support for error handling. An existing alternative is rustls::crypto::SecureRandom. Another, more application specific trait, is cipher::StreamCipherCore.

There is no reason that a crate can't simultaneously provide impls of more than one of these traits, so impls can be shared.

There is a missing link here: applications which are generic over a SecureRandom no longer have access to rand's functionality (without some error-handling adapter), and so on. This doesn't seem to bad though; in particular (a) adapters are possible (even if they must panic) and (b) as far as I am aware, applications of these different traits usually don't need functionality from multiple of these libraries.

(Note: it is also possible to use a custom trait like SecureRandomRng: SecureRandom + RngCore in case generic code does need to use multiple of these libraries.)

Motivation

Simplicity and focus.

We recently had #1412 in order to improve the situation for infallible code. Ultimately, though, rand is a crate for stochastic algorithms like shuffling, bounded uniform sampling and sampling from a Gaussian. Its generators can be used to generate keys or a book of random data and this won't change aside from the inability to plug in a fallible generator with proper error handling, but arguably when you need this error handling you should already be using another library anyway.

Alternatives

We could encode fallibility into RngCore (#1412 implements one method and mentions another in its comments).

We could also provide a FallibleRng with the try_fill_bytes method, however this begs questions like why not move it up to getrandom or why not use SecureRandom instead?

Final word

The rand library has long been a compromise between many competing demands. The only specific functionality we would lose here is the ability to forward errors from byte-generating RNGs. Fallible RNGs must currently panic (or hang) on error when implementing any of the other three RngCore methods, which is a strong indication that something is wrong with the current design.

CC

@newpavlov @vks @tarcieri @ctz @josephlr

[dhardy]

In reply to @newpavlov:

In my opinion, it's tantamount to sweeping the problem under the rug.

IO-based RNGs such as OsRng or hardware-based RNGs (not an unusual thing with cryptographic applications) can fail as any other IO. It's a simple truth of life. Imagine someone has misconfigured, or forgot to plug-in a HW RNG. It's better for a cryptographic application/library to return a sensible error, instead of unconditionally panicking outright. Even PRNGs may fail in some cases, e.g. ChaCha-based RNGs may return error on detected looping, which may happen in practice in the presence of seeking.

As for OsRng, it can be a preferred way of generating sensitive information such as cryptographic keys, especially considering that ThreadRng does not implement any protection against exposed state. You may say "then use getrandom directly", but it would be less flexible, e.g. we would not be able to use PRNGs for reproducible testing of such methods.

As far as I am aware, most Rust cryptographic libraries already do not use our traits, and probably shouldn't either. They could still use our generators with an adapter over their own traits if required.

Saying "but it would be less flexible" is a weak argument. As said above, our PRNGs could still be used in test code (via an adapter).

[newpavlov]

As far as I am aware, most Rust cryptographic libraries already do not use our traits, and probably shouldn't either.

Sorry for being snarky, but you haven't even taken a look at RustCrypto crates before writing this? For example, see docs for the KeyInit trait used by crates like aes-gcm and many others.

[newpavlov]

Re: SecureRandom. @ctz has explicitly stated the following:

rustls::crypto::SecureRandom is the lowest-level primitive that rustls needs to implement TLS, not one that is intended to be useful elsewhere the ecosystem.

By being bounded by Send + Sync, this trait is not suitable for a wider ecosystem (whether this bound is truly needed for rustls is a separate question not relevant to this issue).

why not move it up to getrandom

Because it's outside of its responsibility. Its goal is to provide access to a "default" system entropy source, not to abstract over various IO-based RNGs.

[tarcieri]

Having separate traits for fallible vs infallible RNGs sounds like a reasonable improvement

[dhardy]

For example, see docs for the KeyInit trait used by crates like aes-gcm and many others.

The fact that there is a use-case for an error-reporting byte-generating CSPRNG trait shouldn't surprise anyone here. The question is, should that be part of the rand library? Even if the answer is yes, there is scope for a dedicated trait independant of RngCore.

(And given that SeedableRng already doesn't inherit from RngCore, the only sticking point I can see is CryptoRng. But, do we need CryptoRng on RngCore in this case or only on the fallible byte-RNG trait?)

Because it's outside of its responsibility.

Make an argument for why it should be part of rand_core's responsibility.

The only one I can think of is that it is useful for CSPRNGs to implement both RngCore and a crypto, fallible, byte-RNG trait, and having both in one place makes this slightly easier.

[newpavlov]

The question is, should that be part of the rand library?

Well, if you want to completely remove the cryptographic use cases from rand's area of responsibility, then sure... Personally, I will not be happy about this, but it's your right as the project's lead. I guess, we will just cook something on the RustCrypto side then, interoperability be damned.

Make an argument for why it should be part of rand_core's responsibility.

Because it literally declares "random number generation traits" in the first line of its documentation.

What exact problem are you trying to solve? The InfallibleRng PR started from the complaint that rand_core does not differentiate between fallible and infallible RNG implementations. Sweeping fallibility under the rug (it still will exist for OsRng, ThreadRng, and wrappers around hardware RNGs) will not resolve this issue, on the contrary.

I don't remember a single issue which was caused by try_fill_bytes during existence of rand v0.8.

As for splitting fallibility into a separate trait, I don't see much reason to do that. As I wrote in the InfallibleRng PR, I think we can add an associated error type, this way most PRNGs will use the Infallible type (and eventually !). If you want to keep RngCore clean, we could move it and try_fill_bytes into CryptoRng. This would mean that fallibility will be available only for cryptographic RNGs, but I guess it should be fine? Eventually, it may be worth to move to this design, but it's relatively unimportant.

[dhardy]

Because it literally declares "random number generation traits" in the first line of its documentation.

Okay. I do see that a FallibleRng (name?) trait is needed somewhere, and don't see a good reason that it shouldn't be in rand_core.

Should it just be this?

pub trait FallibleRng {
    fn try_fill_bytes(&mut self, dest: &mut [u8]) -> Result<(), Error>;
}

What relationship should it have to RngCore (i.e. InfallibleRng), if any? We could

impl<R: RngCore + ?Sized> FallibleRng for R

easily; is this enough?

There is a minor argument for not including in rand_core: we could also get rid of the Error type and thus all dependency on getrandom from the public API (though that isn't really an issue given they've agreed to freeze the error values).

---

Sweeping fallibility under the rug (it still will exist for OsRng and ThreadRng)

I propose that if we add a new FallibleRng trait, then OsRng will only implement that; otherwise OsRng will be removed.

ThreadRng already sweeps fallibility under the rug, and has done so as long as I can remember. If you don't think it should, that's a new issue...

---

As I wrote in the InfallibleRng PR, I think we can add an associated error type,

This is acceptable in generic code which does not care about fallibility or which forwards this error type intact. It can be a nuisance to deal with in generic code which must wrap and forward errors (in that simpler solutions are often possible when the error type is known).

Code which wants only infallible RNGs is fine with either this solution or my proposal above, though having to specify R: RngCore<Error = !> is more verbose than just R: RngCore or even R: InfallibleRng.

Code which wants to be generic over RNGs and be able to handle errors properly requires a fixed specification of errors (at least as a conversion target), which is what getrandom::Error is. (And while supporting a conversion target is acceptable, R: RngCore where <R as RngCore>::Error: Into<getrandom::Error> is a lot more verbose than the alternative.)

Object-safe code cannot be generic; the error type must be explicitly specified in dyn RngCore<Error = ..>.

Eventually, it may be worth to move to this design

I don't think there's even an unstable feature for associated constants in object-safe traits? There is certainly interest.

But even if this was supported, it would need to be fixed in the vtable, thus effectively, we would have the following incompatible object-safe traits:

• NoncryptoRng<Error = ..>
• CryptoRng<Error = ..>

Since no one needs the "definitely not a crypto RNG" assertion, the current trait CryptoRng: RngCore makes more sense than making CRYPTO_STRONG an associated constant.

[dhardy]

At a guess, most users of object-safe RNGs will not want to deal with fallibility, thus it may be fine to go with this:

pub trait FallibleRng {
    type Error: Into<getrandom::Error>;

    fn try_fill_bytes(&mut self, dest: &mut [u8]) -> Result<(), Self::Error>;
    
    /// Convert to an InfallibleRng which panics on error
    fn as_infallible(self) -> PanicOnError<Self> where Self: Sized {
        PanicOnError(self)
    }
}

pub struct PanicOnError<R: FallibleRng>(R);
impl<R: FallibleRng> InfallibleRng for PanicOnError<R> { .. }

Also, should we rename what's left of RngCore → InfallibleRng?

[dhardy]

For old discussions on this topic, see: #225 . I do not see mention of using multiple traits for fallible vs infallible RNGs and can't recall why we didn't investigate this approach.

[newpavlov]

we could also get rid of the Error type and thus all dependency on getrandom from the public API (though that isn't really an issue given they've agreed to freeze the error values).

rand_core already does not expose getrandom in its public API. The error type gets re-defined and the only connection is the From<getrandom::Error> impl which can be removed if we really want it.

ThreadRng already sweeps fallibility under the rug, and has done so as long as I can remember.

Huh, I thought that it returns reseeding errors from try_fill_bytes. I agree that it's a separate issue and we can leave it for later.

having to specify R: RngCore<Error = !> is more verbose than just R: RngCore or even R: InfallibleRng.

Yes, this is why I wrote that we probably should wait for trait aliases before doing it.

I don't quite understand why you want to introduce a separate trait with blanket impl instead of using super-traits:

pub trait RngCore {
    fn next_u32(&mut self) -> u32;
    fn next_u64(&mut self) -> u64;
    fn fill_bytes(&mut self, dst: &mut [u8]);
}

pub trait CryptoRng: RngCore {
    type Error;
    // maybe change name to something like `fill_crypto_bytes`?
    fn try_fill_bytes(&mut self) -> Result<(), Self::Error>;
}

This way RngCore will be object safe. Its methods would continue to panic on potential errors as they do today.

I don't think it's important for CryptoRng to be object safe (@tarcieri WDYT?). Even applications which support switching between RNGs at runtime would probably define their own wrapper type. Users which really need it can define their own object safe wrappers which erase errors like SecureRandom or convert them into a concrete type.

Your PanicOnError snippet looks a bit over-engineered for me, but I guess it could work. Though I am not sure about utility of Into<getrandom::Error>. Some HW RNGs may want to use io::Error, which certainly does not satisfy this bound.

[dhardy]

rand_core already does not expose getrandom in its public API.

That's what I meant. Relevant past issue (from you): #768

Huh, I thought that it returns reseeding errors from try_fill_bytes.

The culprit is impl<R: BlockRngCore<Item = u32>> RngCore for BlockRng<R> {..}, or rather that BlockRngCore has an infallible design. This could be changed, but I think all implementors are considered infallible anyway (excepting for cycles).

I don't think we ever bothered detected cycles. Certainly not on small PRNGs (where it would probably require a copy of the state). I think ChaCha at one point incremented the nonce if the counter ever overflowed, but later we decided not to do that.

I don't quite understand why you want to introduce a separate trait with blanket impl instead of using super-traits:

Under the limitation that FallibleRng == CryptoRng, I think this is fine.

We did briefly consider a separate fallible RNG trait.

Though I am not sure about utility of Intogetrandom::Error. Some HW RNGs may want to use io::Error, which certainly does not satisfy this bound.

I mean, getrandom::Error could implement From<io::Error>, though it may be better to define a new error type in rand_core. Not sure. Again, something that has been discussed without finding a better solution: #837.

[tarcieri]

We did briefly consider a separate fallible RNG trait.

@dhardy I do like those names: RngCore/TryRngCore (well maybe not the *Core part so much as using Try* for the fallible version). FallibleRng vs InfallibleRng seem quite verbose.

If you want a blanket impl, perhaps you could have a blanket impl of TryRngCore/FallibleRng for RngCore/InfallibleRng which sets TryRngCore::Error = Infallible?

That would be similar to how the blanket impl of TryFrom for types which impl Into works in core/std.

[dhardy]

@tarcieri that blanket impl is what I had in mind above, but it doesn't work with @newpavlov's suggestion to put try_fill_bytes in CryptoRng. The question is more whether that is a good idea: do we ever want a fallible RNG which is not a CryptoRng?

[tarcieri]

It seems like CryptoRng could have a supertrait bound on TryRngCore, and RngCores could still be marked as CryptoRng via the blanket impl of TryRngCore

[dhardy]

Another consideration is whether fallible RNGs should implement RngCore (the "infallible" trait). @newpavlov's suggestion above requires this.

If we didn't have this, it might make sense to have getrandom (OsRng) implement the fallible trait; I don't think it makes much sense having this implement RngCore however. Okay, so it does right now...

---

ThreadRng already sweeps fallibility under the rug

In my opinion the sensible type of fallibility to consider here is that initialisation might fail. For this, we'd have:
fn thread_rng() -> Result<ThreadRng, GetrandomError>
To make this work, we'd need the error type to support Clone and there would be a little bit more overhead in calling thread_rng() — I think this is viable, though I'm not sure how useful it is when almost everyone would use .unwrap() or similar anyway.

Otherwise the only possible error is a cycle, but as said above we don't report that anyway.