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

Add an implementation of mirage-crypto-rng-miou to initialize the RNG with Miou #227

Merged
merged 12 commits into from
Jun 10, 2024

Conversation

dinosaure
Copy link
Member

This is my first attempt at implementing mirage-crypto-rng-miou. The idea of this initialization is to create a process that will periodically refill the random number generator. I've just tested a program that lasts several hours and I haven't observed any memory leakage: the tasks created to refill the generator are cleaned up properly (every second by default).

The initialization creates a witness to the promise that the user must stop before the end of the application (otherwise Miou raises the Still_has_children exception). Mirage_crypto_rng_miou.kill just cancels the task and finishes all its sub-tasks.

Finally, I've re-implemented Fortuna to be domain-friendly. This is because access may involve changes to the state of the random number generator. We need to ensure that several domains can generate random numbers without conflicts (data-race). A mutex is therefore used to protect exclusive access to the state of the generator. I will verify my implementation with tsan.

@dinosaure
Copy link
Member Author

Note for reviewers, currently a big diff between lwt/async and this initializer is about sources. lwt/async add a new source which corresponds to a function which is executed at every iterations of the scheduler (as a hook). I tried to provides with Miou such a way to register a hook. However, actually such hook can not emit an effect (Miou does not attach an effect handler on them). Due to this fact, it's impossible to run add a new source (such as G.accumulate) for the rng as a hook because these functions perform effects (mainly to lock/unlock the mutex).

@dinosaure
Copy link
Member Author

I can confirm via TSan that the current implementation Pfortuna does not generates data-races and Fortuna does.

@dinosaure dinosaure force-pushed the miou-rng branch 2 times, most recently from 10008ae to 2eb4cf5 Compare April 25, 2024 10:57
@dinosaure dinosaure marked this pull request as ready for review April 27, 2024 15:56
@dinosaure
Copy link
Member Author

I force-pushed the final version which is rechecked with TSan (I found an issue about Miou itself). The documentation was improved and the rng task can be run in parallel or concurrently depending on the number of domains available.

NOTE: On OCaml 5.1, I was unable to compile tests without the (modes native), so I added them to be able to link correctly C stubs. The error is probably related to the bytecode target and, in some ways, dune is not available to link correctly C stubs and bytecode executable.

@hannesm
Copy link
Member

hannesm commented May 6, 2024

great to see this happening, but I'm rather uncomfortable with the fork of fortuna.ml -- this has a high maintenance cost. any chance to have a single code base (is there noticable performance if we use mutex everywhere?)

@dinosaure
Copy link
Member Author

this has a high maintenance cost. any chance to have a single code base (is there noticable performance if we use mutex everywhere?)

I don't have strong opinion and we probably can just use Mutex (instead of Miou.Mutex) to be sure that the Fortuna engine is domain-safe for any schedulers. But yes, it requires some benchmarks.

@hannesm
Copy link
Member

hannesm commented May 7, 2024

But yes, it requires some benchmarks.

