From 055fc819a02eb026b069b52a839330f291df3f31 Mon Sep 17 00:00:00 2001 From: Madhavendra Rathore Date: Sat, 25 Jul 2026 01:44:09 +0530 Subject: [PATCH 1/6] Fix #462: verify Thrift TLS server certificate by default The Thrift HTTPS agent hardcoded `rejectUnauthorized: false`, disabling server-certificate verification entirely. Any self-signed, expired, or wrong-hostname certificate was accepted, exposing bearer-token traffic to man-in-the-middle attacks, and supplying a CA had no effect. Make the Thrift path secure-by-default (matching Node's https default, the JDBC/ODBC drivers, and the SEA backend) and add SEA-parity public options on ConnectionOptions: - checkServerCertificate (default true) -> agent rejectUnauthorized - customCaCert -> additive to the system trust store (appended to tls.rootCertificates, so public warehouses keep validating) - clientCert / clientKey -> client certificate + key for mutual TLS Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore --- lib/DBSQLClient.ts | 10 +++ lib/connection/connections/HttpConnection.ts | 4 +- .../contracts/IConnectionOptions.ts | 6 +- lib/contracts/IDBSQLClient.ts | 40 ++++++++++++ tests/unit/DBSQLClient.test.ts | 61 +++++++++++++++++++ .../connections/HttpConnection.test.ts | 38 +++++++++++- 6 files changed, 155 insertions(+), 4 deletions(-) diff --git a/lib/DBSQLClient.ts b/lib/DBSQLClient.ts index 8054c7c8..6164e578 100644 --- a/lib/DBSQLClient.ts +++ b/lib/DBSQLClient.ts @@ -1,5 +1,6 @@ import thrift from 'thrift'; import os from 'os'; +import tls from 'tls'; import { EventEmitter } from 'events'; import TCLIService from '../thrift/TCLIService'; @@ -196,6 +197,15 @@ export default class DBSQLClient extends EventEmitter implements IDBSQLClient, I https: true, socketTimeout: options.socketTimeout, proxy: options.proxy, + // `customCaCert` is ADDITIVE: Node's `ca` option replaces the system trust + // store, so we append the custom cert to the built-in roots to keep public + // Databricks warehouses trusted while also trusting the caller's CA. + ca: options.customCaCert === undefined ? undefined : [...tls.rootCertificates, options.customCaCert.toString()], + // Client certificate + key for mutual TLS (mTLS). Both must be supplied together. + cert: options.clientCert, + key: options.clientKey, + // Validate the server certificate unless the caller explicitly opts out. + rejectUnauthorized: options.checkServerCertificate ?? true, headers: { 'User-Agent': buildUserAgentString(options.userAgentEntry), }, diff --git a/lib/connection/connections/HttpConnection.ts b/lib/connection/connections/HttpConnection.ts index 8c56019c..e8a511d1 100644 --- a/lib/connection/connections/HttpConnection.ts +++ b/lib/connection/connections/HttpConnection.ts @@ -68,7 +68,9 @@ export default class HttpConnection implements IConnectionProvider { const httpsAgentOptions: https.AgentOptions = { ...this.getAgentDefaultOptions(), minVersion: 'TLSv1.2', - rejectUnauthorized: false, + // Validate the server certificate by default; only skip verification when + // the caller explicitly opts out via `rejectUnauthorized: false`. + rejectUnauthorized: this.options.rejectUnauthorized ?? true, ca: this.options.ca, cert: this.options.cert, key: this.options.key, diff --git a/lib/connection/contracts/IConnectionOptions.ts b/lib/connection/contracts/IConnectionOptions.ts index 340b6fae..cf4e9ba3 100644 --- a/lib/connection/contracts/IConnectionOptions.ts +++ b/lib/connection/contracts/IConnectionOptions.ts @@ -19,7 +19,11 @@ export default interface IConnectionOptions { socketTimeout?: number; proxy?: ProxyOptions; - ca?: Buffer | string; + ca?: Buffer | string | Array; cert?: Buffer | string; key?: Buffer | string; + + // Whether the TLS server certificate is validated against the trusted CA set. + // When omitted the connection provider treats it as `true` (validation enabled). + rejectUnauthorized?: boolean; } diff --git a/lib/contracts/IDBSQLClient.ts b/lib/contracts/IDBSQLClient.ts index b1b2bb7b..3668d8d8 100644 --- a/lib/contracts/IDBSQLClient.ts +++ b/lib/contracts/IDBSQLClient.ts @@ -59,6 +59,46 @@ export type ConnectionOptions = { proxy?: ProxyOptions; enableMetricViewMetadata?: boolean; + /** + * Verify the server's TLS certificate on the primary Thrift transport. + * Secure-by-default: omitting this leaves full chain + hostname verification + * enabled (`true`), matching Node's `https` default, the JDBC/ODBC drivers, + * and the SEA/kernel backend. + * + * Setting it to `false` disables server certificate verification entirely + * (any self-signed, expired, or wrong-hostname certificate is accepted), + * which exposes the connection — including bearer-token auth headers — to + * man-in-the-middle attacks. Only use `false` for local development against a + * trusted endpoint, and prefer supplying `customCaCert` instead. + * + * Mirrors the `checkServerCertificate` option on the SEA backend. + */ + checkServerCertificate?: boolean; + + /** + * PEM-encoded CA certificate (string or `Buffer`) added to the trust store + * **on top of** the system roots — for TLS-inspecting proxies or on-prem + * internal CAs. Because it is additive, connections to public Databricks + * warehouses (trusted via the system roots) keep working. + * + * Mirrors the `customCaCert` option on the SEA backend. + */ + customCaCert?: Buffer | string; + + /** + * PEM-encoded client certificate (string or `Buffer`) presented to the server + * for mutual TLS (mTLS). Must be supplied together with `clientKey`. Leave + * both unset for the usual token/OAuth flows, which do not require a client + * certificate. + */ + clientCert?: Buffer | string; + + /** + * PEM-encoded private key (string or `Buffer`) for `clientCert`, used for + * mutual TLS (mTLS). Must be supplied together with `clientCert`. + */ + clientKey?: Buffer | string; + /** * Retry-policy knobs governing how the driver retries retryable requests. * They apply to **both** backends: the Thrift `HttpRetryPolicy` reads them diff --git a/tests/unit/DBSQLClient.test.ts b/tests/unit/DBSQLClient.test.ts index 4cf16838..7675662c 100644 --- a/tests/unit/DBSQLClient.test.ts +++ b/tests/unit/DBSQLClient.test.ts @@ -62,6 +62,67 @@ describe('DBSQLClient.connect', () => { expect(connectionOptions.path).to.equal(path); }); + it('should validate the server certificate by default', async () => { + const client = new DBSQLClient(); + + const connectionOptions = client['getConnectionOptions'](connectOptions); + + expect(connectionOptions.rejectUnauthorized).to.be.true; + }); + + it('should map checkServerCertificate to rejectUnauthorized', async () => { + const client = new DBSQLClient(); + + expect(client['getConnectionOptions']({ ...connectOptions, checkServerCertificate: false }).rejectUnauthorized).to + .be.false; + expect(client['getConnectionOptions']({ ...connectOptions, checkServerCertificate: true }).rejectUnauthorized).to.be + .true; + }); + + it('should not set a custom CA when customCaCert is omitted', async () => { + const client = new DBSQLClient(); + + const connectionOptions = client['getConnectionOptions'](connectOptions); + + expect(connectionOptions.ca).to.be.undefined; + }); + + it('should add customCaCert on top of the system root certificates (additive)', async () => { + const client = new DBSQLClient(); + const customCaCert = '-----BEGIN CERTIFICATE-----\ncustom\n-----END CERTIFICATE-----\n'; + + const connectionOptions = client['getConnectionOptions']({ ...connectOptions, customCaCert }); + + expect(connectionOptions.ca).to.be.an('array'); + const ca = connectionOptions.ca as Array; + // The custom cert must be present... + expect(ca).to.include(customCaCert); + // ...alongside the built-in system roots (so public warehouses still validate). + expect(ca.length).to.be.greaterThan(1); + }); + + it('should not set client cert/key when mTLS options are omitted', async () => { + const client = new DBSQLClient(); + + const connectionOptions = client['getConnectionOptions'](connectOptions); + + expect(connectionOptions.cert).to.be.undefined; + expect(connectionOptions.key).to.be.undefined; + }); + + it('should map clientCert/clientKey to cert/key for mutual TLS', async () => { + const client = new DBSQLClient(); + + const connectionOptions = client['getConnectionOptions']({ + ...connectOptions, + clientCert: 'client-cert', + clientKey: 'client-key', + }); + + expect(connectionOptions.cert).to.equal('client-cert'); + expect(connectionOptions.key).to.equal('client-key'); + }); + it('should initialize connection state', async () => { const client = new DBSQLClient(); diff --git a/tests/unit/connection/connections/HttpConnection.test.ts b/tests/unit/connection/connections/HttpConnection.test.ts index 44d38a69..ec37e72f 100644 --- a/tests/unit/connection/connections/HttpConnection.test.ts +++ b/tests/unit/connection/connections/HttpConnection.test.ts @@ -26,7 +26,7 @@ describe('HttpConnection.connect', () => { expect(anotherConnection).to.eq(thriftConnection); }); - it('should set SSL certificates and disable rejectUnauthorized', async () => { + it('should set SSL certificates and validate the server certificate by default', async () => { const connection = new HttpConnection( { host: 'localhost', @@ -42,12 +42,46 @@ describe('HttpConnection.connect', () => { const thriftConnection = await connection.getThriftConnection(); - expect(thriftConnection.config.agent.options.rejectUnauthorized).to.be.false; + expect(thriftConnection.config.agent.options.rejectUnauthorized).to.be.true; expect(thriftConnection.config.agent.options.ca).to.be.eq('ca'); expect(thriftConnection.config.agent.options.cert).to.be.eq('cert'); expect(thriftConnection.config.agent.options.key).to.be.eq('key'); }); + it('should allow disabling server certificate validation via rejectUnauthorized', async () => { + const connection = new HttpConnection( + { + host: 'localhost', + port: 10001, + path: '/hive', + https: true, + rejectUnauthorized: false, + }, + new ClientContextStub(), + ); + + const thriftConnection = await connection.getThriftConnection(); + + expect(thriftConnection.config.agent.options.rejectUnauthorized).to.be.false; + }); + + it('should keep server certificate validation enabled when rejectUnauthorized is true', async () => { + const connection = new HttpConnection( + { + host: 'localhost', + port: 10001, + path: '/hive', + https: true, + rejectUnauthorized: true, + }, + new ClientContextStub(), + ); + + const thriftConnection = await connection.getThriftConnection(); + + expect(thriftConnection.config.agent.options.rejectUnauthorized).to.be.true; + }); + it('should initialize http agents', async () => { const connection = new HttpConnection( { From b28ee14592fb8622a09bda12132d3b30e2b9f150 Mon Sep 17 00:00:00 2001 From: Madhavendra Rathore Date: Wed, 5 Aug 2026 01:17:23 +0530 Subject: [PATCH 2/6] fix(ci): clear security-scan CVE findings Bump as far as the registry allows and suppress the rest: - brace-expansion 1.1.15 -> 1.1.16, 2.1.1 -> 2.1.3, clearing GHSA-3jxr-9vmj-r5cp (the finding that was blocking this PR). - ip-address 10.2.0 -> 10.3.1, clearing GHSA-mwp4-54f8-5fhr (7.7), GHSA-22jq-vg5j-6vgg and GHSA-4xrf-jv44-h6hh. This one is a production dependency (via socks). - Suppress GHSA-mh99-v99m-4gvg and GHSA-rgw5-rvv9-x895, two newer brace-expansion DoS advisories with no published fix: they name 1.1.17/1.1.18/2.1.4 as fixed, but npm's latest releases are 1.1.16 and 2.1.3. Both are dev-only (eslint/glob/test-exclude -> minimatch; `npm ls --omit=dev` is empty), so they don't reach the shipped dist/. Time-boxed to 2027-02-05 to force a re-review. Both package.json and the existing overrides were left untouched -- the declared ranges already permitted the patched versions. Verified locally with osv-scanner v2.3.8 (the version CI pins): "No issues found". npm ci, lint, and the unit suite (1262 passing) are green. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore --- osv-scanner.toml | 26 ++++++++++++++++++++++++++ package-lock.json | 36 ++++++++++++++++++------------------ 2 files changed, 44 insertions(+), 18 deletions(-) diff --git a/osv-scanner.toml b/osv-scanner.toml index b15bf0d4..d34d8d9c 100644 --- a/osv-scanner.toml +++ b/osv-scanner.toml @@ -30,3 +30,29 @@ # This file starts empty -- populate iteratively as the first scan run # surfaces real false positives or dev-only findings worth excluding. # Do not pre-populate with speculative suppressions. + +# brace-expansion DoS advisories, both with NO published fix as of +# 2026-08-05: the advisories name 1.1.17/1.1.18 and 2.1.4 as fixed, but +# npm's latest published releases are 1.1.16 and 2.1.3. So there is +# nothing to bump to -- we already bumped as far as the registry allows +# (1.1.15 -> 1.1.16, 2.1.1 -> 2.1.3, which cleared the earlier +# GHSA-3jxr-9vmj-r5cp). +# +# Dev-only: brace-expansion reaches us solely through the eslint / +# glob / test-exclude toolchains via minimatch. `npm ls brace-expansion +# --omit=dev` is empty, and both lockfile entries are marked +# "dev": true, so it is not reachable from the published dist/. +# The impact is DoS (ReDoS / OOM) on adversarial brace patterns, which +# would require untrusted input to our own lint/test globs. +# +# Revisit when 1.1.18 / 2.1.4 land on npm and drop these entries. + +[[IgnoredVulns]] +id = "GHSA-mh99-v99m-4gvg" +ignoreUntil = "2027-02-05T00:00:00Z" +reason = "dev-only (eslint/glob/test-exclude -> minimatch); not reachable from shipped dist/. No published fix: advisory lists 1.1.17 as fixed but 1.1.16 is npm's latest 1.x." + +[[IgnoredVulns]] +id = "GHSA-rgw5-rvv9-x895" +ignoreUntil = "2027-02-05T00:00:00Z" +reason = "dev-only (eslint/glob/test-exclude -> minimatch); not reachable from shipped dist/. No published fix: advisory lists 1.1.18/2.1.4 as fixed but npm's latest are 1.1.16/2.1.3." diff --git a/package-lock.json b/package-lock.json index 417060fe..2e65e587 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1860,9 +1860,9 @@ } }, "node_modules/brace-expansion": { - "version": "1.1.15", - "resolved": "https://registry.npmjs.org/brace-expansion/-/brace-expansion-1.1.15.tgz", - "integrity": "sha512-EwOCDEex4quD37XhqM3omwtMoJjr//isUZz1JopUNWms+4Z2ViyM/k1YIRePpoVNnQhENnxtFjLaxNHrT7xIUg==", + "version": "1.1.16", + "resolved": "https://registry.npmjs.org/brace-expansion/-/brace-expansion-1.1.16.tgz", + "integrity": "sha512-IDw48K2/2kRkg9LdJxurvq3lV3aBgq0REY89duEqFRthjlPdXHKMj7EnQOXVckxzgisinf3nHfrcE2FufFLXMw==", "dev": true, "license": "MIT", "dependencies": { @@ -3993,9 +3993,9 @@ } }, "node_modules/ip-address": { - "version": "10.2.0", - "resolved": "https://registry.npmjs.org/ip-address/-/ip-address-10.2.0.tgz", - "integrity": "sha512-/+S6j4E9AHvW9SWMSEY9Xfy66O5PWvVEJ08O0y5JGyEKQpojb0K0GKpz/v5HJ/G0vi3D2sjGK78119oXZeE0qA==", + "version": "10.3.1", + "resolved": "https://registry.npmjs.org/ip-address/-/ip-address-10.3.1.tgz", + "integrity": "sha512-1e9d3kb97NHJTIJDZW9rKqW2h6+dFa50Dy0fpPSMQp2ADje5gvKsXmdiK6dwY5t76TaTt5+P5N1Y/LoToIxP6g==", "license": "MIT", "engines": { "node": ">= 12" @@ -4786,9 +4786,9 @@ } }, "node_modules/minimatch/node_modules/brace-expansion": { - "version": "2.1.1", - "resolved": "https://registry.npmjs.org/brace-expansion/-/brace-expansion-2.1.1.tgz", - "integrity": "sha512-WR1cURNjuvBLMZBMbqM0UoE+WAfdUcEV1ccD8PVBVOI+Z3ND4+SZbN8RsfT2bMuG1qwz5RFvPukSZm5fF2D5eA==", + "version": "2.1.3", + "resolved": "https://registry.npmjs.org/brace-expansion/-/brace-expansion-2.1.3.tgz", + "integrity": "sha512-DRdx5neNsG/QXbniLFWi2YmC/68oeOOmKz6zOjVk6ZS1ZLXgLIKqVEc6hWsmkjBbgii0SwaBTcJ5XKj5gzY/4A==", "dev": true, "license": "MIT", "dependencies": { @@ -8109,9 +8109,9 @@ "dev": true }, "brace-expansion": { - "version": "1.1.15", - "resolved": "https://registry.npmjs.org/brace-expansion/-/brace-expansion-1.1.15.tgz", - "integrity": "sha512-EwOCDEex4quD37XhqM3omwtMoJjr//isUZz1JopUNWms+4Z2ViyM/k1YIRePpoVNnQhENnxtFjLaxNHrT7xIUg==", + "version": "1.1.16", + "resolved": "https://registry.npmjs.org/brace-expansion/-/brace-expansion-1.1.16.tgz", + "integrity": "sha512-IDw48K2/2kRkg9LdJxurvq3lV3aBgq0REY89duEqFRthjlPdXHKMj7EnQOXVckxzgisinf3nHfrcE2FufFLXMw==", "dev": true, "requires": { "balanced-match": "^1.0.0", @@ -9633,9 +9633,9 @@ } }, "ip-address": { - "version": "10.2.0", - "resolved": "https://registry.npmjs.org/ip-address/-/ip-address-10.2.0.tgz", - "integrity": "sha512-/+S6j4E9AHvW9SWMSEY9Xfy66O5PWvVEJ08O0y5JGyEKQpojb0K0GKpz/v5HJ/G0vi3D2sjGK78119oXZeE0qA==" + "version": "10.3.1", + "resolved": "https://registry.npmjs.org/ip-address/-/ip-address-10.3.1.tgz", + "integrity": "sha512-1e9d3kb97NHJTIJDZW9rKqW2h6+dFa50Dy0fpPSMQp2ADje5gvKsXmdiK6dwY5t76TaTt5+P5N1Y/LoToIxP6g==" }, "is-arrayish": { "version": "0.3.2", @@ -10204,9 +10204,9 @@ }, "dependencies": { "brace-expansion": { - "version": "2.1.1", - "resolved": "https://registry.npmjs.org/brace-expansion/-/brace-expansion-2.1.1.tgz", - "integrity": "sha512-WR1cURNjuvBLMZBMbqM0UoE+WAfdUcEV1ccD8PVBVOI+Z3ND4+SZbN8RsfT2bMuG1qwz5RFvPukSZm5fF2D5eA==", + "version": "2.1.3", + "resolved": "https://registry.npmjs.org/brace-expansion/-/brace-expansion-2.1.3.tgz", + "integrity": "sha512-DRdx5neNsG/QXbniLFWi2YmC/68oeOOmKz6zOjVk6ZS1ZLXgLIKqVEc6hWsmkjBbgii0SwaBTcJ5XKj5gzY/4A==", "dev": true, "requires": { "balanced-match": "^1.0.0" From ecee476773941360c136f4c7c122214f7e03c081 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Tue, 4 Aug 2026 19:59:55 +0000 Subject: [PATCH 3/6] ai: apply changes for #463 (4 review threads) Addresses: - #3715674893 at lib/DBSQLClient.ts:205 - #3715674899 at lib/DBSQLClient.ts:207 - #3715713253 at lib/contracts/IDBSQLClient.ts:94 - #3715713261 at lib/DBSQLClient.ts:207 Signed-off-by: peco-engineer-bot[bot] --- lib/DBSQLClient.ts | 42 ++++++++++++++++++-- lib/contracts/IDBSQLClient.ts | 3 +- lib/kernel/KernelAuth.ts | 25 ++++++++---- tests/unit/DBSQLClient.test.ts | 43 +++++++++++++++++++++ tests/unit/kernel/connectionOptions.test.ts | 14 +++++++ 5 files changed, 116 insertions(+), 11 deletions(-) diff --git a/lib/DBSQLClient.ts b/lib/DBSQLClient.ts index be26fadf..f92f6112 100644 --- a/lib/DBSQLClient.ts +++ b/lib/DBSQLClient.ts @@ -1,5 +1,6 @@ import thrift from 'thrift'; import os from 'os'; +import fs from 'fs'; import tls from 'tls'; import { EventEmitter } from 'events'; @@ -191,7 +192,38 @@ export default class DBSQLClient extends EventEmitter implements IDBSQLClient, I this.logger.log(LogLevel.info, 'Created DBSQLClient'); } + // Node folds `NODE_EXTRA_CA_CERTS` into the default trust store ONLY when the + // `ca` option is left unset — and `tls.rootCertificates` does not include those + // extra roots. Since we set `ca` explicitly to append `customCaCert`, we must + // re-read `NODE_EXTRA_CA_CERTS` ourselves so callers relying on it (e.g. a + // corporate proxy) do not silently lose those roots. + private static getExtraCaCerts(): Array { + const extraCertsPath = process.env.NODE_EXTRA_CA_CERTS; + if (!extraCertsPath) { + return []; + } + try { + return [fs.readFileSync(extraCertsPath, 'utf8')]; + } catch { + // Node itself silently ignores an unreadable NODE_EXTRA_CA_CERTS; mirror that. + return []; + } + } + private getConnectionOptions(options: ConnectionOptions): IConnectionOptions { + // mTLS requires both a client certificate and its private key. If exactly one is + // supplied, Node fails deep in the TLS handshake with an opaque error, so surface + // a clear client-side message instead. + const hasClientCert = options.clientCert !== undefined; + const hasClientKey = options.clientKey !== undefined; + if (hasClientCert !== hasClientKey) { + throw new HiveDriverError( + `DBSQLClient: mutual TLS requires both clientCert and clientKey; only \`${ + hasClientCert ? 'clientCert' : 'clientKey' + }\` was supplied. Provide the matching ${hasClientCert ? '`clientKey` (private key)' : '`clientCert`'}.`, + ); + } + return { host: options.host, port: options.port || 443, @@ -200,9 +232,13 @@ export default class DBSQLClient extends EventEmitter implements IDBSQLClient, I socketTimeout: options.socketTimeout, proxy: options.proxy, // `customCaCert` is ADDITIVE: Node's `ca` option replaces the system trust - // store, so we append the custom cert to the built-in roots to keep public - // Databricks warehouses trusted while also trusting the caller's CA. - ca: options.customCaCert === undefined ? undefined : [...tls.rootCertificates, options.customCaCert.toString()], + // store, so we append the custom cert to the built-in roots AND any roots + // supplied via NODE_EXTRA_CA_CERTS to keep public Databricks warehouses + // trusted while also trusting the caller's CA. + ca: + options.customCaCert === undefined + ? undefined + : [...tls.rootCertificates, ...DBSQLClient.getExtraCaCerts(), options.customCaCert.toString()], // Client certificate + key for mutual TLS (mTLS). Both must be supplied together. cert: options.clientCert, key: options.clientKey, diff --git a/lib/contracts/IDBSQLClient.ts b/lib/contracts/IDBSQLClient.ts index 021020e7..a9bee1d0 100644 --- a/lib/contracts/IDBSQLClient.ts +++ b/lib/contracts/IDBSQLClient.ts @@ -79,7 +79,8 @@ export type ConnectionOptions = { * PEM-encoded CA certificate (string or `Buffer`) added to the trust store * **on top of** the system roots — for TLS-inspecting proxies or on-prem * internal CAs. Because it is additive, connections to public Databricks - * warehouses (trusted via the system roots) keep working. + * warehouses (trusted via the system roots) keep working. Roots supplied via + * the `NODE_EXTRA_CA_CERTS` environment variable are also preserved. * * Mirrors the `customCaCert` option on the SEA backend. */ diff --git a/lib/kernel/KernelAuth.ts b/lib/kernel/KernelAuth.ts index 295fbb9b..919cf94b 100644 --- a/lib/kernel/KernelAuth.ts +++ b/lib/kernel/KernelAuth.ts @@ -321,11 +321,13 @@ function normalizePemBytes(value: Buffer | string, optionName: string, kind: 'ce * master verify toggle is on). Mirrors Python's `tls_verify_hostname`. * - `customCaCert` accepts a PEM string or `Buffer`; normalised to a * `Buffer` via {@link normalizePemBytes}. - * - `clientCertPem` / `clientKeyPem` carry the mutual-TLS client identity. - * They must be supplied **together** — supplying only one is rejected - * here with an actionable error (rather than waiting for the kernel's - * `InvalidArgument` at `openSession`). Each accepts a PEM string or - * `Buffer`, normalised the same way. + * - `clientCertPem` / `clientKeyPem` (or their public aliases + * `clientCert` / `clientKey`) carry the mutual-TLS client identity. The + * internal `*Pem` names win when both are present. They must be supplied + * **together** — supplying only one is rejected here with an actionable + * error (rather than waiting for the kernel's `InvalidArgument` at + * `openSession`). Each accepts a PEM string or `Buffer`, normalised the + * same way. * * Throws `HiveDriverError` when a cert/key is empty, mis-typed, lacks the * expected PEM header, or when only one half of the mTLS pair is set. @@ -334,8 +336,17 @@ export function buildKernelTlsOptions(options: ConnectionOptions): KernelTlsOpti // Read the kernel-only fields through the purpose-built internal options type // rather than an ad-hoc inline cast, so the shape can't silently drift from // its declaration and a typo'd key fails to compile. - const { checkServerCertificate, checkServerCertificateHostname, customCaCert, clientCertPem, clientKeyPem } = - options as ConnectionOptions & InternalConnectionOptions; + const merged = options as ConnectionOptions & InternalConnectionOptions; + const { checkServerCertificate, checkServerCertificateHostname, customCaCert } = merged; + + // The public mTLS options are `clientCert`/`clientKey` (see `ConnectionOptions`); + // the internal kernel-only aliases are `clientCertPem`/`clientKeyPem`. Accept + // both here — preferring the explicit internal alias when present — so a caller + // who sets the public `clientCert`/`clientKey` and runs on the kernel backend + // still gets mTLS configured instead of having their client identity silently + // dropped. + const clientCertPem = merged.clientCertPem ?? merged.clientCert; + const clientKeyPem = merged.clientKeyPem ?? merged.clientKey; const tls: KernelTlsOptions = {}; diff --git a/tests/unit/DBSQLClient.test.ts b/tests/unit/DBSQLClient.test.ts index 7675662c..7ee7290e 100644 --- a/tests/unit/DBSQLClient.test.ts +++ b/tests/unit/DBSQLClient.test.ts @@ -1,5 +1,6 @@ import { expect, AssertionError } from 'chai'; import sinon from 'sinon'; +import fs from 'fs'; import DBSQLClient, { ThriftLibrary } from '../../lib/DBSQLClient'; import DBSQLSession from '../../lib/DBSQLSession'; import ThriftBackend from '../../lib/thrift-backend/ThriftBackend'; @@ -101,6 +102,32 @@ describe('DBSQLClient.connect', () => { expect(ca.length).to.be.greaterThan(1); }); + it('should preserve NODE_EXTRA_CA_CERTS roots when customCaCert is set', async () => { + const client = new DBSQLClient(); + const customCaCert = '-----BEGIN CERTIFICATE-----\ncustom\n-----END CERTIFICATE-----\n'; + const extraCaCert = '-----BEGIN CERTIFICATE-----\nextra\n-----END CERTIFICATE-----\n'; + + const previousEnv = process.env.NODE_EXTRA_CA_CERTS; + process.env.NODE_EXTRA_CA_CERTS = '/path/to/extra-ca.pem'; + const readFileSync = sinon.stub(fs, 'readFileSync').returns(extraCaCert); + try { + const connectionOptions = client['getConnectionOptions']({ ...connectOptions, customCaCert }); + + const ca = connectionOptions.ca as Array; + expect(ca).to.include(customCaCert); + // Roots injected via NODE_EXTRA_CA_CERTS must survive the additive rebuild, + // otherwise callers relying on that env var lose their trust anchors. + expect(ca).to.include(extraCaCert); + } finally { + readFileSync.restore(); + if (previousEnv === undefined) { + delete process.env.NODE_EXTRA_CA_CERTS; + } else { + process.env.NODE_EXTRA_CA_CERTS = previousEnv; + } + } + }); + it('should not set client cert/key when mTLS options are omitted', async () => { const client = new DBSQLClient(); @@ -123,6 +150,22 @@ describe('DBSQLClient.connect', () => { expect(connectionOptions.key).to.equal('client-key'); }); + it('should throw if only clientCert is supplied for mutual TLS', async () => { + const client = new DBSQLClient(); + + expect(() => client['getConnectionOptions']({ ...connectOptions, clientCert: 'client-cert' })).to.throw( + /only `clientCert` was supplied/, + ); + }); + + it('should throw if only clientKey is supplied for mutual TLS', async () => { + const client = new DBSQLClient(); + + expect(() => client['getConnectionOptions']({ ...connectOptions, clientKey: 'client-key' })).to.throw( + /only `clientKey` was supplied/, + ); + }); + it('should initialize connection state', async () => { const client = new DBSQLClient(); diff --git a/tests/unit/kernel/connectionOptions.test.ts b/tests/unit/kernel/connectionOptions.test.ts index efa99aa7..b5b13d41 100644 --- a/tests/unit/kernel/connectionOptions.test.ts +++ b/tests/unit/kernel/connectionOptions.test.ts @@ -172,6 +172,20 @@ describe('KernelAuth mTLS options (buildKernelTlsOptions)', () => { expect(tls.clientKeyPem).to.equal(key); }); + it('accepts the public clientCert/clientKey aliases (kernel path still gets mTLS)', () => { + const tls = buildKernelTlsOptions(opts({ clientCert: CERT_PEM, clientKey: KEY_PEM })); + expect(tls.clientCertPem?.toString('utf8')).to.equal(CERT_PEM); + expect(tls.clientKeyPem?.toString('utf8')).to.equal(KEY_PEM); + }); + + it('prefers the internal clientCertPem/clientKeyPem over the public aliases when both are set', () => { + const tls = buildKernelTlsOptions( + opts({ clientCertPem: CERT_PEM, clientKeyPem: KEY_PEM, clientCert: 'nope', clientKey: 'nope' }), + ); + expect(tls.clientCertPem?.toString('utf8')).to.equal(CERT_PEM); + expect(tls.clientKeyPem?.toString('utf8')).to.equal(KEY_PEM); + }); + it('rejects supplying only the client cert', () => { expect(() => buildKernelTlsOptions(opts({ clientCertPem: CERT_PEM }))).to.throw( HiveDriverError, From 18ee36f09136baad5d27e5a2abec659485fe506d Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Tue, 4 Aug 2026 20:06:04 +0000 Subject: [PATCH 4/6] ai: apply changes for #463 (1 review thread) Addresses: - #3715785609 at lib/DBSQLClient.ts:240 Signed-off-by: peco-engineer-bot[bot] --- lib/contracts/IDBSQLClient.ts | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/lib/contracts/IDBSQLClient.ts b/lib/contracts/IDBSQLClient.ts index a9bee1d0..bbaa4c69 100644 --- a/lib/contracts/IDBSQLClient.ts +++ b/lib/contracts/IDBSQLClient.ts @@ -77,10 +77,17 @@ export type ConnectionOptions = { /** * PEM-encoded CA certificate (string or `Buffer`) added to the trust store - * **on top of** the system roots — for TLS-inspecting proxies or on-prem + * **on top of** the built-in roots — for TLS-inspecting proxies or on-prem * internal CAs. Because it is additive, connections to public Databricks - * warehouses (trusted via the system roots) keep working. Roots supplied via - * the `NODE_EXTRA_CA_CERTS` environment variable are also preserved. + * warehouses keep working. + * + * Note: supplying this rebuilds the trust store from Node's **bundled Mozilla + * roots** (`tls.rootCertificates`) plus any roots from the `NODE_EXTRA_CA_CERTS` + * environment variable, then appends this certificate. It does **not** include + * OS-installed roots that Node would otherwise consult (e.g. on Node >= 22 run + * with `--use-system-ca`). If you rely on an enterprise root installed in the + * OS trust store, add it explicitly via `NODE_EXTRA_CA_CERTS` or `customCaCert` + * when using this option. * * Mirrors the `customCaCert` option on the SEA backend. */ From 81ce4682d7ee63b7c5f283eeff1428e15add5115 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Tue, 4 Aug 2026 20:22:13 +0000 Subject: [PATCH 5/6] ai: apply changes for #463 (1 review thread) Addresses: - #3715840256 at lib/DBSQLClient.ts:241 Signed-off-by: peco-engineer-bot[bot] --- lib/DBSQLClient.ts | 25 +++++++++++--- lib/kernel/KernelAuth.ts | 62 ++++++++-------------------------- lib/utils/index.ts | 2 ++ lib/utils/normalizePemBytes.ts | 52 ++++++++++++++++++++++++++++ tests/unit/DBSQLClient.test.ts | 31 ++++++++++++++--- 5 files changed, 116 insertions(+), 56 deletions(-) create mode 100644 lib/utils/normalizePemBytes.ts diff --git a/lib/DBSQLClient.ts b/lib/DBSQLClient.ts index f92f6112..d30e837e 100644 --- a/lib/DBSQLClient.ts +++ b/lib/DBSQLClient.ts @@ -16,7 +16,7 @@ import IAuthentication from './connection/contracts/IAuthentication'; import HttpConnection from './connection/connections/HttpConnection'; import IConnectionOptions from './connection/contracts/IConnectionOptions'; import HiveDriverError from './errors/HiveDriverError'; -import { buildUserAgentString } from './utils'; +import { buildUserAgentString, normalizePemBytes } from './utils'; import IBackend from './contracts/IBackend'; import { InternalConnectionOptions } from './contracts/InternalConnectionOptions'; import ThriftBackend from './thrift-backend/ThriftBackend'; @@ -224,6 +224,19 @@ export default class DBSQLClient extends EventEmitter implements IDBSQLClient, I ); } + // Validate the PEM inputs up front with the same ordered BEGIN…END check the + // kernel path uses (normalizePemBytes), so a truncated/headerless/DER blob is + // rejected here with a named, actionable error instead of surfacing as an + // opaque failure deep in Node's TLS handshake. + const clientCert = + options.clientCert === undefined + ? undefined + : normalizePemBytes(options.clientCert, 'clientCert', 'certificate', 'DBSQLClient'); + const clientKey = + options.clientKey === undefined + ? undefined + : normalizePemBytes(options.clientKey, 'clientKey', 'private key', 'DBSQLClient'); + return { host: options.host, port: options.port || 443, @@ -238,10 +251,14 @@ export default class DBSQLClient extends EventEmitter implements IDBSQLClient, I ca: options.customCaCert === undefined ? undefined - : [...tls.rootCertificates, ...DBSQLClient.getExtraCaCerts(), options.customCaCert.toString()], + : [ + ...tls.rootCertificates, + ...DBSQLClient.getExtraCaCerts(), + normalizePemBytes(options.customCaCert, 'customCaCert', 'certificate', 'DBSQLClient').toString(), + ], // Client certificate + key for mutual TLS (mTLS). Both must be supplied together. - cert: options.clientCert, - key: options.clientKey, + cert: clientCert, + key: clientKey, // Validate the server certificate unless the caller explicitly opts out. rejectUnauthorized: options.checkServerCertificate ?? true, headers: { diff --git a/lib/kernel/KernelAuth.ts b/lib/kernel/KernelAuth.ts index 919cf94b..289c3e47 100644 --- a/lib/kernel/KernelAuth.ts +++ b/lib/kernel/KernelAuth.ts @@ -16,7 +16,7 @@ import { ConnectionOptions } from '../contracts/IDBSQLClient'; import { InternalConnectionOptions } from '../contracts/InternalConnectionOptions'; import AuthenticationError from '../errors/AuthenticationError'; import HiveDriverError from '../errors/HiveDriverError'; -import { buildUserAgentString } from '../utils'; +import { buildUserAgentString, normalizePemBytes } from '../utils'; /** * Default local listener port for the U2M authorization-code callback. @@ -267,50 +267,6 @@ export function isBlankOrReserved(s: string): boolean { /** napi-rs marshals `maxConnections` as a `u32`; reject values it can't hold. */ const MAX_U32 = 0xffffffff; -/** - * Normalise a PEM input (`string` or `Buffer`) accepted on the public - * surface into the `Buffer` the napi shape requires. Does a light, - * ordered BEGIN…END sanity check so a truncated/headerless blob (or a - * stray page that merely contains the literals out of order, e.g. a - * proxy-intercept page) is rejected here rather than surfacing as an - * opaque kernel TLS error. The bytes are NOT fully parsed in JS — that - * is deferred to the kernel, which returns a meaningful error on a - * malformed PEM/key. - * - * `kind` selects the expected block: `'certificate'` matches a - * `CERTIFICATE` block; `'private key'` matches any `… PRIVATE KEY` block - * (PKCS#8 `PRIVATE KEY`, PKCS#1 `RSA PRIVATE KEY`, SEC1 `EC PRIVATE KEY`). - * - * Throws `HiveDriverError` when the value is empty or (for strings) - * lacks the expected PEM header. - */ -function normalizePemBytes(value: Buffer | string, optionName: string, kind: 'certificate' | 'private key'): Buffer { - if (typeof value === 'string') { - const re = - kind === 'certificate' - ? /-----BEGIN CERTIFICATE-----[\s\S]+?-----END CERTIFICATE-----/ - : /-----BEGIN [A-Z0-9 ]*PRIVATE KEY-----[\s\S]+?-----END [A-Z0-9 ]*PRIVATE KEY-----/; - if (!re.test(value)) { - const expected = - kind === 'certificate' - ? "a '-----BEGIN CERTIFICATE-----' … '-----END CERTIFICATE-----' block" - : "a 'BEGIN … PRIVATE KEY' / 'END … PRIVATE KEY' PEM block (PKCS#8, PKCS#1, or SEC1)"; - throw new HiveDriverError( - `kernel backend: \`${optionName}\` string does not look like a PEM ${kind} (expected ${expected}). ` + - 'Pass PEM text or a Buffer of PEM bytes.', - ); - } - return Buffer.from(value, 'utf8'); - } - if (Buffer.isBuffer(value)) { - if (value.length === 0) { - throw new HiveDriverError(`kernel backend: \`${optionName}\` Buffer is empty.`); - } - return value; - } - throw new HiveDriverError(`kernel backend: \`${optionName}\` must be a PEM string or a Buffer.`); -} - /** * Normalise the public TLS options into the napi shape. * @@ -359,7 +315,7 @@ export function buildKernelTlsOptions(options: ConnectionOptions): KernelTlsOpti } if (customCaCert !== undefined) { - tls.customCaCert = normalizePemBytes(customCaCert, 'customCaCert', 'certificate'); + tls.customCaCert = normalizePemBytes(customCaCert, 'customCaCert', 'certificate', 'kernel backend'); } // mTLS client identity. Enforce both-or-neither up front so a caller who @@ -376,8 +332,18 @@ export function buildKernelTlsOptions(options: ConnectionOptions): KernelTlsOpti ); } if (hasCert && hasKey) { - tls.clientCertPem = normalizePemBytes(clientCertPem as Buffer | string, 'clientCertPem', 'certificate'); - tls.clientKeyPem = normalizePemBytes(clientKeyPem as Buffer | string, 'clientKeyPem', 'private key'); + tls.clientCertPem = normalizePemBytes( + clientCertPem as Buffer | string, + 'clientCertPem', + 'certificate', + 'kernel backend', + ); + tls.clientKeyPem = normalizePemBytes( + clientKeyPem as Buffer | string, + 'clientKeyPem', + 'private key', + 'kernel backend', + ); } return tls; diff --git a/lib/utils/index.ts b/lib/utils/index.ts index daddf482..86cece50 100644 --- a/lib/utils/index.ts +++ b/lib/utils/index.ts @@ -4,6 +4,7 @@ import formatProgress, { ProgressUpdateTransformer } from './formatProgress'; import LZ4 from './lz4'; import * as ProtocolVersion from './protocolVersion'; import serializeQueryTags from './queryTags'; +import normalizePemBytes from './normalizePemBytes'; export { definedOrError, @@ -13,4 +14,5 @@ export { LZ4, ProtocolVersion, serializeQueryTags, + normalizePemBytes, }; diff --git a/lib/utils/normalizePemBytes.ts b/lib/utils/normalizePemBytes.ts new file mode 100644 index 00000000..3c142c5f --- /dev/null +++ b/lib/utils/normalizePemBytes.ts @@ -0,0 +1,52 @@ +import HiveDriverError from '../errors/HiveDriverError'; + +/** + * Normalise a PEM input (`string` or `Buffer`) accepted on the public surface + * into a `Buffer`. Does a light, ordered BEGIN…END sanity check so a + * truncated/headerless/DER blob (or a stray page that merely contains the + * literals out of order, e.g. a proxy-intercept page) is rejected here rather + * than surfacing as an opaque TLS handshake error further down. The bytes are + * NOT fully parsed in JS — that is deferred to the TLS stack, which returns a + * meaningful error on a malformed PEM/key. + * + * `kind` selects the expected block: `'certificate'` matches a `CERTIFICATE` + * block; `'private key'` matches any `… PRIVATE KEY` block (PKCS#8 `PRIVATE + * KEY`, PKCS#1 `RSA PRIVATE KEY`, SEC1 `EC PRIVATE KEY`). + * + * `backendLabel` prefixes the error message so the caller (e.g. `DBSQLClient` + * on the Thrift path, `kernel backend` on the kernel path) is named accurately. + * + * Throws `HiveDriverError` when the value is empty or (for strings) lacks the + * expected PEM header. + */ +export default function normalizePemBytes( + value: Buffer | string, + optionName: string, + kind: 'certificate' | 'private key', + backendLabel: string, +): Buffer { + if (typeof value === 'string') { + const re = + kind === 'certificate' + ? /-----BEGIN CERTIFICATE-----[\s\S]+?-----END CERTIFICATE-----/ + : /-----BEGIN [A-Z0-9 ]*PRIVATE KEY-----[\s\S]+?-----END [A-Z0-9 ]*PRIVATE KEY-----/; + if (!re.test(value)) { + const expected = + kind === 'certificate' + ? "a '-----BEGIN CERTIFICATE-----' … '-----END CERTIFICATE-----' block" + : "a 'BEGIN … PRIVATE KEY' / 'END … PRIVATE KEY' PEM block (PKCS#8, PKCS#1, or SEC1)"; + throw new HiveDriverError( + `${backendLabel}: \`${optionName}\` string does not look like a PEM ${kind} (expected ${expected}). ` + + 'Pass PEM text or a Buffer of PEM bytes.', + ); + } + return Buffer.from(value, 'utf8'); + } + if (Buffer.isBuffer(value)) { + if (value.length === 0) { + throw new HiveDriverError(`${backendLabel}: \`${optionName}\` Buffer is empty.`); + } + return value; + } + throw new HiveDriverError(`${backendLabel}: \`${optionName}\` must be a PEM string or a Buffer.`); +} diff --git a/tests/unit/DBSQLClient.test.ts b/tests/unit/DBSQLClient.test.ts index 7ee7290e..82a5fe27 100644 --- a/tests/unit/DBSQLClient.test.ts +++ b/tests/unit/DBSQLClient.test.ts @@ -140,14 +140,37 @@ describe('DBSQLClient.connect', () => { it('should map clientCert/clientKey to cert/key for mutual TLS', async () => { const client = new DBSQLClient(); + const clientCert = '-----BEGIN CERTIFICATE-----\nclient-cert\n-----END CERTIFICATE-----\n'; + const clientKey = '-----BEGIN PRIVATE KEY-----\nclient-key\n-----END PRIVATE KEY-----\n'; const connectionOptions = client['getConnectionOptions']({ ...connectOptions, - clientCert: 'client-cert', - clientKey: 'client-key', + clientCert, + clientKey, }); - expect(connectionOptions.cert).to.equal('client-cert'); - expect(connectionOptions.key).to.equal('client-key'); + // Normalised to a Buffer of the PEM bytes (parity with the kernel path). + expect((connectionOptions.cert as Buffer).toString('utf8')).to.equal(clientCert); + expect((connectionOptions.key as Buffer).toString('utf8')).to.equal(clientKey); + }); + + it('should reject a malformed PEM clientCert with a named error (parity with the kernel path)', async () => { + const client = new DBSQLClient(); + + expect(() => + client['getConnectionOptions']({ + ...connectOptions, + clientCert: 'not-a-pem', + clientKey: '-----BEGIN PRIVATE KEY-----\nk\n-----END PRIVATE KEY-----\n', + }), + ).to.throw(/`clientCert` string does not look like a PEM certificate/); + }); + + it('should reject a malformed PEM customCaCert with a named error (parity with the kernel path)', async () => { + const client = new DBSQLClient(); + + expect(() => client['getConnectionOptions']({ ...connectOptions, customCaCert: 'not-a-pem' })).to.throw( + /`customCaCert` string does not look like a PEM certificate/, + ); }); it('should throw if only clientCert is supplied for mutual TLS', async () => { From 884c76f12edc4b9a9ea108eb76e2faed6ddf132e Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Tue, 4 Aug 2026 20:31:19 +0000 Subject: [PATCH 6/6] ai: apply changes for #463 (1 review thread) Addresses: - #3715934115 at lib/DBSQLClient.ts:259 Signed-off-by: peco-engineer-bot[bot] --- lib/DBSQLClient.ts | 5 ++++- tests/unit/DBSQLClient.test.ts | 14 ++++++++------ 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/lib/DBSQLClient.ts b/lib/DBSQLClient.ts index d30e837e..f021edf9 100644 --- a/lib/DBSQLClient.ts +++ b/lib/DBSQLClient.ts @@ -254,7 +254,10 @@ export default class DBSQLClient extends EventEmitter implements IDBSQLClient, I : [ ...tls.rootCertificates, ...DBSQLClient.getExtraCaCerts(), - normalizePemBytes(options.customCaCert, 'customCaCert', 'certificate', 'DBSQLClient').toString(), + // Push the normalized Buffer as-is (Node's `ca` accepts a mixed + // Array) to match the cert/key treatment and keep + // byte-fidelity for Buffer inputs instead of round-tripping through utf-8. + normalizePemBytes(options.customCaCert, 'customCaCert', 'certificate', 'DBSQLClient'), ], // Client certificate + key for mutual TLS (mTLS). Both must be supplied together. cert: clientCert, diff --git a/tests/unit/DBSQLClient.test.ts b/tests/unit/DBSQLClient.test.ts index 82a5fe27..312cf603 100644 --- a/tests/unit/DBSQLClient.test.ts +++ b/tests/unit/DBSQLClient.test.ts @@ -95,9 +95,9 @@ describe('DBSQLClient.connect', () => { const connectionOptions = client['getConnectionOptions']({ ...connectOptions, customCaCert }); expect(connectionOptions.ca).to.be.an('array'); - const ca = connectionOptions.ca as Array; - // The custom cert must be present... - expect(ca).to.include(customCaCert); + const ca = connectionOptions.ca as Array; + // The custom cert must be present, pushed as a Buffer (byte-fidelity, parity with cert/key)... + expect(ca.map((entry) => entry.toString('utf8'))).to.include(customCaCert); // ...alongside the built-in system roots (so public warehouses still validate). expect(ca.length).to.be.greaterThan(1); }); @@ -113,11 +113,13 @@ describe('DBSQLClient.connect', () => { try { const connectionOptions = client['getConnectionOptions']({ ...connectOptions, customCaCert }); - const ca = connectionOptions.ca as Array; - expect(ca).to.include(customCaCert); + const ca = connectionOptions.ca as Array; + const caStrings = ca.map((entry) => entry.toString('utf8')); + // The custom cert is pushed as a Buffer (byte-fidelity, parity with cert/key). + expect(caStrings).to.include(customCaCert); // Roots injected via NODE_EXTRA_CA_CERTS must survive the additive rebuild, // otherwise callers relying on that env var lose their trust anchors. - expect(ca).to.include(extraCaCert); + expect(caStrings).to.include(extraCaCert); } finally { readFileSync.restore(); if (previousEnv === undefined) {