-
Notifications
You must be signed in to change notification settings - Fork 44
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
Conversation
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 |
I can confirm via TSan that the current implementation |
10008ae
to
2eb4cf5
Compare
I force-pushed the final version which is rechecked with TSan (I found an issue about Miou itself). The documentation was improved and the NOTE: On OCaml 5.1, I was unable to compile tests without the |
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?) |
I don't have strong opinion and we probably can just use |
We have |
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 So we are ~3 times slower than the initial implementation. |
The PR is ready to be reviewed and merged 👍. |
looks mostly fine, I had some minor comments. thanks for your contribution. |
It seems that |
Indeed, tracked as #216 |
…l flow to set correctly global variables
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 :) |
Once CI passes (see notes in #232 (comment)), I'll go ahead and squash-merge this PR. |
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 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
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). |
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 |
I'm ok with your changes 👍 |
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)
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)
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 withtsan
.