GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Fix #462: verify Thrift TLS server certificate by default [PECOBLR-3837][SEC-20280] by msrathore-db · Pull Request #463 · databricks/databricks-sql-nodejs

Conversation

msrathore-db commented Jul 24, 2026

Copy link

Copy Markdown

Contributor

Summary

Fixes #462. The Thrift HTTPS agent hardcoded rejectUnauthorized: false, which disabled server-certificate verification entirely on the primary Thrift transport. Any self-signed, expired, or wrong-hostname certificate was accepted, exposing the connection — including bearer-token auth headers — to man-in-the-middle attacks. Because verification was off, supplying a CA had no effect, so strict verification was not even possible on the Thrift path.

This PR makes the Thrift path secure-by-default (matching Node's https default, the JDBC/ODBC drivers, and the SEA/kernel backend) and adds SEA-parity public options on ConnectionOptions.

Changes

HttpConnection.createHttpsAgent: rejectUnauthorized now defaults to true; only false when explicitly opted out.
New public ConnectionOptions (SEA-parity naming):
- checkServerCertificate (default true) → maps to the agent's rejectUnauthorized.
- customCaCert → additive to the system trust store (appended to tls.rootCertificates), so trusting an internal/proxy CA does not drop the public roots and break normal Databricks warehouses.
- clientCert / clientKey → client certificate + key for mutual TLS (mTLS).

Behavior

Config	Before	After
default	verification off (insecure)	verification on
bad/self-signed cert	silently accepted	rejected
`checkServerCertificate: false`	(only behavior)	explicit opt-out
`customCaCert`	ignored	trusted, additive to system roots
`clientCert`/`clientKey`	n/a	mTLS supported

Testing

Unit tests (94 passing): default-verify-on, checkServerCertificate ↔ rejectUnauthorized, customCaCert additive, clientCert/clientKey mapping.
Real warehouse (Azure SQL warehouse): default connects; a bogus CA is now rejected (proving verification is enforced); customCaCert with an unrelated CA added still connects (proving additive, not replacing); checkServerCertificate: false still connects.
Hermetic mTLS: against a local server requiring client certs, the public clientCert/clientKey complete the handshake, while omitting them fails at the TLS layer.

Compatibility

Secure-by-default is a behavior change: environments relying on the old permissive default (self-signed/internal certs) must set checkServerCertificate: false or supply customCaCert. This matches Node/JDBC/ODBC/SEA behavior.

This pull request and its description were written by Isaac.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

msrathore-db temporarily deployed to azure-prod

July 24, 2026 20:14

— with

GitHub Actions Inactive

msrathore-db temporarily deployed to azure-prod

July 24, 2026 20:14

— with

GitHub Actions Inactive

msrathore-db temporarily deployed to azure-prod

July 24, 2026 20:14

— with

GitHub Actions Inactive

msrathore-db temporarily deployed to azure-prod

July 24, 2026 20:14

— with

GitHub Actions Inactive

github-actions Bot commented Jul 24, 2026

Copy link

Copy Markdown

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          Fix #462: verify Thrift TLS server certificate by default

055fc81

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 <[email protected]>

msrathore-db force-pushed the fix/issue-462-thrift-tls-verification branch from 126467b to 055fc81 Compare

July 24, 2026 20:40

msrathore-db temporarily deployed to azure-prod

July 24, 2026 20:40

— with

GitHub Actions Inactive

msrathore-db temporarily deployed to azure-prod

July 24, 2026 20:40

— with

GitHub Actions Inactive

msrathore-db temporarily deployed to azure-prod

July 24, 2026 20:40

— with

GitHub Actions Inactive

msrathore-db temporarily deployed to azure-prod

July 24, 2026 20:40

— with

GitHub Actions Inactive

msrathore-db changed the title ~~Fix #462: verify Thrift TLS server certificate by default~~ Fix #462: verify Thrift TLS server certificate by default [PECOBLR-3837][SEC-20280]

Jul 26, 2026

sreekanth-db approved these changes

Jul 29, 2026

View reviewed changes

vikrantpuppala added the engineer-bot Maintainer-only: triggers the autonomous engineer-bot (bug-fix author on issues; follow-up on PRs) label

Jul 30, 2026

msrathore-db temporarily deployed to azure-prod

August 4, 2026 19:43

— with

GitHub Actions Inactive

msrathore-db temporarily deployed to azure-prod

August 4, 2026 19:43

— with

GitHub Actions Inactive

msrathore-db temporarily deployed to azure-prod

August 4, 2026 19:43

— with

GitHub Actions Inactive

msrathore-db had a problem deploying to azure-prod

August 4, 2026 19:43

— with

GitHub Actions Failure

github-actions Bot commented Aug 4, 2026

Copy link

Copy Markdown

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

peco-review-bot Bot reviewed

Aug 4, 2026

View reviewed changes

peco-review-bot Bot left a comment

Copy link

Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Verdict: 1 Medium · 1 Low

Solid, correctly-scoped security fix — flipping the Thrift HTTPS agent to verify server certs by default is the right call, the additive-CA approach avoids breaking public warehouses, and the unit coverage (default-on, opt-out mapping, additive CA, mTLS mapping) is good. Two edge-case notes: customCaCert silently drops NODE_EXTRA_CA_CERTS roots (medium), and clientCert/clientKey pairing isn't validated despite the docs requiring it (low). The proxy path is fine since createProxyAgent delegates to createHttpsAgent, so the new TLS defaults flow through there too.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

lib/DBSQLClient.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

lib/DBSQLClient.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

msrathore-db added 2 commits

August 5, 2026 01:16


          Merge remote-tracking branch 'origin/main' into fix/issue-462-thrift-…

2dad965

…tls-verification

Signed-off-by: Madhavendra Rathore <[email protected]>


          fix(ci): clear security-scan CVE findings

b28ee14

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 <[email protected]>

msrathore-db force-pushed the fix/issue-462-thrift-tls-verification branch from 5446896 to b28ee14 Compare

August 4, 2026 19:47

msrathore-db temporarily deployed to azure-prod

August 4, 2026 19:47

— with

GitHub Actions Inactive

msrathore-db temporarily deployed to azure-prod

August 4, 2026 19:47

— with

GitHub Actions Inactive

msrathore-db temporarily deployed to azure-prod

August 4, 2026 19:47

— with

GitHub Actions Inactive

msrathore-db temporarily deployed to azure-prod

August 4, 2026 19:47

— with

GitHub Actions Inactive

peco-review-bot Bot reviewed

Aug 4, 2026

View reviewed changes

peco-review-bot Bot left a comment

Copy link

Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Verdict: 1 Medium · 1 Low

Solid, well-motivated security fix: flipping the Thrift HTTPS agent to verify-by-default is correct, the additive customCaCert handling is right, and the unit tests cover the mapping well. Two consistency concerns worth addressing before merge, both around parity between the new public options and the kernel/SEA path.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

lib/contracts/IDBSQLClient.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

lib/DBSQLClient.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.


          ai: apply changes for #463 (4 review threads)

ecee476

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] <peco-engineer-bot[bot]@users.noreply.github.com>

