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

TLS tunnel over Lwt_io.channel #428

Merged
merged 3 commits into from
Sep 19, 2024
Merged

TLS tunnel over Lwt_io.channel #428

merged 3 commits into from
Sep 19, 2024

Conversation

art-w
Copy link
Contributor

@art-w art-w commented Jul 18, 2024

I'm marking this PR as a draft, because it exposes the functionalities of TLS-over-TLS added by this PR mirleft/ocaml-tls#499 which hasn't yet been reviewed... but I have some questions on how to best present this feature in Conduit :)

For context, @MisterDA has a working implementation of cohttp-lwt-unix client requests with http/https proxying, which relies on conduit-lwt-unix for TLS and on this PR for the "https over http(s)", which happens because https proxies provide a safe tunnel for the client to negotiate the TLS connection with the server. For this to work though, we need to able to reuse an existing connection instead of having Conduit create a new socket. The mirleft/ocaml-tls#499 PR has more details on how this works, but I've also added an example in Conduit to test the flow if you are curious.

@hannesm
Copy link
Member

hannesm commented Aug 29, 2024

What state is this in? Is there anyone interested in conduit reviewing feature PRs? I still don't understand conduits purpose, and only do maintenance (mainly because I want to release mirage) ;)

@art-w art-w marked this pull request as ready for review August 29, 2024 10:01
@art-w
Copy link
Contributor Author

art-w commented Aug 29, 2024

I've rebased and removed the draft status, since the tls dependency has since been released :)
The feature has been thoroughly tested with mirage/ocaml-cohttp#1080 so I think it's in good shape, although it would be nice to have a conduit expert opinion on the endp_of_flow question.

@art-w
Copy link
Contributor Author

art-w commented Aug 30, 2024

I went with exposing the TLS tunneling over an existing Lwt_io.channel connection by exposing a new type Conduit_lwt_unix.endp which extends Conduit.endp with the Lwt-specific feature, as this seemed like the most natural solution... @samoht Would you have some time to look at this PR? :)

src/conduit-lwt-unix/conduit_lwt_unix.ml Outdated Show resolved Hide resolved
@samoht samoht merged commit 055956a into mirage:main Sep 19, 2024
7 checks passed
@samoht
Copy link
Member

samoht commented Sep 19, 2024

LGTM

samoht added a commit to samoht/opam-repository that referenced this pull request Sep 19, 2024
CHANGES:

* Support TLS tunnel over Lwt_io.channel (mirage/ocaml-conduit#428, @art-w)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants