HyukjinKwon opened a new pull request, #93:
URL: https://github.com/apache/spark-connect-rust/pull/93

   ### What changes were proposed in this pull request?
   
   Follow-up to #92 (addresses [this review 
comment](https://github.com/apache/spark-connect-rust/pull/92#issuecomment-5486670118)).
 That PR started attaching the connection-string `token` as an `Authorization: 
Bearer <token>` header, but the transport scheme is chosen from `use_ssl()` 
alone and the header was attached unconditionally. As a result 
`sc://host/;token=SECRET` (no `use_ssl`) would send the bearer token over plain 
`http://`.
   
   This change refuses to attach the token when the connection is not TLS and 
the host is not loopback, returning an error that names `use_ssl=true`, instead 
of silently downgrading to cleartext. A token over plaintext loopback 
(`localhost`/`127.0.0.1`/`[::1]`) is still allowed for local development, 
mirroring the reference client's `local_channel_credentials()` exception in 
`core.py` `toChannel`.
   
   Two minor cleanups the reviewer noted are folded in:
   - the `"authorization"` header key is now a named `const` rather than a 
string literal;
   - `test_token_bearer_in_metadata` now builds a real `Request`, runs it 
through `_attach_metadata`, and asserts on the resulting gRPC metadata rather 
than on the `client.metadata` field.
   
   ### Why are the changes needed?
   
   To stop a mistyped or SSL-less connection string from leaking the 
authentication token in the clear. The reference client can never reach that 
state because grpcio refuses call credentials on an insecure channel; this 
restores parity by failing loudly.
   
   ### Does this PR introduce _any_ user-facing change?
   
   Yes. `SparkConnectClient::connect` now returns an error for a connection 
string that supplies a `token` (or the `SPARK_CONNECT_AUTHENTICATE_TOKEN` env 
var) without `use_ssl=true` when the host is not loopback. The remedy is to set 
`use_ssl=true`.
   
   ### How was this patch tested?
   
   New unit tests cover the rejection on a remote host, the loopback exception 
for `localhost`/`127.0.0.1`/`[::1]`, and that the bearer header reaches a real 
request's metadata. `cargo test -p apache-spark-connect-core` passes.
   
   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: [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]

Reply via email to