Copilot commented on code in PR #18299: URL: https://github.com/apache/iotdb/pull/18299#discussion_r3643975029
########## iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/service/TrustedChannelAuditServerEventHandler.java: ########## @@ -0,0 +1,135 @@ +/* + * 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); + return delegate.createContext(input, output); + } + + @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()); + } + try { + socket.close(); + } catch (IOException closeFailure) { + e.addSuppressed(closeFailure); + } + throw new UncheckedIOException(e); + } + } + + private void notifyFailure(Throwable failure, SocketAddress remoteAddress) { + TEndPoint initiator = toEndPoint(remoteAddress); + if (initiator == null) { + return; + } + try { + failureHandler.onFailure(failure, initiator, target); + } catch (RuntimeException auditFailure) { + failure.addSuppressed(auditFailure); + } Review Comment: If the provided failureHandler rethrows the same Throwable instance (or otherwise returns `failure` as the RuntimeException), `failure.addSuppressed(failure)` will throw IllegalArgumentException and mask the original TLS failure. Other call sites in this PR guard against this; this handler should do the same to keep reporting failures from replacing transport failures. ########## iotdb-core/datanode/src/main/java/org/apache/iotdb/db/audit/DNAuditLogger.java: ########## @@ -58,6 +60,15 @@ public void createViewIfNecessary() {} @Override public synchronized void log(IAuditEntity auditLogFields, Supplier<String> log) {} + public void recordTrustedChannelFailureAuditLogIfNecessary(Throwable failure, TEndPoint target) { + recordTrustedChannelFailureAuditLogIfNecessary( + failure, + new TEndPoint( + IoTDBDescriptor.getInstance().getConfig().getInternalAddress(), + IoTDBDescriptor.getInstance().getConfig().getInternalPort()), + target); + } Review Comment: This overload always uses `internalAddress:internalPort` as the initiator endpoint. Several new call sites (e.g. MPP exchange, consensus RPC, data exchange) report failures against non-internal-RPC ports (like `mppDataExchangePort=10740`), so audit logs will record a mismatched initiator port and can mislead operators about which service endpoint failed. Consider adding an overload that accepts the initiator `TEndPoint` (or a `Supplier<TEndPoint>`) and updating outbound call sites to pass the actual local service endpoint for the transport. ########## external-service-impl/rest/src/main/java/org/apache/iotdb/rest/TrustedChannelAuditHandshakeListener.java: ########## @@ -0,0 +1,73 @@ +/* + * 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.rest; + +import org.apache.iotdb.common.rpc.thrift.TEndPoint; +import org.apache.iotdb.commons.audit.TrustedChannelFailureHandler; + +import org.eclipse.jetty.io.EndPoint; +import org.eclipse.jetty.io.ssl.SslHandshakeListener; + +import java.net.InetSocketAddress; +import java.net.SocketAddress; +import java.util.Objects; + +final class TrustedChannelAuditHandshakeListener implements SslHandshakeListener { + + private final TrustedChannelFailureHandler failureHandler; + + TrustedChannelAuditHandshakeListener(TrustedChannelFailureHandler failureHandler) { + this.failureHandler = Objects.requireNonNull(failureHandler); + } + + @Override + public void handshakeFailed(Event event, Throwable failure) { + recordHandshakeFailure(event.getEndPoint(), failure); + } + + void recordHandshakeFailure(EndPoint endPoint, Throwable failure) { + if (endPoint == null) { + return; + } + + TEndPoint initiator = toEndPoint(endPoint.getRemoteSocketAddress()); + TEndPoint target = toEndPoint(endPoint.getLocalSocketAddress()); + if (initiator == null || target == null) { + return; + } + + try { + failureHandler.onFailure(failure, initiator, target); + } catch (RuntimeException auditFailure) { + failure.addSuppressed(auditFailure); + } Review Comment: If the TrustedChannelFailureHandler throws the same Throwable instance it was passed (or otherwise throws `failure`), calling `failure.addSuppressed(failure)` will throw IllegalArgumentException and can mask the original handshake failure. Add the same self-suppression guard used in other reporting hooks in this PR. -- 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]