peco-engineer-bot Bot temporarily deployed to azure-prod

August 4, 2026 20:00

Inactive

peco-engineer-bot Bot temporarily deployed to azure-prod

August 4, 2026 20:00

Inactive

peco-engineer-bot Bot temporarily deployed to azure-prod

August 4, 2026 20:00

Inactive

peco-review-bot Bot approved these changes

Aug 4, 2026

View reviewed changes

peco-review-bot Bot left a comment

Copy link

Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Verdict: 1 Low

Solid, well-tested secure-by-default TLS fix — the hardcoded rejectUnauthorized: false is correctly replaced with ?? true, the additive-CA and mTLS-pair validation are sound, and the kernel-path alias handling (clientCert/clientKey → clientCertPem/clientKeyPem) is consistent. One low-severity edge around how the additive ca interacts with OS-trust-store roots (F1). Minor nit not filed inline: the Thrift-path mTLS guard in getConnectionOptions checks only !== undefined, so a defined-but-empty clientCert: ''/clientKey: '' pair slips past and fails opaquely in Node's TLS layer (the kernel path's normalizePemBytes catches this) — a defensive PEM/empty check there would mirror the kernel path.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

lib/DBSQLClient.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.


          ai: apply changes for #463 (1 review thread)

18ee36f

Addresses:
  - #3715785609 at lib/DBSQLClient.ts:240

Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>

peco-engineer-bot Bot temporarily deployed to azure-prod

August 4, 2026 20:06

Inactive

peco-engineer-bot Bot temporarily deployed to azure-prod

August 4, 2026 20:06

Inactive

peco-engineer-bot Bot temporarily deployed to azure-prod

August 4, 2026 20:06

Inactive

peco-engineer-bot Bot temporarily deployed to azure-prod

August 4, 2026 20:06

Inactive

peco-review-bot Bot approved these changes

Aug 4, 2026

View reviewed changes

peco-review-bot Bot left a comment

Copy link

Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Verdict: 1 Low

Solid, well-tested security fix — Thrift TLS now verifies the server cert by default and the secure default correctly propagates to the proxy agent and CloudFetch downloads (both reuse createHttpsAgent/getAgent). mTLS pairing is validated on both backends and the kernel clientCert/clientKey alias merge is sound. One low-severity note: the Thrift path skips the PEM structural validation the kernel path performs, so malformed cert input yields an opaque TLS error there. Minor coverage gap (not posted inline): no test exercises a Buffer-typed customCaCert on the Thrift path (the .toString() branch) — worth adding.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

lib/DBSQLClient.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.


          ai: apply changes for #463 (1 review thread)

81ce468

Addresses:
  - #3715840256 at lib/DBSQLClient.ts:241

Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>

peco-engineer-bot Bot temporarily deployed to azure-prod

August 4, 2026 20:22

Inactive

peco-engineer-bot Bot temporarily deployed to azure-prod

August 4, 2026 20:22

Inactive

peco-engineer-bot Bot temporarily deployed to azure-prod

August 4, 2026 20:22

Inactive

peco-engineer-bot Bot temporarily deployed to azure-prod

August 4, 2026 20:22

Inactive

peco-review-bot Bot approved these changes

Aug 4, 2026

View reviewed changes

peco-review-bot Bot left a comment

Copy link

Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Verdict: 1 Low

Solid, well-tested security fix — server-cert verification now defaults on across both the direct HTTPS agent and the proxy's inner agent, and I confirmed no other rejectUnauthorized: false sites remain. One Low inconsistency in how a Buffer customCaCert is validated/serialized vs clientCert/clientKey; otherwise looks good.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

lib/DBSQLClient.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.


          ai: apply changes for #463 (1 review thread)

884c76f

Addresses:
  - #3715934115 at lib/DBSQLClient.ts:259

Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>

peco-engineer-bot Bot temporarily deployed to azure-prod

August 4, 2026 20:31

Inactive

peco-engineer-bot Bot temporarily deployed to azure-prod

August 4, 2026 20:31

Inactive

peco-engineer-bot Bot temporarily deployed to azure-prod

August 4, 2026 20:31

Inactive

peco-engineer-bot Bot temporarily deployed to azure-prod

August 4, 2026 20:31

Inactive

peco-review-bot Bot approved these changes

Aug 4, 2026

View reviewed changes

peco-review-bot Bot left a comment

Copy link

Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

✅ No issues identified by the review bot.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

msrathore-db added this pull request to the merge queue

Aug 4, 2026

Merged via the queue into main with commit 38ead45

Aug 4, 2026

29 checks passed

Uh oh!

There was an error while loading. Please reload this page.

msrathore-db mentioned this pull request

Aug 4, 2026

Revert the brace-expansion CVE suppressions from #463 #484

Merged

msrathore-db added a commit that referenced this pull request

Aug 4, 2026


          Revert the brace-expansion CVE suppressions from #463 (#484)

86e8337

Removes the two [[IgnoredVulns]] entries added in #463, restoring
osv-scanner.toml to having no suppressions. The real dependency
bumps from that PR are deliberately kept: brace-expansion 1.1.16 /
2.1.3 and ip-address 10.3.1 (the latter a production dependency via
socks) all stay in package-lock.json.

Why revert: [[IgnoredVulns]] entries are CVE-id global in OSV-Scanner
v2.3.8 -- they silence the advisory across every package it is
reported against, not just brace-expansion. That is broader than the
problem being solved, and the entries landed inside a security PR
without a security reviewer explicitly signing off on them. Backing
them out so the decision can be made on its own merits.

Consequence, intentional: the Security Scan gate fails again on three
dev-only brace-expansion findings (GHSA-mh99-v99m-4gvg,
GHSA-rgw5-rvv9-x895). There is no published fix -- the advisories name
1.1.17/1.1.18/2.1.4 but npm's latest releases are 1.1.16 and 2.1.3.
These are dev-only: brace-expansion reaches us solely via the
eslint/glob/test-exclude toolchains through minimatch, `npm ls
brace-expansion --omit=dev` is empty, and both lockfile entries are
marked "dev": true, so they are not reachable from the published
dist/. Impact is DoS on adversarial brace patterns, which would
require untrusted input to our own lint/test globs.

Note the gate was already failing on main before #463 for these same
dependencies, so this restores the prior state rather than causing a
new regression.

Signed-off-by: Madhavendra Rathore <[email protected]>

This was referenced Aug 4, 2026

[coverage] Conformance findings: AUTH-012 #485

Open

[coverage] Conformance findings: AUTH-012,AUTH-015 databricks/databricks-sql-python#892

Open

vuanhphung mentioned this pull request

Oct 29, 2026

Allow setting TLS settings on the underlying HTTPS agent #197

Closed

vuanhphung mentioned this pull request

Nov 11, 2026

fix: apply TLS options to Thrift connections made through a proxy #595

Open

This branch was previously deployed

1 inactive deployment

azure-prod — 884c76f1 Deployed Aug 4, 2026 by peco-engineer-bot[bot] via e2e-test (24) #1409

Reviewers

peco-engineer-bot[bot]

peco-engineer-bot[bot] left review comments

sreekanth-db

sreekanth-db approved these changes

peco-review-bot[bot]

peco-review-bot[bot] approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

engineer-bot

Maintainer-only: triggers the autonomous engineer-bot (bug-fix author on issues; follow-up on PRs)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Thrift backend hardcodes rejectUnauthorized: false — no way to enable TLS certificate verification

Uh oh!

There was an error while loading. Please reload this page.

3 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.