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

conduit-lwt-unix no longer calls Mirage_crypto_rng_unix.initialize #318

Closed
xguerin opened this issue Jun 16, 2020 · 12 comments · Fixed by ocaml/opam-repository#16697 or #320
Closed

Comments

@xguerin
Copy link

xguerin commented Jun 16, 2020

Since updating to 2.2.2 my web app based on opium fails in SSL mode with the following error:

[WARNING] Uncaught exception in handler: The default generator is not yet initialized.
...
  If you are using Lwt, execute `Mirage_crypto_rng_lwt.initialize ()` at the beginning of your event loop (`Lwt_main.run`) execution.

I just saw your CHANGES.md and have a question: whose responsibility should it be to call that Mirage_crypto_rng_lwt.initialize () function now that conduit_lwt does not call it anymore?

Thanks,

@avsm
Copy link
Member

avsm commented Jun 16, 2020

Oh dear, this is a dynamic failure indeed. I might have merged that into opam-repo too soon.

@hannesm, we could propose a PR to Lwt that also adds enter_hooks alongside the existing iter hooks and exit hooks. Would that remove the need for every cohttp user to add this to their Lwt main?

@hannesm
Copy link
Member

hannesm commented Jun 16, 2020

propose a PR to Lwt that also adds enter_hooks

@avsm that is a possibility.

I might have merged that into opam-repo too soon.

for a sound entropy experience, I don't see an option right now how to initialize the rng in a library.

@xguerin the responsibility is that the application should call Mirage_crypto_rng_lwt.initialize () in Lwt_main.run. This is nothing a library can do (and using Mirage_crypto_rng_unix.initialize () -- as previously done by conduit -- does not give the same result, i.e. no entropy collection).

@hannesm hannesm closed this as completed Jun 16, 2020
@hannesm hannesm reopened this Jun 16, 2020
@hannesm
Copy link
Member

hannesm commented Jun 16, 2020

sorry, had a mouse issue and closed prematurely.

@xguerin
Copy link
Author

xguerin commented Jun 16, 2020

Thanks for the clarification.

@avsm
Copy link
Member

avsm commented Jun 16, 2020

As a workaround for now, we could use the Lwt_main enter iter hooks to do something on the lines of

let register_hook () =
  let crypto_init = ref false in
  let enter_hook () =
    if not !crypto_init then
      Mirage_crypto_rng_unix.initialize ()
  in
  Lwt_main.Enter_iter_hooks.add_first enter_hook

Although, this is roughly what mirage-crypto-rng-lwt is already doing in:
https://github.com/mirage/mirage-crypto/blob/master/rng/lwt/mirage_crypto_rng_lwt.ml#L58

Why does it need to be run within Lwt_main? Is it because multiple invocations of it warn, and so it's bad for a library to do this?

@hannesm
Copy link
Member

hannesm commented Jun 16, 2020

Why does it need to be run within Lwt_main?

ah, you're right - a solution may be to move Mirage_crypto_rng_lwt.initialize out of the Lwt monad, and call it from toplevel in conduit-lwt-unix. How does Lwt.async behave when not executed within the Lwt monad?

Is it because multiple invocations of it warn, and so it's bad for a library to do this?

it is best to have only one entropy collection periodic task running for a single default rng.

@avsm
Copy link
Member

avsm commented Jun 16, 2020

Right, I definitely agree that just one entropy collection task is appropriate. However, calling the initialiser multiple times could be idempotent. The reason is that we can stash the enter hook that returns from Lwt_main.Enter_iter_hooks and not take any action if one is already registered.

This way a toplevel initialiser in conduit-lwt-unix would be safe. Otherwise its not, as multiple libraries trying to initialise would result in link time warnings. Lwt.async outside the main loop looks fine -- in general in Lwt, everything just runs to "blocking" and is then woken up by the main loop, so the only unsafe thing is to nest Lwt_main.run calls.

avsm added a commit to avsm/opam-repository that referenced this issue Jun 16, 2020
This causes a number of upstream users of cohttp-lwt-unix to
dynamically fail. Fix being discussed in mirage/ocaml-conduit#318
so disabling package for now.
@hannesm
Copy link
Member

hannesm commented Jun 17, 2020

