Require `RngCore` instead of `CryptoRngCore` for various random methods by fjarri · Pull Request #710 · RustCrypto/crypto-bigint

fjarri · 2024-12-07T07:01:52Z

Relaxes CryptoRngCore requirement to RngCore. Fixes #137

AaronFeickert

There are many doc comments that reference cryptographically-secure generation, and are no longer accurate with the relaxed trait bound.

AaronFeickert · 2024-12-07T22:15:22Z

 {
    /// Generate a random `NonZero<T>`.
-    fn random(mut rng: &mut impl CryptoRngCore) -> Self {
+    fn random(mut rng: &mut impl RngCore) -> Self {


It may be useful to move aspects of the internal comment to the doc comment now that a CSPRNG is no longer guaranteed.

I removed this doc comment and allowed the Random::random() one take over.

AaronFeickert · 2024-12-07T22:15:48Z

 pub trait Random: Sized {
    /// Generate a cryptographically secure random value.
-    fn random(rng: &mut impl CryptoRngCore) -> Self;
+    fn random(rng: &mut impl RngCore) -> Self;


The doc comment is no longer accurate.

Added a note

AaronFeickert · 2024-12-07T22:15:57Z

    ///
    /// A wrapper for [`RandomBits::try_random_bits`] that panics on error.
-    fn random_bits(rng: &mut impl CryptoRngCore, bit_length: u32) -> Self {
+    fn random_bits(rng: &mut impl RngCore, bit_length: u32) -> Self {


The doc comment is no longer accurate.

Added notes for the methods in this trait

AaronFeickert · 2024-12-07T22:16:07Z

-        rng: &mut impl CryptoRngCore,
-        bit_length: u32,
-    ) -> Result<Self, RandomBitsError>;
+    fn try_random_bits(rng: &mut impl RngCore, bit_length: u32) -> Result<Self, RandomBitsError>;


The doc comment is no longer accurate.

AaronFeickert · 2024-12-07T22:16:16Z

    /// A wrapper for [`RandomBits::try_random_bits_with_precision`] that panics on error.
    fn random_bits_with_precision(
-        rng: &mut impl CryptoRngCore,
+        rng: &mut impl RngCore,


The doc comment is no longer accurate.

AaronFeickert · 2024-12-07T22:16:47Z

    /// CSRNG, where previous outputs are unrelated to subsequent
    /// outputs and do not reveal information about the RNG's internal state.
-    fn random_mod(rng: &mut impl CryptoRngCore, modulus: &NonZero<Self>) -> Self;
+    fn random_mod(rng: &mut impl RngCore, modulus: &NonZero<Self>) -> Self;


I would be very careful about this doc comment, as the user may not be aware of what constitutes a CSPRNG.

At any rate, the top-line comment is no longer accurate.

AaronFeickert · 2024-12-07T22:17:22Z

    /// underlying random number generator is truly a CSRNG, where previous outputs are unrelated to
    /// subsequent outputs and do not reveal information about the RNG's internal state.
-    fn random_mod(rng: &mut impl CryptoRngCore, modulus: &NonZero<Self>) -> Self {
+    fn random_mod(rng: &mut impl RngCore, modulus: &NonZero<Self>) -> Self {


I would be very careful about this doc comment, as the user may not be aware of what constitutes a CSPRNG.

At any rate, the top-line comment is no longer accurate.

AaronFeickert · 2024-12-07T22:17:43Z

 impl<const LIMBS: usize> Random for Uint<LIMBS> {
    /// Generate a cryptographically secure random [`Uint`].
-    fn random(mut rng: &mut impl CryptoRngCore) -> Self {
+    fn random(mut rng: &mut impl RngCore) -> Self {


The doc comment is no longer accurate.

AaronFeickert · 2024-12-07T22:17:56Z

    /// CSRNG, where previous outputs are unrelated to subsequent
    /// outputs and do not reveal information about the RNG's internal state.
-    fn random_mod(rng: &mut impl CryptoRngCore, modulus: &NonZero<Self>) -> Self {
+    fn random_mod(rng: &mut impl RngCore, modulus: &NonZero<Self>) -> Self {


See earlier comments.

fjarri · 2024-12-07T22:50:25Z

Thanks for checking, that's what happens when you make a PR late at night

fjarri · 2024-12-07T23:08:47Z

I was in the process of adding a little more stuff to the docs... sorry, should have switched it to the draft form

fjarri · 2024-12-07T23:09:58Z

Basically just the additional lines

/// To find a CSRNG, look for RNGs implementing the marker trait [`rand_core::CryptoRngCore`].

Maybe it would be too much hand-holding anyway.

tarcieri · 2024-12-07T23:16:42Z

That seems okay if you want to submit a followup

AaronFeickert · 2024-12-08T00:41:46Z

Does this change mean that random number generation functions using rejection sampling should be explicitly tagged with a _vartime suffix? Previously, their variable-time nature was guaranteed not to leak anything about output values. This is no longer a guarantee given the relaxed trait bound. The alternative would be to retain the existing naming, but update the documentation to carefully note this.

tarcieri · 2024-12-08T01:00:13Z

Ugh, I mean it was always "vartime". I agree that now there are more footguns.

AaronFeickert · 2024-12-08T01:02:15Z

Made some documentation updates in #711 toward this.

Require RngCore instead of CryptoRngCore for various random methods

573b478

fjarri force-pushed the rng-core branch from 0294e1d to 573b478 Compare December 7, 2024 08:53

AaronFeickert suggested changes Dec 7, 2024

View reviewed changes

Adjust docs

dc76950

fjarri force-pushed the rng-core branch from 4f884f0 to dc76950 Compare December 7, 2024 23:00

tarcieri approved these changes Dec 7, 2024

View reviewed changes

tarcieri merged commit e87d7f4 into RustCrypto:master Dec 7, 2024

AaronFeickert mentioned this pull request Dec 8, 2024

Update documentation for random functionality #711

Merged

fjarri deleted the rng-core branch December 8, 2024 18:31

tarcieri mentioned this pull request Jan 8, 2025

Merge signed-int into master #723

Merged

tarcieri mentioned this pull request Jan 22, 2025

v0.6.0 #750

Merged

Conversation

fjarri commented Dec 7, 2024

Uh oh!

AaronFeickert left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fjarri commented Dec 7, 2024

Uh oh!

fjarri commented Dec 7, 2024

Uh oh!

fjarri commented Dec 7, 2024

Uh oh!

tarcieri commented Dec 7, 2024

Uh oh!

AaronFeickert commented Dec 8, 2024

Uh oh!

tarcieri commented Dec 8, 2024

Uh oh!

AaronFeickert commented Dec 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants