davsclaus commented on code in PR #27803: URL: https://github.com/apache/camel/pull/27803#discussion_r4268423174
########## components/camel-smpp/src/test/java/org/apache/camel/component/smpp/SmppConnectionFactoryHostnameVerificationTest.java: ########## @@ -0,0 +1,222 @@ +/* + * 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.InetSocketAddress; +import java.net.ServerSocket; +import java.net.Socket; +import java.nio.charset.StandardCharsets; +import java.security.KeyStore; +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.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.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 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 { + 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(); + + Connection connection = factory(true, false).createConnection("127.0.0.1", server.getLocalPort()); + try { + // the handshake runs on the first write Review Comment: With the handshake moved into `createConnection` (see the comment on `SmppConnectionFactory`), this test can assert on `createConnection`, like `failsThroughTheProxyWhenTheCertificateDoesNotMatchTheHost`. That is also exactly what `SMPPSession.connectAndBind` rethrows, so it covers the producer and consumer start as well: ```java 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()); ``` ########## components/camel-smpp/src/main/java/org/apache/camel/component/smpp/SmppConnectionFactory.java: ########## @@ -86,11 +96,11 @@ public Connection createConnection(String host, int port) throws IOException { Socket socket; SocketFactory socketFactory; socketFactory = config.isUsingSSL() && config.getHttpProxyHost() == null - ? SSLSocketFactory - .getDefault() + ? getSslSocketFactory() : SocketFactory.getDefault(); // NOTE: the socket must be closed by the factory method client. socket = socketFactory.createSocket(); // NOSONAR + configureHostnameVerification(socket); Review Comment: On the direct path the handshake is not started here. It runs later in `SMPPSession.connectAndBind`, where `PDUReaderWorker`'s read and the bind write race in `ensureNegotiated()`. The thread that loses gets `SocketException: Connection or outbound has closed` instead of the `SSLHandshakeException`, so the route can fail to start with a message that does not mention the certificate. Starting the handshake right after the direct `connect(...)` makes it fail the same way as the proxy path: ```java } else { // jsmpp uses enquire link timer as socket read timeout, so also use it to establish the initial connection socket.connect(new InetSocketAddress(host, port), config.getEnquireLinkTimer()); if (socket instanceof SSLSocket sslSocket) { // handshake now, so a certificate problem fails here with the SSLHandshakeException as cause sslSocket.setSoTimeout(config.getEnquireLinkTimer()); sslSocket.startHandshake(); } } ``` jSMPP calls `conn.setSoTimeout(getEnquireLinkTimer())` right after `createConnection`, so setting it before the handshake does not change anything later. Without it, a server that accepts but never answers would block the handshake with no timeout. The proxy path's `startHandshake()` has the same gap today, so it could get the same `setSoTimeout`. ########## docs/user-manual/modules/ROOT/pages/camel-4x-upgrade-guide-4_24.adoc: ########## @@ -272,6 +272,16 @@ failed (for example a 404 for a deleted account, or a server error). Such a comm for it. Its `CamelGitHubCommitAuthor` and `CamelGitHubCommitCommitter` headers then have the name that git recorded in the commit instead of the name of the GitHub profile, and are not set when the commit has no name either. +=== camel-smpp - the host name of the SMSC is verified over SSL + +With `smpps` (or `usingSSL=true`), directly or through an HTTP proxy, the connection now checks that the certificate of +the SMSC matches the host it connects to, with the same rules as an HTTPS client. Before, any certificate trusted by the +JVM was accepted, whatever host it was issued for. + +A connection to a host name or an IP address that the certificate does not list now fails the SSL handshake; an IP +address must be listed as a subject alternative name of the certificate. Use a certificate that matches the host of the +endpoint, or set the new `hostnameVerification` option to `false` to keep the previous behaviour. Review Comment: Once the direct path starts the handshake in the connection factory, it would help to say how a mismatch shows, so users can find it in their logs. For example: "The producer or consumer then fails to start with an `IOException` caused by an `SSLHandshakeException` such as `No subject alternative names matching IP address ... found`." The same sentence would fit the component page. -- 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]
