Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Osrng doc #333

Merged
merged 3 commits into from
Mar 27, 2018
Merged

Osrng doc #333

merged 3 commits into from
Mar 27, 2018

Conversation

pitdicker
Copy link
Contributor

Based on top of #328.

I took a stab at improving the documentation of OsRng.

Somewhere in a long paragraph it mentioned the problem with /dev/urandom during early boot. Instead of making things complicated for users, I think this is something we can handle, especially now that we only keep a single handle to /dev/urandom open. Opened #332 for that and other possible improvements to OsRng.

Also updated the changelog with SecRandomCopyBytes(#322), #321 (comment), and removed HighPrecision01 (maybe want to think things through a bit more for that one?).

@pitdicker
Copy link
Contributor Author

Rebased (and fixed spelling)

@pitdicker pitdicker added the C-docs Documentation label Mar 25, 2018
@@ -87,7 +87,6 @@ You may also find the [Update Guide](UPDATING.md) useful.
- Use widening multiply method for much faster integer range reduction. (#274)
- `Uniform` distributions for `bool` uses `Range`. (#274)
- `Uniform` distributions for `bool` uses sign test. (#274)
- Add `HighPrecision01` distribution. (#320)
Copy link
Member

Choose a reason for hiding this comment

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

Can we not assume #320 will land before 0.5?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can, but there is some open discussion around using a wrapper type, and adding range code. I now feel like it is better to land HighPrecision01 in combination with the range code, as that seems like the more common use. And I don't mind if that takes some longer to get right...

Copy link
Member

@dhardy dhardy Mar 26, 2018

Choose a reason for hiding this comment

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

Okay. I was only thinking that I don't want to make a release which removes functionality without a viable replacement, but there isn't really much removed that HighPrecision01 replaces (I was thinking about the change to gen() for floats, but nothing is actually missing).

src/os.rs Outdated
/// Platform sources:
/// Sometimes there is the misconception that `OsRng` somehow provides 'better'
/// random numbers than some good user-space cryptographic random number
/// generator. This is not true, but `OsRng` has a role in initializing such an
Copy link
Member

Choose a reason for hiding this comment

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

This is a very incomplete note on the topic; it would be better just to refer to elsewhere (e.g. the doc I wrote, or maybe we should refer to an external article on the topic specific to our Rand lib).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My goal was mostly to say that CSPRNG's generate values that can be just as good as those from OsRng, but I can see what you mean. There may be other considerations. I did a second try at this paragraph.

If it is not yet really what you have in mind, can you make a suggestion?

/// Note: many Unix systems provide `/dev/random` as well as `/dev/urandom`.
/// On all modern systems these two interfaces offer identical quality, with
/// the difference that on some systems `/dev/random` may block. This is a
/// dated design, and `/dev/urandom` is preferred by cryptography experts. [1]
Copy link
Member

Choose a reason for hiding this comment

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

The urandom man page on my system still mentions:

When read during early boot time, /dev/urandom may return data prior to the entropy pool being initialized.

You noted wanting to fix this in #332; I don't know that we should remove all doc about this vulnerability before then however (although in practice it only affects things like systemd).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With #338 this is fixed on our side.

src/os.rs Outdated
/// the operating system.
/// A random number generator that retrieves randomness straight from the
/// operating system. This is the preferred external source of randomness for
/// most applications. Commonly it is used to initialize a user-space RNG, which
Copy link
Member

Choose a reason for hiding this comment

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

I still don't think the guidance about what to use this for / which RNG is 'better' belongs here at all. (I'm thinking of moving the doc about crypto-stuff from lib.rs to the README; we can reference that.)

The note about EntropyRng and details like not blocking and how this interfaces with the OS make sense.

I think we should also add a note that OsRng::new() is guaranteed to be very cheap (after first successful call), and will never consume more than one file handle per process.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the paragraph and added a bit about OsRng::new().

Copy link
Member

Choose a reason for hiding this comment

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

Also the bit about this is the preferred source of randomness — I presume you mean entropy source, but it could also be read to mean preferred way to get each new random value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😢 And I already don't like writing documentation 😄.

@dhardy
Copy link
Member

dhardy commented Mar 26, 2018

Okay, I'm finally happy with this PR! I'll let you merge in the morning if you think it's ready.

@pitdicker pitdicker merged commit f1d5546 into rust-random:master Mar 27, 2018
@pitdicker pitdicker deleted the osrng_doc branch March 27, 2018 06:22
pitdicker added a commit that referenced this pull request Apr 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-docs Documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants