Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
mirage-crypto-rng-eio: Eio backend of mirage-crypto-rng #155
mirage-crypto-rng-eio: Eio backend of mirage-crypto-rng #155
Changes from 9 commits
48c60db
73cbdfe
5b8ab13
c3dec68
bbeabf6
bee8fb7
587c3a7
3d580a5
1cd96a5
5df3c85
96d7ce6
52179d7
218193c
badf557
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the changes above. Could you minimize the difference in this file, please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
opam 2.1 command
opam install -y --deps-only .
didn't work - possibly because currently the repo contains an unpublished package(mirage-crypto-rng-eio). This is a way to get around that issue.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, thanks for your reply. now that it'll soon be merged into opam-repository, would you mind to PR something that removes the mentions of specific versions and packages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've no idea why here there are explicit names for all the packages, when above
opam-local-packages
already should contain exactly this list?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is so that it doesn't pick up mirage-crypto-rng-eio.opam dependencies. I think the workflow failed if this line wasn't there then. Perhaps it has changed now, but not sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After thinking again about 3871e3b, I wonder what the semantics of
run
is (and why it is as it is). According to the documentation, it accepts as last argument a functionfn
"that will have access to a running RNG".So is it expected that
run
is executed multiple times (on different domains)?While it sets and resets the module-local mutable state
running
, a second execution will print a warning message. Also, if I understand the implementation correctly (please help me since I'm a bit lost in respect to cancellation and eio), afterrun
has been executed (and finished), anyMirage_crypto_rng.generate
will produce random numbers. But once thefn
inrun
finishes, the periodic entropy feeding is stopped (and thus the rng keeps to produce random numbers, but that opens attacks if some bits of the rng state can be predicted -- this is the reason why periodic entropy feeding is necessary).The other implementations (lwt, mirage, async) expect
initialize
(and why is it calledrun
here?) to be called exactly once, and thisinitialize
registers several tasks, and finishes immediately -- but the entropy feeding is never stopped.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-reading the discussion, I noticed @talex5 is in favour of the
run
semantics "so that periodic task quits once the main task finishes". I wonder whether that should the imply that the defaiult_rng is reset to None (at the moment there's no API for this)? It is also a bit tricky, since certainlyfn
may contain the set to a different default generator, whichrun
would once the execution finishes, be reset toNone
.I'm a bit puzzled: what is the advantage of cancelling the periodic tasks? Is there an example application which only needs for a startup phase the rng? E.g. the lwt implementation registers (via Lwt.async) some tasks, so execution will be done whenever Lwt_main.run is executed -- does eio have similar functionality?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was assuming that only one domain would be collecting entropy, and mirage-crypto would be able to use it from all domains. The documentation should certainly say something about that.
The idea is that
run
runs at the entry point of your application and everything else happens inside it (including spawning more domains). Because of the structured concurrency, any domains spawned withinrun
will finish beforerun
does, so as long as you follow therun env @@ fun () ->
pattern it should be hard to go wrong.Structured concurrency prevents leaking fibers, which is what an
initialize
function with the same API as the others would do here. You could pass a switch argument (as the original version of this PR did), and have fibers added to that, but that causes the application to wait for the RNG thread to finish before exiting, which isn't what we want. I recently added aFiber.fork_daemon
function in the Git version of eio, which solves that problem, but therun
API would still be easiest for most uses, I think.That would be ideal.
The
run
API requires you to passenv
, which you get fromEio_main.run
, so you can't call this before entering the main loop anyway.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 96d7ce6, does that make sense in that way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that eio and multicore is different in the semantics, just to be clear:
run
API is then different (and adds some burden e.g. for those only interested in establishing a TLS client connection -- since now they've to think about initializing the RNG somewhere in their application)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need
running
at all now, then? We can just look at whether the default generator is set.Doesn't that mean that the user's choice of
sleep
duration or of which generator to use would get overridden by the arguments passed by Tls?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's right. Assuming nobody else sets the default generator.
Since there's the mutable
running
, only the first call to Mirage_crypto_rng_lwt.initialize will setup tasks etc. So indeed that may be that Tls_lwt sets thesleep
parameter already.I do think that this API could be improved, but I don't have the headspace at the moment to figure out what would be sensible. Putting the burden to the application to call Mirage_crypto_rng_....initialize / run (and not calling it in library top-level code) is definitively one option (but that has been a source of misunderstandings in the recent decade (issue reports to nocrypto etc. were mostly about that). That is how the current API came to existance, application developers are not bothered with executing initialization code. For MirageOS, the mirage utility executes the initialization function call.