Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions docs/runtime/sql.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -588,6 +588,10 @@ Without a connection URL, Bun checks these individual parameters:
| `PGDATABASE` | - | username | Database name |
| `PGSSLMODE` | - | `disable` | SSL mode (`disable`, `allow`, `prefer`, `require`, `verify-ca`, `verify-full`) |

`PGSSLMODE` also applies when a connection URL is set. A URL from `TLS_POSTGRES_DATABASE_URL` or `TLS_DATABASE_URL` uses at least the `require` SSL mode. `PGSSLMODE=verify-ca` and `PGSSLMODE=verify-full` raise that mode and verify the server certificate. For a certificate from a private CA, pass the CA in the `tls` option: see [Custom CA Certificates](#custom-ca-certificates).

An `sslmode`, `ssl` or `tls` parameter in the URL takes priority over the variable name and over `PGSSLMODE`. An explicit `tls` or `ssl` option takes priority over all three.

### SQLite Environment Variables

You can configure SQLite connections with `DATABASE_URL` when it contains a SQLite-compatible URL:
Expand Down
10 changes: 7 additions & 3 deletions src/js/internal/sql/shared.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1861,10 +1861,14 @@ function parseOptions(
// The rest of this function is logic specific to postgres/mysql/mariadb (they have the same options object)

let sslMode: SSLMode = sslModeFromConnectionDetails || SSLMode.disable;
if (sslMode === SSLMode.disable) {
if (adapter === "postgres") {
// libpq honours PGSSLMODE as the default; a URL ?sslmode= below overrides it.
const envSslMode = adapter === "postgres" ? env.PG_SSLMODE || env.PGSSLMODE : undefined;
if (envSslMode) sslMode = normalizeSSLMode(envSslMode);
const envSslMode = env.PG_SSLMODE || env.PGSSLMODE;
if (envSslMode) {
// A TLS_* URL variable is a floor of `require`: PGSSLMODE raises the mode, it does not lower it.
const envMode = normalizeSSLMode(envSslMode);
if (envMode > sslMode) sslMode = envMode;
Comment thread
robobun marked this conversation as resolved.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Apps passing tls.ca or rejectUnauthorized: true next to a TLS_* URL lose hostname verification when the environment has PGSSLMODE=verify-ca; the base verified the name. shared.ts:1870 raises sslMode to 3 first, so the sslMode < SSLMode.verify_ca guard at shared.ts:2158 skips the promotion to verify_full the base made from require. Fix: an env-derived mode must never lower what explicit tls options resolve to, e.g. apply the PGSSLMODE floor after the shared.ts:2158 upgrade, and change the expectation at adapter-env-var-precedence.test.ts:490. The PR lists this as a downside; for TLS_* users it is a 4->3 downgrade from base.

Why this was flagged

Environment has TLS_DATABASE_URL (or TLS_POSTGRES_DATABASE_URL) set to a postgres URL and PGSSLMODE=verify-ca, and the program calls new SQL({ tls: { ca: bundle } }) or new SQL({ tls: { rejectUnauthorized: true } }). On the base branch shared.ts:1864 skipped the env read because sslMode was already require (2); then shared.ts:2158-2161 saw $isObject(tls) && sslMode < SSLMode.verify_ca with tls.ca or rejectUnauthorized === true and set sslMode = SSLMode.verify_full (4). After this change shared.ts:1866-1870 sets sslMode = 3 before that block, the sslMode < SSLMode.verify_ca guard is false, and sslMode stays verify_ca (3). In src/sql_jsc/postgres/PostgresSQLConnection.rs:893-896 native_identity_hostname returns None unless ssl_mode == SSLMode::VerifyFull, so PostgresSQLConnection.rs:906-931 checks only the chain and never the server name. With rejectUnauthorized: true and no ca, any publicly trusted certificate for any host now passes. The new test at test/js/sql/adapter-env-var-precedence.test.ts:490-496 locks the 3 in.

Verification: After the change shared.ts:1864-1870 reads the env and sets sslMode=3; the promotion at shared.ts:2158-2161 is now skipped by the < verify_ca guard, so sslMode stays 3. src/sql_jsc/postgres/PostgresSQLConnection.rs:893-896 native_identity_hostname returns a hostname only when ssl_mode == SSLMode::VerifyFull, so a certificate from the trusted CA issued for a different host is now accepted where the base rejected it.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The facts are right. In this one shape the resolved mode goes from 4 to 3 next to a TLS_* URL, and the hostname check is lost. The PR lists it under Downsides. I kept it because it is the rule that main applies on every other URL source:

  • DATABASE_URL + PGSSLMODE=verify-ca + tls: { ca } resolves 3 on main and on 1.4.2. So does tls: { rejectUnauthorized: true }. A verify-ca that something names is kept, and the options supply the CA.
  • Base resolved 4 next to a TLS_* URL only because it never read PGSSLMODE there. That is the bug.

The suggested fix does not hold as written. A PGSSLMODE floor after the upgrade at shared.ts:2158 also raises the mode again after a URL ?sslmode= or after tls: false. The URL and the options then no longer override the environment, and the "URL ?sslmode= overrides PGSSLMODE" tests pin that order.

Two changes keep 4 in this shape. Each one is a decision for a maintainer:

  1. Let the tls options lift an environment verify-ca to verify-full on every URL source. That changes DATABASE_URL + PGSSLMODE=verify-ca + tls.ca from 3 to 4, and a deployment whose certificate name does not match the host stops connecting.
  2. Do that only next to a TLS_* URL. That needs a flag that five later assignments clear, and the same environment then resolves differently by URL variable.

I left this thread open for that decision.

}
Comment thread
robobun marked this conversation as resolved.
}

let url = _url;
Expand Down
92 changes: 92 additions & 0 deletions test/js/sql/adapter-env-var-precedence.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,98 @@ describe("SQL adapter environment variable precedence", () => {
expect(options.options.sslMode).toBe(2);
expect(options.options.tls).toBeTypeOf("object");
});

// The TLS_ variable name asks for at least `require`. It is a floor for PGSSLMODE, not a ceiling.
describe.each(["TLS_DATABASE_URL", "TLS_POSTGRES_DATABASE_URL"])("next to a URL from %s", urlVariable => {
const url = "postgres://user@host:5432/db";

test.each([
["PGSSLMODE", "disable", 2],
["PGSSLMODE", "allow", 2],
["PGSSLMODE", "prefer", 2],
["PGSSLMODE", "require", 2],
["PGSSLMODE", "verify-ca", 3],
["PGSSLMODE", "verify-full", 4],
["PG_SSLMODE", "prefer", 2],
["PG_SSLMODE", "verify-ca", 3],
["PG_SSLMODE", "verify-full", 4],
])("%s=%s selects sslMode %d", (modeVariable, mode, expected) => {
process.env[urlVariable] = url;
process.env[modeVariable] = mode;

expect(new SQL().options).toMatchObject({
adapter: "postgres",
hostname: "host",
sslMode: expected,
tls: { serverName: "host" },
});
});

test.each([
["a ?ssl=true in the URL", "?ssl=true", {}],
["an explicit tls: true", "", { tls: true }],
["an explicit tls: {}", "", { tls: {} }],
["an explicit adapter", "", { adapter: "postgres" }],
] as const)("PGSSLMODE=verify-full applies with %s", (_, query, options) => {
process.env[urlVariable] = url + query;
process.env.PGSSLMODE = "verify-full";

expect(new SQL(options).options).toMatchObject({ sslMode: 4, tls: { serverName: "host" } });
});

test.each([
["?sslmode=disable", 0],
["?sslmode=prefer", 1],
["?sslmode=require", 2],
["?sslmode=verify-ca", 3],
["?ssl=false", 0],
])("a URL %s overrides PGSSLMODE=verify-full", (query, expected) => {
process.env[urlVariable] = url + query;
process.env.PGSSLMODE = "verify-full";

expect(new SQL().options.sslMode).toBe(expected);
});

test.each([
["tls: false", { tls: false }, 0],
["ssl: 'prefer'", { ssl: "prefer" }, 1],
] as const)("an explicit %s overrides PGSSLMODE=verify-full", (_, options, expected) => {
process.env[urlVariable] = url;
process.env.PGSSLMODE = "verify-full";

expect(new SQL(options).options.sslMode).toBe(expected);
});

test("PGSSLMODE=verify-ca is kept when the options give a CA", () => {
process.env[urlVariable] = url;
process.env.PGSSLMODE = "verify-ca";

// verify-ca already checks the chain, so the CA does not raise the mode to verify-full.
expect(new SQL({ tls: { ca: "x" } }).options.sslMode).toBe(3);
});

test("an invalid PGSSLMODE throws", () => {
process.env[urlVariable] = url;
process.env.PGSSLMODE = "bogus";

expect(() => new SQL()).toThrow(
expect.objectContaining({ code: "ERR_INVALID_ARG_VALUE", message: expect.stringContaining("sslmode") }),
);
});
});

test.each([
["TLS_MYSQL_DATABASE_URL", "mysql://user@host:3306/db", "mysql"],
["TLS_MARIADB_DATABASE_URL", "mariadb://user@host:3306/db", "mariadb"],
["TLS_DATABASE_URL", "mysql://user@host:3306/db", "mysql"],
])("PGSSLMODE is not read next to %s=%s", (urlVariable, url, adapter) => {
process.env[urlVariable] = url;

for (const mode of ["verify-full", "bogus"]) {
process.env.PGSSLMODE = mode;
expect(new SQL().options).toMatchObject({ adapter, sslMode: 2 });
}
});
});

describe("TLS settings from the connection URL query string", () => {
Expand Down
108 changes: 105 additions & 3 deletions test/js/sql/postgres-pgsslmode-env.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@
// Buffer.alloc frame construction here.

import { expect, test } from "bun:test";
import { bunEnv, bunExe } from "harness";
import { bunEnv, bunExe, tls as selfSignedTls } from "harness";
import type net from "node:net";
import tls from "node:tls";
import { listeningServer, pgAuthenticationOk, pgReadyForQuery, pgSSLResponse } from "./wire-frames";

// Bun.SQL picks up PGHOST/PGPORT/PGUSER/PGPASSWORD/PGDATABASE from the
Expand All @@ -22,7 +24,7 @@ const fixture = /* js */ `
const sql = new SQL({ max: 1, connectionTimeout: 5 });
try {
await sql.connect();
console.log("CONNECTED_PLAINTEXT");
console.log("CONNECTED");
} catch (e) {
console.log("ERROR:" + (e?.code ?? e?.message ?? String(e)));
} finally {
Expand Down Expand Up @@ -55,11 +57,59 @@ async function plaintextOnlyServer() {
});
}

async function selfSignedTlsServer() {
// A mock and not the postgres_tls container: the tests need what a real server
// cannot report, which is whether a StartupMessage arrived and over what.
//
// Answers SSLRequest with 'S' and upgrades to TLS with the harness certificate:
// self-signed, so no trust store accepts it, with a SAN for 127.0.0.1. Answers a
// StartupMessage, over TLS or in plaintext, with AuthenticationOk + ReadyForQuery.
const ready = Buffer.concat([pgAuthenticationOk(), pgReadyForQuery("I")]);
const startups: ("tls" | "plaintext")[] = [];
const acceptStartup = (socket: net.Socket, via: "tls" | "plaintext", buffered = Buffer.alloc(0)) => {
const onData = (chunk: Buffer) => {
buffered = Buffer.concat([buffered, chunk]);
if (buffered.length >= 8 && buffered.readInt32BE(4) === 196608) {
startups.push(via);
socket.write(ready);
buffered = Buffer.alloc(0);
}
};
socket.on("data", onData);
onData(Buffer.alloc(0));
};
const { server, port } = await listeningServer(rawSocket => {
rawSocket.on("error", () => {});
let first = Buffer.alloc(0);
const onFirstBytes = (chunk: Buffer) => {
first = Buffer.concat([first, chunk]);
if (first.length < 8) return;
rawSocket.removeListener("data", onFirstBytes);
if (first.readInt32BE(0) !== 8 || first.readInt32BE(4) !== 80877103) {
// Not an SSLRequest: the client skipped TLS.
acceptStartup(rawSocket, "plaintext", first);
return;
}
rawSocket.pause();
// Bytes past the 8-byte SSLRequest are the start of the ClientHello.
if (first.length > 8) rawSocket.unshift(first.subarray(8));
rawSocket.write(pgSSLResponse("S"));
const socket = new tls.TLSSocket(rawSocket, { isServer: true, key: selfSignedTls.key, cert: selfSignedTls.cert });
socket.on("error", () => {});
acceptStartup(socket, "tls");
};
rawSocket.on("data", onFirstBytes);
});
return { server, port, startups };
}

function pgEnv(port: number, extra: Record<string, string> = {}) {
const env: Record<string, string> = { ...bunEnv };
for (const key of Object.keys(env)) {
if (/^(PG|PG_|POSTGRES_|DATABASE_|TLS_|MYSQL|MARIADB|SQLITE)/.test(key)) delete env[key];
}
// `0` in the runner's shell turns the certificate check off for a verify-* mode.
delete env.NODE_TLS_REJECT_UNAUTHORIZED;
env.PGHOST = "127.0.0.1";
env.PGPORT = String(port);
env.PGUSER = "u";
Expand Down Expand Up @@ -115,9 +165,61 @@ test.concurrent("URL ?sslmode=disable overrides PGSSLMODE=require", async () =>
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stderr).toBe("");
expect(stdout.trim()).toBe("CONNECTED_PLAINTEXT");
expect(stdout.trim()).toBe("CONNECTED");
expect(exitCode).toBe(0);
} finally {
await new Promise<void>(r => server.close(() => r()));
}
});
Comment thread
robobun marked this conversation as resolved.

/** Runs the fixture against a fresh self-signed TLS server, with the connection URL in `urlVariable`. */
async function connectToSelfSignedServer(urlVariable: string, query: string, extra: Record<string, string>) {
const { server, port, startups } = await selfSignedTlsServer();
let output: [string, string, number];
try {
await using proc = Bun.spawn({
cmd: [bunExe(), "-e", fixture],
env: pgEnv(port, { [urlVariable]: `postgres://u:pw@127.0.0.1:${port}/db${query}`, ...extra }),
stderr: "pipe",
});
output = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
} finally {
// Resolves once every connection has ended, so `startups` is final.
await new Promise<void>(r => server.close(() => r()));
}
const [stdout, stderr, exitCode] = output;
return { stdout: stdout.trim(), stderr, exitCode, startups };
}

