TLS Support for Postgres

[oeed]

I'm in need of TLS support for Postgres and can see it currently is hard coded to NoTls.

Adding TLS support to the connection seems fairly straight forward and I'm happy to PR it, but a little unsure what the prefered interface is, as AsyncConnection::establish just takes a string, whereas it'd ideally also take T: tokio_postgres::tls::MakeTlsConnect<Socket> – but that doesn't make sense to add to the agnostic AsyncConnection trait.

Potentially, a dedicated AsyncPgConnection::establish_with_tls function which AsyncConnection::establish calls would work. But not sure if you'd prefer a builder pattern, etc. for future compatibility.

[omid]

@oeed check this example out.
https://github.com/weiznich/diesel_async/blob/main/examples/postgres/pooled-with-rustls/src/main.rs

[vriesk]

Conversely, I think there's a lot of value to provide at least one "easy" path for the users who are not very concerned about the SSL backend choice, which CA roots to use etc., while still exposing the existing flexibility of being able to create the AsyncPgConnection from underlying client.

So, e.g. a shortcut method that would assemble rustls with webpki-roots together, together with sslmode=require, also serving as a ready example how to do things alternatively, if you need customized roots or whatnot.

[weiznich]

So, e.g. a shortcut method that would assemble rustls with webpki-roots together, together with sslmode=require, also serving as a ready example how to do things alternatively, if you need customized roots or whatnot.

At least I'm not interested in having such a "shortcut" method in diesel-async itself. I expect that people widely disagree about what's exactly the right default there. There are several sensible choices for the ssl library and there are just too much sensible choices for the exact configuration. I cannot maintain and test them all, so it's up to the user to decide how to best manage them.

If you really feel that's a problem worth to be solved you can always publish a crate build on top of diesel-async that provides such a "sensible" default. If it turns out to be widely popular we can talk about pulling it in at some future point.

[talves]

Adding to the conversation here, I worked through the issues of getting the CA roots and made the recommended changes for the connection in an AWS lambda example to change from rustls-native-certs to rustls-platform-verifier (recommended) and not have to deal with what backend method was being used. This example works on Netlify functions which is just an AWS lambda underneath.

https://gist.github.com/talves/92070c8320f7c5c35c3bc640698e9aec#file-main-rs-L151-L168

I really appreciate all the great/hard work on all these changes to make this work so well. It's turned into a really enjoyable experience to get this working for my rust lambdas.

BTW: This is a lambda connecting to a Neon Postgres instance. I'm writing a full serverless Leptos web app using csr. diesel_async has taken out so much of the pain.

[weiznich]

Thanks for bringing this up. It would be great to receive a PR that updates the example

[talves]

done: #208

[ollyswanson]

Hey @weiznich

I thought I'd piggy back on this discussion rather than opening "yet another TLS related" question/discussion/issue.

While having a poke around in the AsyncPgConnection code I noticed a bit of disparity between connections created using AsyncPgConnection::establish and AsyncPgConnection::try_from. Namely, when adding better handling of the background task there was no way to add better error handling for connections constructed using try_from (when we need TLS).

I get that this is by design, but the error handling as implemented by you vs. the error handling as suggested by tokio_postgres is night and day, that is, it's a lot more pleasant in diesel_async's API, and losing out on this or having to reimplement the whole connection to get this error handling with TLS feels like throwing the baby out with the bath water.

Before opening a PR I wanted to check if adding an extra method that accepts both a tokio_postgres::Client and tokio_postgres::Connection in order to construct an AsyncPgConnection would be something you'd be alright with. It would allow us to have the same error handling as the AsyncPgConnection created by establish.

I get that it's a bit of a clunky API and a little esoteric so definitely understand if it's something you're not interested in adding, but it feels like an alright compromise between supporting TLS (and making it easier to use) and not wanting to take on extra dependencies.

Suggestion (naming and documentation to be improved):

diff --git a/examples/postgres/pooled-with-rustls/src/main.rs b/examples/postgres/pooled-with-rustls/src/main.rs
index a18451c..87a8eb4 100644
--- a/examples/postgres/pooled-with-rustls/src/main.rs
+++ b/examples/postgres/pooled-with-rustls/src/main.rs
@@ -49,12 +49,8 @@ fn establish_connection(config: &str) -> BoxFuture<ConnectionResult<AsyncPgConne
         let (client, conn) = tokio_postgres::connect(config, tls)
             .await
             .map_err(|e| ConnectionError::BadConnection(e.to_string()))?;
