Rename random_mod ↦ random_mod_vartime
#1030
Conversation
Rejection sampling means that `random_mod` is inherently variable-time, and the current name violates the contract in the `README`: > All functions contained in the crate are designed to execute in > constant time unless explicitly specified otherwise (via a `*_vartime` > name suffix). This renames `random_mod` to `random_mod_vartime`, adding in deprecated aliases at the old names for backwards-compatibility. It preserves the `ConstantTimeLess` functionality for good measure and to not needlessly increase the amount of timing information leaked. Of course, `random_mod` leaks a pretty small amount of timing information; by my reckoning, at worst an average 2 bits per call, and approaching 0 as the modulus approaches `2^n-1`. (We would have exactly 0 at `2^n` if we did a check for powers of 2, but this is currently a worst case for our `random_mod`; if the user knows they have a power of 2, they should use `random_bits` instead.) So this is a bit of an edge case; but I argue that the `vartime` naming here reduces surprise.
| /// Generate a random number which is less than a given `modulus`. | ||
| /// | ||
| /// This uses rejection sampling. | ||
| /// | ||
| /// As a result, it runs in variable time that depends in part on | ||
| /// `modulus`. If the generator `rng` is cryptographically secure (for | ||
| /// example, it implements `CryptoRng`), then this is guaranteed not to | ||
| /// leak anything about the output value aside from it being less than | ||
| /// `modulus`. | ||
| #[deprecated(since = "0.7.0", note = "please use `random_mod_vartime` instead")] | ||
| fn random_mod<R: RngCore + ?Sized>(rng: &mut R, modulus: &NonZero<Self>) -> Self { | ||
| Self::random_mod_vartime(rng, modulus) | ||
| } | ||
|
|
||
| /// Generate a random number which is less than a given `modulus`. | ||
| /// | ||
| /// This uses rejection sampling. | ||
| /// | ||
| /// As a result, it runs in variable time that depends in part on | ||
| /// `modulus`. If the generator `rng` is cryptographically secure (for | ||
| /// example, it implements `CryptoRng`), then this is guaranteed not to | ||
| /// leak anything about the output value aside from it being less than | ||
| /// `modulus`. | ||
| #[deprecated(since = "0.7.0", note = "please use `try_random_mod_vartime` instead")] | ||
| fn try_random_mod<R: TryRngCore + ?Sized>( | ||
| rng: &mut R, | ||
| modulus: &NonZero<Self>, | ||
| ) -> Result<Self, R::Error> { | ||
| Self::try_random_mod_vartime(rng, modulus) | ||
| } |
There was a problem hiding this comment.
NB. these are the deprecated aliases added (which might otherwise get lost in the noise of the mechanical rename diff.)
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1030 +/- ##
==========================================
- Coverage 79.81% 79.79% -0.03%
==========================================
Files 163 163
Lines 17750 17759 +9
==========================================
+ Hits 14168 14170 +2
- Misses 3582 3589 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Rereading the argument about cryptographic security of the RNG, the case for the old name being okay seems stronger than I initially thought. I do think it’s a bit of a funny edge case; I could go either way on this change, feel free to reject if it doesn’t seem worthwhile. |
tarcieri
left a comment
tarcieri
left a comment
There was a problem hiding this comment.
It should be possible to have a fallible version that runs in constant time, takes a select (possibly const generic) number of samples, selects the first one that fits the criteria in constant-time among all of the samples, and returns an error if none of them fit the criteria.
For now though, I agree, this should be named *_vartime.
|
Not sure what's going on with codecov. I think it's mad you're adding methods which aren't being tested, even if they're deprecated. |
2 participants
Rejection sampling means that
random_modis inherently variable-time, and the current name violates the contract in theREADME:This renames
random_modtorandom_mod_vartime, adding in deprecated aliases at the old names for backwards-compatibility. It preserves theConstantTimeLessfunctionality for good measure and to not needlessly increase the amount of timing information leaked.Of course,
random_modleaks a pretty small amount of timing information; by my reckoning, at worst an average 2 bits per call, and approaching 0 as the modulus approaches2^n-1. So this is a bit of an edge case; but I think thevartimenaming here reduces surprise.