store: Fix TLS regression in diesel-async connection pool#6473
Open
nate-staked wants to merge 3 commits intographprotocol:masterfrom
Open
store: Fix TLS regression in diesel-async connection pool#6473nate-staked wants to merge 3 commits intographprotocol:masterfrom
nate-staked wants to merge 3 commits intographprotocol:masterfrom
Conversation
The migration from synchronous diesel (r2d2 + PgConnection) to diesel-async (deadpool + AsyncPgConnection) in the "Make the store async" change inadvertently broke TLS support for the database connection pool. The old code used diesel::r2d2::ConnectionManager<PgConnection> which is backed by libpq (via pq-sys). libpq defaults to sslmode=prefer, meaning it transparently negotiates TLS with the server when available. The new code uses diesel_async::AsyncPgConnection::establish() which internally calls tokio_postgres::connect() with tokio_postgres::NoTls, meaning TLS is never negotiated regardless of the sslmode parameter in the connection URL. This breaks connections to any PostgreSQL server that requires encrypted connections via pg_hba.conf. Fix this by replacing AsyncPgConnection::establish() with a manual tokio_postgres::connect() call using postgres-openssl as the TLS connector (with SslVerifyMode::NONE to match libpq's default prefer behavior), then constructing the AsyncPgConnection via try_from_client_and_connection(). This restores the pre-v0.42.0 behavior where connections are encrypted by default. Note: tokio-postgres does not support sslmode=verify-ca or sslmode=verify-full in its URL parser — only disable, prefer, and require are recognized. Certificate verification would require upstream changes to tokio-postgres. The openssl and postgres-openssl crates were already dependencies of graph-store-postgres (used by the notification listener). Only tokio-postgres was added as a new direct dependency.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Warning: vibes ahead
We require ssl verification of database connections. I am not super familiar with the test harness, so unfortunately I don't have any tests to go along with this. That said, I manually tested this and it fixed the issue I was having connecting to our database after upgrading to v0.42.1.
Here's claude's explanation:
The migration from synchronous diesel (r2d2 + PgConnection) to diesel-async (deadpool + AsyncPgConnection) in the "Make the store async" change inadvertently broke TLS support for the database connection pool.
The old code used diesel::r2d2::ConnectionManager which is backed by libpq (via pq-sys). libpq defaults to sslmode=prefer, meaning it transparently negotiates TLS with the server when available.
The new code uses diesel_async::AsyncPgConnection::establish() which internally calls tokio_postgres::connect() with tokio_postgres::NoTls, meaning TLS is never negotiated regardless of the sslmode parameter in the connection URL. This breaks connections to any PostgreSQL server that requires encrypted connections via pg_hba.conf.
Fix this by replacing AsyncPgConnection::establish() with a manual tokio_postgres::connect() call using postgres-openssl as the TLS connector (with SslVerifyMode::NONE to match libpq's default prefer behavior), then constructing the AsyncPgConnection via try_from_client_and_connection(). This restores the pre-v0.42.0 behavior where connections are encrypted by default.
Note: tokio-postgres does not support sslmode=verify-ca or sslmode=verify-full in its URL parser — only disable, prefer, and require are recognized. Certificate verification would require upstream changes to tokio-postgres.
The openssl and postgres-openssl crates were already dependencies of graph-store-postgres (used by the notification listener). Only tokio-postgres was added as a new direct dependency.