Uh oh!
There was an error while loading. Please reload this page.
ThreadRng / EntropyRng improvements - #579
Conversation
dhardy
commented
Aug 4, 2018
Updated: I added a There are a number of things I don't like about this. The code is untidy. We can't actually unimplement I'm also unsure whether we want |
dhardy
commented
Aug 10, 2018
I would appreciate some feedback on this, in particular the design using a
Alternatives with static linkage are (1)
or (2) an external crate
So perhaps using an external crate is the best option? |
| /// Is this source available? | ||
| /// | ||
| /// The default implementation returns `true`. | ||
| fn is_available() -> bool { true } |
There was a problem hiding this comment.
Should we even provide a default implementation? I feel like this should always be implemented explicitly.
| #[derive(Debug)] | ||
| pub struct EntropyRng { | ||
| source: Source, | ||
| _dummy: Rc<()> // enforce !Send |
There was a problem hiding this comment.
It's probably cleaner to use core::marker::PhantomData here.
There was a problem hiding this comment.
Yes, probably. I'm not sure that Rc<()> is a zero-sized type.
There was a problem hiding this comment.
Better to not rely on Rc just for !Send, raw pointers are not send: https://play.rust-lang.org/?gist=e350c6d98704bae32e0feb593687bf62&version=stable&mode=debug&edition=2015
vks
commented
Aug 28, 2018
Do we have a use case for The other two commits look fine to me. |
dhardy
commented
Aug 28, 2018
Yes, this is for |
vks
commented
Aug 28, 2018
Maybe we should get some input from the embedded community to make sure we are addressing their needs? cc @japaric |
japaric
commented
Aug 28, 2018
I don't have the bandwidth to look into this right now but I'll tell the other WG members to look into this during our next meeting (which is in 5 minutes) |
therealprof
commented
Aug 28, 2018
Can someone sum up what the changes are vs using |
The main point is that Specifically, this PR is about making There are still some other things to do to make |
therealprof
commented
Aug 28, 2018
@dhardy That's a noble goal but let me rephrase my question: With |
therealprof
commented
Aug 28, 2018
Hm, I just had a closer look at the proposal. The
All information between those two must be exchanged using In my opinion it would be more useful (and idiomatic) if the RNG conceptually worked more like a |
dhardy
commented
Aug 29, 2018
letmut entropy_source = EntropyRng::new();let r = Hc128Core::from_rng(&mut entropy_source).unwrap_or_else(|err| panic!(...));There are no changes planned for Yes, Do you mean the main loop and interrupt handlers cannot share code? Or that global memory cannot be used to host the implementation? As shown by the When it comes to |
therealprof
commented
Aug 29, 2018
Well, only via functions called from all sides.
Sharing via memory is only possible using synchronisation primitives (e.g. RefCell and Mutex) or In a nutshell: It's much easier to share something with an interrupt handler that was obtained from a call during initialisation than it is to share something that should ultimately be handled outside of main.
There're no threads in
In a single_threaded world it's not as bad as it sounds. ;) Coming back to my example above: What we'd really like to have here is separation of concerns; some MCUs have really nice TRNGs which can be set up and trigger an interrupt when they have a new nice true random value for us, so an interrupt handler should be able to seed that into the software RNG. On the other hand you might need random values in one or more other interrupt handlers so those should be able to fetch values when they need them. So the best approach would either be: Take an RNG, stuff it in a |
dhardy
commented
Aug 30, 2018
Using entropy when it becomes available rather than when your program/device starts is a challenge... the same one that has led to questionable behaviour in Linux in the past (producing output from What is In that case, there are really only three directions:
Have you thoughts on this? I don't like the idea of being forced into a "best effort" compromise with potential security issues, but I don't know that the other options are viable in general (any kind of persistent storage would have to be handled outside of this lib). That said, my idea for making
If we use something like the spin crate for mutexes, is there some way these can be dropped when single threaded? Or is the overhead likely to be insignificant anyway? |
TeXitoi
commented
Aug 30, 2018
The spin crate might not be appropriate in some cases. For example, if you are on an interrupt driven paradigm, you can deadlock with the spin crate, but have better alternative as, for example, https://docs.rs/cortex-m/0.5.6/cortex_m/interrupt/struct.Mutex.html See for example https://docs.rs/shared-bus/0.1.2/shared_bus/ |
dhardy
commented
Aug 30, 2018
Great, so there isn't even a good broadly-applicable synchronisation library available? Rand is supposed to be a general-purpose lib, not something targetting ARM Cortex M. |
TeXitoi
commented
Aug 30, 2018
shared bus propose an approach to this problem: an interface of a mutex that can be implemented by the user, with an implementation using std's mutex. The embedded dev can then use the spin crate or any custom mutex he want. |
therealprof
commented
Aug 30, 2018
A hardware RNG peripheral.
Depends. In this case you can poll the RNG if it has entropy (well, in some cases they're random numbers and not raw entropy) available and it will be somewhat quick, in the milliseconds range. In other cases you might not be so lucky.
Yeah, best effort is horrible. But OTOH shouldn't there be a way to re-seed entropy at a later point? I also don't think blocking is a good idea but you really want to let the user know: "Hey, I can't give you random values because: not enough entropy, you might want seed some", e.g. by returning a
Sounds great to me, but certainly want the option to re-seed later. That would also allow for re-seeding on demand (not sure the RNG will deplete the entropy pool or just continue chugging along) instead of periodically, e.g. when the RNG hands out an |
Make the user specify the synchronisation implementation to use. ;) |
dhardy
commented
Aug 30, 2018
That's what I mean by best-effort. Output low-entropy (potentially guessable) "random" numbers now, but allow some type of entropy injection in order to improve security. But really if we go this route, we want some API to allow users the choice between "best effort available now" and "wait until it's secure".
Are you talking about Parking Lot's lock_api? This looks more like a building block for mutexes than something another lib can use. Or is there some other crate with user-replaceable mutexes?
Yes, but how? |
dhardy
commented
Aug 30, 2018
PRNGs don't deplete the entropy pool (theoretically they have finite length, but the cycle length is huge). However, if they are initialised with zero entropy or very little entropy then an observer can potentially guess the next output. The trick to making them secure is to use sufficient entropy to start with (> ~100 bits). It's also important that "reseeding" or "entropy injection" only actually uses this once enough is available for whatever security criterion is chosen (e.g. 100 or 256 bits); since if the PRNG is directly adjusted each time a little entropy is available then "entropy exhaustion" attacks are still possible. But it's possible the TRNG implementation will do the necessary accumulation. |
therealprof
commented
Aug 30, 2018
That's not what I meant, though. 😉 I would want the combination:
Check out https://crates.io/crates/shared-bus, I think it explains it nicely. |
dhardy
commented
Aug 30, 2018
let manager = shared_bus::BusManager::<std::sync::Mutex<_>,_>::new(i2c);This is not compatible with What might be best is a new crate, |
therealprof
commented
Aug 30, 2018
I see. For me it would be fine to just ignore additional entropy once enough has been supplied. I would still find it beneficial for initialisation purposes if I could try to use and if the PRNG deems that not enough entropy is available it'll just return an |
This is actually what djb recommends (because additional entropy does not really improve security but might reduce it), so I agree this is probably the best approach. |
dhardy
commented
Aug 30, 2018
@vks what do you think of the idea of making a |
burdges
commented
Sep 27, 2018
Are the I kinda dislike exposing anything like |
alexcrichton
commented
Sep 27, 2018
@dhardy sorry I'm absolutely overloaded right now and so I don't have time to work through the design here. Sorry about that :( |
dhardy
commented
Sep 27, 2018
No that's fine Alex. |
newpavlov
commented
Sep 27, 2018
Can't we make a mutable static which will hold a pointer to function which will be source of entropy? For example for modern linux it will be simply |
So here is a small prototype which seems to work:
// in the real crate if `std` is enabled use syscalls and panicking function otherwise// function must fill the whole buffer or return an errorpubfndefault_entropy(buf:&mut[u8]) -> Result<(),u8>{
buf.iter_mut().for_each(|v| *v = 1);Ok(())}pubstaticmutENTROPY_SOURCE:fn(&mut[u8]) -> Result<(),u8> = default_entropy;// we probably want to keep it as thin as possiblepubstructDefaultRng;// in real crate use RngCore+CryptoRng insteadimplDefaultRng{pubfnnew() -> Self{DefaultRng}pubfntry_fill_bytes(&mutself,buf:&mut[u8]) -> Result<(),Error>{unsafe{ENTROPY_SOURCE(buf)}}}Crate library: externcrate rand;use rand::DefaultRng;pubfnfoo() -> u32{letmut buf = [0u8;32];letmut rng = DefaultRng::new();
rng.try_fill_bytes(&mut buf).unwrap();letmut c = 0u32;for v in buf.iter(){ c += *v asu32;}
c
}App crate: externcrate crate_lib;externcrate rand;// this function can use HW source or accumulated entropy poolpubfncustom_entropy(buf:&mut[u8]) -> Result<(),Error>{
buf.iter_mut().for_each(|v| *v = 2);Ok(())}fnmain(){unsafe{
rand::ENTROPY_SOURCE = custom_entropy;}println!("{}", crate_lib::foo());}App crate prints 64 instead of 32 as expected. Of course Alternatively we may want to split We also could make entropy source signature a bit closer to |
dhardy
commented
Sep 28, 2018
@newpavlov isn't that approach similar to the one implemented in this PR, except that it's slightly lower level (function pointer instead of trait object pointer)? Functionally I don't see much difference, and @burdges points still stand. As @burdges says, it may make sense to hide this behind a feature flag, or only when not using |
Yes, it's similar, but IMHO much simpler, you are always guaranteed to have "default" entropy source (but without I think we could omit And yes, I think this "default RNG" should be defined outside of Contrary to your proposal I don't think that we should provide anything for |
You are aware that even reading
An "unavailable" stub for |
I think If we need Afaik, there is never any reason to call Why distinguish between |
Yes, this is why I've used It's possible to create pubtypeEntropySource = fn(&mut[u8]) -> Result<usize,Error>;#[cfg(target_has_atomic = "ptr")]staticENTROPY_SOURCE:AtomicPtr<EntropySource> =
AtomicPtr::new(default_entropy as*mutEntropySource);#[cfg(not(target_has_atomic = "ptr"))]staticmutENTROPY_SOURCE:EntropySource = default_entropy;#[cfg(target_has_atomic = "ptr")]pubunsafefnset_custom_entropy(source:EntropySource){// Not sure if `Release` is enough, to be safe we could use `SeqCst` insteadENTROPY_SOURCE.store(source as*mutEntropySource,Ordering::Release);}#[cfg(not(target_has_atomic = "ptr"))]pubunsafefnset_custom_entropy(source:EntropySource){ENTROPY_SOURCE = source;}I guess we could remove
Can you provide a link with the problem description? I think at the very least it should be disabled for cryptographic entropy source. And if it will be possible to redefine entropy source with |
burdges
commented
Sep 28, 2018
I'd assume Also, we can poison Is there a discussion of the receiver type Is it okay that |
dhardy
commented
Sep 29, 2018
Thanks for the suggestion @burdges; sounds sensible. Yes, the interface must be thread-safe.
Right, this is an abuse of trait objects instead of function pointers (the idea being that the user sets a trait-object, which can then provide all the above functions). For our purposes
See 180 ("fixed" by #181, which was superseded by #196).
|
therealprof
commented
Sep 29, 2018
As long as there's awareness that in |
So it's about I am a bit hesitant about using trait objects, please keep in mind, that entropy source should be usable on embedded platforms without allocations, atomics and mutexes. Simple function is easy to understand and reason about in the presence of interrupts, while IIRC it's currently impossible to create trait objects from raw pointers on stable. What is the motivation behind
In all three cases we don't need those methods. For entropy source users they are not needed as well. I see why we would need |
dhardy
commented
Sep 30, 2018
This was done a long time ago (0.4).
Trait objects are part of the language. Atomics are part of the core lib, essential with multiple CPU (core)s, and should be trivial with a single CPU. The current design doesn't need allocations and wouldn't need real mutexes except that I took the lazy option here (it's only a proof of concept).
|
vks
commented
Apr 9, 2019
@dhardy What is the status of this? It seems like some of the changes here rather belong to the |
dhardy
commented
Apr 9, 2019
That's a complicated question. IIRC there are several topics of discussion:
So yes, at least some of this now belongs in the |
vks
commented
Apr 9, 2019
Ok, then let's close this and move the discussion to rust-random/getrandom#4. |
dhardy
commented
Apr 9, 2019
No, lets not. There are several points in here not yet tracked elsewhere. I'll re-read the conversation when I can find the time. |
dhardy
commented
Jun 3, 2019
Okay, I read through and added a few notes to getrandom#4. Some discussion points may be worth referencing if anyone wishes to pursue |
ThreadRng(Future ofthread_rng#463 and ??) — I think we only didn't do this before because of safety concerns, but (1) use of raw pointers automatically implies the type is neither send nor sync, (2)thread_localinstantiates automatically so there is always a valid object and (3) none of the types involved implementDropEntropyRnga little (working towards Makethread_rng()available withoutstd#313)set_custom_entropyfunctionNext step: allow custom entropy source. Unfortunately theEntropySourcetrait is still not good enough for run-time injection because the size & alignment of instances is unknown (it would be nice if this could be enforced somehow).