mfp / oraft

Raft consensus algorithm implementation
Other
33 stars 5 forks source link

Tls #2

Closed vbmithr closed 10 years ago

vbmithr commented 10 years ago

This PR adds TLS support using ocaml-tls. Due to reindentation of the modified code with ocaml-indent, the changeset is a bit hard to read, sorry about that.

mfp commented 10 years ago

On Wed, Aug 20, 2014 at 02:04:36AM -0700, Vincent Bernardoff wrote:

This PR adds TLS support using ocaml-tls. Due to reindentation of the modified code with ocaml-indent, the changeset is a bit hard to read, sorry about that.

While I do want ocaml-tls to mature and succeed, I fear it's too early to rely only on it at this stage, for performance and security reasons. I'm going to try to generalize the code in a branch so as to allow to use either ocaml-tls or lwt.ssl (i.e. OpenSSL). Ideally, the code should be enabled at compilation time depending on whether the corresponding dependencies are installed (as done e.g. by lwt.ssl), but for the time being they will be hard dependencies.

As for the reindentation of the code, I've run into

https://github.com/OCamlPro/ocp-indent/blob/master/.ocp-indent

which would allow to match the existing style with a few changes like "in = 2", "with = 2" and "match_clause = 4". I do see the merits of e.g. "with = 0", but it's better to keep indent style consistency and maybe switch to new settings altogether at some later point. I'll reformat in the new branch, and try to test a .ocp-indent that preserves existent style.

Mauricio Fernández

vbmithr commented 10 years ago

Ok, I understand your points. I'll have a look at your new branch then :) Closing this for now.

mfp commented 10 years ago

On Fri, Aug 22, 2014 at 02:03:43AM -0700, Vincent Bernardoff wrote:

Closed #2.

I've reworked TLS support so that you can trivially use ocaml-tls and ocaml-ssl via lwt.ssl while keeping Oraft_lwt and RSM oblivious to them (will allow eventually to include ocaml-tls and/or lwt.ssl support at compile-time).

I've also added a .ocp-indent which for the most part follows the conventions used so far in the codebase and reformatted the code changed in this branch with it, with some minor manual tweaks.

Could you take a look at my tls branch and check everything's still working? It seemed to be OK in some quick testing with dict.opt.

https://github.com/mfp/oraft/tree/tls

Mauricio Fernández

vbmithr commented 10 years ago

In Make_client: val make : ?conn_wrapper:Oraft_lwt.conn_wrapper -> id:string -> unit -> t

The client needs to have a server certificate, which is not good for the interface. The interface should be different IMO.

vbmithr commented 10 years ago

But otherwise it seems fine.

mfp commented 10 years ago

On Tue, Aug 26, 2014 at 08:33:38AM -0700, Vincent Bernardoff wrote:

In Make_client: val make : ?conn_wrapper:Oraft_lwt.conn_wrapper -> id:string -> unit -> t

The client needs to have a server certificate, which is not good for the interface. The interface should be different IMO.

OK, changed it to

val make : ?conn_wrapper:[> `Outgoing] Oraft_lwt.conn_wrapper -> id:string -> unit -> t

I used a phantom type with subtyping to avoid introducing two types and because the server must reuse its wrapper in a client context (to join the cluster).

Oraft_lwt exposes

type -'a conn_wrapper

type simple_wrapper =
  Lwt_unix.file_descr -> (Lwt_io.input_channel * Lwt_io.output_channel) Lwt.t

val make_client_conn_wrapper : simple_wrapper -> [`Outgoing] conn_wrapper

val make_server_conn_wrapper :
  incoming:simple_wrapper -> outgoing:simple_wrapper ->
  [`Incoming | `Outgoing] conn_wrapper

val wrap_outgoing_conn :
  [> `Outgoing] conn_wrapper -> Lwt_unix.file_descr ->
  (Lwt_io.input_channel * Lwt_io.output_channel) Lwt.t

val wrap_incoming_conn :
  [> `Incoming] conn_wrapper -> Lwt_unix.file_descr ->
  (Lwt_io.input_channel * Lwt_io.output_channel) Lwt.t

val trivial_conn_wrapper :
  ?buffer_size:int -> unit -> [< `Incoming | `Outgoing] conn_wrapper

If it's OK by you, I'll merge into master, thus closing this PR.

Mauricio Fernández

vbmithr commented 10 years ago

On 27/08/2014 01:19, mfp wrote:

If it's OK by you, I'll merge into master, thus closing this PR.

Ok by me, I like the interface now. You can merge! :)

vbmithr commented 10 years ago

TLS support has been added.