arenadotio / pgx

A pure OCaml PostgreSQL client library
Other
122 stars 16 forks source link

Support TLS encryption #107

Closed brendanlong closed 3 years ago

brendanlong commented 3 years ago

I'm guessing to do this, I'll need to change the functor to use Conduit: https://github.com/mirage/ocaml-conduit

brendanlong commented 3 years ago

I'm wondering if it's possible to ignore this in the main functor and handle it entire in Pgx_async / Pgx_lwt /etc. We'll have to make sure there's reasonable error handling though.

brendanlong commented 3 years ago

Using Conduit was surprisingly easy:

diff --git a/pgx_async.opam b/pgx_async.opam
index e9dbcfe..8f9e4b8 100644
--- a/pgx_async.opam
+++ b/pgx_async.opam
@@ -12,8 +12,10 @@ depends: [
   "dune" {>= "1.11"}
   "alcotest-async" {with-test & >= "1.0.0"}
   "async_kernel" {>= "v0.13.0"}
+  "async_ssl"
   "async_unix" {>= "v0.13.0"}
   "base64" {with-test & >= "3.0.0"}
+  "conduit-async"
   "ocaml" {>= "4.08"}
   "pgx" {= version}
   "pgx_value_core" {= version}
diff --git a/pgx_async/src/dune b/pgx_async/src/dune
index 5e63847..e361930 100644
--- a/pgx_async/src/dune
+++ b/pgx_async/src/dune
@@ -11,6 +11,6 @@ let () = Jbuild_plugin.V1.send @@ {|
 (library
  (public_name pgx_async)
  (wrapped false)
- (libraries async_kernel async_unix pgx_value_core)
+ (libraries async_kernel async_unix conduit-async pgx_value_core)
  |} ^ preprocess ^ {|)
 |}
diff --git a/pgx_async/src/pgx_async.ml b/pgx_async/src/pgx_async.ml
index 7aec8ae..27c954e 100644
--- a/pgx_async/src/pgx_async.ml
+++ b/pgx_async/src/pgx_async.ml
@@ -73,19 +73,13 @@ module Thread = struct
   let close_in = Reader.close

   let open_connection sockaddr =
-    let get_reader_writer socket =
-      let fd = Socket.fd socket in
-      Reader.create fd, Writer.create fd
-    in
     match sockaddr with
-    | Unix path ->
-      let unix_sockaddr = Tcp.Where_to_connect.of_unix_address (`Unix path) in
-      Tcp.connect_sock unix_sockaddr >>| get_reader_writer
+    | Unix path -> Conduit_async.connect (`Unix_domain_socket path)
     | Inet (host, port) ->
-      let inet_sockaddr =
-        Tcp.Where_to_connect.of_host_and_port (Host_and_port.create ~host ~port)
-      in
-      Tcp.connect_sock inet_sockaddr >>| get_reader_writer
+      Uri.make ~scheme:"tcp" ~host ~port ()
+      |> Conduit_async.V3.resolve_uri
+      >>= Conduit_async.V3.connect
+      >>| fun (_socket, in_channel, out_channel) -> in_channel, out_channel
   ;;

   (* The unix getlogin syscall can fail *)

Unfortunately, if I switch the scheme to "https" (to trick it into trying to use TLS), I get errors like:

[exception] (monitor.ml.Error ("attempt to use closed writer" ((file_descr 12) (info (writer async_conduit_ssl_writer)) (kind Fifo))) ("Raised at BaseError.raise in file \"src/error.ml\" (inlined), line 8, characters 14-30" "Called from BaseError.raise_s in file \"src/error.ml\", line 9, characters 19-40" "Called from Async_unixWriter0.write_char in file \"src/writer0.ml\" (inlined), line 1594, characters 2-20" "Called from Pgx_async.Thread.output_binary_int in file \"pgx_async/src/pgx_async.ml\", line 38, characters 4-40" "Called from Pgx.Make.send_message.(fun) in file \"pgx/src/pgx.ml\", line 448, characters 4-30" "Called from Async_kernelDeferred0.bind.(fun) in file \"src/deferred0.ml\", line 54, characters 64-69" "Called from Async_kernelJob_queue.run_jobs in file \"src/job_queue.ml\", line 167, characters 6-47")) Raised at BaseError.raise in file "src/error.ml" (inlined), line 8, characters 14-30 Called from BaseError.raise_s in file "src/error.ml", line 9, characters 19-40 Called from Async_unixWriter0.write_char in file "src/writer0.ml" (inlined), line 1594, characters 2-20 Called from Pgx_async.Thread.output_binary_int in file "pgx_async/src/pgx_async.ml", line 38, characters 4-40 Called from Pgx.Make.send_message.(fun) in file "pgx/src/pgx.ml", line 448, characters 4-30 Called from Async_kernelDeferred0.bind.(fun) in file "src/deferred0.ml", line 54, characters 64-69 Called from Async_kernelJob_queue.run_jobs in file "src/job_queue.ml", line 167, characters 6-47

I'm having trouble finding documentation, but I suspect Postgres is using startls and Conduit wants normal TLS.

brendanlong commented 3 years ago

Here's the Postgres documentation for doing TLS: https://www.postgresql.org/docs/9.3/protocol-flow.html#AEN100021

Essentially we need to:

  1. Send an SSLRequest message
  2. Wait for a single byte from the server ('S' or 'N')
  3. If the byte was 'S', upgrade the current connection to use TLS. If the byte was 'N' just continue to the next step.
  4. Continue the normal setup