Add the miou implementation by dinosaure · Pull Request #503 · mirleft/ocaml-tls

dinosaure · 2024-08-20T13:02:27Z

#443 rebased with string. /cc @hannesm feel free to remove the execution of the fuzzer which can take a long time.

tls-miou-unix.opam

hannesm · 2024-08-20T14:07:58Z

looks great apart from minor comments.

dinosaure · 2024-08-20T15:46:29Z

miou/tests/dune

@@ -0,0 +1,12 @@
+(test


Not sure that you want to execute this executable every time, it takes a very long time.

how long is "very long"? And what are the options? marking it as executable? then we can't attach it to a package without a public_name...

how long is "very long"?

2mins on my (good) machine.

And what are the options? marking it as executable? then we can't attach it to a package without a public_name...

We can set it as a simple executable if the test bother you too much (because it takes to much time).

I'm fine leaving it as test. I usually develop in ocaml 4 land where I don't have miou anyways, and I prefer to have tests that are useful enabled and running (otherwise we hit the "what was the last time that feedback.exe worked" again).

hannesm · 2024-08-21T15:30:16Z

This looks fine to merge, but:

When given the input:


    ["\203\171\240\014\003\191\176Q\174TZ\212\134\127&\171\029\0315}\1720I\207\132\156\172\238:gy\157\214\180\152\003\204T\161kT\016\224$O\224\183@2\140\142\004\146\155\022]\177\185i\163\229\218u;";
     [[[#1 client-to-server;
        ">8\127%\158\206\218\161u\131T\141\\&\217\178\016>q\028\1402)"]];
      ([#3 noop; #0 close]) => [[[#1 client-to-server;
                                  "\229g\229,`yr\172\252\217\209\026\243\006\223\030\207\n!\175>\174R\185{1\172\255O\215\025\194$7\159f\221\166dA\191\253\173<^\135/!\154\169\132H\202f\024\015\173x?U\135\232Fr"]];
                                ([#0 close; #1 shutdown-recv]) => [[[#1 client-to-server;
                                                                    "\244,#gnm\028\244\241(\144\131v\006\157Z\236\191\243\252\020*\027$\168y\235\154\174\128\031=~\251\235\180\192\184\163\243\170\222\173\171\140\166{\170G\152\135vIa\149\227\194\rD\180C\135L\136"];
                                                                    [?; ?]];
                                                                   ([#0 close;
                                                                    #1 shutdown-recv]) => 
                                                                   [[[?; ?]];
                                                                    (
                                                                    [?; ?]) => 
                                                                    [[[?; ?]];
                                                                    ([?; ?]) => ?]]]]]]
    
the test failed:


    [ERROR] Unexpected result: Ok [] & Error
                                     Unix.Unix_error(Unix.EMFILE, "socket", "")

so is there by chance somewhere a socket leak? or can we reduce the number of sockets being opened?

dinosaure · 2024-08-21T15:41:45Z

so is there by chance somewhere a socket leak? or can we reduce the number of sockets being opened?

I'm currently on it, due to the random path produced by the fuzzer and taken by the state machine, it's not sure that all sockets are correctly closed. I will ping you back and I will fix that.

Co-authored-by: Hannes Mehnert <hannes@mehnert.org>

hannesm · 2024-08-21T22:05:35Z

Thanks, I rebased on the main branch and added 6328427. This works nicely on my laptop :)

@art-w

CHANGES: * API breaking change: remove usage of Cstruct.t inside of TLS, use bytes and string instead (mirleft/ocaml-tls#497 by @art-w, @hannesm, @dinosaure, @reynir) Performance is up to 3x improved (bandwidth), 2x improvement for handshake/s on an Intel Core(TM) i7-5600U CPU @ 2.60GHz * FEATURE: add tls-miou-unix package, which adds miou support for TLS (mirleft/ocaml-tls#494 mirleft/ocaml-tls#503 @dinosaure) * FEATURE: tls-lwt and tls-async: allow TLS over an existing connection `Tls_lwt.client_of_channels : Tls.Config.client -> ?host:[`host] Domain_name.t -> Lwt_io.input_channel * Lwt_io.output_channel -> t Lwt.t` and `Tls_lwt.server_of_channels : Tls.Config.server -> Lwt_io.input_channel * Lwt_io.output_channel -> t Lwt.t` (mirleft/ocaml-tls#499 @art-w @MisterDA) * API breaking changes: revise errors - reduce the polymorphic variant in size, align it with RFC specified errors, be in parts more precise about errors, in other parts skip data (mirleft/ocaml-tls#505, @hannesm - fixes mirleft/ocaml-tls#491) NB: if you relied on a specific error constructor, please open an issue * Remove unused constructors from Packet.{alert_type, compression_methods, client_certificate_type, extension_type} (mirleft/ocaml-tls#505, @hannesm) NB: if you relied on specific constructors, please open an issue * API breaking change: Tls.Config.{server,client} now return a result type instead of raising an exception (mirleft/ocaml-tls#502, @hannesm, fixes mirleft/ocaml-tls#411) * FEATURE: add bench/speed.exe, a benchmark for bandwidth (for different ciphersuites) and handshakes (different key exchanges and private keys) (mirleft/ocaml-tls#500 @hannesm @dinosaure @reynir) * BUGFIX: tests/feedback.exe update with TLS 1.3 semantics, run as test (mirleft/ocaml-tls#501, @hannesm - reported by @dinosaure)

@art-w

CHANGES: * API breaking change: remove usage of Cstruct.t inside of TLS, use bytes and string instead (mirleft/ocaml-tls#497 by @art-w, @hannesm, @dinosaure, @reynir) Performance is up to 3x improved (bandwidth), 2x improvement for handshake/s on an Intel Core(TM) i7-5600U CPU @ 2.60GHz * FEATURE: add tls-miou-unix package, which adds miou support for TLS (mirleft/ocaml-tls#494 mirleft/ocaml-tls#503 @dinosaure) * FEATURE: tls-lwt and tls-async: allow TLS over an existing connection `Tls_lwt.client_of_channels : Tls.Config.client -> ?host:[`host] Domain_name.t -> Lwt_io.input_channel * Lwt_io.output_channel -> t Lwt.t` and `Tls_lwt.server_of_channels : Tls.Config.server -> Lwt_io.input_channel * Lwt_io.output_channel -> t Lwt.t` (mirleft/ocaml-tls#499 @art-w @MisterDA) * API breaking changes: revise errors - reduce the polymorphic variant in size, align it with RFC specified errors, be in parts more precise about errors, in other parts skip data (mirleft/ocaml-tls#505, @hannesm - fixes mirleft/ocaml-tls#491) NB: if you relied on a specific error constructor, please open an issue * Remove unused constructors from Packet.{alert_type, compression_methods, client_certificate_type, extension_type} (mirleft/ocaml-tls#505, @hannesm) NB: if you relied on specific constructors, please open an issue * API breaking change: Tls.Config.{server,client} now return a result type instead of raising an exception (mirleft/ocaml-tls#502, @hannesm, fixes mirleft/ocaml-tls#411) * FEATURE: add bench/speed.exe, a benchmark for bandwidth (for different ciphersuites) and handshakes (different key exchanges and private keys) (mirleft/ocaml-tls#500 @hannesm @dinosaure @reynir) * BUGFIX: tests/feedback.exe update with TLS 1.3 semantics, run as test (mirleft/ocaml-tls#501, @hannesm - reported by @dinosaure)

@art-w

CHANGES: * API breaking change: remove usage of Cstruct.t inside of TLS, use bytes and string instead (mirleft/ocaml-tls#497 by @art-w, @hannesm, @dinosaure, @reynir) Performance is up to 3x improved (bandwidth), 2x improvement for handshake/s on an Intel Core(TM) i7-5600U CPU @ 2.60GHz * FEATURE: add tls-miou-unix package, which adds miou support for TLS (mirleft/ocaml-tls#494 mirleft/ocaml-tls#503 @dinosaure) * FEATURE: tls-lwt and tls-async: allow TLS over an existing connection `Tls_lwt.client_of_channels : Tls.Config.client -> ?host:[`host] Domain_name.t -> Lwt_io.input_channel * Lwt_io.output_channel -> t Lwt.t` and `Tls_lwt.server_of_channels : Tls.Config.server -> Lwt_io.input_channel * Lwt_io.output_channel -> t Lwt.t` (mirleft/ocaml-tls#499 @art-w @MisterDA) * API breaking changes: revise errors - reduce the polymorphic variant in size, align it with RFC specified errors, be in parts more precise about errors, in other parts skip data (mirleft/ocaml-tls#505, @hannesm - fixes mirleft/ocaml-tls#491) NB: if you relied on a specific error constructor, please open an issue * Remove unused constructors from Packet.{alert_type, compression_methods, client_certificate_type, extension_type} (mirleft/ocaml-tls#505, @hannesm) NB: if you relied on specific constructors, please open an issue * API breaking change: Tls.Config.{server,client} now return a result type instead of raising an exception (mirleft/ocaml-tls#502, @hannesm, fixes mirleft/ocaml-tls#411) * FEATURE: add bench/speed.exe, a benchmark for bandwidth (for different ciphersuites) and handshakes (different key exchanges and private keys) (mirleft/ocaml-tls#500 @hannesm @dinosaure @reynir) * BUGFIX: tests/feedback.exe update with TLS 1.3 semantics, run as test (mirleft/ocaml-tls#501, @hannesm - reported by @dinosaure)

hannesm reviewed Aug 20, 2024

View reviewed changes

tls-miou-unix.opam Outdated Show resolved Hide resolved

hannesm reviewed Aug 20, 2024

View reviewed changes

tls-miou-unix.opam Outdated Show resolved Hide resolved

hannesm reviewed Aug 20, 2024

View reviewed changes

tls-miou-unix.opam Outdated Show resolved Hide resolved

dinosaure commented Aug 20, 2024

View reviewed changes

dinosaure force-pushed the miou-string-2 branch 2 times, most recently from ced7d6e to 01200a3 Compare August 21, 2024 12:05

dinosaure and others added 10 commits August 22, 2024 00:02

Add the Miou implementation of TLS

f37f2bd

Rename tls-miou tls-miou-unix

be4075d

Remove Cstruct.t from the miou implementation

17080ba

Upgrade the PR with miou.0.2.0

a1ae0d9

Fix the fuzzer about the Miou implementation

452d2f0

Apply @hannesm's suggestions from code review

53152e4

Co-authored-by: Hannes Mehnert <hannes@mehnert.org>

The fuzzer about miou is a part of the tls-miou-unix package

f1f1d9b

Fix leaked file-descriptor on the Miou fuzzer

ade7032

Show the unexpected error on our fuzzer

ff20648

Close the file-descriptor on the client side in any situations

77ea165

hannesm force-pushed the miou-string-2 branch from 8b872c1 to 6328427 Compare August 21, 2024 22:05

hannesm mentioned this pull request Aug 21, 2024

Add the Miou implementation of TLS #494

Closed

adapt to API change (Tls.Config.{client,server} may result in an error)

89c51b1

hannesm force-pushed the miou-string-2 branch from 6328427 to 89c51b1 Compare August 21, 2024 22:21

hannesm merged commit f2ce295 into mirleft:main Aug 21, 2024

hannesm mentioned this pull request Aug 21, 2024

[new release] tls (6 packages) (1.0.0) ocaml/opam-repository#26387

Merged

reynir mentioned this pull request Aug 22, 2024

Add an unix implementation of TLS using Unix.file_descr #443

Closed

3 tasks

dinosaure deleted the miou-string-2 branch August 22, 2024 09:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the miou implementation#503

Add the miou implementation#503
hannesm merged 11 commits intomirleft:mainfrom
dinosaure:miou-string-2

dinosaure commented Aug 20, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hannesm commented Aug 20, 2024

Uh oh!

dinosaure Aug 20, 2024

Uh oh!

hannesm Aug 20, 2024

Uh oh!

dinosaure Aug 20, 2024

Uh oh!

hannesm Aug 21, 2024

Uh oh!

hannesm commented Aug 21, 2024

Uh oh!

dinosaure commented Aug 21, 2024

Uh oh!

hannesm commented Aug 21, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

Conversation

dinosaure commented Aug 20, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hannesm commented Aug 20, 2024

Uh oh!

dinosaure Aug 20, 2024

Choose a reason for hiding this comment

Uh oh!

hannesm Aug 20, 2024

Choose a reason for hiding this comment

Uh oh!

dinosaure Aug 20, 2024

Choose a reason for hiding this comment

Uh oh!

hannesm Aug 21, 2024

Choose a reason for hiding this comment

Uh oh!

hannesm commented Aug 21, 2024

Uh oh!

dinosaure commented Aug 21, 2024

Uh oh!

hannesm commented Aug 21, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants