Fix #462: verify Thrift TLS server certificate by default#463
Open
msrathore-db wants to merge 1 commit into
Open
Fix #462: verify Thrift TLS server certificate by default#463msrathore-db wants to merge 1 commit into
msrathore-db wants to merge 1 commit into
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>
msrathore-db
force-pushed
the
fix/issue-462-thrift-tls-verification
branch
from
July 24, 2026 20:40
126467b to
055fc81
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
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.