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

Use getrandom, simplify OsRng, deprecate EntropyRng #765

Merged
merged 9 commits into from
Apr 6, 2019

Conversation

dhardy
Copy link
Member

@dhardy dhardy commented Apr 2, 2019

Implements #760

This implements several of the mentioned changes. In theory the re-implementation of OsRng can be omitted but I think for most users this will decrease the number of crates required with very little issue; if necessary we can revert this easily later.

This also removes the re-export of JitterRng which is a breaking change. I do not think this will impact many users, and switching to rand_jitter is easy enough for those people.

This does not deprecate FromEntropy, which is simply sugar for from_rng(OsRng).unwrap(). This trait already has some usage. We could move the method to SeedableRng but this requires dependence on getrandom from rand_core; I'm not sure if it is justified.

Review @newpavlov?

@dhardy
Copy link
Member Author

dhardy commented Apr 2, 2019

@newpavlov the test fails on cloudabi because of this rule in getrandom:

#[cfg(any(
    feature = "std",
    windows, unix,
    target_os = "redox",
    target_arch = "wasm32",
))]
mod error_impls;

This is not the usual no_std support approach, though since we don't have an env var or build-wide cfg item it does make some sense to "guess" whether we're using std. But we still have a std feature; shouldn't we choose one option or the other? Otherwise we see things like this: std support works on common platforms, but fails unexpectedly when testing on other platforms.

Copy link
Member

@newpavlov newpavlov left a comment

Choose a reason for hiding this comment

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

Looks good! Though I think we should follow the approach with optional dependency of rand_core on getrandom.

wasm-bindgen = ["rand_os/wasm-bindgen"]
stdweb = ["rand_os/stdweb"]
wasm-bindgen = ["getrandom/wasm-bindgen"]
stdweb = ["getrandom/stdweb"]
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure if we need to bubble these features. Users can write in application crates getrandom = { version = "*", features = [ .. ] }. Something similar will have to be done if rand is used indirectly via dependencies, which I think will be fairly common.

Copy link
Member Author

Choose a reason for hiding this comment

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

We do if we don't want to make a breaking change — though we can't ever lose these without a breaking change.

Copy link
Member

Choose a reason for hiding this comment

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

Well, v0.7 is already a breaking release, so we could use this chance to cleanup those features. Though if you want to keep number of breaking changes to a minimum, then it's fine to keep it as-is.

Copy link
Member Author

Choose a reason for hiding this comment

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

On second thoughts, no, lets keep the feature forwarding. It's convenient for use in our tests and something we might have some other reason to break in the future (hopefully eventually these features won't be needed), so there's not much sense in a breaking change to achieve so little.

src/lib.rs Show resolved Hide resolved
src/rngs/os.rs Show resolved Hide resolved
src/rngs/os.rs Show resolved Hide resolved
if dest.len() == 0 { return Ok(()); }

getrandom(dest).map_err(|e|
Error::with_cause(ErrorKind::Unavailable, "OsRng failed", e))
Copy link
Member

Choose a reason for hiding this comment

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

Hm, I thought the old error will be replaced with getrandom::Error, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

Possibly, yes, but not as part of this PR, hence I hacked in a map for now.

Would you like to look into this?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I can do it a bit later.

src/rngs/os.rs Outdated
fn try_fill_bytes(&mut self, dest: &mut [u8]) -> Result<(), Error> {
// Some systems do not support reading 0 random bytes.
// (And why waste a system call?)
if dest.len() == 0 { return Ok(()); }
Copy link
Member

Choose a reason for hiding this comment

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

I think this branching should be removed as it does not follow the principle of least astonishment. Systems which do not support reading 0 bytes should be handled on getrandom level. Let's say someone will want to first check if OsRng works, why force user to use 1 byte array?

Copy link
Member Author

Choose a reason for hiding this comment

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

True, this should be done as part of getrandom if at all. I don't agree that retrieving 0 bytes can be considered a test of readiness however (precisely because this kind of code may exist).

Copy link
Member Author

Choose a reason for hiding this comment

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

Should we add a test for 0-byte reads to getrandom?

Copy link
Member

Choose a reason for hiding this comment

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

Should we add a test for 0-byte reads to getrandom?

Yes, I think we should.

Ok(())

getrandom(dest).map_err(|e|
Error::with_cause(ErrorKind::Unavailable, "OsRng failed", e))
Copy link
Member

Choose a reason for hiding this comment

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

See rand::OsRng comments.

@newpavlov
Copy link
Member

newpavlov commented Apr 2, 2019

Hm, I thought that cloudabi is considered unix. Worth to create rust-lang/rust issue? Ideally I would like to automatically enable std feature for std-capable target, but unfortunately AFAIK cargo does not provide the required functionality.

I will send a PR with the fix.

UPD: Ups, I've accidentally commited the fix directly instead of creating a PR.

src/rngs/os.rs Outdated Show resolved Hide resolved
@@ -480,35 +479,16 @@ impl_as_byte_slice_arrays!(!div 4096, N,N,N,N,N,N,N,);
/// [`OsRng`]: rngs::OsRng
#[cfg(feature="std")]
pub trait FromEntropy: SeedableRng {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to deprecate this trait as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

That is the question from the top comment (whoops, EntropyRngFromEntropy).

@dhardy dhardy mentioned this pull request Apr 3, 2019
@dhardy
Copy link
Member Author

dhardy commented Apr 3, 2019

UPD: Ups, I've accidentally commited the fix directly instead of creating a PR.

Hah, okay. Lets remove the WASM-feature forwarding as suggested and re-run the tests. Rebased.

This is controversial: a small amount of code redundancy to
avoid an extra import.
This is a breaking change for anyone using rngs::JitterRng;
such users should switch to rand_jitter::JitterRng.
@dhardy
Copy link
Member Author

dhardy commented Apr 4, 2019

Okay, I rewrote the rngs module documentation completely (a little shorter and hopefully clearer), as well as adding the requested usage example.

I also pushed a hack to pull getrandom from master, to test @newpavlov's fix.

@dhardy
Copy link
Member Author

dhardy commented Apr 5, 2019

Hopefully this is good to go now (not all questions resolved, but follow-up PRs allowed).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants