Reload the PostgreSQL client certificate on each connection - #111
Open
arpitjain099 wants to merge 1 commit into
Open
Reload the PostgreSQL client certificate on each connection#111arpitjain099 wants to merge 1 commit into
arpitjain099 wants to merge 1 commit into
Conversation
ParseConfig reads sslcert and sslkey once and stores the result in TLSConfig.Certificates, and that config backs every connection the pool opens afterwards. Where the certificate is rotated underneath a running process, as with a cert-manager CSI volume, connections established after the original expires are still offered the expired certificate and the server rejects them, even though the files on disk hold a valid one. The only way out today is to restart the process. Install a GetClientCertificate callback instead, which crypto/tls calls per handshake, and clear the eagerly loaded copy so it cannot be reached. The callback goes back through ParseConfig rather than reading the files itself, so the DSN keeps being interpreted the way libpq does, including the keyword and URL forms and the PGSSLCERT and PGSSLKEY environment variables, without a second implementation to keep in step. sslmode=prefer and friends leave the non-TLS attempt in Fallbacks with their own TLSConfig, so those are covered too. A DSN with no client certificate is left alone. Fixes smallstep#110 Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
|
|
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
3 participants
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.
Fixes #110.
Confirmed the mechanism before changing anything.
pgx.ParseConfigreadssslcert/sslkeyat parse time and leaves the result in the TLS config that every later connection uses:Openparses once and hands that config topgxstdlib.OpenDB, so a certificate rotated afterwards is never looked at again.This installs a
GetClientCertificatecallback, whichcrypto/tlsinvokes per handshake, and clearsCertificatesso the stale copy is not reachable at all.The one design choice worth explaining: the callback calls
pgx.ParseConfig(dataSourceName)again rather than reading the two files directly. Reading them directly would mean re-implementing how libpq resolves them — URL versus keyword DSN forms,PGSSLCERT/PGSSLKEY, and anything else pgx honours — and keeping that second implementation in step. Going back throughParseConfigkeeps one interpretation of the DSN. It costs a parse per handshake, which is rare next to queries on a pooled connection.sslmode=preferand friends leave the non-TLS attempt inFallbackswith its ownTLSConfig, so those get the same treatment.Tests
Three cases in a new
postgresql_tls_test.go, none of which need a PostgreSQL server:cert-A, wire it up, assert the handshake would offercert-A; overwrite the files withcert-Bas a CSI volume would, assert it now offerscert-BParseConfigreally did load one certificate, then that it is gone afterwards, so a later change preferringCertificatesover the callback cannot quietly reintroduce thissslcert/sslkeyis untouched, forsslmode=requireandsslmode=disableAgainst the current behaviour the first two fail with
no GetClientCertificate callback installedandexpected the eagerly loaded certificate to be cleared, got 1.go build ./...,go vet ./postgresql/,gofmtandgo test ./...are clean, andgo build -tags nopgx ./...still builds.What I have not done is exercise a real rotation against a live PostgreSQL with mutual TLS; the tests assert on what the handshake would be handed rather than on a completed connection. If you would like that as an integration test I am happy to add it, though it needs a server with client-cert auth in CI.