gauravkm commented on code in PR #3857:
URL: https://github.com/apache/celeborn/pull/3857#discussion_r4129321822
##########
common/src/main/java/org/apache/celeborn/common/network/client/TransportClient.java:
##########
@@ -401,6 +406,58 @@ public void setClientId(String id) {
this.clientId = id;
}
+ /** Installs a connection-bound policy once, after the plugin has
authenticated its peer. */
+ public synchronized void setSecurityContext(ConnectionSecurityContext
context) {
+ Preconditions.checkNotNull(context, "securityContext");
+ Preconditions.checkState(securityContext == null, "Security context has
already been set.");
+ securityContext = context;
Review Comment:
CI has failing checks; I listed the current failures in the [review
summary](https://github.com/apache/celeborn/pull/3857#issuecomment-5883036944).
[P1] Wire the installed security context into RPC authorization. With a
custom bootstrap authenticating a connection and installing a context here, but
native SASL disabled, `clientId` remains null. A `PbUnregisterShuffle` request
for another application then goes through `NettyRpcHandler` to
`Master.receiveAndReply`; `RpcEndpoint.checkAuth` checks only the native client
ID, so it permits the request without invoking the installed policy. Master can
consequently remove another application’s shuffle even when that context would
deny it. The new direct `client.authorize(...)` tests do not exercise this
dispatch path. Please route RPC operations through the connection policy before
state changes, and add a regression test that sends a cross-application
unregister RPC through an authenticated connection and verifies both rejection
and unchanged shuffle state.
--
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]
