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

Derive Clone for OsRng #384

Merged
merged 1 commit into from
Apr 11, 2018
Merged

Conversation

pitdicker
Copy link
Contributor

This only recently become possible for all platforms, and adding Clone to OsRng seems in the spirit of ThreadRng. Both are then clonable for the reason that a clone returns "another handle to the same generator".

@pitdicker
Copy link
Contributor Author

We could in theory also implement Clone for JitterRng, and then for EntropyRng.

I think there should be no harm in cloning JitterRng, but I feel slightly uneasy with the idea of 'cloning the entropy pool' (nice term for a single u64).

And adding Clone to EntropyRng might conflict with the ideas to fall back to some configurable external RNG in the future?

@vks
Copy link
Collaborator

vks commented Apr 10, 2018

Well, if OsRng and JitterRng support clone, why shouldn't EntropyRng?

Note that this is contrary to the advice currently given in the docs:

Clone if, and only if, the clone will have identical output to the
original (i.e. all deterministic PRNGs but not external generators)

@pitdicker
Copy link
Contributor Author

Note that this is contrary to the advice currently given in the docs:

I touched that bit of doc in #383 with the comments from #312 (comment) in mind:

  • Clone, but only if the clone will have identical output to the original (i.e. be a true clone), or if the clone would be another handle to the same generator.

If that PR gets accepted, things should be more consistent than they were.

if OsRng and JitterRng support clone, why shouldn't EntropyRng?

I don't know the answer yet. Are we confident implementing Clone for JitterRng is a good idea? And if we are, are we also sure Clone for EntropyRng won't clash with future plans (#313)?

@vks
Copy link
Collaborator

vks commented Apr 10, 2018

I don't know the answer yet. Are we confident implementing Clone for JitterRng is a good idea? And if we are, are we also sure Clone for EntropyRng won't clash with future plans (#313)?

How would #313 clash with Clone impls?

@dhardy
Copy link
Member

dhardy commented Apr 11, 2018

OsRng is a zero-sized or at least tiny handle to the OS interface, so implementing Clone seems fine.

@pitdicker made JitterRng very small, so again a Clone impl seems fine.

For EntropyRng I also have some concern since there is a greater chance we would change this in the future — we haven't decided on a solution to #313 yet, though probably anything added to EntropyRng would be easy to clone.

@pitdicker pitdicker merged commit 95ea68c into rust-random:master Apr 11, 2018
@pitdicker pitdicker deleted the clone_osrng branch April 11, 2018 20:53
@pitdicker
Copy link
Contributor Author

I will make another PR that add a comment like this to JitterRng:

Note: JitterRng maintains a small 64-bit entropy pool. With every generate 64 new bits should be integrated in the pool. If a round of generate were to collect less than the expected 64 bit, then the returned value, and the new state of the entropy pool, would be in some way related to the initial state. It is therefore better if the initial state of the entropy pool is different on each call to generate. This has a few implications:

  • generate should be called once before using JitterRng to produce the first usable value (this is done by default in new);
  • We do not zero the entropy pool after generating a result. The reference implementation also does not support zeroing, but recommends generating a new value without using it if you want to protect a previously generated 'secret' value from someone inspecting the memory;
  • JitterRng does not implement Clone to prevent multiple clones having the same initial entropy pool. This might be overly paranoid however.

Note that all but the last point are not my own ideas, but just the code from the original version.

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