CLI database TLS: honour sslmode/sslrootcert, bundle the Supabase root CA, name the fix in cert errors - #903
Conversation
🦋 Changeset detectedLatest commit: 67dfa05 The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
freshtonic
left a comment
There was a problem hiding this comment.
Request changes — one connection site escaped the sweep, and it's the one that matters most.
Blocking
stash encrypt backfill bypasses the TLS layer. packages/cli/src/commands/encrypt/backfill.ts:118 builds its connection directly:
const pool = new pg.Pool({
connectionString: stashConfig.databaseUrl,
max: 2,
})The PR's sweep counted pg.Client construction sites — this is the sole pg.Pool, and it never touches buildPgClientConfig. Consequences, against a standard Supabase URL (sslmode=require):
- pg still applies its alias-to-verify-full handling itself, so the #822 SECURITY WARNING still prints on every backfill run — the regression this PR fixes everywhere else.
- Certificate verification still fails with the raw
self-signed certificate in certificate chain, with no bundled Supabase CA and noexplainTlsError()shaping — i.e. the exact discoverability gap that pushed the Lovable agent toNODE_TLS_REJECT_UNAUTHORIZED=0(#889) stays live on the long-running, plaintext-handling command where a process-wide TLS off switch is most dangerous. - The changeset and the new
stash-cliskill section both claim "Every CLI database connection honourssslmodeandsslrootcert" — currently false for backfill, and the skill ships that claim into customer repos.
Fix is mechanical since pg.PoolConfig extends pg.ClientConfig:
const pool = new pg.Pool({
...buildPgClientConfig(stashConfig.databaseUrl),
max: 2,
})…plus wiring explainTlsError into backfill's connect-failure path. Given how this slipped through, consider a small lint-style test asserting no new pg.Client(/new pg.Pool( in packages/cli/src outside src/db/ and tests — same spirit as lintWiring.test.ts — so the fifteenth site can't bypass either.
What I verified independently (all good)
- The vendored Supabase root CA is genuine. I extracted the PEM from the branch and fingerprinted it: SHA-256
80:70:25:AD:…:E6:CA:FA, matching the file header — and byte-identical toprod-ca-2021.crtfetched fresh fromsupabase/cli(apps/cli-go/internal/gen/types/templates/). Subject and 2031 expiry match too. Appending it to the system roots (rather than replacing) is the right call, and verify-full's hostname check plus the*.supabase.co|comscoping keep the added trust narrow. - All
pg.Clientproduction sites route through the factory — grepped the branch; the only direct constructions left are live-test files against local databases. config.tspolicy logic: passthrough onsslcert/sslkey/ssland on TLS-param-free URLs is byte-untouched as claimed; non-URL-parseable strings (bare socket paths, multi-host) fall through to pg verbatim; libpq sole-trust-anchor semantics forsslrootcert=<path>,systemhandled, unreadable file fails loudly. Therequire/verify-ca/prefer→ verify-full preservation is correctly documented as behaviour-preserving, not tightening.- Tests: the 21 config-builder cases cover every arm including the CA resolution order and the once-per-process warning, and the #822 regression test pins the fix. CI green; e2e green.
Nit (non-blocking)
supabase-ca.tsheader: "seeresolveCain client.ts" —resolveCalives inconfig.ts.
Route backfill through the builder and this is ready — the design and the CA provenance work are excellent.
…hape TLS errors Fixes the TLS story the Lovable report exposed (#889) and the node-postgres sslmode advisory passthrough (#822). - New src/db/config.ts builds every CLI pg.ClientConfig: it consumes sslmode/sslrootcert from the connection string and hands pg an explicit ssl config with those params stripped. verify-full semantics are kept for require/verify-ca/prefer (node-postgres's current behaviour), no-verify is honoured with a one-line stderr warning, disable turns TLS off, and client-certificate URLs (sslcert/sslkey/ssl) pass through untouched — as do URLs with no TLS params at all. Because the URL pg receives carries no sslmode, the upstream 'aliases for verify-full' SECURITY WARNING is gone (regression-tested). - CA resolution: sslrootcert path (libpq semantics — sole trust anchor; 'system' selects the system store) → PGSSLROOTCERT → bundled Supabase root CA for *.supabase.co|com hosts (appended to the system roots so a future move to a public CA keeps verifying) → system store. The vendored CA (src/db/supabase-ca.ts) was captured from a live pooler handshake and verified byte-identical to the copy in supabase/cli; provenance and rotation notes are in the file. - explainTlsError() turns certificate-verification failures into a message naming the host and the supported remedies in order, explicitly warning against NODE_TLS_REJECT_UNAUTHORIZED=0 (process-wide — it would also disable verification for CipherStash credential traffic). Wired into the installer's connect wrappers and db test-connection. - src/db/client.ts is now the one place the CLI constructs pg.Client; all fourteen construction sites go through it (eql repair's applied probe uses the config builder with its existing lazy pg import, which stays runtime-import-free on pg). - Live-verified against aws-0-us-east-1.pooler.supabase.com: verify-full with the bundled CA reaches the pooler's auth layer; the control without it reproduces 'self-signed certificate in certificate chain'. - Skills: stash-cli (TLS section) and stash-supabase (verify-full works out of the box; never NODE_TLS_REJECT_UNAUTHORIZED=0).
Review: the sweep counted pg.Client sites and missed the CLI's one pg.Pool — encrypt backfill connected with a raw connectionString, so the #822 advisory still printed there and a Supabase cert failure still surfaced raw, on the long-running plaintext-handling command where the NODE_TLS_REJECT_UNAUTHORIZED=0 workaround is most dangerous. - The pool now spreads buildPgClientConfig(...) (pg.PoolConfig extends pg.ClientConfig), inheriting sslmode/sslrootcert handling and the bundled Supabase CA. - A certificate-verification failure on pool.connect() is routed through BackfillConfigError so explainTlsError's remedy prints verbatim — the generic backfill handler deliberately suppresses message text (plaintext leak guard), which would have buried the fix; cert errors are author-shaped and row-data-free. - New lint-style test (pg-construction-lint.test.ts, same spirit as protect-ffi's lintWiring): any new pg.Client(/new pg.Pool( in production src must pass buildPgClientConfig or be the factory itself, so the sixteenth connection site cannot bypass the layer. - Nit: supabase-ca.ts header now points at resolveCa in config.ts (not client.ts).
|
Addressed in 14780d6. Blocker — Lint-style guard — taken: Nit — fixed: The changeset/skill claim "every CLI database connection" is true again — and now enforced rather than asserted. Full unit+live and e2e suites green. |
|
Review findings from GPT-5.6 Sol:
I, GPT-5.6 Sol, performed this analysis. |
…y in the factory Codex review findings on the TLS layer: - Environment tier: a URL with no TLS parameters now consults PGSSLMODE (mirroring node-postgres's own recognised values exactly — disable, prefer/require/verify-ca/verify-full, no-verify; anything else stays a pure passthrough). Previously the builder passed such URLs through and pg enabled TLS from PGSSLMODE itself while ignoring PGSSLROOTCERT entirely, so an env-configured Supabase connection missed the bundled CA and failed verification. URL parameters win (libpq precedence), and the verify arms run the same CA resolution as the URL tier. - Central error shaping: createPgClient now wraps connect() to re-throw certificate-verification failures as TlsVerificationError carrying the shaped remedy, so every command that awaits connect() — encrypt plan/drop/status, eql validate, init's introspection, db status — surfaces the host-specific fix without knowing about TLS. The installer and test-connection rethrow/print it verbatim instead of re-shaping (nesting) or re-framing it; backfill keeps its explicit explainTlsError call since pg.Pool is not factory-wrapped. - Proven end-to-end against the live Supabase pooler: PGSSLMODE=verify-full with a deliberately wrong PGSSLROOTCERT produces the shaped remedy from db test-connection; unit coverage for every env-tier arm and the connect wrapper (TLS error shaped, non-TLS passthrough, success untouched).
|
Both Codex findings addressed in 67dfa05. P2 — PGSSLMODE/PGSSLROOTCERT env tier: confirmed against pg's source ( P2 — central TLS error shaping: Proof: end-to-end against the live Supabase pooler — |
freshtonic
left a comment
There was a problem hiding this comment.
Approve. Both the blocker and the nit from my previous review are fixed, and the two follow-up commits strengthen the design beyond what I asked for. Re-reviewed the full delta (786a5211..67dfa052); CI is green.
The blocker, resolved properly
encrypt backfillnow routes through the TLS layer:new pg.Pool({ ...buildPgClientConfig(url), max: 2 }), exactly the shape needed sincePoolConfigextendsClientConfig.- The connect-failure wiring is better than the straightforward fix: routing the shaped TLS explanation through
BackfillConfigErroris the right call, because backfill's generic error handler deliberately suppresseserror.message(plaintext-leak guard) — a naivep.log.error(message)would have either leaked or buried the remedy. This threads it through the one path that prints author-controlled diagnostics verbatim. - The regression guard landed (
pg-construction-lint.test.ts): anynew pg.Client(/new pg.Pool(in productionsrc/outsidedb/client.tsmust showbuildPgClientConfigat the call site. The sixteenth connection site now fails a test instead of a review.
The follow-ups, reviewed
TlsVerificationError+ the factory connect wrapper is a genuine improvement over per-call-siteexplainTlsError: every command that awaitsconnect()now surfaces the host-specific remedy with zero knowledge of TLS, the installer sites collapse to a clean rethrow-if-shaped pattern, and the double-framing risk ("Failed to connect: TLS certificate verification failed…") is gone. The promise-onlyconnectassertion is justified — no CLI call site uses the callback overload — and the wrap has direct unit coverage (shaped rethrow, non-TLS passthrough untouched by identity, success path).- The
PGSSLMODEenvironment tier closes a real hole I hadn't flagged: previously an env-configured connection fell through to pg's own env handling, which enables verification but ignoresPGSSLROOTCERTentirely — so it verified against the wrong trust anchors and skipped the bundled Supabase CA. The implementation mirrors pg's recognised value list exactly, keeps libpq precedence (URL wins — tested withPGSSLMODE=requirevs?sslmode=disable), and stays a pure passthrough on unrecognised values. Since the explicitsslconfig takes precedence over pg'sreadSSLConfigFromEnvironment, there's no double-handling. - The nit is fixed (
resolveCapointer now saysconfig.ts), and the changeset +stash-cliskill were updated to match the new env-tier and central-shaping claims — the "every CLI database connection" sentence is now true.
Nothing further. Nice work — the earlier CA provenance verification stands, and this is now a coherent, guarded connection layer rather than a swept set of call sites.
Fixes #889, fixes #822. Stacked on #902.
The Lovable agent got past a TLS failure by setting
NODE_TLS_REJECT_UNAUTHORIZED=0— process-wide, covering the connections that carry ZeroKMS credentials — because the CLI did nothing with TLS and the raw error offered no better path. This PR makes the right thing the easy thing.What changed
One TLS-aware connection layer
packages/cli/src/db/config.tsnow builds every CLIpg.ClientConfig(all fourteenpg.Clientconstruction sites route throughsrc/db/client.ts;eql repair's applied probe keeps its lazypgimport and uses the config builder, which is runtime-import-free onpg):sslmodeandsslrootcertare consumed from the connection string and turned into an explicitsslconfig; the URL pg receives has them stripped.require/verify-ca/preferkeep verify-full semantics — node-postgres's current behaviour, preserved rather than tightened.sslmode=no-verifyis honoured, with a once-per-process stderr warning: encrypted, not authenticated.sslmode=disableturns TLS off.sslcert/sslkey/ssl), pass through byte-untouched — zero behaviour change outside the parameters handled.CA resolution — Supabase verifies out of the box
First hit wins:
sslrootcert=<path>(libpq semantics — sole trust anchor;sslrootcert=systemselects the system store) →PGSSLROOTCERT→ for*.supabase.co|comhosts, a bundled Supabase root CA appended to the system roots (so a future Supabase move to a publicly-trusted CA keeps verifying) → system store.The vendored CA (
src/db/supabase-ca.ts) was captured from a live handshake withaws-0-us-east-1.pooler.supabase.com:5432and verified byte-identical (DER) to the copy Supabase vendors in its own CLI repo; subject, SHA-256 fingerprint, expiry (2031), and rotation notes are in the file header.Errors name the fix, not the symptom (#889 §3)
explainTlsError()turns cert-verification failures into a message naming the failing host and the supported remedies in order —sslrootcert=<path>first,sslmode=no-verifylast with the consequence spelled out — and explicitly warns never to setNODE_TLS_REJECT_UNAUTHORIZED=0. Wired into the installer's connect wrappers anddb test-connection.#822 fixed as a side effect
Because pg no longer sees
sslmodein the connection string, its "SSL modes … are treated as aliases for verify-full" SECURITY WARNING no longer prints on every invocation against production URLs. Regression-tested (no-sslmode-advisory.test.ts).Verification
sslmode=verify-fullwith the bundled CA gets past TLS to the pooler's tenant-lookup error; the control without the CA reproducesself-signed certificate in certificate chainexactly.stash-cligains a "TLS to the database" section;stash-supabasenotes verify-full works out of the box.