as update, conduit-lwt-unix 2.2.2 was disabled (in ocaml/opam-repository#16648).

I came up with the following snippet:

open Lwt.Infix

let () =
  Lwt.async (fun () ->
      let rec go i =
        Printf.printf "async %d\n%!" i;
        Lwt_unix.sleep 1. >>= fun () ->
        go (succ i)
      in
      go 0);
  let foo = ref 0 in
  let _ = Lwt_main.Enter_iter_hooks.add_first
      (fun () -> Printf.printf "entering %d\n%!" !foo; incr foo)
  in
  Printf.printf "now starting main\n%!";
  Lwt_main.run (Lwt_unix.sleep 10.);
  Printf.printf "now main stopped\n%!";
  Unix.sleepf 3.;
  Printf.printf "now starting main again\n%!";
  Lwt_main.run (Lwt_unix.sleep 10.)

which (both native and bytecode) behaves as follows:

async 0
now starting main
entering 0
async 1
entering 1
async 2
entering 2
async 3
entering 3
async 4
entering 4
async 5
entering 5
async 6
entering 6
async 7
entering 7
async 8
entering 8
async 9
entering 9
now main stopped
now starting main again
entering 10
async 10
entering 11
async 11
entering 12
async 12
entering 13
async 13
entering 14
async 14
entering 15
async 15
entering 16
async 16
entering 17
async 17
entering 18
async 18
entering 19
async 19
entering 20

I hope that behaviour is intentional (and guaranteed by lwt). If this is the case, I'd suggest to (a) modify Mirage_crypto_rng_lwt.initialize to not be in the Lwt monad (since it only registers async and Enter_iter_hooks, there's no need to be in the monad) and (b) call that function from top-level in conduit-lwt-unix (will depend on a mirage-crypto-rng release).

Does this sound good to you? //cc @avsm

@avsm
Copy link
Member

avsm commented Jun 17, 2020

This sounds great to me @hannesm!

@talex5
Copy link
Contributor

talex5 commented Jun 17, 2020

This will also be helpful with capnp-rpc-unix (it also calls Mirage_crypto_rng_unix.initialize).

hannesm added a commit to hannesm/opam-repository that referenced this issue Jun 18, 2020
…mirage-crypto-rng-mirage (0.8.0)

CHANGES:

* New package mirage-crypto-rng-mirage which contains the entropy collection
  code for MirageOS (mirage/mirage-crypto#69 requested by @samoht, implemented by @hannesm)
* Mirage_crypto_rng_lwt.initialize is not inside the Lwt monad anymore, and
  thus can be called by libraries at top level (mirage/mirage-crypto#69, requested by @avsm @xguerin
  @talex5 in mirage/ocaml-conduit#318, implemented by @hannesm)
* Both Mirage_crypto_rng_lwt.initialize and Mirage_crypto_rng_unix.initialize
  don't do anything if called a second time (mirage/mirage-crypto#69, implemented by @hannesm)
* Entropy source registration is now open and done via
  `Entropy.register_source : string -> source`, instead of a closed variant
  (mirage/mirage-crypto#69, fixes mirage/mirage-crypto#68, implemented by @hannesm)
@hannesm
Copy link
Member

hannesm commented Jun 20, 2020

updates: mirage-crypto-rng 0.8 is released where the Mirage_crypto_rng_lwt.initialize () no longer is inside the lwt monad.
additionally, ocaml/opam-repository#16682 releases tls 0.12.2 which again (similar to 0.12.0 and before) initializes the rng at the toplevel of tls.lwt. also marks 0.12.1 as unavailable to avoid the subtle changes in semantics.

since conduit-lwt-unix in Conduit_lwt_tls_real uses tls.lwt, we don't need further conduit changes (no need to initialize the rng in here), but can (a) bump the tls conflict to < 0.12.2 (b) mark conduit-lwt-unix 2.2.2 as available in opam repository (once tls 0.12.2 is in). WDYT?

@avsm
Copy link
Member

avsm commented Jun 20, 2020

Thanks @hannesm, this is excellent. I'll mark conduit-lwt-unix as available once tls 0.12.2 is merged. @talex5 @xguerin this should then continue to work with opium and capnp-rpc without you having to do any changes. Please let us know if this is not the case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants