-
Notifications
You must be signed in to change notification settings - Fork 43
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
Conversation
Thanks for your work. I've no good understanding of "eio", but doesn't that provide tasks / threads or effects and timers? Wouldn't it be more sensible to integrate a continuous (periodic) reseeding approach, as done in lwt and mirage and async implementations? |
Indeed |
And while at it, please describe in more detail what let rand_source = Eio.Stdenv.secure_random env in
let got = Eio.Flow.read rand_source buf in Does -- i.e. where does it take its entropy from? Does it work for arbitrary big buffers Also, the CI seems to fail "cannot find package eio". |
For For other OS, the entropy source is as defined in libuv lib - http://docs.libuv.org/en/v1.x/misc.html#c.uv_random The buffers can be abitrary/user-defined size.
The eio package is here - https://opam.ocaml.org/packages/eio/. Perhaps the opam repo needs to be updated? |
Hmm, looks like there are multiple issues:
There are differences between these two C implementations, esp. if getrandom returned a short read.
shrug maybe
That sounds good.
Ok - please have a look and report back (this is a blocker for merging).
Sorry, I don't understand "perhaps the opam repo needs to be updated". The "ocaml-ci" run here now only executes on the "debian-11-4.12+domains" (and reutrns (failed: Library "eio" not found.)). This is a regression to earlier runs, where lots of compilers and platforms were used. Maybe an issue with ocaml-ci (with multiple opam packages in a ssingle repository), and definitively a blocker to merge this PR. I'm sorry I won't have time to look into further details before mid May (or even early June), |
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.
Thanks - I've filed an issue about EINTR with getrandom ocaml-multicore/eio#211.
The "ocaml-ci" run here now only executes on the "debian-11-4.12+domains"
Yes, this is a problem with the CI. I think we should fix this before trying to upstream any eio support (ocurrent/ocaml-ci#297).
@bikallem: it might be convenient to have a Mirage_crypto_rng_eio.run
function, to avoid the need for a switch here. e.g.
Eio_main.run @@ fun env ->
Mirage_crypto_rng_eio.run env @@ fun () ->
...
You might want to set running := false
when the periodic thread finishes (is cancelled) too, if you want to be very tidy (might be useful for unit-tests?).
@talex5 with regards to handling EINTR for getrandom, this has to be done separately in both |
I was thinking of something like this: let run ?(sleep = Duration.of_sec 1) env fn =
if !running then (
Log.debug
(fun m -> m "Mirage_crypto_rng_eio.initialize has already been called, \
ignoring this call.");
fn ()
) else (
running := true;
Fun.protect ~finally:(fun () -> running := false) @@ fun () ->
(try
let _ = default_generator () in
Log.warn (fun m -> m "Mirage_crypto_rng.default_generator has already \
been set, check that this call is intentional");
with
No_default_generator -> ());
let seed =
let init =
Entropy.[ bootstrap ; whirlwind_bootstrap ; bootstrap ; getrandom_init env ] in
List.mapi (fun i f -> f i) init |> Cstruct.concat
in
let rng = create ~seed ~time:Mtime_clock.elapsed_ns (module Fortuna) in
set_default_generator rng;
let source = Entropy.register_source "getrandom" in (* xxx: no way to unregister? *)
let feed_entropy () = periodically_feed_entropy env (Int64.mul sleep 10L) source in
Eio.Fiber.any (rdrand_tasks env sleep @ [feed_entropy; fn])
) I used |
I was thinking more in terms of API ergonomics not having to do the following:
Perhaps, a version of |
You need to know when the user's main function returns so that the rng fiber can be stopped too. let () =
Eio_main.run @@ fun env ->
Mirage_crypto_rng_eio.run env @@ fun () ->
main () (* Can use mirage_crypto in here *) |
Ah, yes. I get it now. The API looks nice |
3b9cab4
to
df02d5d
Compare
EINTR is now handled correctly by eio. ocaml-multicore/eio#212 |
Thanks for your work. I did not follow the discusion, but as you may see the CI is not happy. And as remarked earlier, please make sure that the CI continues to run on x86_32 etc. (see https://github.com/mirage/mirage-crypto/runs/5740484133 for what the CI should run on).
You may need to cut a release of In the current state this is not mergeable. I will try to look into this PR again (likely) early June. |
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.
This is a regression to earlier runs, where lots of compilers and platforms were used. Maybe an issue with ocaml-ci (with multiple opam packages in a ssingle repository), and definitively a blocker to merge this PR.
I deployed a fix for this (ocurrent/ocaml-ci#468). However, it's not accepted into ocaml-ci yet, so might get reverted.
6dd9b26
to
05ee8cf
Compare
please remove the trailing whitespaces all over (I started to comment on them, but there are too many). |
I took the liberty to (a) remove the trailing whitespaces (b) unset the default generator when What is left to do:
|
|
@bikallem uhm, but isn't the |
@hannesm I have moved |
4dae18c
to
7dcc9c8
Compare
@hannesm Are there any more issues to be addressed? |
Thanks, I merged this. Release will happen shortly. |
…age, mirage-crypto-rng-eio, mirage-crypto-rng-async, mirage-crypto-pk and mirage-crypto-ec (0.10.7) CHANGES: - mirage-crypto-rng-eio: new package for seeding and feeding entropy to the rng with eio (mirage/mirage-crypto#155 @bikallem, @talex5, @hannesm) - mirage-crypto-ec: expose Dsa.byte_length (mirage/mirage-crypto#164 @hannesm) - CI: various fixes (mirage/mirage-crypto#154 mirage/mirage-crypto#164 @hannesm) - mirage-crypto-rng-mirage: use 'a generator type alias - mirage-crypto-rng: improve setup_rng message (add async, revise lwt) (mirage/mirage-crypto#161 @hannesm) - mirage-crypto-rng-mirage: always feed the default generator (as done in a8c7bbd2552a9d2177450e95f280342f80fba01d for the lwt feeding) (mirage/mirage-crypto#161 @hannesm) - ec: update generated code to recent fiat-crypto (mirage/mirage-crypto#156 @hannesm)
ocaml-compiler: ${{ matrix.ocaml-version }} | ||
|
||
- name: Install dependencies | ||
run: opam install --deps-only -t . | ||
run: opam install --deps-only -t mirage-crypto mirage-crypto-rng mirage-crypto-rng-mirage mirage-crypto-pk mirage-crypto-ec mirage-crypto-rng-async |
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.
…age, mirage-crypto-rng-lwt, mirage-crypto-rng-eio, mirage-crypto-rng-async, mirage-crypto-pk and mirage-crypto-ec (0.11.0) CHANGES: - BREAKING split mirage-crypto-rng-lwt away from mirage-crypto-rng (mirage/mirage-crypto#168 @hannesm, reported by @bikallem mirage/mirage-crypto#158) - AEAD API improvements: provide tag_size, of_secret, and functions that deal with the tag separately (mirage/mirage-crypto#171 @hannesm, fixes mirage/mirage-crypto#74 mirage/mirage-crypto#144 @orbitz @anmonteiro) Only CCM16 (with tag size 16) is now exposed, the former API does not exist anymore (passing `~maclen` to `of_secret`), according to sherlocode the only usage was CCM16 anyways - BREAKING unify RNG initialization (reported by @talex5 in mirage/mirage-crypto#155, fixes mirage/mirage-crypto#160, PR mirage/mirage-crypto#162 @hannesm) - remove mirage 3 cross-compilation runes (mirage/mirage-crypto#163 @hannesm) - CI: mirage-crypto-rng-eio requires ocaml 5 and dune 2.7 (mirage/mirage-crypto#170 @hannesm, fixes mirage/mirage-crypto#169 thanks to @bikallem @talex5) - CI: use miage 4 (mirage/mirage-crypto#166 @hannesm)
…age, mirage-crypto-rng-lwt, mirage-crypto-rng-eio, mirage-crypto-rng-async, mirage-crypto-pk and mirage-crypto-ec (0.11.0) CHANGES: - BREAKING split mirage-crypto-rng-lwt away from mirage-crypto-rng (mirage/mirage-crypto#168 @hannesm, reported by @bikallem mirage/mirage-crypto#158) - AEAD API improvements: provide tag_size, of_secret, and functions that deal with the tag separately (mirage/mirage-crypto#171 @hannesm, fixes mirage/mirage-crypto#74 mirage/mirage-crypto#144 @orbitz @anmonteiro) Only CCM16 (with tag size 16) is now exposed, the former API does not exist anymore (passing `~maclen` to `of_secret`), according to sherlocode the only usage was CCM16 anyways - BREAKING unify RNG initialization (reported by @talex5 in mirage/mirage-crypto#155, fixes mirage/mirage-crypto#160, PR mirage/mirage-crypto#162 @hannesm) - remove mirage 3 cross-compilation runes (mirage/mirage-crypto#163 @hannesm) - CI: mirage-crypto-rng-eio requires ocaml 5 and dune 2.7 (mirage/mirage-crypto#170 @hannesm, fixes mirage/mirage-crypto#169 thanks to @bikallem @talex5) - CI: use miage 4 (mirage/mirage-crypto#166 @hannesm)
…age, mirage-crypto-rng-lwt, mirage-crypto-rng-eio, mirage-crypto-rng-async, mirage-crypto-pk and mirage-crypto-ec (0.11.0) CHANGES: - BREAKING split mirage-crypto-rng-lwt away from mirage-crypto-rng (mirage/mirage-crypto#168 @hannesm, reported by @bikallem mirage/mirage-crypto#158) - AEAD API improvements: provide tag_size, of_secret, and functions that deal with the tag separately (mirage/mirage-crypto#171 @hannesm, fixes mirage/mirage-crypto#74 mirage/mirage-crypto#144 @orbitz @anmonteiro) Only CCM16 (with tag size 16) is now exposed, the former API does not exist anymore (passing `~maclen` to `of_secret`), according to sherlocode the only usage was CCM16 anyways - BREAKING unify RNG initialization (reported by @talex5 in mirage/mirage-crypto#155, fixes mirage/mirage-crypto#160, PR mirage/mirage-crypto#162 @hannesm) - remove mirage 3 cross-compilation runes (mirage/mirage-crypto#163 @hannesm) - CI: mirage-crypto-rng-eio requires ocaml 5 and dune 2.7 (mirage/mirage-crypto#170 @hannesm, fixes mirage/mirage-crypto#169 thanks to @bikallem @talex5) - CI: use miage 4 (mirage/mirage-crypto#166 @hannesm)
…age, mirage-crypto-rng-lwt, mirage-crypto-rng-eio, mirage-crypto-rng-async, mirage-crypto-pk and mirage-crypto-ec (0.11.0) CHANGES: - BREAKING split mirage-crypto-rng-lwt away from mirage-crypto-rng (mirage/mirage-crypto#168 @hannesm, reported by @bikallem mirage/mirage-crypto#158) This means, a "mirage-crypto-rng.lwt" should now be "mirage-crypto-rng-lwt" in your dune file (or in META requires, or in _tags). - AEAD API improvements: provide tag_size, of_secret, and functions that deal with the tag separately (mirage/mirage-crypto#171 @hannesm, fixes mirage/mirage-crypto#74 mirage/mirage-crypto#144 @orbitz @anmonteiro) Only CCM16 (with tag size 16) is now exposed, the former API does not exist anymore (passing `~maclen` to `of_secret`), according to sherlocode the only usage was CCM16 anyways This means any "Mirage_crypto.AES.CCM" should now be "Mirage_crypto.AES.CCM16" and any "CCM.of_secret ~maclen:16 key" should now be "CCM16.of_secret key" Any occurrence of "Mirage_crypto.Cipher_block.S.CCM" should now be "Mirage_crypto.Cipher_block.S.CCM16" - BREAKING unify RNG initialization (reported by @talex5 in mirage/mirage-crypto#155, fixes mirage/mirage-crypto#160, PR mirage/mirage-crypto#162 @hannesm) This means: - "Mirage_crypto_rng_lwt.initialize ()" should now be "Mirage_crypto_rng_lwt.initialize (module Mirage_crypto_rng.Fortuna)" - "Mirage_crypto_rng_unix.initialize ()" should now be "Mirage_crypto_rng_unix.initialize (module Mirage_crypto_rng.Fortuna)" - remove mirage 3 cross-compilation runes (mirage/mirage-crypto#163 @hannesm) - CI: mirage-crypto-rng-eio requires ocaml 5 and dune 2.7 (mirage/mirage-crypto#170 @hannesm, fixes mirage/mirage-crypto#169 thanks to @bikallem @talex5) - CI: use miage 4 (mirage/mirage-crypto#166 @hannesm)
…age, mirage-crypto-rng-lwt, mirage-crypto-rng-eio, mirage-crypto-rng-async, mirage-crypto-pk and mirage-crypto-ec (0.11.0) CHANGES: - BREAKING split mirage-crypto-rng-lwt away from mirage-crypto-rng (mirage/mirage-crypto#168 @hannesm, reported by @bikallem mirage/mirage-crypto#158) This means, a "mirage-crypto-rng.lwt" should now be "mirage-crypto-rng-lwt" in your dune file (or in META requires, or in _tags). - AEAD API improvements: provide tag_size, of_secret, and functions that deal with the tag separately (mirage/mirage-crypto#171 @hannesm, fixes mirage/mirage-crypto#74 mirage/mirage-crypto#144 @orbitz @anmonteiro) Only CCM16 (with tag size 16) is now exposed, the former API does not exist anymore (passing `~maclen` to `of_secret`), according to sherlocode the only usage was CCM16 anyways This means any "Mirage_crypto.AES.CCM" should now be "Mirage_crypto.AES.CCM16" and any "CCM.of_secret ~maclen:16 key" should now be "CCM16.of_secret key" Any occurrence of "Mirage_crypto.Cipher_block.S.CCM" should now be "Mirage_crypto.Cipher_block.S.CCM16" - BREAKING unify RNG initialization (reported by @talex5 in mirage/mirage-crypto#155, fixes mirage/mirage-crypto#160, PR mirage/mirage-crypto#162 @hannesm) This means: - "Mirage_crypto_rng_lwt.initialize ()" should now be "Mirage_crypto_rng_lwt.initialize (module Mirage_crypto_rng.Fortuna)" - "Mirage_crypto_rng_unix.initialize ()" should now be "Mirage_crypto_rng_unix.initialize (module Mirage_crypto_rng.Fortuna)" - remove mirage 3 cross-compilation runes (mirage/mirage-crypto#163 @hannesm) - CI: mirage-crypto-rng-eio requires ocaml 5 and dune 2.7 (mirage/mirage-crypto#170 @hannesm, fixes mirage/mirage-crypto#169 thanks to @bikallem @talex5) - CI: use miage 4 (mirage/mirage-crypto#166 @hannesm)
…age, mirage-crypto-rng-lwt, mirage-crypto-rng-eio, mirage-crypto-rng-async, mirage-crypto-pk and mirage-crypto-ec (0.11.0) CHANGES: - BREAKING split mirage-crypto-rng-lwt away from mirage-crypto-rng (mirage/mirage-crypto#168 @hannesm, reported by @bikallem mirage/mirage-crypto#158) This means, a "mirage-crypto-rng.lwt" should now be "mirage-crypto-rng-lwt" in your dune file (or in META requires, or in _tags). - AEAD API improvements: provide tag_size, of_secret, and functions that deal with the tag separately (mirage/mirage-crypto#171 @hannesm, fixes mirage/mirage-crypto#74 mirage/mirage-crypto#144 @orbitz @anmonteiro) Only CCM16 (with tag size 16) is now exposed, the former API does not exist anymore (passing `~maclen` to `of_secret`), according to sherlocode the only usage was CCM16 anyways This means any "Mirage_crypto.AES.CCM" should now be "Mirage_crypto.AES.CCM16" and any "CCM.of_secret ~maclen:16 key" should now be "CCM16.of_secret key" Any occurrence of "Mirage_crypto.Cipher_block.S.CCM" should now be "Mirage_crypto.Cipher_block.S.CCM16" - BREAKING unify RNG initialization (reported by @talex5 in mirage/mirage-crypto#155, fixes mirage/mirage-crypto#160, PR mirage/mirage-crypto#162 @hannesm) This means: - "Mirage_crypto_rng_lwt.initialize ()" should now be "Mirage_crypto_rng_lwt.initialize (module Mirage_crypto_rng.Fortuna)" - "Mirage_crypto_rng_unix.initialize ()" should now be "Mirage_crypto_rng_unix.initialize (module Mirage_crypto_rng.Fortuna)" - remove mirage 3 cross-compilation runes (mirage/mirage-crypto#163 @hannesm) - CI: mirage-crypto-rng-eio requires ocaml 5 and dune 2.7 (mirage/mirage-crypto#170 @hannesm, fixes mirage/mirage-crypto#169 thanks to @bikallem @talex5) - CI: use miage 4 (mirage/mirage-crypto#166 @hannesm)
This PR introduces a new opam package
mirage-crypto-rng-eio
. The purpose of this package is to implement an eio dependent random number generator.It closely follows
mirage-crypto-rng.unix
library.