Copilot commented on code in PR #18299: URL: https://github.com/apache/iotdb/pull/18299#discussion_r3841341195
########## iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/service/TrustedChannelAuditServerEventHandler.java: ########## @@ -0,0 +1,155 @@ +/* + * 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.iotdb.commons.service; + +import org.apache.iotdb.common.rpc.thrift.TEndPoint; +import org.apache.iotdb.commons.audit.TrustedChannelFailureHandler; +import org.apache.iotdb.rpc.TElasticFramedTransport; + +import org.apache.thrift.protocol.TProtocol; +import org.apache.thrift.server.ServerContext; +import org.apache.thrift.server.TServerEventHandler; +import org.apache.thrift.transport.TSocket; +import org.apache.thrift.transport.TTransport; + +import javax.net.ssl.SSLException; +import javax.net.ssl.SSLSocket; + +import java.io.IOException; +import java.io.UncheckedIOException; +import java.net.InetSocketAddress; +import java.net.Socket; +import java.net.SocketAddress; +import java.util.Objects; + +/** + * Performs the server-side TLS handshake before the first Thrift request is processed and reports + * handshake failures together with the raw socket peer and the local service endpoint. + */ +public class TrustedChannelAuditServerEventHandler implements TServerEventHandler { + + private final TServerEventHandler delegate; + private final TEndPoint target; + private final TrustedChannelFailureHandler failureHandler; + + public TrustedChannelAuditServerEventHandler( + TServerEventHandler delegate, TEndPoint target, TrustedChannelFailureHandler failureHandler) { + this.delegate = Objects.requireNonNull(delegate); + this.target = Objects.requireNonNull(target); + this.failureHandler = Objects.requireNonNull(failureHandler); + } + + @Override + public void preServe() { + delegate.preServe(); + } + + @Override + public ServerContext createContext(TProtocol input, TProtocol output) { + startHandshakeIfNecessary(output); + try { + return delegate.createContext(input, output); + } catch (RuntimeException | Error contextFailure) { + // A delegate may have already allocated connection state before createContext fails. The + // Thrift server subsequently invokes this wrapper with a null context, which is deliberately + // ignored by deleteContext below, so clean up the partially created delegate context here. + try { + delegate.deleteContext(null, input, output); + } catch (RuntimeException | Error cleanupFailure) { + if (cleanupFailure != contextFailure) { + contextFailure.addSuppressed(cleanupFailure); + } + } + throw contextFailure; + } + } + + @Override + public void deleteContext(ServerContext serverContext, TProtocol input, TProtocol output) { + if (serverContext != null) { + delegate.deleteContext(serverContext, input, output); + } + } + + @Override + public void processContext( + ServerContext serverContext, TTransport inputTransport, TTransport outputTransport) { + delegate.processContext(serverContext, inputTransport, outputTransport); + } + + private void startHandshakeIfNecessary(TProtocol output) { + Socket socket = getSocket(output); + if (!(socket instanceof SSLSocket)) { + return; + } + + try { + ((SSLSocket) socket).startHandshake(); + } catch (IOException e) { + if (e instanceof SSLException) { + notifyFailure(e, socket.getRemoteSocketAddress(), socket.getLocalSocketAddress()); + } Review Comment: This direct `instanceof` check prevents wrapped SSL failures from reaching the callback. For example, an `IOException` whose cause is `SSLHandshakeException` is rejected here even though `recordTrustedChannelFailureAuditLogIfNecessary` deliberately recognizes SSL failures through the cause chain. Report every failure from `SSLSocket.startHandshake()` to the callback and let the callback apply its cause-chain filter. ########## iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/audit/TrustedChannelFailureHandler.java: ########## @@ -0,0 +1,33 @@ +/* + * 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.iotdb.commons.audit; + +import org.apache.iotdb.common.rpc.thrift.TEndPoint; + +@FunctionalInterface +public interface TrustedChannelFailureHandler { + + TrustedChannelFailureHandler NO_OP = + (failure, initiator, target) -> { + // Do nothing. + }; + + void onFailure(Throwable failure, TEndPoint initiator, TEndPoint target); Review Comment: The callback is currently invoked only by the two inbound handshake listeners; no outbound client or transport path calls it. Consequently, the ConfigNode-to-DataNode, DataNode-to-ConfigNode, DataNode-to-DataNode, and DataNode-to-AINode reporting coverage stated in the PR description is not implemented. Thread this hook through those client factories/transports and invoke it with the local and remote endpoints when connection setup fails. -- 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]
