TLS over an existing connection by art-w · Pull Request #499 · mirleft/ocaml-tls

art-w · 2024-07-15T16:05:21Z

With @MisterDA , we have a use-case which requires TLS tunneling over an existing TLS connection. This should be pretty easy, but the current tls-lwt API only allow TLS negotiation over an Lwt_unix.file_descr which limits our ability to use an existing TLS connection as the carrier. This PR adds a new Tls_lwt.Unix.client_of_channels function to establish the TLS over input/output Lwt_io.channel instead, which were potentially created by a previous TLS.

To explain why TLS-over-TLS is useful, I've attached an example to show how HTTPS proxying work:

The client first does an HTTPS request to the proxy, and ask it to CONNECT to_some_server.net:443
The proxy hopefully responds with a 200 OK success code after connecting to the server
From there, the TLS connection with the proxy remains open, but further messages on that socket are transparently relayed by the proxy between the client and server
To send an HTTPS request to the now-reachable target server, the client must do a new TLS negotiation with that server (over the existing TLS connection with the proxy)

(Note that this mode of proxying is rather nice as it guarantees that the proxy can't do a man-in-the-middle, since the client is in charge of checking that the nested TLS connection has been established with the expected server certificates!)

I would like to rebase this PR on top of #497 to get rid of the cstructs, but in the meantime, let me know if there's anything else that should be changed! :)

art-w · 2024-07-15T16:09:25Z

lwt/tls_lwt.ml

+    read  : Cstruct.t -> int Lwt.t ;
+    write : Cstruct.t -> unit Lwt.t ;
+    close : unit -> unit Lwt.t ;
+  }


I didn't want to mess with the carefully crafted write/close implementation present for file descriptors, but perhaps using a pair of input/output Lwt_io.channels would work instead of a new abstraction? (I'm not sure of the impact, because the doc for lwt channels mentions that they do buffering)

lwt/examples/tls_over_tls.ml

art-w · 2024-07-19T16:23:52Z

Rebased on top of #497 to get rid of the cstructs at art-w@16418ea

lwt/examples/tls_over_tls.ml

hannesm · 2024-07-24T16:50:37Z

so what is the story of the other backends?

but more importantly, why only a client over an existing connection? wouldn't a symmetric server side be useful as well?

hannesm · 2024-07-24T16:53:39Z

lwt/examples/tls_over_tls.ml

@@ -0,0 +1,103 @@
+open Lwt


this executable is nice, but i don't quite understand: are tickets being used? and I may bite, but would it be possible to refactor the executables so we don't repeat that much code over and over!?

obviously it makes sense to wait after 497 for this, since otherwise that's quite a rebase burden. I'm also fine to pioneer the refactoring myself (in August).

Thanks, I didn't realize the example could be simplified to get rid of tickets and redundant bits! I like that this test is self-contained, but I may have gone too far: Should I had some cmdliner options to define the proxy and target server?

art-w · 2024-07-25T08:49:35Z

Thanks for the review!

tls-eio already supported this, as the client/server are using two-way Flow as input and output. For tls-async, I added a commit to allow clients to skip the Tcp.connect step, and use existing Reader/Writer instead.

Considering the code organization, it was easy to also support server over channels instead of file descriptors... Although I don't personally have a use-case for this :)

(I've already rebased this PR to get rid of Cstructs in anticipation of #497 , so it shouldn't be a burden to update it once #497 is merged)

hannesm · 2024-08-20T11:27:38Z

This should be rebased on top of main (#497 got merged). I still haven't reviewed in much detail, and am a bit hesitant to merge.

lwt/tls_lwt.ml

hannesm · 2024-08-20T11:31:27Z

This should be rebased on top of main (#497 got merged). I still haven't reviewed in much detail, and am a bit hesitant to merge.

Actually, once rebased and the review comments are cleared, it is fine to merge.

hannesm · 2024-08-21T22:18:21Z

I rebased, and when CI is happy I plan to merge & release.

@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 · 2024-08-29T09:08:02Z

Thanks a ton! (again sorry for the late reply, I was on break without internet)

@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 commented Jul 15, 2024

View reviewed changes

MisterDA reviewed Jul 16, 2024

View reviewed changes

lwt/examples/tls_over_tls.ml Outdated Show resolved Hide resolved

art-w mentioned this pull request Jul 18, 2024

TLS tunnel over Lwt_io.channel mirage/ocaml-conduit#428

Merged

MisterDA reviewed Jul 22, 2024

View reviewed changes

lwt/examples/tls_over_tls.ml Outdated Show resolved Hide resolved

MisterDA mentioned this pull request Jul 23, 2024

Support proxying client requests through direct and tunneling HTTP/HTTPS proxies mirage/ocaml-cohttp#1080

Merged

3 tasks

hannesm reviewed Jul 24, 2024

View reviewed changes

hannesm reviewed Aug 20, 2024

View reviewed changes

lwt/tls_lwt.ml Show resolved Hide resolved

art-w and others added 5 commits August 22, 2024 00:11

TLS over Lwt_io.channel

32bed65

simplify tls_over_tls example

aa31393

tls-async: TLS over Reader/Writer

c61658b

tls-lwt: server TLS over Lwt_io.channels

bd03b86

adapt to API change

7c1b153

hannesm force-pushed the tunnel branch from a8b82ae to 7c1b153 Compare August 21, 2024 22:16

hannesm merged commit def137a 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TLS over an existing connection#499

TLS over an existing connection#499
hannesm merged 5 commits intomirleft:mainfrom
art-w:tunnel

art-w commented Jul 15, 2024

Uh oh!

art-w Jul 15, 2024

Uh oh!

Uh oh!

art-w commented Jul 19, 2024

Uh oh!

Uh oh!

hannesm commented Jul 24, 2024

Uh oh!

hannesm Jul 24, 2024

Uh oh!

art-w Jul 25, 2024

Uh oh!

art-w commented Jul 25, 2024 •

edited

Loading

Uh oh!

hannesm commented Aug 20, 2024

Uh oh!

Uh oh!

hannesm commented Aug 20, 2024

Uh oh!

hannesm commented Aug 21, 2024

Uh oh!

art-w commented Aug 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

Conversation

art-w commented Jul 15, 2024

Uh oh!

art-w Jul 15, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

art-w commented Jul 19, 2024

Uh oh!

Uh oh!

hannesm commented Jul 24, 2024

Uh oh!

hannesm Jul 24, 2024

Choose a reason for hiding this comment

Uh oh!

art-w Jul 25, 2024

Choose a reason for hiding this comment

Uh oh!

art-w commented Jul 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hannesm commented Aug 20, 2024

Uh oh!

Uh oh!

hannesm commented Aug 20, 2024

Uh oh!

hannesm commented Aug 21, 2024

Uh oh!

art-w commented Aug 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

art-w commented Jul 25, 2024 •

edited

Loading