ocaml-tls icon indicating copy to clipboard operation
ocaml-tls copied to clipboard

WIP: Shutdown

Open hannesm opened this issue 1 year ago • 1 comments

this is still work in progress. The idea is to track in engine the status of (half-)closed connections - and implement the revised close_notify semantics (i.e. not replying immediately with a close_notify).

There are still some checks missing (e.g. for reneg/rekey/send_application_data), also should the effectful layers inform the engine when a read or write failed (unclear, tendency towards no)?

hannesm avatar Jan 04 '24 18:01 hannesm

In this PR, the changes to tls-lwt are done now. Curious to hear feedback if there's any.

hannesm avatar Jan 05 '24 15:01 hannesm

now the tls-mirage bits as well use the new API and provide the mirage-flow 4 interface (shutdown).

reviews / suggestions would be welcome (esp. @reynir @dinosaure who had great reviews for the awa-ssh PR that is similar to this here)

also, if there is anyone eager to adjust async (@torinnd @bcc32 @tmcgilchrist) and eio (@talex5 @bikallem) bits, that'd be great. I'm open for discussions about the changes to the Engine API above.

hannesm avatar Mar 21 '24 19:03 hannesm

I actually tested this PR with Miou and httpaf and it seems to work pretty well at a glance :+1:.

dinosaure avatar Mar 22 '24 13:03 dinosaure

Thanks - this is great!

I pushed updates for tls-eio to https://github.com/talex5/ocaml-tls/commits/shutdown/ (needs review).

The first commit just ports the changes to tls-lwt directly to tls-eio. The second removes the work-arounds for missing half-shutdown in the fuzz tests, which are passing so far (after 5 minutes of afl-fuzz).

talex5 avatar Mar 25 '24 12:03 talex5

CI failures are in the eio, I don't quite understand the issues there. @talex5 I see two options: you open your changes as a PR to this repository, I merge and release -- or I release without the eio part. any preference?

hannesm avatar Mar 26 '24 12:03 hannesm

I think you can just git pull my branch into yours to fix it.

talex5 avatar Mar 26 '24 13:03 talex5

Or I can open a PR against this branch if you prefer. The logs for the fuzz failure on your branch look like this:

+dispatch_commands: done
+client-to-server: receiver ready
+server-to-client: sender ready
+server-to-client: shutdown send (Tls level)
+server: wrote 24 bytes to network
+client: read 24 bytes from network
+server-to-client: recv thread done (got EOF)
+client-to-server: shutdown send (Tls level)
Fatal error: exception Eio_mock__Backend.Deadlock_detected

But I didn't investigate since I assumed it wouldn't work without the changes anyway.

talex5 avatar Mar 26 '24 13:03 talex5

if you open a PR against my branch, that'd be excellent

hannesm avatar Mar 26 '24 13:03 hannesm

OK: https://github.com/hannesm/ocaml-tls/pull/5

talex5 avatar Mar 26 '24 13:03 talex5