gnodet-bot commented on code in PR #27803:
URL: https://github.com/apache/camel/pull/27803#discussion_r4268942402
##########
components/camel-smpp/src/main/java/org/apache/camel/component/smpp/SmppConnectionFactory.java:
##########
@@ -102,21 +112,59 @@ public Connection createConnection(String host, int port)
throws IOException {
socket.connect(new InetSocketAddress(host, port),
config.getEnquireLinkTimer());
}
- if (config.isUsingSSL() && config.getHttpProxyHost() != null) {
- // Init the SSL socket which is based on the proxy socket
- SSLSocketFactory sslSocketFactory = (SSLSocketFactory)
SSLSocketFactory.getDefault();
- // NOTE: the socket must be closed by the factory method
client.
- SSLSocket sslSocket = (SSLSocket)
sslSocketFactory.createSocket(socket, host, port, true); // NOSONAR
- sslSocket.startHandshake();
- socket = sslSocket;
+ if (config.isUsingSSL()) {
+ if (config.getHttpProxyHost() != null) {
+ // Init the SSL socket which is based on the proxy socket
+ // NOTE: the socket must be closed by the factory method
client.
+ socket = getSslSocketFactory().createSocket(socket, host,
port, true); // NOSONAR
+ configureHostnameVerification(socket);
+ }
+ startHandshake((SSLSocket) socket);
}
return new SocketConnection(socket);
} catch (Exception e) {
+ // no Connection is returned, so jSMPP cannot close the socket
either. The JDK closes it on a fatal
+ // alert but not on a timed out connect or handshake, which leaks
one socket per reconnect attempt
+ // against an SMSC that accepts TCP and then does not answer. On
the proxy path the layered SSLSocket
+ // was created with autoClose=true, so closing it closes the
tunnel underneath as well
+ IOHelper.close(socket);
throw new IOException(e.getMessage(), e);
}
}
+ private SSLSocketFactory getSslSocketFactory() {
+ return sslSocketFactory != null ? sslSocketFactory :
(SSLSocketFactory) SSLSocketFactory.getDefault();
+ }
+
+ /**
+ * Completes the TLS handshake here, on the thread that is creating the
connection, instead of leaving it to the
+ * first read or write.
+ * <p/>
+ * {@code SMPPSession.connectAndBind} starts its {@code PDUReaderWorker}
before it writes the bind, so both threads
+ * reach the first I/O on this socket and only whichever of them drives
{@code SSLSocketImpl.ensureNegotiated()}
+ * sees the {@link javax.net.ssl.SSLHandshakeException}. The other gets
+ * {@code SocketException: Connection or outbound has closed}, which never
mentions the certificate - so a host name
+ * mismatch could surface as a bare closed-connection error on route
start. Handshaking eagerly makes the cause
+ * deterministic.
+ * <p/>
Review Comment:
💡 **Nit:** Same `<p/>` → `<p>` here.
```suggestion
* <p>
```
##########
components/camel-smpp/src/test/java/org/apache/camel/component/smpp/SmppConnectionFactoryHostnameVerificationTest.java:
##########
@@ -0,0 +1,330 @@
+/*
+ * 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.camel.component.smpp;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
+import java.net.InetAddress;
+import java.net.InetSocketAddress;
+import java.net.ServerSocket;
+import java.net.Socket;
+import java.net.SocketTimeoutException;
+import java.nio.charset.StandardCharsets;
+import java.security.KeyStore;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+
+import javax.net.ssl.KeyManagerFactory;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLHandshakeException;
+import javax.net.ssl.SSLSocket;
+import javax.net.ssl.SSLSocketFactory;
+import javax.net.ssl.TrustManagerFactory;
+
+import org.jsmpp.session.connection.Connection;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertInstanceOf;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * The connection factory checks that the certificate of the SMSC matches the
host it connects to, directly and through
+ * an HTTP proxy.
+ * <p>
+ * The certificate in jsse/localhost.p12 lists only DNS:localhost as subject
alternative name, so it matches "localhost"
+ * but not "127.0.0.1".
+ */
+class SmppConnectionFactoryHostnameVerificationTest {
+
+ private static final char[] PASSWORD = "changeit".toCharArray();
+ // the proxy test server ignores the target of the CONNECT request and
answers the TLS handshake itself
+ private static final int SMSC_PORT = 2775;
+
+ private final ExecutorService executor = Executors.newCachedThreadPool();
+ private final CountDownLatch stop = new CountDownLatch(1);
+ private SSLContext serverContext;
+ private SSLContext clientContext;
+ private ServerSocket server;
+
+ @BeforeEach
+ void createSslContexts() throws Exception {
+ KeyStore keyStore = KeyStore.getInstance("PKCS12");
+ try (InputStream in =
getClass().getClassLoader().getResourceAsStream("jsse/localhost.p12")) {
+ keyStore.load(in, PASSWORD);
+ }
+ KeyManagerFactory keyManagerFactory =
KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm());
+ keyManagerFactory.init(keyStore, PASSWORD);
+ serverContext = SSLContext.getInstance("TLS");
+ serverContext.init(keyManagerFactory.getKeyManagers(), null, null);
+
+ TrustManagerFactory trustManagerFactory
+ =
TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm());
+ trustManagerFactory.init(keyStore);
+ clientContext = SSLContext.getInstance("TLS");
+ clientContext.init(null, trustManagerFactory.getTrustManagers(), null);
+ }
+
+ @AfterEach
+ void stopServer() throws IOException {
+ stop.countDown();
+ if (server != null) {
+ server.close();
+ }
+ executor.shutdownNow();
+ }
+
+ @Test
+ void connectsWhenTheCertificateMatchesTheHost() throws Exception {
+ startTlsServer();
+
+ Connection connection = factory(true,
false).createConnection("localhost", server.getLocalPort());
+ try {
+ write(connection);
+ } finally {
+ connection.close();
+ }
+ }
+
+ @Test
+ void failsWhenTheCertificateDoesNotMatchTheHost() throws Exception {
+ startTlsServer();
+
+ // the handshake runs in createConnection, so the mismatch is reported
there rather than on the first
+ // read or write - which is what SMPPSession.connectAndBind rethrows
when the bind fails
+ SmppConnectionFactory factory = factory(true, false);
+ IOException e = assertThrows(IOException.class, () ->
factory.createConnection("127.0.0.1", server.getLocalPort()));
+
+ SSLHandshakeException cause =
assertInstanceOf(SSLHandshakeException.class, e.getCause());
+ assertTrue(cause.getMessage().contains("127.0.0.1"),
cause.getMessage());
+ }
+
+ /**
+ * createConnection returns no Connection when it fails, so jSMPP cannot
close the socket either.
+ * <p/>
Review Comment:
💡 **Nit:** `<p/>` → `<p>` in Javadoc HTML.
```suggestion
* <p>
```
##########
components/camel-smpp/src/main/java/org/apache/camel/component/smpp/SmppConnectionFactory.java:
##########
@@ -102,21 +112,59 @@ public Connection createConnection(String host, int port)
throws IOException {
socket.connect(new InetSocketAddress(host, port),
config.getEnquireLinkTimer());
}
- if (config.isUsingSSL() && config.getHttpProxyHost() != null) {
- // Init the SSL socket which is based on the proxy socket
- SSLSocketFactory sslSocketFactory = (SSLSocketFactory)
SSLSocketFactory.getDefault();
- // NOTE: the socket must be closed by the factory method
client.
- SSLSocket sslSocket = (SSLSocket)
sslSocketFactory.createSocket(socket, host, port, true); // NOSONAR
- sslSocket.startHandshake();
- socket = sslSocket;
+ if (config.isUsingSSL()) {
+ if (config.getHttpProxyHost() != null) {
+ // Init the SSL socket which is based on the proxy socket
+ // NOTE: the socket must be closed by the factory method
client.
+ socket = getSslSocketFactory().createSocket(socket, host,
port, true); // NOSONAR
+ configureHostnameVerification(socket);
+ }
+ startHandshake((SSLSocket) socket);
}
return new SocketConnection(socket);
} catch (Exception e) {
+ // no Connection is returned, so jSMPP cannot close the socket
either. The JDK closes it on a fatal
+ // alert but not on a timed out connect or handshake, which leaks
one socket per reconnect attempt
+ // against an SMSC that accepts TCP and then does not answer. On
the proxy path the layered SSLSocket
+ // was created with autoClose=true, so closing it closes the
tunnel underneath as well
+ IOHelper.close(socket);
throw new IOException(e.getMessage(), e);
}
}
+ private SSLSocketFactory getSslSocketFactory() {
+ return sslSocketFactory != null ? sslSocketFactory :
(SSLSocketFactory) SSLSocketFactory.getDefault();
+ }
+
+ /**
+ * Completes the TLS handshake here, on the thread that is creating the
connection, instead of leaving it to the
+ * first read or write.
+ * <p/>
Review Comment:
💡 **Nit:** `<p/>` is self-closing XHTML. In Javadoc HTML, the correct form
is `<p>` (paragraph separator, not closed).
```suggestion
* <p>
```
--
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]
