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

Shutdown #488

Merged
merged 17 commits into from
Mar 26, 2024
Merged

Shutdown #488

merged 17 commits into from
Mar 26, 2024

Conversation

hannesm
Copy link
Member

@hannesm hannesm commented Jan 4, 2024

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
Copy link
Member Author

hannesm commented Jan 5, 2024

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

@hannesm
Copy link
Member Author

hannesm commented Mar 21, 2024

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 hannesm changed the title WIP: Shutdown Shutdown Mar 21, 2024
@dinosaure
Copy link
Contributor

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

@talex5
Copy link
Contributor

talex5 commented Mar 25, 2024

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).

@hannesm
Copy link
Member Author

hannesm commented Mar 26, 2024

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?

@talex5
Copy link
Contributor

talex5 commented Mar 26, 2024

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

@talex5
Copy link
Contributor

talex5 commented Mar 26, 2024

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.

@hannesm
Copy link
Member Author

hannesm commented Mar 26, 2024

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

This is a direct port of the changes to tls-lwt to tls-eio.
@talex5
Copy link
Contributor

talex5 commented Mar 26, 2024

OK: hannesm#5

tls-eio: update for new shutdown system
@hannesm hannesm merged commit e6a52d8 into mirleft:main Mar 26, 2024
1 check passed
@hannesm hannesm deleted the shut branch March 26, 2024 17:08
hannesm added a commit to hannesm/opam-repository that referenced this pull request Mar 26, 2024
CHANGES:

* tls: handle half-closed connection properly: a received CLOSE_NOTIFY does not
  lead to a CLOSE_NOTIFY to be sent (a `send_close_notify` sends it explicitly)
  (mirleft/ocaml-tls#488 @hannesm)
* tls: modify return type of `handle_tls` - the Alert is now in the right hand
  side, and `` `Eof `` is explicit in the second part of the tuple
  (mirleft/ocaml-tls#488 @hannesm)
* tls: remove `can_handle_appdata`, the function `handshake_in_progress` is
  available (mirleft/ocaml-tls#488 @hannesm)
* tls-mirage: avoid exceptions in reneg and rekey (mirleft/ocaml-tls#487 @hannesm)
* tls: remove HEARTBEAT decoding - HEARTBEAT was never supported in this
  library, the decoder was superfluous (mirleft/ocaml-tls#487 @hannesm)
* tls-mirage: provide `underlying : flow -> FLOW.flow` (mirleft/ocaml-tls#487 @hannesm,
  fixes mirleft/ocaml-tls#425 @dinosaure)
* tls-mirage: implement mirage-flow 4 API (`val shutdown`) (mirleft/ocaml-tls#488 @hannesm)
* tls-eio: adapt to half-closed connections (mirleft/ocaml-tls#488 @talex5)
hannesm added a commit to hannesm/opam-repository that referenced this pull request Mar 26, 2024
CHANGES:

* tls: handle half-closed connection properly: a received CLOSE_NOTIFY does not
  lead to a CLOSE_NOTIFY to be sent (a `send_close_notify` sends it explicitly)
  (mirleft/ocaml-tls#488 @hannesm)
* tls: modify return type of `handle_tls` - the Alert is now in the right hand
  side, and `` `Eof `` is explicit in the second part of the tuple
  (mirleft/ocaml-tls#488 @hannesm)
* tls: remove `can_handle_appdata`, the function `handshake_in_progress` is
  available (mirleft/ocaml-tls#488 @hannesm)
* tls-mirage: avoid exceptions in reneg and rekey (mirleft/ocaml-tls#487 @hannesm)
* tls: remove HEARTBEAT decoding - HEARTBEAT was never supported in this
  library, the decoder was superfluous (mirleft/ocaml-tls#487 @hannesm)
* tls-mirage: provide `underlying : flow -> FLOW.flow` (mirleft/ocaml-tls#487 @hannesm,
  fixes mirleft/ocaml-tls#425 @dinosaure)
* tls-mirage: implement mirage-flow 4 API (`val shutdown`) (mirleft/ocaml-tls#488 @hannesm)
* tls-eio: adapt to half-closed connections (mirleft/ocaml-tls#488 @talex5)
* tls-eio: implement Eio.Resource.Close (mirleft/ocaml-tls#489 @paurkedal, reviewed by @talex5)
hannesm added a commit to hannesm/opam-repository that referenced this pull request Mar 27, 2024
CHANGES:

* tls: handle half-closed connection properly: a received CLOSE_NOTIFY does not
  lead to a CLOSE_NOTIFY to be sent (a `send_close_notify` sends it explicitly)
  (mirleft/ocaml-tls#488 @hannesm)
* tls: modify return type of `handle_tls` - the Alert is now in the right hand
  side, and `` `Eof `` is explicit in the second part of the tuple
  (mirleft/ocaml-tls#488 @hannesm)
* tls: remove `can_handle_appdata`, the function `handshake_in_progress` is
  available (mirleft/ocaml-tls#488 @hannesm)
* tls-mirage: avoid exceptions in reneg and rekey (mirleft/ocaml-tls#487 @hannesm)
* tls: remove HEARTBEAT decoding - HEARTBEAT was never supported in this
  library, the decoder was superfluous (mirleft/ocaml-tls#487 @hannesm)
* tls-mirage: provide `underlying : flow -> FLOW.flow` (mirleft/ocaml-tls#487 @hannesm,
  fixes mirleft/ocaml-tls#425 @dinosaure)
* tls-mirage: implement mirage-flow 4 API (`val shutdown`) (mirleft/ocaml-tls#488 @hannesm)
* tls-eio: adapt to half-closed connections (mirleft/ocaml-tls#488 @talex5)
* tls-eio: implement Eio.Resource.Close (mirleft/ocaml-tls#489 @paurkedal, reviewed by @talex5)
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants