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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
