Use postgres-native-tls to connect to the database with TLS
#9192
Conversation
Turbo87
added
C-internal 🔧
|
I've deployed and tested this on staging and everything appears to be working with this PR |
`diesel-async` by default does not support TLS directly. Instead it offers the `custom_setup` option which allows its users to choose their TLS stack between `native-tls`, `openssl` and `rustls`. This change implements a `custom_setup` hook based on `postgres-native-tls`, whose dependencies are mostly already in our lockfile anyway. The code was mostly adapted from the official example code in `diesel-async`. Before `diesel-async` we relied on the regular `diesel` connection implementation, which uses `pg-sys` under the hood, which uses `libpq`, which uses OpenSSL for TLS. In other words: this slightly changes how TLS is handled for the database connections. Specifically, on OSX the server certificate is rejected as "not trusted" due to a long validity period. On production this should not be relevant, but during local development against the staging database this can cause issues, thus I've included a code comment with a temporary workaround for these cases.
… because otherwise the native TLS stack on our server complains about the self-signed certificate in the cert chain.
|
obviously CI is now stalling... this fix is driving me mad 🫣 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9192 +/- ##
==========================================
- Coverage 89.19% 89.17% -0.02%
==========================================
Files 282 282
Lines 28583 28622 +39
==========================================
+ Hits 25494 25525 +31
- Misses 3089 3097 +8 ☔ View full report in Codecov by Sentry. |
... otherwise on CI our tests start failing due to the TLS stack complaining about a self-signed certificate in the chain.
There was a problem hiding this comment.
I assume the reasoning behind native-tls instead of, say, rustls is to minimise how much change occurs relative to the previous behaviour?
I don't love the workarounds here — essentially disabling TLS validity checks to handle an iffy macOS behaviour and CI that really doesn't have a need for TLS at all isn't ideal — but I'm happy enough to merge this for now and iterate later.
| // certificate being self-signed. On CI we are connecting to a locally | ||
| // running database, so we also don't need to enforce the validity of | ||
| // the certificate either. | ||
| .danger_accept_invalid_certs(!enforce_tls) |
There was a problem hiding this comment.
I promise I stared at this line for a while to make sure it was correct.
I tried
yeah, I don't love them either... though to be fair, as @weiznich pointed out on Zulip, the default |
diesel-asyncby default does not support TLS directly. Instead it offers thecustom_setupoption which allows its users to choose their TLS stack betweennative-tls,opensslandrustls.This change implements a
custom_setuphook based onpostgres-native-tls, whose dependencies are mostly already in our lockfile anyway. The code was mostly adapted from the official example code indiesel-async.Before
diesel-asyncwe relied on the regulardieselconnection implementation, which usespg-sysunder the hood, which useslibpq, which uses OpenSSL for TLS. In other words: this slightly changes how TLS is handled for the database connections. Specifically, on OSX the server certificate is rejected as "not trusted" due to a long validity period. On production this should not be relevant, but during local development against the staging database this can cause issues, thus I've included a code comment with a temporary workaround for these cases.