Crypto / rand trait & crate split #2118

dhardy · 2017-08-18T17:02:25Z

Background

This RFC is an offshoot of the Rand revision RFC, on the subject of:

splitting rand into multiple crates
splitting the current Rng trait into a crypto-oriented trait and
numerical-application-oriented trait
impl of above trait(s) for OS-provided randomness
naming conventions for above things

jonas-schievink · 2017-08-18T17:07:21Z

text/0000-rand-crypto.md

+
+```rust
+pub enum Error {
+    pub Unspecified,


The pub here is invalid syntax

newpavlov

Good work! Some comments and suggestions.

UPD: Also what about SeedableRng? I think we need crypto counterpart, either fully copied SeedableCsrng (plus errors for incorrect seed length) or something like trait Csprng: Csrng { .. }, otherwise CSPRNG crates will have to import rand or not use traits for relevant functionality.

newpavlov · 2017-08-18T17:04:46Z

text/0000-rand-crypto.md

+
+Create a new crate, `crypto-rand`, with only the following contents:
+
+```rust


You forgot to add #![no_std] here.

newpavlov · 2017-08-18T17:09:59Z

text/0000-rand-crypto.md

+```rust
+pub enum Error {
+    pub Unspecified,
+}


Not sure if we need enum here. Can't we just use pub struct Error; ? @briansmith can you provide motivation for using enum? I think it's quite unlikely we'll need to expand this error in the future.

Also what about implementing Error trait (btw conflict of the names) and deriving/implementing others, e.g. Copy, Clone, Debug, Display, Read, etc.? For Error we'll need to introduce std feature gate.

newpavlov · 2017-08-18T17:14:14Z

text/0000-rand-crypto.md

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


I think Generator is a bit too long, how about Csrng? (you propose something similar in alternatives) It's a bit more cryptic, but will be in a good alignment with my further proposal regarding crate names. Also I don't fully agree with Brian regarding the need to remove "r" from "rng" just because crate name is based on "random".

newpavlov · 2017-08-18T17:17:32Z

text/0000-rand-crypto.md

+```
+
+Associated with this, create another crate called `crypto-rng-os` which defines
+a struct `OsGen`, implementing `Generator` via OS functionality (using code from


If we want to use a prefix for crate names, I think something like csrng-* will fit better. Although I like suffixes like os-csrng a bit more, to me they feel more natural for this use-case. Also if we use rng in the crate name, then it's logical to use OsRng instead (plus it will be familiar to users).

newpavlov · 2017-08-18T17:18:56Z

text/0000-rand-crypto.md

+    // possibly also next_u128
+
+    // possibly also the following (no Result unlike try_fill above):
+    fn fill(&mut self, dest: &mut [u8]) { /* default impl here */ }


You need to specify what will happen here in case of an error, will it panic? Ideally directly provide default implementation for this method. Something like:

fn fill(&mut self, dest: &mut [u8]) { self.try_fill(dest).except("..."); }

est31 · 2017-08-18T22:37:41Z

Random number generation is a complex topic, and usage can be categorised into
two main groups: cryptographic applications requiring secure keys, and
non-cryptographic applications (which can be further divided into at least
randomised algorithms, stochastic simulations, and games).

Note that some games have the additional requirement of determinism: that for a given seed, you are guaranteed to get some specific value. This means that you can't use floats for computation, and you can't store stuff into usize because both are not guaranteed to behave the same. I've personally seen noise-rs do the mistake of taking the seed in usize format.

For example, take a minecraft/minetest clone which stores the map by storing the difference to the generated terrain. Here you want perfect reproducability, especially if client and server have different platforms (server x86 based, client ARM based).

Most use cases in games are of the "fast, no need for determinism" kind, but for the use cases outlined above, determinism is very important.

Probably this use case is very niche and doesn't deserve its own trait, but instead implementations of the trait can signify whether they are 100% deterministic in their documentation, but I think its great to be aware.

dhardy · 2017-08-19T08:36:47Z

Actually, @est31, I think determinism is very useful for a bunch of stuff (e.g. see this comment of mine). Luckily for us all PRNG algorithms are deterministic (the 'P' means pseudo — algorithms can never generate real randomness).

Maybe that comment should be a little clearer then...

newpavlov · 2017-08-19T21:38:25Z

@dhardy
Your forgot to update link to the rendered text after your commit.

Interesting point was raised in the parent RFC. Should we use error which will be associated type in the CryptoRng trait or use ZST Error provided by the crypto-rand crate. First allows more flexibility, but the second could be less secure and will make it a bit harder to write abstract code.

Regarding crypto-rng-os, I think we can just go with rng-os or os-rng.

scottmcm · 2017-08-21T19:13:09Z

text/0000-rand-crypto.md

+
+The `CryptoRng` trait and `Error` enum are specified above. We should make it
+clear that although `Error::Unspecified` is directly accessible, `Error` could
+be extended in the future (is formal specification of this possible?).


#[non_exhaustive]? #2008

scottmcm · 2017-08-21T19:17:20Z

text/0000-rand-crypto.md

+}
+
+impl OsRng {
+    pub fn new() -> OsRng {


Would it be possible to move the fallibility into the constructor? So the trait method would be infallible, but you'd use OsRng::try_new() to get an instance, which might fail with something like Err(Error::NotYetSecurelySeeded) or Err(Error::NotAvailableOnPlatform). (My recollection from the previous threads and sponge papers is that once the CSPRNG has sufficient entropy to start producing values, it will no longer fail.)

Well, some errors can be detected on creation, but the interfaces used by most or all OSes have some potential to fail later. Whether or not they fail in practice I have no idea.

est31 · 2017-08-22T00:55:47Z

@dhardy

I think determinism is very useful for a bunch of stuff

great to hear!

Luckily for us all PRNG algorithms are deterministic

If you use floats you are likely not deterministic (as floats are strange beasts where order of operations matter but compilers don't care and reorder happily, and many more things). And if you use usize for seed information or internal state, you are platform dependent, which obviously doesn't give you deterministic behaviour if you want to carry out the same computation on multiple devices and want the same result.

So its not a trivially fulfilled property. Surely, its easy to make most PRNG algorithms deterministic, but we should have some consensus on what to do about non deterministic/platform dependent ones... Fix them? Mark them somehow? Are docs enough?

dhardy · 2017-08-22T10:55:29Z

@est31 if you're talking specifically about PRNG algorithms, nearly all algorithms are integer based, and we pretty-much agreed to remove the next_f32 / next_f64 methods from Rng. Also, I don't think any PRNG algorithms would use usize internally since the algorithms tend to be specialised to the integer size (e.g. see IsaacRng vs Isaac64Rng in the current code).

Of course, a complex application using floating-point operations may end up producing different results on different platforms (OS/CPU/compiler) even if the PRNG generates the same numbers; I've had that problem before (even totally different results due to decisions based on randomised FP numbers) but it's not a PRNG problem.

est31 · 2017-08-22T11:03:50Z

Also, I don't think any PRNG algorithms would use usize internally

Noise-rs uses usize for seeding: https://docs.rs/noise/0.4.1/noise/struct.Perlin.html

dhardy · 2017-08-23T06:44:12Z

@est31 Perlin Noise is not a general purpose random number generator: sampled values are deliberately not independent. That said, if you think it should be cross-platform reproducible, by all means ask the author not to use usize. But not here please; it doesn't even attempt to implement Rng (with good reason).

zackw · 2017-08-31T19:44:51Z

Just as an observation, if you care about determinism and/or reproducibility of your RNG, you need to pin both the seed and the algorithm. It's important, I think, to give the people who need those things what they want - but it is also important to make sure that the "default" RNG isn't tied down to either determinism or any particular algorithm.

Basically what I'm saying is that the seedable API should require you to specify the algorithm as well as the seed, and APIs that don't specify the algorithm should not be manually seedable.

dhardy · 2017-09-01T09:57:43Z

I'm going to close this RFC; it's not up-to-date with the main one and having a separate discussion channel for the split didn't work out.

There are some good points about reproducibility here; not quite sure how we got to that subject though!

See rust-lang/rfcs#2118 (comment)

rand-crypto: new RFC, split off from rand-refactor discussion

8f6791b

dhardy changed the title ~~rand-crypto: new RFC, split off from rand-refactor discussion~~ Crypto / rand split & traits Aug 18, 2017

dhardy changed the title ~~Crypto / rand split & traits~~ Crypto / rand trait & crate split Aug 18, 2017

jonas-schievink reviewed Aug 18, 2017

View reviewed changes

text/0000-rand-crypto.md

```rust

pub enum Error {

pub Unspecified,

Copy link

Contributor

jonas-schievink Aug 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pub here is invalid syntax

newpavlov reviewed Aug 18, 2017

View reviewed changes

withoutboats added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Aug 18, 2017

rand-crypto: rename Generator → CryptoRng; mention single trait design

8e1c2c1

jethrogb mentioned this pull request Aug 21, 2017

Rand crate revision (pre-stabilisation) #2106

Closed

scottmcm reviewed Aug 21, 2017

View reviewed changes

aturon self-assigned this Aug 22, 2017

dhardy closed this Sep 1, 2017

dhardy added a commit to dhardy/rand that referenced this pull request Sep 4, 2017

Remove SeedableRng impl for StdRng

eb162fe

See rust-lang/rfcs#2118 (comment)


		Create a new crate, `crypto-rand`, with only the following contents:

		```rust

Crypto / rand trait & crate split #2118

Crypto / rand trait & crate split #2118

Uh oh!

Conversation

dhardy commented Aug 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Background

Uh oh!

jonas-schievink Aug 18, 2017

Choose a reason for hiding this comment

Uh oh!

newpavlov left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

newpavlov Aug 18, 2017

Choose a reason for hiding this comment

Uh oh!

newpavlov Aug 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

newpavlov Aug 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

newpavlov Aug 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

newpavlov Aug 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

est31 commented Aug 18, 2017

Uh oh!

dhardy commented Aug 19, 2017

Uh oh!

newpavlov commented Aug 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scottmcm Aug 21, 2017

Choose a reason for hiding this comment

Uh oh!

scottmcm Aug 21, 2017

Choose a reason for hiding this comment

Uh oh!

dhardy Aug 21, 2017

Choose a reason for hiding this comment

Uh oh!

est31 commented Aug 22, 2017

Uh oh!

dhardy commented Aug 22, 2017

Uh oh!

est31 commented Aug 22, 2017

Uh oh!

dhardy commented Aug 23, 2017

Uh oh!

zackw commented Aug 31, 2017

Uh oh!

dhardy commented Sep 1, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

dhardy commented Aug 18, 2017 •

edited

Loading

newpavlov left a comment •

edited

Loading

newpavlov Aug 18, 2017 •

edited

Loading

newpavlov Aug 18, 2017 •

edited

Loading

newpavlov Aug 18, 2017 •

edited

Loading

newpavlov Aug 18, 2017 •

edited

Loading

newpavlov commented Aug 19, 2017 •

edited

Loading