Copilot commented on code in PR #3857: URL: https://github.com/apache/celeborn/pull/3857#discussion_r4091647676
########## common/src/main/java/org/apache/celeborn/common/network/security/ServiceAuthorizer.java: ########## @@ -0,0 +1,43 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.celeborn.common.network.security; + +import javax.annotation.Nullable; + +/** Connection-local policy for the Master's service request/reply operations. */ +@FunctionalInterface +public interface ServiceAuthorizer { + enum Operation { + REGISTER_WORKER, + WORKER_HEARTBEAT, + REPORT_WORKER_UNAVAILABLE, + REPORT_WORKER_DECOMMISSION, + EXCLUDE_WORKERS, + WORKER_LOST, + WORKER_EVENT, + GET_APPLICATION_META, + REMOVE_WORKERS_UNAVAILABLE_INFO Review Comment: `PbCheckWorkersAvailable` is not represented here and `Master.receiveAndReply` dispatches that RPC without calling `checkServiceAuth`. Since `LifecycleManager.checkWorkersAvailable()` uses this endpoint, a connection accepted by a custom service bootstrap can invoke it regardless of its service role. Add an operation and guard this case (with a regression test), or explicitly route it through the intended application policy. ########## common/src/main/java/org/apache/celeborn/common/network/security/ServiceAuthorizer.java: ########## @@ -0,0 +1,43 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.celeborn.common.network.security; + +import javax.annotation.Nullable; + +/** Connection-local policy for the Master's service request/reply operations. */ +@FunctionalInterface +public interface ServiceAuthorizer { + enum Operation { + REGISTER_WORKER, + WORKER_HEARTBEAT, + REPORT_WORKER_UNAVAILABLE, + REPORT_WORKER_DECOMMISSION, + EXCLUDE_WORKERS, + WORKER_LOST, + WORKER_EVENT, + GET_APPLICATION_META, + REMOVE_WORKERS_UNAVAILABLE_INFO + } + + /** + * Throws if this connection cannot perform the operation. {@code appId} identifies the target + * application for {@link Operation#GET_APPLICATION_META}; it is null for other operations. This + * policy does not cover one-way messages, which have no RPC call context. Review Comment: This exemption leaves the one-way `PbWorkerLost` path outside the new authorization boundary: `Master.receive` handles the wire `WORKER_LOST` message without an `RpcCallContext`, while `Operation.WORKER_LOST` is checked only in `receiveAndReply`. A remotely authenticated connection can therefore send a one-way worker-loss message and mutate master state without its service policy being consulted. Route this operation through an authorization-bearing RPC or otherwise restrict the one-way path to trusted local messages instead of making the bypass part of the public contract. ########## master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala: ########## @@ -621,6 +627,7 @@ private[celeborn] class Master( pb.getRequestId)) case pb: PbWorkerLost => + checkServiceAuth(context, Operation.WORKER_LOST) Review Comment: This adds the service check only to the request/reply branch, but `Master.receive` still accepts `PbWorkerLost` as a one-way message and calls `handleWorkerLost` without any `ServiceAuthorizer` check. A remote authenticated client can therefore send the same worker-loss operation via `RpcEndpointRef.send` and bypass the new service-role policy. Remove the remote one-way handling or route it through an authenticated context (while preserving the internal timeout path). -- 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]
