Fix #462: verify Thrift TLS server certificate by default [PECOBLR-3837][SEC-20280] - #463
Conversation
|
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 ( |
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 <madhavendra.rathore@databricks.com>
126467b to
055fc81
Compare
msrathore-db
changed the title
vikrantpuppala
added
the
engineer-bot
GitHub Actions
Failure
|
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 ( |
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
…tls-verification Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
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 <madhavendra.rathore@databricks.com>
5446896 to
b28ee14
Compare
There was a problem hiding this comment.
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.
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>
There was a problem hiding this comment.
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.
Addresses: - #3715785609 at lib/DBSQLClient.ts:240 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
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.
Addresses: - #3715840256 at lib/DBSQLClient.ts:241 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
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.
Addresses: - #3715934115 at lib/DBSQLClient.ts:259 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
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 <madhavendra.rathore@databricks.com>
3 participants
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
httpsdefault, the JDBC/ODBC drivers, and the SEA/kernel backend) and adds SEA-parity public options onConnectionOptions.Changes
HttpConnection.createHttpsAgent:rejectUnauthorizednow defaults totrue; onlyfalsewhen explicitly opted out.ConnectionOptions(SEA-parity naming):checkServerCertificate(defaulttrue) → maps to the agent'srejectUnauthorized.customCaCert→ additive to the system trust store (appended totls.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
checkServerCertificate: falsecustomCaCertclientCert/clientKeyTesting
checkServerCertificate↔rejectUnauthorized,customCaCertadditive,clientCert/clientKeymapping.customCaCertwith an unrelated CA added still connects (proving additive, not replacing);checkServerCertificate: falsestill connects.clientCert/clientKeycomplete 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: falseor supplycustomCaCert. This matches Node/JDBC/ODBC/SEA behavior.This pull request and its description were written by Isaac.