// A URL from a TLS_* variable asks for at least `require`, which does not check
// the server certificate. PGSSLMODE=verify-ca / verify-full next to it must
// still check it: the client then stops inside the TLS handshake, before it
// sends the startup packet. A ?sslmode= in the URL overrides both.
test.concurrent.each([
[
"a URL from TLS_DATABASE_URL alone connects without checking the certificate",
["TLS_DATABASE_URL", "", {}],
{ stdout: "CONNECTED", startups: ["tls"] },
],
[
"PGSSLMODE=verify-full next to a URL from TLS_DATABASE_URL checks the certificate",
["TLS_DATABASE_URL", "", { PGSSLMODE: "verify-full" }],
{ stdout: "ERROR:DEPTH_ZERO_SELF_SIGNED_CERT", startups: [] },
],
[
"PGSSLMODE=verify-ca next to a URL from TLS_POSTGRES_DATABASE_URL checks the certificate",
["TLS_POSTGRES_DATABASE_URL", "", { PGSSLMODE: "verify-ca" }],
{ stdout: "ERROR:DEPTH_ZERO_SELF_SIGNED_CERT", startups: [] },
],
[
"?sslmode=disable in a URL from TLS_DATABASE_URL overrides PGSSLMODE=verify-full",
["TLS_DATABASE_URL", "?sslmode=disable", { PGSSLMODE: "verify-full" }],
{ stdout: "CONNECTED", startups: ["plaintext"] },
],
] as const)("%s", async (_, [urlVariable, query, extra], expected) => {
expect(await connectToSelfSignedServer(urlVariable, query, extra)).toEqual({
...expected,
stderr: "",
exitCode: 0,
});
});
Loading