Add TLS support for Pgx_async #108
Conversation
This adds TLS support for Pgx_async using Conduit. This is only a proof of concept because: - We're using the Conduit.V1 interface, which we may not want to rely on (the latest is V3) - We need to add support for Pgx_async - We probably need better error handling than asserts Resolves #107
|
See mirage/ocaml-conduit#391 for a question about how Conduit's maintainers want us to do this with V3 |
|
@anuragsoni Not sure if you're still interested in Pgx stuff, but if you are, do you have any opinions on how I'm implementing this? |
@brendanlong This looks nice! I implemented ssl support in a similar way in my proof of concept at https://github.com/anuragsoni/postgres-protocol/ How would you feel about not pulling in conduit and just using |
|
Yeah I asked the Conduit people about upgrading in >V1 in mirage/ocaml-conduit#391 I could pull in Async_ssl and ocaml-tls directly, but I'm worried that I'd have to re-implement a bunch of stuff, and Conduit seems to be the "standard" way of doing this. If Conduit had a way to do this upgrade in V3, would there still be reason you think we should avoid it? |
I don't think so. If Conduit works for how we need to communicate with Postgres, it'd be nice to just use it for setting up all the connections. |
|
That makes sense, I'll wait to see what they say then. |
|
I haven't heard back from the Conduit people, so for now I think I'm going to implement this using the V1 interface and then I'll update it if/when other options become available. |
| | `Always _ -> | ||
| failwith | ||
| "TLS support is not compiled into this Pgx library but ~ssl was set to \ | ||
| `Always" |
There was a problem hiding this comment.
I'd like to make this impossible but I'm not sure how.
There was a problem hiding this comment.
#suggestion: We can also enforce that the io wrappers for pgx should always pick an ssl option (async_ssl, tls or lwt_ssl), and get rid of this branch. I'd expect that eventually all io wrappers in the pgx repo will support encrypted connections, but if we don't want to support them in a particular backend, we can probably tweak the wrapper itself to not expose any ssl related options in its mli file
| | `Auto -> None | ||
| | `Always ssl_config -> Some ssl_config |
There was a problem hiding this comment.
I'm not completely sure on this interace. My idea was that if people actually care about SSL, they probably want to send a config.
|
@anuragsoni What do you think of this interface? I think I want to keep the conduit implementation for Pgx_async for now since it's much simpler, but I'd be open to replacing it with a different one. I think my Pgx interface would allow us to implement Pgx_lwt_unix in whatever way is more convenient (doesn't need to be conduit). I'm mostly trying to finish this up since I don't actually have a lot of time to work on it right now :( But I'm hoping to come up with an interface that's at least stable enough that there won't be major sure in the main Pgx functor. |
| -> ?verbose:int | ||
| -> ?max_message_length:int | ||
| -> (t -> 'a Deferred.t) | ||
| -> 'a Deferred.t |
There was a problem hiding this comment.
This is unnecessary since it's exactly the same interface that we import above in include Pgx.S
| val upgrade_ssl | ||
| : [ `Not_supported | ||
| | `Supported of | ||
| ?ssl_config:ssl_config | ||
| -> in_channel | ||
| -> out_channel | ||
| -> (in_channel * out_channel) t | ||
| ] | ||
|
|
There was a problem hiding this comment.
Now that the async_ssl dependency is mandatory, will there still be a scenario where attempting to setup a tls connection via conduit will fail because of missing support? I'm guessing with the current setup we might not need the Not_supported branch at all?
There was a problem hiding this comment.
We need the Not_supported branch because Pgx_lwt and Pgx_unix don't support TLS right now. Hopefully at some point we can get this supported for Pgx_lwt, but I doubt we'll ever support it for Pgx_unix.
I'm open to alternative ways of doing this though if there is one?
…x_lwt_unix, pgx_lwt and pgx (2.0) CHANGES: ### Breaking changes * The Pgx module is now wrapped, which means `Pgx_aux`, `Types`, `Access`, etc. aren't added to the global scope. The main result of this is that `Pgx_value` now needs to be accessed as `Pgx.Value`. (arenadotio/pgx#103) * `Pgx_async.connect` and `with_conn` now have an additional optional `?ssl` argument (see below). ### Added * Pgx_async now supports TLS connections using Conduit_async. This is enabled by default and can be controlled with the new `?ssl` argument to `connect` and `with_conn`. (arenadotio/pgx#108) ### Fixed * Improved message for authentication errors. Previously these raised `Pgx_eof`, and now they raise `PostgreSQL_Error("Failed to authenticate with postgres server", additional details ...)`. (arenadotio/pgx#105) ### Changed * Support new Mirage-conduit timeout argument (arenadotio/pgx#95).
This adds TLS support for Pgx_async using Conduit. There are a few things that aren't ideal about this: - We're using the Conduit.V1 interface, which we may not want to rely on (the latest is V3) - We haven't implemented this for Lwt yet since they don't expose the same SSL upgrade interface in Conduit Resolves arenadotio#107
This adds TLS support for Pgx_async using Conduit.
There are a few things that aren't ideal about this:
(the latest is V3)
Resolves #107