We have dune build --rel bench/speed.exe && _build/default/bench/speed.exe fortuna -- which generates random bytes (but there's no entropy collection happening during that phase, so it may be not the right thing).

@dinosaure
Copy link
Member Author

I got this result:

➜  mirage-crypto git:(miou-rng) ✗ dune build --rel bench/speed.exe && _build/default/bench/speed.exe fortuna pfortuna
accel: XOR AES GHASH             

* [fortuna]
       16:  220.113893 MB/s  (28879114 iters in 2.002 s)
       64:  705.788863 MB/s  (23177441 iters in 2.004 s)
      256:  2716.850044 MB/s  (22237640 iters in 1.998 s)
     1024:  5060.986346 MB/s  (10351918 iters in 1.997 s)
     8192:  5159.824384 MB/s  (1317303 iters in 1.995 s)

* [pfortuna]
       16:  65.395947 MB/s  (8762398 iters in 2.045 s)
       64:  261.715039 MB/s  (8594904 iters in 2.004 s)
      256:  867.953303 MB/s  (7247164 iters in 2.039 s)
     1024:  1246.217245 MB/s  (2548325 iters in 1.997 s)
     8192:  1758.976589 MB/s  (475333 iters in 2.111 s)

With this patch:

+  bm "pfortuna" (fun name ->
+    let open Mirage_crypto_rng_miou_unix.Pfortuna in
+    Miou_unix.run ~domains:2 @@ fun () ->
+    let rng = Mirage_crypto_rng_miou_unix.(initialize (module Pfortuna)) in
+    let g = create () in
+    reseed ~g "abcd" ;
+    throughput name (fun buf ->
+        let buf = Bytes.unsafe_of_string buf in
+        generate_into ~g buf ~off:0 (Bytes.length buf));
+    Mirage_crypto_rng_miou_unix.kill rng) ;

I deleted the initialize function - so we have no entropy collection during the benchmark and it does not affect results. I also just add a Mutex.t into the Fortuna module (as I did for Pfortuna but it uses Miou.Mutex.t) and I got the same result than pfortuna above. In other words, if we add Mutex.t or Miou.Mutex.t into the Fortuna implementation we got the pfortuna results above (and the initialize does not have an impact).

So we are ~3 times slower than the initial implementation.

@dinosaure
Copy link
Member Author

The PR is ready to be reviewed and merged 👍.

bench/dune Outdated Show resolved Hide resolved
bench/speed.ml Outdated Show resolved Hide resolved
@hannesm
Copy link
Member

hannesm commented Jun 6, 2024

looks mostly fine, I had some minor comments. thanks for your contribution.

@dinosaure
Copy link
Member Author

It seems that test_entropy.exe fails on MacOS (and it's unrelated to this PR). Otherwise, I splitted out the benchmark about pfortuna and added a comment about why we re-implemented the module.

@hannesm
Copy link
Member

hannesm commented Jun 9, 2024

It seems that test_entropy.exe fails on MacOS (and it's unrelated to this PR).

Indeed, tracked as #216

@hannesm
Copy link
Member

hannesm commented Jun 9, 2024

Thanks, I rebased on main and force-pushed, after merging #232 there was the only conflict in .cirrus.yml. Also, I added a comment to keep fortuna and pfortuna in sync :)

@hannesm
Copy link
Member

hannesm commented Jun 9, 2024

Once CI passes (see notes in #232 (comment)), I'll go ahead and squash-merge this PR.

@hannesm
Copy link
Member

hannesm commented Jun 9, 2024

Hm, I've no clue what to do with bench/miou -- marking it optional doesn't lead to the OCaml-CI not building it -- and marking it with (package mirage-crypto-rng-miou-unix) results in a dune error since there's no public_name -- but we don't want a public_name since we don't want it to be installed when installing mirage-crypto-rng-miou-unix.

Any pointers how to get out of this situation? Maybe commenting the dune rule out for the time being?

since (package mirage-crypto-rng-miou-unix) is not supported without
(public_names ..) in dune, there's no easy alternative.

Marking it (optional) still results in failures with OCaml-CI
@hannesm
Copy link
Member

hannesm commented Jun 9, 2024

Maybe commenting the dune rule out for the time being?

This is what I pushed to this PR. CI looks good. I'll squash & merge if @dinosaure is happy with this PR (esp. with my changes).

@hannesm
Copy link
Member

hannesm commented Jun 9, 2024

NOTE: On OCaml 5.1, I was unable to compile tests without the (modes native), so I added them to be able to link correctly C stubs. The error is probably related to the bytecode target and, in some ways, dune is not available to link correctly C stubs and bytecode executable.

Please note, I had this issue as well, the problem is that if you have mirage-crypto installed, it uses in bytecode mode the installed C stubs instead of the locally compiled ones... And I've a deja-vu ocaml/dune#9979

So, let's remove the (modes native). It is a dune issue, but our tests shouldn't be a scapegoat for that.

@dinosaure
Copy link
Member Author

I'm ok with your changes 👍

@hannesm hannesm merged commit a5fec37 into mirage:main Jun 10, 2024
11 of 16 checks passed
@dinosaure dinosaure deleted the miou-rng branch June 10, 2024 09:20
hannesm added a commit to hannesm/opam-repository that referenced this pull request Jun 29, 2024
CHANGES:

### Breaking changes

* mirage-crypto: Poly1305 API now uses string (mirage/mirage-crypto#203 @hannesm)
* mirage-crypto: Poly1305 no longer has type alias "type mac = string"
  (mirage/mirage-crypto#232 @hannesm)
* mirage-crypto: the API uses string instead of cstruct (mirage/mirage-crypto#214 @reynir @hannesm)
* mirage-crypto: Hash module has been removed. Use digestif if you need hash
  functions (mirage/mirage-crypto#213 @hannesm)
* mirage-crypto: the Cipher_block and Cipher_stream modules have been removed,
  its contents is inlined:
  Mirage_crypto.Cipher_block.S -> Mirage_crypto.Block
  Mirage_crypto.Cipher_stream.S -> Mirage_crypto.Stream
  Mirage_crypto.Cipher_block.AES.CTR -> Mirage_crypto.AES.CTR
  (mirage/mirage-crypto#225 @hannesm, suggested in mirage/mirage-crypto#224 by @reynir)
* mirage-crypto-pk: s-expression conversions for private and public keys (Dh,
  Dsa, Rsa) have been removed. You can use PKCS8 for encoding and decoding
  `X509.{Private,Public}_key.{en,de}code_{der,pem}` (mirage/mirage-crypto#208 @hannesm)
* mirage-crypto-pk: in the API, Cstruct.t is no longer present. Instead,
  string is used (mirage/mirage-crypto#211 @reynir @hannesm)
* mirage-crypto-rng: the API uses string instead of Cstruct.t. A new function
  `generate_into : ?g -> bytes -> ?off:int -> int -> unit` is provided
  (mirage/mirage-crypto#212 @hannesm @reynir)
* mirage-crypto-ec: remove NIST P224 support (mirage/mirage-crypto#209 @hannesm @Firobe)
* mirage-crypto: in Uncommon.xor_into the arguments ~src_off and ~dst_off are
  required now (mirage/mirage-crypto#232 @hannesm), renamed to unsafe_xor_into
  (98f01b14f5ebf98ba0e7e9c2ba97ec518f90fddc)
* mirage-crypto-pk, mirage-crypto-rng: remove type alias "type bits = int"
  (mirage/mirage-crypto#236 @hannesm)

### Bugfixes

* mirage-crypto (32 bit systems): CCM with long adata (mirage/mirage-crypto#207 @reynir)
* mirage-crypto-ec: fix K_gen for bitlen mod 8 != 0 (reported in mirage/mirage-crypto#105 that
  P521 test vectors don't pass, re-reported mirage/mirage-crypto#228, fixed mirage/mirage-crypto#230 @Firobe)
* mirage-crypto-ec: zero out bytes allocated for Field_element.zero (reported
  mirleft/ocaml-x509#167, fixed mirage/mirage-crypto#226 @dinosaure)

### Data race free

* mirage-crypto (3DES): avoid global state in key derivation (mirage/mirage-crypto#223 @hannesm)
* mirage-crypto-rng: use atomic instead of reference to be domain-safe (mirage/mirage-crypto#221
  @dinosaure @reynir @hannesm)
* mirage-crypto, mirage-crypto-rng, mirage-crypto-pk, mirage-crypto-ec:
  avoid global buffers, use freshly allocated strings/bytes instead, avoids
  data races (mirage/mirage-crypto#186 mirage/mirage-crypto#219 @dinosaure @reynir @hannesm)

### Other changes

* mirage-crypto: add {de,en}crypt_into functions (and unsafe variants) to allow
  less buffer allocations (mirage/mirage-crypto#231 @hannesm)
* mirage-crypto-rng-miou: new package which adds rng support with miou
  (mirage/mirage-crypto#227 @dinosaure)
* PERFORMANCE mirage-crypto: ChaCha20/Poly1305 use string instead of Cstruct.t,
  ChaCha20 interface unchanged, performance improvement roughly 2x
  (mirage/mirage-crypto#203 @hannesm @reynir)
* mirage-crypto-ec, mirage-crypto-pk, mirage-crypto-rng: use digestif for
  hashes (mirage/mirage-crypto#212 mirage/mirage-crypto#215 @reynir @hannesm)
* mirage-crypto-rng: use a set for entropy sources instead of a list
  (mirage/mirage-crypto#218 @hannesm)
* mirage-crypto-rng-mirage: provide a module type S (for use instead of
  mirage-random in mirage) (mirage/mirage-crypto#234 @hannesm)
avsm pushed a commit to avsm/opam-repository that referenced this pull request Sep 5, 2024
CHANGES:

### Breaking changes

* mirage-crypto: Poly1305 API now uses string (mirage/mirage-crypto#203 @hannesm)
* mirage-crypto: Poly1305 no longer has type alias "type mac = string"
  (mirage/mirage-crypto#232 @hannesm)
* mirage-crypto: the API uses string instead of cstruct (mirage/mirage-crypto#214 @reynir @hannesm)
* mirage-crypto: Hash module has been removed. Use digestif if you need hash
  functions (mirage/mirage-crypto#213 @hannesm)
* mirage-crypto: the Cipher_block and Cipher_stream modules have been removed,
  its contents is inlined:
  Mirage_crypto.Cipher_block.S -> Mirage_crypto.Block
  Mirage_crypto.Cipher_stream.S -> Mirage_crypto.Stream
  Mirage_crypto.Cipher_block.AES.CTR -> Mirage_crypto.AES.CTR
  (mirage/mirage-crypto#225 @hannesm, suggested in mirage/mirage-crypto#224 by @reynir)
* mirage-crypto-pk: s-expression conversions for private and public keys (Dh,
  Dsa, Rsa) have been removed. You can use PKCS8 for encoding and decoding
  `X509.{Private,Public}_key.{en,de}code_{der,pem}` (mirage/mirage-crypto#208 @hannesm)
* mirage-crypto-pk: in the API, Cstruct.t is no longer present. Instead,
  string is used (mirage/mirage-crypto#211 @reynir @hannesm)
* mirage-crypto-rng: the API uses string instead of Cstruct.t. A new function
  `generate_into : ?g -> bytes -> ?off:int -> int -> unit` is provided
  (mirage/mirage-crypto#212 @hannesm @reynir)
* mirage-crypto-ec: remove NIST P224 support (mirage/mirage-crypto#209 @hannesm @Firobe)
* mirage-crypto: in Uncommon.xor_into the arguments ~src_off and ~dst_off are
  required now (mirage/mirage-crypto#232 @hannesm), renamed to unsafe_xor_into
  (98f01b14f5ebf98ba0e7e9c2ba97ec518f90fddc)
* mirage-crypto-pk, mirage-crypto-rng: remove type alias "type bits = int"
  (mirage/mirage-crypto#236 @hannesm)

### Bugfixes

* mirage-crypto (32 bit systems): CCM with long adata (mirage/mirage-crypto#207 @reynir)
* mirage-crypto-ec: fix K_gen for bitlen mod 8 != 0 (reported in mirage/mirage-crypto#105 that
  P521 test vectors don't pass, re-reported mirage/mirage-crypto#228, fixed mirage/mirage-crypto#230 @Firobe)
* mirage-crypto-ec: zero out bytes allocated for Field_element.zero (reported
  mirleft/ocaml-x509#167, fixed mirage/mirage-crypto#226 @dinosaure)

### Data race free

* mirage-crypto (3DES): avoid global state in key derivation (mirage/mirage-crypto#223 @hannesm)
* mirage-crypto-rng: use atomic instead of reference to be domain-safe (mirage/mirage-crypto#221
  @dinosaure @reynir @hannesm)
* mirage-crypto, mirage-crypto-rng, mirage-crypto-pk, mirage-crypto-ec:
  avoid global buffers, use freshly allocated strings/bytes instead, avoids
  data races (mirage/mirage-crypto#186 mirage/mirage-crypto#219 @dinosaure @reynir @hannesm)

### Other changes

* mirage-crypto: add {de,en}crypt_into functions (and unsafe variants) to allow
  less buffer allocations (mirage/mirage-crypto#231 @hannesm)
* mirage-crypto-rng-miou: new package which adds rng support with miou
  (mirage/mirage-crypto#227 @dinosaure)
* PERFORMANCE mirage-crypto: ChaCha20/Poly1305 use string instead of Cstruct.t,
  ChaCha20 interface unchanged, performance improvement roughly 2x
  (mirage/mirage-crypto#203 @hannesm @reynir)
* mirage-crypto-ec, mirage-crypto-pk, mirage-crypto-rng: use digestif for
  hashes (mirage/mirage-crypto#212 mirage/mirage-crypto#215 @reynir @hannesm)
* mirage-crypto-rng: use a set for entropy sources instead of a list
  (mirage/mirage-crypto#218 @hannesm)
* mirage-crypto-rng-mirage: provide a module type S (for use instead of
  mirage-random in mirage) (mirage/mirage-crypto#234 @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.

2 participants