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

Add tls-eio #451

Merged
merged 18 commits into from
Sep 27, 2022
Merged

Add tls-eio #451

merged 18 commits into from
Sep 27, 2022

Conversation

talex5
Copy link
Contributor

@talex5 talex5 commented Aug 31, 2022

This PR adds a tls-eio package, allowing ocaml-tls to be used from Eio programs. At present, various people have been copying the hacky version I made in gemini-eio, which is not ideal.

The first commit copies the lwt directory as eio, to make the other changes easier to see. The second adds an MDX test, based on lwt/examples/test_client.ml and lwt/examples/test_server.ml.

Remove Lwt from Tls_eio is the main commit. I tried to keep the code mostly the same (I also deleted a lot of stuff to do with connections and address lookup, which aren't really part of TLS).

Then the later commits try to tidy things up a bit.

We might want to reintroduce some of the helper stuff, either here or in Eio itself. e.g. the code that looks up a hostname in DNS, and also passes it as the ?host argument if it's a name.

The API is similar to the lwt one. e.g.

val key_update : ?request:bool -> t -> unit Lwt.t   (* Before *)
val key_update : ?request:bool -> t -> unit         (* After *)

The main two functions are:

val server_of_flow : Tls.Config.server -> #Eio.Flow.two_way -> t
val client_of_flow : Tls.Config.client -> ?host:[ `host ] Domain_name.t -> #Eio.Flow.two_way -> t

The resulting t is itself a subtype of Eio.Flow.two_way, and so can be used anywhere one of those can.

Notes

The Lwt and Unix code have some bits flagged as e.g. XXX bad XXX and I kept those in the Eio version too.

close support looks a bit broken to me. In TLS 1.2 close-notify closed the flow in both directions, but in TLS 1.3 it should only close the sending direction. This means that Eio.Flow.shutdown is a bit broken because closing for sending also prevents receiving.

After receiving eof, the lwt version would raise Invalid_argument if you tried to send, whereas tls-mirage returns Closed. tls-eio raises End_of_file in this case. In general, we probably need to have some kind of per-direction end-of-file flag.

A user of the library will need a RNG, which will require @bikallem's mirage/mirage-crypto#155 to be merged. For the tests, I install a deterministic mock RNG so this isn't a problem for this PR. The mock RNG has the same API as the real one, so if someone copies the example it should work correctly, and the mock version prints a warning to stderr to prevent people copying that accidentally. It would be useful if mirage-crypto provided a deterministic RNG for testing.

The tests depend on eio_main, which brings in some extra (test) dependencies. We could use an in-process network instead of the OS one, which would avoid that.

Further clean-ups are likely possible. For example, this code in lwt/x509_lwt.m:

  catch_invalid_arg
    (read_file cert >|= fun pem ->
     match X509.Certificate.decode_pem_multiple pem with
     | Ok cs -> cs
     | Error (`Msg m) -> invalid_arg ("failed to parse certificates " ^ m))
    (o failure @@ Printf.sprintf "Private certificates (%s): %s" cert)

