You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "HyukjinKwon (via GitHub)" <gi...@apache.org> on 2026/08/31 23:33:27 UTC
[PR] [SPARK-59139][CORE] Fix Rust client connection to TLS + token-auth Spark Connect servers [spark-connect-rust]
HyukjinKwon opened a new pull request, #92:
URL: https://github.com/apache/spark-connect-rust/pull/92
### What changes were proposed in this pull request?
Fix two bugs that prevented the Rust Spark Connect client (and the `pyspark-client-rust` wrapper) from connecting to a Spark Connect server that uses **TLS** and requires **bearer-token authentication**. With either bug present the first RPC hangs.
1. **TLS trust store was never loaded.** The client built `ClientTlsConfig::new().domain_name(..)` but never called `.with_native_roots()`. Since tonic >= 0.11 does not load a trust store from the `tls-native-roots` feature alone, the TLS handshake had no CA anchors and failed; the client's retry policy then masked the failure as an apparent hang. Now calls `.with_native_roots()`.
2. **The token was never sent.** The connection-string `token` parameter was parsed but never attached to requests as an `Authorization: Bearer <token>` header — it is a reserved key excluded from request metadata. It is now attached to the client metadata, so it propagates to every request (including the reattach stream).
Also converts `test_token_bearer_in_metadata` into a real regression test: it previously only asserted the token *parsed*; it now asserts the client actually attaches the `Authorization: Bearer` header.
### Why are the changes needed?
Without these fixes the client cannot talk to any TLS-secured, token-authenticated Spark Connect endpoint — the first RPC hangs instead of returning an error. Plaintext, no-auth servers were unaffected, which is why local testing did not catch it.
### Does this PR introduce _any_ user-facing change?
No behavior change for existing (plaintext / no-auth) connections. It enables connections to TLS + token-authenticated servers that previously hung.
### How was this patch tested?
- New/updated unit test `test_token_bearer_in_metadata` asserts the `Authorization: Bearer` header is attached (previously it only checked parsing); `cargo test -p apache-spark-connect-core test_token_bearer_in_metadata` passes.
- Manually verified end to end: with a `use_ssl=true;token=…` connection string, a rebuilt wheel connects and runs queries against a live TLS-secured, token-authenticated Spark Connect endpoint, where the unpatched client hung on the first RPC.
This pull request and its description were written by Isaac.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59139] Fix Rust client connection to TLS + token-auth Spark Connect servers [spark-connect-rust]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon closed pull request #92: [SPARK-59139] Fix Rust client connection to TLS + token-auth Spark Connect servers
URL: https://github.com/apache/spark-connect-rust/pull/92
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59139] Fix Rust client connection to TLS + token-auth Spark Connect servers [spark-connect-rust]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #92:
URL: https://github.com/apache/spark-connect-rust/pull/92#issuecomment-5486670118
Post-merge note — I went through this after it landed on master (`7116d72`) and branch-4.2 (`30a2d71`). Both fixes are right, and I traced the mechanisms rather than taking the description's word for it: `metadata` (`client.rs:60`) → `_attach_metadata` (`client.rs:836-842`) inserts each pair on every RPC, and `with_new_session_id` (`client.rs:153`) clones it, so "propagates to every request including the reattach stream" checks out. The `with_native_roots()` diagnosis is accurate too — since tonic 0.11 the `tls-native-roots` feature only pulls in `rustls-native-certs` without installing it into `ClientTlsConfig`, so the handshake had no anchors and the 15-retry backoff turned that failure into an apparent hang. Sweeping the repo, TLS setup and token attachment each have exactly one site, so there's no sibling instance either fix missed. And converting `test_token_bearer_in_metadata` from "the token parses" to "the header is attached" closes a genuinely vacuous test — it pa
sses offline, since `connect_lazy()` means `connect()` needs no live server.
**One thing the fix introduces, though: the bearer token can now go out in cleartext.** `secure()` is `use_ssl() || token().is_some()` (`channel.rs:159`), but the scheme is chosen from `use_ssl()` alone (`client.rs:88`), and the new header is attached unconditionally (`client.rs:124`) without consulting either. So `sc://example.com/;token=SECRET` sends `Authorization: Bearer SECRET` over plain `http://`. I confirmed it with a probe against the merged commit:
```
PROBE use_ssl=false secure=true auth_header=Some(("authorization", "Bearer SECRET123"))
```
The reference client can't reach that state. `toChannel()` (`core.py:648-665`) is three-way: no token → insecure channel; token with `use_ssl=false` **and host == localhost** → `local_channel_credentials()`; everything else → `ssl_channel_credentials()`. The token is only ever attached via `grpc.access_token_call_credentials`, and grpcio refuses call credentials on an insecure channel by design — precisely to stop credentials leaking in the clear. So upstream's token always rides an encrypted (or loopback) channel.
Worth noting `secure()` is computed here but never actually gates channel construction — which is the same "declared but never applied" shape as SPARK-59032 (parsed keepalive never applied) and SPARK-59037 (128 MiB cap declared but unused), and indeed as the token half of this very PR. Also relevant: `token()` falls back to the `SPARK_CONNECT_AUTHENTICATE_TOKEN` env var (`channel.rs:154`), so a connection string with no `token` at all can still take this path — the exposure is wider than the connection-string case alone.
I'd suggest matching upstream's semantics: when a token is present with `use_ssl=false` and the host is not `localhost`/`127.0.0.1`, return an error naming `use_ssl=true` rather than silently downgrading. Keeping the localhost case working matters for local development (upstream allows it too), just as an explicit exception. Failing loudly is better than a silent cleartext send: a mistyped connection string currently leaks the credential, whereas an error message gets it fixed on the spot.
To be clear, I don't think this needs a revert — the two bugs it fixes are real and the fixes are correct. This is a follow-up.
Two minor things while I'm here. The header key is a hardcoded `"authorization"`; `http::header::AUTHORIZATION` (or a `const`) would avoid re-parsing it through `MetadataKey::from_bytes` on every request. And the test asserts on the `client.metadata` field rather than on a real request — legal from the in-crate `mod tests`, but it verifies "the value is in the vec", not "the header reaches the wire". Building a `Request`, calling `_attach_metadata`, and checking `req.metadata()` would survive a future change to the insert logic. Neither is worth a follow-up on its own.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org