Skip to content

Commit b893b83

Browse files
tianzhouclaude
andauthored
fix(postgres): re-read sslcert/sslkey/sslrootcert per connection so rotated certificates take effect (#463)
* fix(postgres): re-read sslcert/sslkey/sslrootcert per connection so rotated certificates take effect (#455) The DSN parser read each PEM file once and stored the contents as strings in the pool config, so a long-running server kept the certificate it started with for the life of the process. With short-lived client certificates reissued on a timer, every new connection failed once the cached pair expired, while the MCP transport stayed up and nothing upstream could tell. node-postgres copies the `ssl` object into the TLS options on every new connection (`Object.assign(options, ssl)` then `options.key = ssl.key`), which invokes property getters. Expose `ca`, `cert` and `key` as getters backed by a RotatingPemFile that re-reads the file on each access. This matches libpq, which opens sslcert/sslkey/sslrootcert per connection. Startup validation is unchanged: a missing file or encrypted key still fails `parse()` with FailedToReadCertificate. After that, a failed re-read or a rotated key that is encrypted falls back to the last good contents and logs a warning once per distinct problem, so the atomic-rename window of a rotation or a transiently missing file does not fail connections outright. The getters stay configurable because pg redefines `key` as non-enumerable to keep it out of logs; that only toggles enumerability and leaves the getter in place. The unit tests mirror that sequence. Co-Authored-By: Claude Fable 5.1 <[email protected]> * fix(postgres): reload PEMs in a pg.Client subclass and fail the connection instead of falling back Address review on #463: - Replace the per-property getters with a pg.Client subclass installed via PoolConfig.Client. Its connect() reads sslrootcert/sslcert/sslkey together before any socket is opened and writes them onto the shared ssl object, so a half-rotated cert/key pair can never reach TLS. - Drop the last-good-contents fallback. Like libpq, a connection whose PEM file cannot be read, or whose key is encrypted, fails with FailedToReadCertificate naming the file; the next connection retries. A getter cannot do this safely because pg copies the ssl object inside a stream 'data' listener outside its try block, where a throw would be an uncaught exception. - Add an integration test that rotates real certificate files and forces the pool to open a new connection, asserting on pg_stat_ssl.client_serial, plus one for the missing-file failure path. - Reword the docs: files are read at startup, not validated. Co-Authored-By: Claude Fable 5.1 <[email protected]> * docs(postgres): describe the cert/key read window accurately Co-Authored-By: Claude Fable 5.1 <[email protected]> --------- Co-authored-by: Claude Fable 5.1 <[email protected]>
1 parent 70cd17c commit b893b83

4 files changed

Lines changed: 312 additions & 15 deletions

File tree

‎docs/config/toml.mdx‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -441,7 +441,9 @@ Sources define database connections. Each source represents a database that DBHu
441441
<ParamField path="sslkey" type="string">
442442
Path to the PEM private key for `sslcert`. The key must be **unencrypted** (an encrypted key is rejected at startup; `sslpassword` is not supported). DER-encoded keys are not accepted.
443443

444-
Keep the key file private (`chmod 0600`); DBHub reads it once at startup and never logs its contents.
444+
Keep the key file private (`chmod 0600`); DBHub never logs its contents.
445+
446+
Both files are read at startup (a missing file or an encrypted key fails startup) and then re-read each time a new connection is opened, matching libpq. Short-lived client certificates that are reissued on a timer are picked up without a restart: replace the files in place (an atomic rename is safest) and the next connection presents the new pair. If a file cannot be read when a connection is opened, that connection fails with an error naming the file, and the next one retries. The same applies to `sslrootcert`.
445447

446448
Supports `~/` path expansion.
447449

‎src/connectors/__tests__/dsn-parser.test.ts‎

Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
22
import fs from 'fs';
33
import os from 'os';
44
import path from 'path';
5+
import pg from 'pg';
56
import { PostgresConnector } from '../postgres/index.js';
67
import { MySQLConnector } from '../mysql/index.js';
78
import { MariaDBConnector } from '../mariadb/index.js';
@@ -221,6 +222,120 @@ describe('DSN Parser - PostgreSQL client certificate (sslcert/sslkey)', () => {
221222
});
222223
});
223224