turned into this:

    try
      let pem = read_file cert in
      match X509.Certificate.decode_pem_multiple pem with
      | Ok cs -> cs
      | Error (`Msg m) -> invalid_arg ("failed to parse certificates " ^ m)
    with Invalid_argument m ->
      Fmt.failwith "Private certificates %a: %s" Path.pp cert m

I'm not sure if the original code is intending to catch exceptions from read_file. If not, this could obviously be shortened! In any case, I think the old code is wrong as it will miss any exception raised before blocking, since read_file starts before catch_invalid_arg.

It would be convenient if the config values held the RNG to use. Then we could track the need to initialise the RNG in the type-system instead of it being a runtime error.

I have tested this briefly using gemini-eio and capnp-rpc and it seems to be working. However, there is only one test-case at present.

Removed most of the examples for now.
This MDX is based on the Lwt test_client.ml and test_server.ml files.
It runs them both together, in an Eio main loop, using Lwt_eio.
Doesn't work on Eio, and also prevents users from choosing any
parameters, since future inits are ignored.
Avoids depending on the (unreleased) mirage-crypto-rng-eio.
It doesn't have anything to do with Unix now.
It's (probably) not the sender's fault the connection is closed.
tls-mirage returns `Closed` in this case, while tls_lwt faised
Invalid_argument.
@talex5
Copy link
Contributor Author

talex5 commented Sep 7, 2022

I constrained tls-async < ocaml.5.0.0 to let the CI run. I looks like the tls-eio bit passed, although there are some errors because it can't build the async examples (see dune bug ocaml/dune#5621).

@hannesm
Copy link
Member

hannesm commented Sep 7, 2022

Hmm, @talex5 is there any way to get the CI working? From a maintenance point of view, it is really bad to have a CI that fails due to some build system deficiencies.

tls-async can't be tested on OCaml 5 yet, and this prevents the CI from
testing other packages there. This annotation can be removed once a
compatible version of `core` is released.
@talex5
Copy link
Contributor Author

talex5 commented Sep 7, 2022

is there any way to get the CI working?

Ah, yes! I found the work-around I added to Prometheus, which is to also include the OCaml version constraint in the example's dune file.

@hannesm
Copy link
Member

hannesm commented Sep 13, 2022

Once ocaml/opam-repository#22103 lands, could you please update this PR to use the mirage-crypto-rng-eio. I'll merge it then and cut a release.

This adds a test dependency on mirage-crypto-rng-eio.

Requested by Hannes Mehnert.
@talex5
Copy link
Contributor Author

talex5 commented Sep 17, 2022

could you please update this PR to use the mirage-crypto-rng-eio. I'll merge it then and cut a release.

OK, I added that as a test dependency.

@hannesm
Copy link
Member

hannesm commented Sep 26, 2022

So, coming back here. When you remark:

close support looks a bit broken to me. In TLS 1.2 close-notify closed the flow in both directions, but in TLS 1.3 it should only close the sending direction. This means that Eio.Flow.shutdown is a bit broken because closing for sending also prevents receiving.

I'm wondering what the issue is. AFAIK, the following is the case:

  • TLS 1.0: only properly closed connections could be used for resumption
  • TLS 1.0 - 1.2: a peer receiving a close_notify immediately has to drop everything and reply with a close_notify
  • TLS 1.3: close_notify is sending direction only.

So, is there something to do in ocaml-tls? For eio (and the other backends), of course we could implement shutdown_write only for TLS 1.3 appropriately (and ignore or close() in earlier versions). So, do we need any guidance in this area? Eio could as well only use TLS 1.3 and have an appropriate shutdown... so I'm not sure what to do in this niche.

@talex5
Copy link
Contributor Author

talex5 commented Sep 26, 2022

With shutdown, I copied the existing pattern in lwt/tls_lwt.ml, which does:

ocaml-tls/lwt/tls_lwt.ml

Lines 169 to 175 in b37d7ec

let close_tls t =
match t.state with
| `Active tls ->
let (_, buf) = Tls.Engine.send_close_notify tls in
t.state <- `Eof ;
write_t t buf
| _ -> return_unit

Because it sets the state to Eof, you can't read from it after doing this. I just copied that pattern in Eio:

ocaml-tls/eio/tls_eio.ml

Lines 147 to 153 in b37d7ec

let close_tls t =
match t.state with
| `Active tls ->
let (_, buf) = Tls.Engine.send_close_notify tls in
t.state <- `Eof ; (* XXX: this looks wrong - we're only trying to close the sending side *)
write_t t buf
| _ -> ()

Will the engine handle things correctly if I just don't change state here, or is there more to it?

@hannesm
Copy link
Member

hannesm commented Sep 27, 2022

Will the engine handle things correctly if I just don't change state here, or is there more to it?

The implementation does not track close_notify being sent or received, but it replies to a close_notify (and any other alert) with a close_notify itself.

I suspect to fully support the TLS 1.3 half-close semantics, there's some more work needed in the pure implementation. I'll open an issue and come back to that at some point.

dune-project Show resolved Hide resolved
dune-project Outdated Show resolved Hide resolved
dune-project Show resolved Hide resolved
@hannesm
Copy link
Member

hannesm commented Sep 27, 2022

Looks fine to me for an initial release -- please see the questions for the dune-project file changes.

@hannesm
Copy link
Member

hannesm commented Sep 27, 2022

@talex5 do you prefer to keep the commit history, or is it fine to squash everything into a single commit?

@talex5
Copy link
Contributor Author

talex5 commented Sep 27, 2022

@talex5 do you prefer to keep the commit history, or is it fine to squash everything into a single commit?

Squashing is fine with me.

@hannesm hannesm merged commit ea1631c into mirleft:main Sep 27, 2022
hannesm added a commit to hannesm/opam-repository that referenced this pull request Sep 27, 2022
@hannesm
Copy link
Member

hannesm commented Sep 27, 2022

merged and released (PRed ocaml/opam-repository#22175) -- unfortunately I've no clue how to generate and upload documentation for tls-eio, since there's no switch that I can run dune-release bistro successfully (on 4.14, eio fails, on 5.0.0 async fails). Any hints would be welcome.

@talex5
Copy link
Contributor Author

talex5 commented Sep 27, 2022

Async should install OK on 5.0 with Kate's alpha repository (git+https://github.com/kit-ty-kate/opam-alpha-repository.git). Might be enough to publish the docs.

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.

2 participants