Conversation
9a7da00 to
90e7395
Compare
pan3793
changed the title
HyukjinKwon
left a comment
HyukjinKwon
left a comment
There was a problem hiding this comment.
0 blocking, 0 non-blocking, 0 nits.
Clean, faithful backport: the StreamManager wrapper fails closed on chunk/stream serving before authentication and delegates lifecycle callbacks unconditionally; the documentation and tests are accurate.
Verification
Confirmed the fail-closed contract: getChunk, openStream, and both checkAuthorization overloads call checkAuthenticated() (AbstractAuthRpcHandler.java:149/155/161/167), which throws SecurityException while !isAuthenticated (line 143); the six lifecycle/accounting callbacks delegate with no auth check (e.g. connectionTerminated, line 173). Verified the base StreamManager.checkAuthorization overloads are empty no-ops, matching the Javadoc. Verified TransportContext wires the chunk-fetch handler through the per-channel handler (channelRpcHandler.getStreamManager(), line 211). Checked the one cross-thread question: isAuthenticated is non-volatile but has a single writer on the channel I/O thread (receive(), line 62); the chunk-fetch path reads it on the chunkFetchWorkers executor, and Netty's per-message executor handoff establishes happens-before from the auth write to that read, so visibility holds and the worst case is fail-closed. Both concrete subclasses inherit getStreamManager without override.
PR metadata suggestions
- Document: the title/body frame this as a delegation "improvement" tagged [MINOR], but the effect is a security hardening -- StreamRequest/ChunkFetchRequest were served pre-auth on channels behind an auth bootstrap (the file-distribution channel was unprotected even with spark.authenticate enabled) and now fail closed. Consider stating that in the body so the change's intent is clear.
HyukjinKwon
left a comment
HyukjinKwon
left a comment
There was a problem hiding this comment.
One follow-up on the default-behavior change in this backport (see inline).
| @Override | ||
| public StreamManager getStreamManager() { | ||
| return delegate.getStreamManager(); | ||
| return new AuthCheckingStreamManager(delegate.getStreamManager()); |
There was a problem hiding this comment.
This flips a default on a maintenance branch: on a channel behind an authentication bootstrap, StreamRequest/ChunkFetchRequest that used to be served now throw SecurityException until the channel authenticates. On master that hardening is clearly the right call, but shipping it in a [3.5] patch release changes runtime behavior operators did not opt into -- anything that today relies on stream/chunk serving before the handshake completes on such a channel would start getting rejected after a minor upgrade.
Could we gate the fail-closed path behind a config that defaults to the current (pre-PR) behavior on branch-3.5 (and can default on in master), so the patch release stays behavior-compatible and operators opt into the stricter check? If we would rather not add a flag, let's at least call the behavior change out explicitly in the description and release notes so it is not a silent default change in a maintenance line.
90e7395 to
03807fc
Compare
Co-Authored-By: Holden Karau <holden@pigscanfly.ca>
03807fc to
5b0ea72
Compare
Add spark.network.auth.requireAuthForStreamRequests (default false on branch-3.5) so the AuthCheckingStreamManager fail-closed path is opt-in and the patch release keeps the historical pre-auth serving behavior unless operators enable it. Co-authored-by: Cursor <cursoragent@cursor.com> Co-Authored-By: Holden Karau <holden@pigscanfly.ca>
This branch has not been deployed
3 participants
Improve Transport Context Stream Manager Delegation by requiring authentication when enabled
This is a backport of 3b47eee behind a feature flag since 3.5 is in extended LTS mode.
Generated-By: Claude (multiple models) and Cursor with Kimi (K3)