225+
describe('DSN Parser - PostgreSQL certificate rotation (sslrootcert/sslcert/sslkey)', () => {
226+
const parser = new PostgresConnector().dsnParser;
227+
let tempDir: string;
228+
let caPath: string;
229+
let certPath: string;
230+
let keyPath: string;
231+
const CA = '-----BEGIN CERTIFICATE-----\nca\n-----END CERTIFICATE-----\n';
232+
const CERT = '-----BEGIN CERTIFICATE-----\nclient\n-----END CERTIFICATE-----\n';
233+
const KEY = '-----BEGIN PRIVATE KEY-----\nclient\n-----END PRIVATE KEY-----\n';
234+
const NEW_CA = '-----BEGIN CERTIFICATE-----\nca-rotated\n-----END CERTIFICATE-----\n';
235+
const NEW_CERT = '-----BEGIN CERTIFICATE-----\nclient-rotated\n-----END CERTIFICATE-----\n';
236+
const NEW_KEY = '-----BEGIN PRIVATE KEY-----\nclient-rotated\n-----END PRIVATE KEY-----\n';
237+
const ENCRYPTED_KEY =
238+
'-----BEGIN ENCRYPTED PRIVATE KEY-----\nx\n-----END ENCRYPTED PRIVATE KEY-----\n';
239+
240+
const dsn = () =>
241+
'postgres://user:pass@localhost:5432/db?sslmode=verify-full' +
242+
`&sslrootcert=${encodeURIComponent(caPath)}` +
243+
`&sslcert=${encodeURIComponent(certPath)}&sslkey=${encodeURIComponent(keyPath)}`;
244+
245+
type ClientCtor = new (config: pg.PoolConfig) => pg.Client;
246+
/** The pool constructs one client per physical connection from config.Client. */
247+
const newPoolClient = (config: pg.PoolConfig): pg.Client =>
248+
new (config.Client as unknown as ClientCtor)(config);
249+
const pems = (config: pg.PoolConfig) => {
250+
const ssl = config.ssl as { ca?: string; cert?: string; key?: string };
251+
return { ca: ssl.ca, cert: ssl.cert, key: ssl.key };
252+
};
253+
254+
beforeEach(() => {
255+
tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'dbhub-certrotation-test-'));
256+
caPath = writeTempPem(tempDir, 'ca.pem', CA);
257+
certPath = writeTempPem(tempDir, 'client.crt', CERT);
258+
keyPath = writeTempPem(tempDir, 'client.key', KEY);
259+
// Stop at the point where pg would open a socket; the PEM reload happens before it.
260+
vi.spyOn(pg.Client.prototype, 'connect').mockImplementation(function (
261+
this: pg.Client,
262+
callback?: (err: Error) => void
263+
) {
264+
if (callback) {
265+
callback(undefined as unknown as Error);
266+
return;
267+
}
268+
return Promise.resolve();
269+
} as typeof pg.Client.prototype.connect);
270+
});
271+
272+
afterEach(() => {
273+
vi.restoreAllMocks();
274+
fs.rmSync(tempDir, { recursive: true, force: true });
275+
});
276+
277+
it('installs a custom pool Client only when PEM files are configured', async () => {
278+
const withPems = await parser.parse(dsn());
279+
expect(withPems.Client).toBeDefined();
280+
const withoutPems = await parser.parse('postgres://user:pass@localhost:5432/db?sslmode=require');
281+
expect(withoutPems.Client).toBeUndefined();
282+
});
283+
284+
it('re-reads rotated PEM files when a new connection is opened', async () => {
285+
const config = await parser.parse(dsn());
286+
expect(config.ssl).toEqual({ rejectUnauthorized: true, ca: CA, cert: CERT, key: KEY });
287+
288+
fs.writeFileSync(caPath, NEW_CA);
289+
fs.writeFileSync(certPath, NEW_CERT);
290+
fs.writeFileSync(keyPath, NEW_KEY);
291+
292+
await newPoolClient(config).connect();
293+
// pg marks ssl.key non-enumerable when a Client is constructed, so
294+
// compare the fields explicitly rather than with toEqual.
295+
expect(pems(config)).toEqual({ ca: NEW_CA, cert: NEW_CERT, key: NEW_KEY });
296+
expect(pg.Client.prototype.connect).toHaveBeenCalledTimes(1);
297+
});
298+
299+
it('fails the new connection, naming the file, when a PEM file cannot be read', async () => {
300+
const config = await parser.parse(dsn());
301+
fs.rmSync(keyPath);
302+
303+
const err = await newPoolClient(config).connect().catch((e: unknown) => e as Error);
304+
expect((err as Error).name).toBe('FailedToReadCertificate');
305+
expect((err as Error).message).toContain(`Failed to read SSL client key at '${keyPath}'`);
306+
expect(pg.Client.prototype.connect).not.toHaveBeenCalled();
307+
});
308+
309+
it('reports the failure through the callback form of connect as well', async () => {
310+
const config = await parser.parse(dsn());
311+
fs.rmSync(certPath);
312+
313+
const err = await new Promise<Error>((resolve) => newPoolClient(config).connect(resolve));
314+
expect(err.message).toContain(`Failed to read SSL client certificate at '${certPath}'`);
315+
expect(pg.Client.prototype.connect).not.toHaveBeenCalled();
316+
});
317+
318+
it('never publishes a half-rotated cert/key pair', async () => {
319+
const config = await parser.parse(dsn());
320+
fs.writeFileSync(certPath, NEW_CERT);
321+
fs.rmSync(keyPath);
322+
323+
await expect(newPoolClient(config).connect()).rejects.toThrow('Failed to read SSL client key');
324+
// The pair in the pool config is still the one that was read together.
325+
expect(pems(config)).toEqual({ ca: CA, cert: CERT, key: KEY });
326+
});
327+
328+
it('rejects a rotated key that is encrypted', async () => {
329+
const config = await parser.parse(dsn());
330+
fs.writeFileSync(keyPath, ENCRYPTED_KEY);
331+
332+
await expect(newPoolClient(config).connect()).rejects.toThrow(
333+
'encrypted private keys are not supported'
334+
);
335+
expect(pg.Client.prototype.connect).not.toHaveBeenCalled();
336+
});
337+
});
338+
224339
describe('DSN Parser - PostgreSQL query timeout', () => {
225340
it('configures a server-side statement timeout before the client fallback', async () => {
226341
const parser = new PostgresConnector().dsnParser;

‎src/connectors/__tests__/postgres-client-cert.integration.test.ts‎

Lines changed: 73 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,9 @@ function generateCerts(dir: string): void {
3838
'req', '-x509', '-newkey', 'rsa:2048', '-nodes', '-days', '1',
3939
'-subj', '/CN=dbhub-test-ca', '-keyout', 'ca.key', '-out', 'ca.crt',
4040
]);
41-
for (const [name, cn] of [['server', 'localhost'], ['client', DB_USER]]) {
41+
// client and client-rotated share a CN so both authenticate as DB_USER; they
42+
// differ by serial number, which is how the rotation test tells them apart.
43+
for (const [name, cn] of [['server', 'localhost'], ['client', DB_USER], ['client-rotated', DB_USER]]) {
4244
openssl([
4345
'req', '-newkey', 'rsa:2048', '-nodes', '-subj', `/CN=${cn}`,
4446
'-keyout', `${name}.key`, '-out', `${name}.csr`,
@@ -143,6 +145,76 @@ describe.skipIf(!hasOpenssl())('PostgreSQL client certificate authentication', (
143145
}
144146
});
145147

148+
/** Decimal serial of a generated certificate, as pg_stat_ssl.client_serial reports it. */
149+
const serialOf = (name: string): string => {
150+
const out = execFileSync('openssl', ['x509', '-noout', '-serial', '-in', path.join(certDir, `${name}.crt`)])
151+
.toString()
152+
.trim();
153+
return BigInt(`0x${out.split('=')[1]}`).toString();
154+
};
155+
156+
/** Copy a generated client cert/key pair over the files the DSN points at. */
157+
const installClientPair = (dir: string, name: string): void => {
158+
fs.copyFileSync(path.join(certDir, `${name}.crt`), path.join(dir, 'client.crt'));
159+
fs.copyFileSync(path.join(certDir, `${name}.key`), path.join(dir, 'client.key'));
160+
};
161+
162+
const SERIAL_SQL =
163+
'SELECT pg_sleep(0.5), client_serial::text AS serial FROM pg_stat_ssl WHERE pid = pg_backend_pid()';
164+
165+
it('should present a client certificate rotated on disk to new pool connections', async () => {
166+
const rotatingDir = fs.mkdtempSync(path.join(certDir, 'rotating-'));
167+
installClientPair(rotatingDir, 'client');
168+
const dsn =
169+
`${baseUri}?sslmode=verify-ca&sslrootcert=${encodeURIComponent(path.join(certDir, 'ca.crt'))}` +
170+
`&sslcert=${encodeURIComponent(path.join(rotatingDir, 'client.crt'))}` +
171+
`&sslkey=${encodeURIComponent(path.join(rotatingDir, 'client.key'))}`;
172+
const connector = new PostgresConnector();
173+
try {
174+
await connector.connect(dsn);
175+
const before = await connector.executeSQL(SERIAL_SQL, {});
176+
expect(before.resultSets[0].rows[0].serial).toBe(serialOf('client'));
177+
178+
installClientPair(rotatingDir, 'client-rotated');
179+
180+
// Two overlapping queries: one reuses the idle connection, the other
181+
// forces the pool to open a new one, which must present the new pair.
182+
const results = await Promise.all([
183+
connector.executeSQL(SERIAL_SQL, {}),
184+
connector.executeSQL(SERIAL_SQL, {}),
185+
]);
186+
const serials = results.map((r) => String(r.resultSets[0].rows[0].serial)).sort();
187+
expect(serials).toEqual([serialOf('client'), serialOf('client-rotated')].sort());
188+
} finally {
189+
await connector.disconnect();
190+
}
191+
});
192+
193+
it('should fail new pool connections, naming the file, when a rotated key is missing', async () => {
194+
const rotatingDir = fs.mkdtempSync(path.join(certDir, 'rotating-'));
195+
installClientPair(rotatingDir, 'client');
196+
const keyPath = path.join(rotatingDir, 'client.key');
197+
const dsn =
198+
`${baseUri}?sslmode=require&sslcert=${encodeURIComponent(path.join(rotatingDir, 'client.crt'))}` +
199+
`&sslkey=${encodeURIComponent(keyPath)}`;
200+
const connector = new PostgresConnector();
201+
try {
202+
await connector.connect(dsn);
203+
fs.rmSync(keyPath);
204+
205+
const outcomes = await Promise.allSettled([
206+
connector.executeSQL(SERIAL_SQL, {}),
207+
connector.executeSQL(SERIAL_SQL, {}),
208+
]);
209+
// The existing connection keeps working; only the new one fails.
210+
expect(outcomes.filter((o) => o.status === 'fulfilled')).toHaveLength(1);
211+
const rejected = outcomes.find((o) => o.status === 'rejected') as PromiseRejectedResult;
212+
expect(rejected.reason.message).toContain(`Failed to read SSL client key at '${keyPath}'`);
213+
} finally {
214+
await connector.disconnect();
215+
}
216+
});
217+
146218
it('should be rejected by the server without a client certificate', async () => {
147219
const connector = new PostgresConnector();
148220
try {

0 commit comments

Comments
 (0)