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

mirage-crypto-rng-async #90

Merged
merged 18 commits into from Dec 7, 2020
Merged

mirage-crypto-rng-async #90

merged 18 commits into from Dec 7, 2020

Conversation

seliopou
Copy link
Contributor

This is an implementation of the async-analogue of Mirage_crypto_rng_lwt. It's exposes a single initialize function that schedules several periodic jobs to collect entropy from the system and feed it into the Entropy module for use by the PRNGs.

I haven't tested this yet, but wanted to throw this up here to gauge whether this would be a welcome contribution. If this looks good, I will follow it up with getting @dinosaure's async ocaml-tls implementation into a state where it can be merged into that project as well.

Copy link
Member

@hannesm hannesm left a comment

Choose a reason for hiding this comment

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

Thanks for your PR.
I have not used async (yet) - this looks like a good addition. I'd be happy to merge this PR (apart from the question about blocking behaviour of getrandom).

rng/async/mirage_crypto_rng_async.ml Outdated Show resolved Hide resolved
rng/async/mirage_crypto_rng_async.ml Outdated Show resolved Hide resolved
If that code is reached then the random source has been initialized it
will never block.
@seliopou
Copy link
Contributor Author

So a few things to deal with: the win32 build is broken and, reading .github/test.yml, I'm not sure exactly why. Second, some testing. I created this gist just to verify that initialization was unique across runs and timers were being scheduled properly.

@dinosaure
Copy link
Member

async does not support Windows, you should may be update this line:

opam install -t --deps-only .

@hannesm
Copy link
Member

hannesm commented Nov 17, 2020

@seliopou

So a few things to deal with: the win32 build is broken and, reading .github/test.yml, I'm not sure exactly why.

Took me a while to figure out what the issue is. Turns out, the windows runner uses a different opam-repository (https://github.com/fdopen/opam-repository-mingw/) - where async is marked as unavailable on windows. To have the CI succeed, I guess all that is needed is to specify available: os != "win32" in mirage-crypto-rng-async.

Second, some testing. I created this gist just to verify that initialization was unique across runs and timers were being scheduled properly.

That looks good. There is test_entropy.ml which checks that individual entropy sources do not output identical data. There's as well test_entropy_collection which "tests" the mirage-crypto-rng-mirage implementation (there's not really a test unfortunately).

For lwt (and async), it would be good to have tests which validate "that initialization was unique across runs and timers were being scheduled properly" (but I haven't bothered developing such a test so far). Your gist looks good, it may be worth to put it in the test subdirectory.

Synchronous_time_source.run_at_intervals
time_source
span
(fun () -> Entropy.cpu_rng None)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

On a RaspberryPI 2, this doesn't collect any entropy. Somewhat orthogonal to this PR, but it might be nice to expose something like val Entropy.cpu_native_support : bool so that implementations can check if it's worthwhile creating the timer.

@seliopou
Copy link
Contributor Author

seliopou commented Dec 5, 2020

I updated the interface to make it easier to write the entropy collection test. Assuming CI passes, I think this is good to go?

Though, see my note about cpu native entropy sources inline.

@hannesm hannesm merged commit ab6c5f3 into mirage:master Dec 7, 2020
@hannesm
Copy link
Member

hannesm commented Dec 7, 2020

thanks spiros, I squash-merged this to master :)

kit-ty-kate pushed a commit to ocaml/opam-repository that referenced this pull request Jan 4, 2021
…age-crypto-rng-mirage and mirage-crypto-rng-async (0.8.8)

CHANGES:

- new package mirage-crypto-rng-async, entropy feeding using async (mirage/mirage-crypto#90 @seliopou)
- Entropy.cpu_rng and Entropy.cpu_rng_bootstrap result in Error `Not_supported
  on CPUs without RDRAND/RDSEED support (previously an exception was raised
  in cpu_rng_bootstrap, and cpu_rng resulted in a no-op) (mirage/mirage-crypto#92 @seliopou)
- Entropy.cpu_rng delays entropy feeding (returns unit -> unit instead of unit).
  This fixes a memory leak, reported by @talex5 mirage/mirage-crypto#94, fixed in mirage/mirage-crypto#95 by @hannesm
- Avoid illegal instructions on X86 CPUs without SSSE3 instruction set. Both
  SHA256 and ChaCha used PSHUFB which is not available on e.g. AMD Phenom II
  (report mirage/mirage-crypto#93 by @dinosaure @samoht @pirbo @RichAyotte @sebeec, fixed in mirage/mirage-crypto#96 by
  @hannesm)
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.

None yet

3 participants