HTTPS support for the Lwt bindings #88
Conversation
anmonteiro
force-pushed
the
anmonteiro/lwt-https-support
branch
3 times, most recently
from
8adf5ba
to
2797b61
Compare
|
An issue I see with this PR in its current state is that users will need to have either I will work on removing that limitation (at build time), the same way |
anmonteiro
force-pushed
the
anmonteiro/lwt-https-support
branch
from
53f58c8
to
8d34de7
Compare
anmonteiro
force-pushed
the
anmonteiro/lwt-https-support
branch
2 times, most recently
from
21e8e4f
to
617d434
Compare
lwt/dune
Outdated
| if ssl then "ssl_io_real", "lwt_ssl " | ||
| else "ssl_io_dummy", "" | ||
| in | ||
| let tls, tls_d = |
There was a problem hiding this comment.
It looks like in this one you've taken a pretty different approach from the one in #92 - can you explain why they're so different? Sorry, I'm not super familiar with dune file conventions.
There was a problem hiding this comment.
I went with different approaches because there isn't an OCaml-TLS binding for Async yet. It makes things easier there because there's only one library to choose from.
lwt/ssl_io_dummy.ml
Outdated
| Lwt.fail_with "Ssl not available" | ||
|
|
||
| type client = [ `Ssl_not_available ] | ||
| type server = [ `Ssl_not_available ] |
There was a problem hiding this comment.
It looks like this mostly solves the issue raised in #92 , by requiring the user to pass in a value of type [`Tls_not_available] or [`Ssl_not_available] in order to get one of the runtime failures - could we use this approach in the async PR? Maybe we could make this a little more avoidable by adding of Nothing.t to those types to make them zero-valued?
There was a problem hiding this comment.
I'm quite certain that #92 has a type like this one.
There was a problem hiding this comment.
Oh, I see, the problem is that the argument of that type is optional and thus it can be easy to miss the warning in the case that it's missing.
There was a problem hiding this comment.
That’s a great point. Perhaps we should also fabricate types for certfile and keyfile that won’t typecheck. What do you think?
Additionally, I’m thinking once the mirage adapter in #83 is in, we can reorganize this code to account for the functorization of httpaf-lwt and the IO module type.
There was a problem hiding this comment.
Aren't the certfile and keyfile also optional? I was thinking maybe do what you're suggesting but with the Lwt_unix.file_descr argument, which is required and appears in all four signatures.
Okay, in case of a pending reorganization here it probably makes sense to iron out the interface but not look too hard at the implementation for now.
There was a problem hiding this comment.
Aren't the certfile and keyfile also optional?
They are indeed optional. Forgive my confusion.
I was thinking maybe do what you're suggesting but with the Lwt_unix.file_descr argument
This is a great idea. I really like it
Okay, in case of a pending reorganization here it probably makes sense to iron out the interface but not look too hard at the implementation for now.
Another discussion we can have is whether this code actually belongs in http/af or not. With the reorganization in #83 - if the changeset is accepted - httpaf-lwt becomes a library that provides a functor and a signature for that functor's argument. HTTPS support could well be a third party library that applies that functor with a TLS / SSL Io module. That would also reduce the maintenance burden in http/af.
There was a problem hiding this comment.
Hrmm, I don't do enough public OCaml work to be familiar with the trade-offs of breaking up libraries like that. @seliopou What do you think about the idea of HTTPS support coming in a separate library?
anmonteiro
force-pushed
the
anmonteiro/lwt-https-support
branch
from
617d434
to
d4671b5
Compare
|
Any news on this front @thedufer? Master is going on and I'm currently on @anmonteiro's fork which has been running great for httpkit. Anything I can do to help? 🙌 |
|
@Ostera I'd like to get this simplified down, perhaps by removing the lwt_ssl bits and just relying on ocaml-tls. Would that still work for your use-case? I suspect we'll want to mostly start over with a new PR at that point. |
anmonteiro
force-pushed
the
anmonteiro/lwt-https-support
branch
from
96ec09d
to
2b51e07
Compare
based on the comment in #77 regarding TLS integration, this PR adds HTTPS support to the Lwt bindings. Async support will be added later - @seliopou, would you like a different PR? The approach I took in this PR allows downstream users of http/af to use either `ocaml-tls` or `lwt_ssl` (build time disambiguation based on installed libraries - preference is given to `ocaml-tls`). The reason why I did the extra work of including support for `lwt_ssl` is due to the fact that `ocaml-tls` doesn't yet include support for elliptic curve ciphersuites (upstream issue: mirleft/ocaml-tls#362).
anmonteiro
force-pushed
the
anmonteiro/lwt-https-support
branch
from
2b51e07
to
92939a5
Compare
anmonteiro
force-pushed
the
anmonteiro/lwt-https-support
branch
from
92939a5
to
6d2c80e
Compare
…bitedtype#88) similar to what we tested for in inhabitedtype#87
based on the comment in #77 regarding TLS integration, this PR adds
HTTPS support to the Lwt bindings.
Async support will be added later - @seliopou, would you like a
different PR?
The approach I took in this PR allows downstream users of http/af to use
either
ocaml-tlsorlwt_ssl(build time disambiguation based oninstalled libraries - preference is given to
ocaml-tls). The reasonwhy I did the extra work of including support for
lwt_sslis due tothe fact that
ocaml-tlsdoesn't yet include support for elliptic curveciphersuites (upstream issue:
mirleft/ocaml-tls#362).