feat(sea): expose TLS verification toggle + custom CA on ConnectionOptions#407
Open
msrathore-db wants to merge 5 commits into
Open
feat(sea): expose TLS verification toggle + custom CA on ConnectionOptions#407msrathore-db wants to merge 5 commits into
msrathore-db wants to merge 5 commits into
Conversation
…tions Surface the kernel/napi TLS knobs on the public SEA ConnectionOptions so NodeJS callers reach the connectivity-parity TLS controls: - checkServerCertificate?: boolean — default false. Matches the legacy Thrift driver's permissive rejectUnauthorized:false (accept self-signed/untrusted/expired + skip hostname check) for drop-in parity. Set true for strict validation (JDBC/ODBC parity). - customCaCert?: Buffer | string — PEM CA added to the trust store on top of system roots, for corporate TLS-inspecting proxies / on-prem internal CAs. Honoured regardless of checkServerCertificate. A PEM string is normalised to a Buffer before crossing the FFI boundary. buildSeaTlsOptions validates customCaCert (PEM header / non-empty) and SeaBackend.connect emits a one-line DEBUG note whenever verification is left disabled, so an insecure connection is discoverable in logs without being noisy on every connect (the default is deliberately permissive for thrift parity). mTLS (clientCert/clientKey) is intentionally out of scope. Pairs with the kernel napi change exposing checkServerCertificate + customCaCert. 215 -> 224 SEA unit tests pass (14 new TLS/log cases). Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
End-to-end coverage against a real workspace (PAT), skipped when secrets are absent: - default permissive: connects + SELECT 1 - strict (checkServerCertificate: true): the real publicly-signed workspace cert validates against the system roots - strict + additive customCaCert as PEM string and as Buffer: an extra self-signed root does not break system-root validation - malformed customCaCert (PEM header, garbage body): kernel rejects The additive CA is embedded so the suite is self-contained in CI. Verified locally: 5/5 pass. Separately confirmed the negative security case with a local self-signed HTTPS server — strict is rejected at the TLS layer while permissive gets past TLS to the HTTP response. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
msrathore-db
force-pushed
the
msrathore/sea-async-execute-impl
branch
from
b4ab849 to
1a327fd
Compare
msrathore-db
force-pushed
the
msrathore/sea-tls-connectivity
branch
from
f857d15 to
1a72cad
Compare
This was referenced May 31, 2026
Closed
Closed
|
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 ( |
Two clean-install blockers flagged in review: 1. `flatbuffers` was declared `^25.9.23`, but `apache-arrow@13` is built against flatbuffers ~23.x. A fresh `npm ci` pulled 25.x, whose `ByteBuffer`/`Builder` types are incompatible with apache-arrow's generated `fb/*` accessors → dozens of TS2345 in `SeaArrowIpcDurationFix.ts`, blocking `npm run build` and all e2e. Pin to `23.5.26` (the version apache-arrow@13 resolves) so the top-level and arrow-nested flatbuffers types match. tsc clean. 2. The napi-rs router `native/sea/index.js` was not committed (only `.node` + `.d.ts` were), so a fresh worktree failed at runtime with `Cannot find module '../../native/sea/index.js'`. Commit the router (it's stable/branch-independent) so the published tarball and fresh checkouts can load SEA without first running `build:native`. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
|
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 ( |
Reverses the permissive-by-default TLS decision. The SEA backend now defaults to strict certificate verification (chain + expiry + hostname) — secure by default, like JDBC/ODBC. Omitting checkServerCertificate leaves the napi/kernel secure default (pass-through unchanged: absent ⇒ not forwarded ⇒ kernel TlsConfig::default()). Disabling is still supported for parity with the legacy Thrift driver (`rejectUnauthorized: false`): set `checkServerCertificate: false`. That path now emits a warn-level log (was debug, and fired at the old permissive default) since disabling is a deliberate insecure opt-out rather than the default. Updated the ConnectionOptions doc, SeaAuth comments, the warn logic + its tests, and the tls-options default-label. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
|
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 ( |
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
1 participant
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.
What
Surfaces the kernel/napi TLS knobs on the public SEA
ConnectionOptionsso NodeJS callers reach the connectivity-parity TLS controls. Depends on kernel PR databricks/databricks-sql-kernel#101 (msrathore/krn-tls-napi).Two new public options:
checkServerCertificate?: boolean— defaultfalse. Matches the legacy Thrift driver's permissiverejectUnauthorized: false(accepts self-signed / untrusted / expired certs and skips the hostname check) for drop-in parity. Settruefor strict validation (JDBC/ODBC parity).customCaCert?: Buffer | string— PEM CA added to the trust store on top of system roots, for corporate TLS-inspecting proxies / on-prem internal CAs. Honoured regardless ofcheckServerCertificate. A PEM string is normalised to aBufferbefore crossing the FFI boundary.A single master switch maps onto the kernel's two knobs (
accept_self_signed+skip_hostname_verification=!check).Behavior details
buildSeaTlsOptionsvalidatescustomCaCert(PEM header for strings / non-empty for Buffers) and converts string → Buffer. The PEM bytes are not parsed in JS — the kernel returns a meaningful error on malformed input.SeaBackend.connectemits a one-line DEBUG note whenever verification is left disabled (the default), so an insecure connection is discoverable in logs without being noisy on every connect.Default-behavior note
Per owner direction, SEA is permissive by default to match the Thrift driver's historical behavior — the opposite of the strict default recommended in the
tls-rejectUnauthorized-researchspec. Strict validation is one opt-in flag away (checkServerCertificate: true), ideally paired withcustomCaCertfor corporate/on-prem CAs.Scope
mTLS (
clientCert/clientKey) is intentionally out of scope (kernel support not yet wired).Testing
index.d.tsmatches the hand-written surface.prettier --check+eslintclean on changed files.tls-options.test.ts—buildSeaTlsOptionsdefaults/strict/explicit-false, string→Buffer conversion, custom-CA in all modes, PEM-header and empty-Buffer rejection, end-to-end passthrough.SeaBackend-tls-warn.test.ts— debug note fires when disabled (default + explicit false), silent when enabled.This pull request and its description were written by Isaac.
Downstream fixes / reviewer note
flatbuffers, one-pass IPC duration rewrite, cancel/close local-state rollback on native RPC failure, closed fetch →OperationStateError(Closed), and Arrow duration rewriter cleanup.sessionId/statementIdtyping so logs and operation ids use server-issued ids where available.