crypto-common: Generate trait

[tarcieri]

Replaces all of the previous RNG functionality with a Generate trait which has the same basic shape as the existing methods, but allows types to impl only try_from_rng and receive provided versions of all of the other methods:

• generate: infallible generation using system RNG with panic on error
• try_generate: fallible version of above with RNG error Results
• from_rng: infallible generation with RNG parameter that panics on error

The generate and try_generate methods are available when the getrandom feature is enabled.

Impls are provided for hybrid_array::Array<u8, U>.

With this approach we can generate things like symmetric keys and IVs like:

type Aes256CbcEnc = cbc::Encryptor<aes::Aes256>;
let key = Key::<Aes256CbcEnc>::generate();
let iv = Iv::<Aes256CbcEnc>::generate();

The trait is also intended to be impl'd by consuming crates for other key generation use cases, e.g. RsaPrivateKey, dsa::SigningKey, ecdsa::SigningKey, or KEM decapsulation keys.

[tarcieri]

I think in the past rand_core experimented with traits like this, and offhand I don't know why they weren't accepted.

One thing that's nice about defining them in crypto-common is we can always mandate CryptoRng.

[newpavlov]

crypto-common feels like a wrong place for this trait... It looks like a re-invention of Rng::random and of the unstable std::random::Distribution trait.

As long as we rely on hybrid-array and we don't have stabilized random-value generation traits in core, I think we should keep such functionality in hybrid-array. Rejection sampling makes this problem a bit harder, but I will comment on it in #2092.

It may be worth to ask about random-value generation trait in rand_core, I do not remember rationale for not including it. Probably we wanted to keep rand_coreas simple as possible.

[tarcieri]

crypto-common feels like a wrong place for this trait

crypto-common feels like the wrong place for key generation traits? Why oh why do you say that?

Note there are two traits of note: one which uses the ambient RNG (Generate), and one which is parameterized (FromCryptoRng).

I personally care a lot more about the former one, which I don't think is covered by any existing traits anywhere. It's been a huge source of pain to have users deal with OsRng, incompatible rand(_core) versions, and now they have to deal with fallibility and things like unwrap_err/unwrap_mut. I just want APIs they can call that infallibly give them random keys that work consistently everywhere, without having to futz with an additional parameter which simply signals "I want to use the system RNG".

It looks like a re-invention of Rng::random

Which we can't use, because it's in rand, not rand_core. And that's only for FromCryptoRng, not Generate.

I find myself implementing these two patterns all over the place, but there are subtle inconsistencies. Some places are fallible and some are not. Having traits to wrap up the fallibility can reduce a lot of duplication, and generally having traits for this ensures consistency.

To me having a trait for key generation which works everywhere, not just for uniformly random bytestrings, seems like a no brainer and I am somewhat perplexed why you don't want to move forward with this. The alternatives you suggested don't apply.

I guess we can keep doing this all as same-shaped-inherent-methods, and try to manually adopt some consistency if you are truly unwilling to entertain traits to wrap this up.

Perhaps if you don't like FromCryptoRng we could still have Generate?

[newpavlov]

crypto-common feels like the wrong place for key generation traits? Why oh why do you say that?

Because I think the correct place for it is rand_core or core::random. There is nothing cryptography-specific in the proposed traits.

I am fine with having the Generate trait as a temporary convenience trait until rand_core v1.0 is released or value-generation items in core::random are stabilized, i.e. under condition that it should be eventually removed in a future breaking release.

Also, is there a reason to keep the from_rng/try_from_rng methods in a separate trait?

[tarcieri]

This PR factors and feature-gates the traits which depend on getrandom and rand_core respectively, so the whole trait is gated by the feature, rather than having selectively available methods. This allows either-or (in addition to both) usage of getrandom or rand_core.

I guess we could gate the whole thing as any(feature = "getrandom", feature = "rand_core") or have getrandom pull in rand_core if you'd like them combined, but I thought it was cleaner this way. I think users will be generally interested in one or the other.

[newpavlov]

I think we can write something like this:

pub trait Generate: Sized {
    // find a better name?
    fn from_closure<E>(fill: impl FnMut(&mut [u8]) -> Result<(), E>) -> Result<Self, E>;

    #[cfg(feature = "getrandom")]
    fn generate() -> Self {
        Self::try_generate().expect("RNG failure")
    }

    #[cfg(feature = "getrandom")]
    fn fn try_generate() -> Result<Self, RngError> {
        Self::from_closure(|buf| getrandom::fill(buf).map_err(RngError))
    }

    #[cfg(feature = "rand_core")]
    fn from_rng<R: CryptoRng + ?Sized>(rng: &mut R) -> Self {
        let Ok(ret) = Self::try_from_rng(rng);
        ret
    }

    #[cfg(feature = "rand_core")]
    fn try_from_rng<R: TryCryptoRng + ?Sized>(rng: &mut R) -> Result<Self, R::Error> {
        Self::from_closure(|buf| rng.fill_bytes(buf))
    }
}

It also helps to reduce the implementation boilerplate.

[tarcieri]

I'm not sure that sort of abstraction is going to make sense in a lot of cases. Some keygen is going to require a lot of structured randomness, e.g. RSA and DSA.

[newpavlov]

Note that the closure can be called as many times as you want and with different buffer sizes. It's no different from calling rng.try_fill_bytes or getrandom::fill, so it should not result in any problems in implementation crates apart from having the somewhat awkward closure in internal signatures.

Meanwhile with two separate traits you would have to either duplicate implementation or introduce a similar closure on the implementation side.

[tarcieri]

I think in some cases like rsa, where everything is currently implemented in terms of rand_core (e.g. crypto-primes for random prime generation), the best way to implement generate/try_generate is going to be by pulling in rand::rngs::OsRng for the caller so they don't have to.

Otherwise this pattern needs to be implemented all the way down the stack. I think it's too onerous.

[newpavlov]

In that case, accounting for rust-random/getrandom#751 we should implement all methods in terms of try_from_rng and configure getrandom = ["rand_core"] + getrandom = { version = "0.4", features = ["sys_rng"] }. It would mean that we will always pull rand_core even when only getrandom-based generation is used, but I think it should be fine.

[tarcieri]

Oh, getrandom will have an OsRng-alike in the next breaking release? That's fine then, when is that going to happen?

[newpavlov]

In the next breaking release (i.e. v0.4), most likely immediately after rand_core v0.10 is released.

[tarcieri]

@newpavlov any ETA on a prerelease with that feature?

[newpavlov]

Not sure. I will ping in the PR and we probably can cut a pre-release after merge.

[tarcieri]

If getrandom::Error adds a core::error::Error impl it would also get rid of the need for RngError

[tarcieri]

@newpavlov okay, I've pushed up a new version like what you proposed, and updated the title/description accordingly.

To fill the gap for now I have privately vendored SysRng from rust-random/getrandom#751

[newpavlov]

Looks good (with the caveat that it should be replaced someday with rand_core or core::random). Although I would prefer to wait for the getrandom pre-release before merging.

Also, please add a changelog entry.

[tarcieri]

Although I would prefer to wait for the getrandom pre-release before merging.

I guess I can do stacked PRs for other changes like #1897 and updating other crates to use this API. It's a bit less convenient and also means crate releases are blocked until a getrandom prerelease.

The only thing that will change when migrating to getrandom is RngError. I think we can probably re-export getrandom::Error as RngError, though the type will change.

[newpavlov]

The getrandom PR is merged, so we can use dependency patching for now. I am fine with merging this PR with temporarily patched getrandom.