-        tokio::spawn(async move {
-            if let Err(e) = conn.await {
-                eprintln!("Database connection: {e}");
-            }
-        });
-        AsyncPgConnection::try_from(client).await
+
+        AsyncPgConnection::try_from_client_and_connection(client, conn).await
     };
     fut.boxed()
 }
diff --git a/src/pg/mod.rs b/src/pg/mod.rs
index d39e803..c030e09 100644
--- a/src/pg/mod.rs
+++ b/src/pg/mod.rs
@@ -156,16 +156,8 @@ impl AsyncConnection for AsyncPgConnection {
         let (client, connection) = tokio_postgres::connect(database_url, tokio_postgres::NoTls)
             .await
             .map_err(ErrorHelper)?;
-        let (tx, rx) = tokio::sync::broadcast::channel(1);
-        let (shutdown_tx, shutdown_rx) = tokio::sync::oneshot::channel();
-        tokio::spawn(async move {
-            match futures_util::future::select(shutdown_rx, connection).await {
-                Either::Left(_) | Either::Right((Ok(_), _)) => {}
-                Either::Right((Err(e), _)) => {
-                    let _ = tx.send(Arc::new(e));
-                }
-            }
-        });
+
+        let (rx, shutdown_tx) = drive_connection(connection);
 
         let r = Self::setup(
             client,
@@ -174,6 +166,7 @@ impl AsyncConnection for AsyncPgConnection {
             Arc::clone(&instrumentation),
         )
         .await;
+
         instrumentation
             .lock()
             .unwrap_or_else(|e| e.into_inner())
@@ -367,6 +360,28 @@ impl AsyncPgConnection {
         .await
     }
 
+    /// Constructs a new `AsyncPgConnection` from an existing [`tokio_postgres::Client`] and
+    /// [`tokio_postgres::Connection`].
+    pub async fn try_from_client_and_connection<S>(
+        client: tokio_postgres::Client,
+        conn: tokio_postgres::Connection<tokio_postgres::Socket, S>,
+    ) -> ConnectionResult<Self>
+    where
+        S: tokio_postgres::tls::TlsStream + Unpin + Send + 'static,
+    {
+        let (rx, shutdown_tx) = drive_connection(conn);
+
+        Self::setup(
+            client,
+            Some(rx),
+            Some(shutdown_tx),
+            Arc::new(std::sync::Mutex::new(
+                diesel::connection::get_default_instrumentation(),
+            )),
+        )
+        .await
+    }
+
     async fn setup(
         conn: tokio_postgres::Client,
         connection_future: Option<broadcast::Receiver<Arc<tokio_postgres::Error>>>,
@@ -826,6 +841,30 @@ async fn drive_future<R>(
     }
 }
 
+fn drive_connection<S>(
+    conn: tokio_postgres::Connection<tokio_postgres::Socket, S>,
+) -> (
+    broadcast::Receiver<Arc<tokio_postgres::Error>>,
+    oneshot::Sender<()>,
+)
+where
+    S: tokio_postgres::tls::TlsStream + Unpin + Send + 'static,
+{
+    let (tx, rx) = tokio::sync::broadcast::channel(1);
+    let (shutdown_tx, shutdown_rx) = tokio::sync::oneshot::channel();
+
+    tokio::spawn(async move {
+        match futures_util::future::select(shutdown_rx, conn).await {
+            Either::Left(_) | Either::Right((Ok(_), _)) => {}
+            Either::Right((Err(e), _)) => {
+                let _ = tx.send(Arc::new(e));
+            }
+        }
+    });
+
+    (rx, shutdown_tx)
+}
+
 #[cfg(any(
     feature = "deadpool",
     feature = "bb8",

[weiznich]

I think that's a very reasonable addition and I would happily accept a PR for this. Would it be possible that you also add a short note about TLS to the top level documentation of the AsyncPgConnection type?

[vriesk]

Also, stuff examples/ should already use this if feasible.

[ollyswanson]

Been bogged down with work, just thought I'd leave a comment here to say thanks for being so receptive, and that for anyone reading this that it's been merged 🙂 #189

[vriesk]

@ollyswanson thanks! @weiznich , could we have 0.5.1 with this? :)

[weiznich]

@vriesk I generally do not want to be pinged just to ask a question about something when a release happens. The next release will happen if I have capacity to deal with the remaining open issues, which especially includes #186. Feel free to help resolving that issue to make a release happen faster.