davsclaus commented on code in PR #27770: URL: https://github.com/apache/camel/pull/27770#discussion_r4257877584
########## components/camel-mina/src/test/java/org/apache/camel/component/mina/MinaSslContextParametersHostnameVerificationTest.java: ########## @@ -0,0 +1,84 @@ +/* + * 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.mina; + +import org.apache.camel.CamelExecutionException; +import org.apache.camel.builder.RouteBuilder; +import org.apache.camel.component.mock.MockEndpoint; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; + +/** + * The producer checks that the certificate of the server matches the host it connects to. + * <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 MinaSslContextParametersHostnameVerificationTest extends BaseMinaTest { + + @Test + void sendsWhenTheCertificateMatchesTheHost() { + Object reply = template.requestBody(producerUri("localhost", ""), "Hello"); + + assertEquals("Bye Hello", reply); + } + + @Test + void failsWhenTheCertificateDoesNotMatchTheHost() throws Exception { + MockEndpoint result = getMockEndpoint("mock:result"); + result.expectedMessageCount(0); + + // MINA keeps the handshake error in its SslHandler, so the producer fails when the write does not complete: + // short timeouts keep the test fast + assertThrows(CamelExecutionException.class, Review Comment: This passes for any failure: a refused connection, a timeout for another reason, a codec error. Unlike the MLLP test, it does not show that the certificate was rejected because of the host. Could it assert the cause, at least that it is a write/read timeout and not a connect failure? Better still, assert on the `SSLHandshakeException` if it can be reached (for example through the session's `SslHandler`, or by checking that `mock:result` got nothing *and* the exception is the write timeout). `MinaClientSslFilterTest` only checks the engine parameters, so this is the only end-to-end check of the mismatch. ########## docs/user-manual/modules/ROOT/pages/camel-4x-upgrade-guide-4_24.adoc: ########## @@ -234,6 +246,17 @@ In both cases the objects are no longer kept in the `inProgressRepository`; befo the same key) was skipped by all later polls until the endpoint was restarted. With the `fileName` option, a failed get still fails the poll. +=== camel-mina - the host name of the server is verified over SSL + +When `sslContextParameters` is set (on the endpoint, or with `useGlobalSslContextParameters` on the component), the +producer, and the consumer with `clientMode=true`, now check that the certificate of the server matches the `host` of +the endpoint, with the same rules as an HTTPS client. Before, any certificate trusted by the trust store was accepted, +whatever host the certificate 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 Review Comment: As the test comment says, with mina a host name mismatch does not show up as an SSL error: MINA keeps the handshake failure in its `SslHandler`, and the producer fails with a write/read timeout after `writeTimeout`/`timeout` (30 s by default). Users who upgrade will see unexplained timeouts. Could this entry (and the mina doc) say that a mismatch shows as a timeout, and to check the certificate's subject alternative names or the debug log? ########## docs/user-manual/modules/ROOT/pages/camel-4x-upgrade-guide-4_24.adoc: ########## @@ -202,6 +202,18 @@ poll returns only the tweets newer than the last one consumed, and a page has `c default of 15 applied before). Raise `count` and `numberOfPages` if a search can match more tweets than that between two polls. A consumer with `type=direct` returned nothing when keywords were given, and now runs the search at startup. +=== camel-mllp - the producer verifies the host name of the server over TLS + +When `sslContextParameters` is set (on the endpoint, or with `useGlobalSslContextParameters` on the component), the +MLLP producer now checks that the certificate of the server matches the host it connects to, with the same rules as an +HTTPS client. Before, it accepted any certificate trusted by the trust store, whatever host the certificate was issued +for. + +A producer that connects with a host name or an IP address that the certificate does not list now fails the TLS Review Comment: Nit: an MLLP producer URI without a host name (`mllp://8888`) connects through `new InetSocketAddress(port)` in `MllpTcpClientProducer.configureSocketAddress`, so the peer host is the wildcard address and the check now always fails. That form is rare for a producer, but it would help to say here (and in the doc) that a TLS producer needs a host name that the certificate lists. -- 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]
