Skip to content

TLS tunnel over Lwt_io.channel#428

Merged
samoht merged 3 commits intomirage:mainfrom
art-w:tunnel
Sep 19, 2024
Merged

TLS tunnel over Lwt_io.channel#428
samoht merged 3 commits intomirage:mainfrom
art-w:tunnel

Conversation

@art-w
Copy link
Copy Markdown
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.

art-w
art-w commented Jul 18, 2024
View reviewed changes
@art-w art-w marked this pull request as draft July 19, 2024 07:58
@MisterDA MisterDA mentioned this pull request Jul 23, 2024
Merged
3 tasks
@hannesm
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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? :)

MisterDA
MisterDA approved these changes Sep 4, 2024
View reviewed changes
@samoht @MisterDA
Update src/conduit-lwt-unix/conduit_lwt_unix.ml
5025964 
Co-authored-by: Antonin Décimo <antonin.decimo@gmail.com>
@samoht samoht merged commit 055956a into mirage:main Sep 19, 2024
@samoht
Copy link
Copy Markdown
Member

samoht commented Sep 19, 2024

LGTM

@samoht samoht mentioned this pull request Sep 19, 2024
Merged
samoht added a commit to samoht/opam-repository that referenced this pull request Sep 19, 2024
@samoht
[new release] conduit (5 packages) (7.1.0)
d6afc82 
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

Reviewers

1 more reviewer

@MisterDA MisterDA MisterDA approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@art-w @hannesm @samoht @MisterDA