Fix #462: verify Thrift TLS server certificate by default [PECOBLR-3837][SEC-20280]

lib/DBSQLClient.ts

@@ -1,5 +1,7 @@
 import thrift from 'thrift';
 import os from 'os';
+import fs from 'fs';
+import tls from 'tls';
 
 import { EventEmitter } from 'events';
 import TCLIService from '../thrift/TCLIService';
@@ -14,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';
@@ -190,14 +192,78 @@ 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<string> {
+    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`'}.`,
+      );
+    }
+
+    // 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,
       path: prependSlash(options.path),
       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 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(),
+              // Push the normalized Buffer as-is (Node's `ca` accepts a mixed
+              // Array<string | Buffer>) 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,
+      key: clientKey,
+      // Validate the server certificate unless the caller explicitly opts out.
+      rejectUnauthorized: options.checkServerCertificate ?? true,
       headers: {
         'User-Agent': buildUserAgentString(options.userAgentEntry),
       },

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,

lib/connection/contracts/IConnectionOptions.ts

@@ -19,7 +19,11 @@ export default interface IConnectionOptions {
   socketTimeout?: number;
   proxy?: ProxyOptions;
 
-  ca?: Buffer | string;
+  ca?: Buffer | string | Array<Buffer | string>;
   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;
 }

lib/contracts/IDBSQLClient.ts

@@ -59,6 +59,54 @@ 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 built-in roots — for TLS-inspecting proxies or on-prem
+   * internal CAs. Because it is additive, connections to public Databricks
+   * 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.
+   */
+  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

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.
  *
@@ -321,11 +277,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 +292,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 = {};
 
@@ -348,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
@@ -365,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;

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,
 };

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.`);
+}