oscerd commented on code in PR #27803:
URL: https://github.com/apache/camel/pull/27803#discussion_r4268605121
##########
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:
You're right, and this was a defect in my own commit rather than a
preference — the feature does not do its job without it. Fixed in `283ef11`.
I confirmed the race from the bytecode before changing anything, because the
whole value of the fix rests on it. In jsmpp 3.0.2 `SMPPSession.connectAndBind`:
```
180: PDUReaderWorker.start() <- the reader thread starts
185: BindParameter.getBindType() <- the bind is written after
```
So both threads reach the first I/O and only whichever drives
`SSLSocketImpl.ensureNegotiated()` sees the `SSLHandshakeException`; the other
gets `Connection or outbound has closed`. A host name mismatch could surface on
route start as a bare closed-connection error with no mention of the
certificate.
Both paths now go through one `startHandshake` helper, so the proxy branch
only layers the socket:
```java
if (config.isUsingSSL()) {
if (config.getHttpProxyHost() != null) {
socket = getSslSocketFactory().createSocket(socket, host, port,
true);
configureHostnameVerification(socket);
}
startHandshake((SSLSocket) socket);
}
```
and the helper sets the read timeout first, which closes **your second point
too** — the proxy path was calling `startHandshake()` with no timeout, so a
server that accepts and never answers blocked indefinitely. Good catch; that
one was independent of the direct path.
_Claude Code on behalf of oscerd_
##########
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:
Done in `283ef11`. `failsWhenTheCertificateDoesNotMatchTheHost` now mirrors
the proxy test:
```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());
```
which is the shape `connectAndBind` rethrows.
Revert-checked: removing the eager handshake from the direct path fails
exactly this test with `Expected java.io.IOException to be thrown, but nothing
was thrown`, and leaves the other five passing. Module suite 153 tests, 4
skipped, green.
_Claude Code on behalf of oscerd_
##########
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:
Added to both, in `283ef11`.
Upgrade guide:
> A mismatch shows as the producer or consumer failing to start, with an
`IOException` whose cause names the host that was checked:
> ```
> java.io.IOException: No subject alternative names matching IP address
127.0.0.1 found
> Caused by: javax.net.ssl.SSLHandshakeException: No subject alternative
names matching IP address 127.0.0.1 found
> ```
The component page says the same and adds the part that is easy to get wrong
in practice: an IP address has to be listed as a subject alternative name,
because a certificate issued for the host name alone does not match a
connection made to its address — and it gives the `No name matching ... found`
wording for the host-name case.
The catalog copy of the component page is a verbatim mirror, so it is synced
in the same commit; I checked the two were byte-identical beforehand so nothing
drifts in the regen check.
_Claude Code on behalf of oscerd_
--
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]
