* Fix Tls - invalid reuse of SSLParameters (helidon-io#7622)

* Fix ServerLister - close sockets that are not going to be processed
* Fix MutualTlsTest - intermittently failing test

Author: tomas-langer

common/tls/src/main/java/io/helidon/common/tls/Tls.java

@@ -184,24 +184,6 @@ public SSLServerSocket createServerSocket() {
         }
     }
 
-    /**
-     * Create a socket for the chosen protocol.
-     *
-     * @param alpnProtocol protocol to use
-     * @return a new socket ready for TLS communication
-     */
-    public SSLSocket createSocket(String alpnProtocol) {
-        checkEnabled();
-        try {
-            SSLSocket socket = (SSLSocket) sslSocketFactory.createSocket();
-            sslParameters.setApplicationProtocols(new String[] {alpnProtocol});
-            socket.setSSLParameters(sslParameters);
-            return socket;
-        } catch (IOException e) {
-            throw new UncheckedIOException(e);
-        }
-    }
-
     /**
      * Create a SSLSocket for the chosen protocol and the given socket.
      *
@@ -215,7 +197,24 @@ public SSLSocket createSocket(List<String> alpnProtocols, Socket socket, InetSoc
         try {
             SSLSocket sslSocket = (SSLSocket) sslSocketFactory
                     .createSocket(socket, address.getHostName(), address.getPort(), true);
-            sslParameters.setApplicationProtocols(alpnProtocols.toArray(new String[0]));
+
+            // create a copy of SSLParameters, as otherwise we may overwrite alpnProtocols of requests in parallel
+            SSLParameters parameters = new SSLParameters();
+            parameters.setApplicationProtocols(alpnProtocols.toArray(new String[0]));
+            parameters.setServerNames(this.sslParameters.getServerNames());
+            parameters.setCipherSuites(this.sslParameters.getCipherSuites());
+            parameters.setAlgorithmConstraints(this.sslParameters.getAlgorithmConstraints());
+            parameters.setEnableRetransmissions(this.sslParameters.getEnableRetransmissions());
+            parameters.setEndpointIdentificationAlgorithm(this.sslParameters.getEndpointIdentificationAlgorithm());
+            parameters.setMaximumPacketSize(this.sslParameters.getMaximumPacketSize());
+            parameters.setNamedGroups(this.sslParameters.getNamedGroups());
+            parameters.setNeedClientAuth(this.sslParameters.getNeedClientAuth());
+            parameters.setProtocols(this.sslParameters.getProtocols());
+            parameters.setSignatureSchemes(this.sslParameters.getSignatureSchemes());
+            parameters.setSNIMatchers(this.sslParameters.getSNIMatchers());
+            parameters.setUseCipherSuitesOrder(this.sslParameters.getUseCipherSuitesOrder());
+            parameters.setWantClientAuth(this.sslParameters.getWantClientAuth());
+
             sslSocket.setSSLParameters(sslParameters);
             return sslSocket;
         } catch (IOException e) {
@@ -270,12 +269,6 @@ Optional<X509TrustManager> trustManager() {
         return tlsManager.trustManager();
     }
 
-    private void checkEnabled() {
-        if (sslContext == null) {
-            throw new IllegalStateException("TLS config is disabled, SSL related methods cannot be called.");
-        }
-    }
-
     private static int hashCode(SSLParameters first) {
         int result = Objects.hash(first.getAlgorithmConstraints(),
                                   first.getEnableRetransmissions(),
@@ -307,4 +300,10 @@ private static boolean equals(SSLParameters first, SSLParameters second) {
                 && first.getServerNames().equals(second.getServerNames())
                 && first.getSNIMatchers().equals(second.getSNIMatchers());
     }
+
+    private void checkEnabled() {
+        if (sslContext == null) {
+            throw new IllegalStateException("TLS config is disabled, SSL related methods cannot be called.");
+        }
+    }
 }

webclient/tests/webclient/src/test/java/io/helidon/webclient/tests/MutualTlsTest.java

@@ -15,9 +15,13 @@
  */
 package io.helidon.webclient.tests;
 
+import java.io.IOException;
 import java.io.UncheckedIOException;
+import java.net.SocketException;
 import java.util.concurrent.atomic.AtomicBoolean;
 
+import javax.net.ssl.SSLHandshakeException;
+
 import io.helidon.common.tls.Tls;
 import io.helidon.config.Config;
 import io.helidon.config.ConfigSources;
@@ -37,12 +41,13 @@
 import static org.hamcrest.CoreMatchers.is;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.fail;
 
 /**
  * This test is testing ability to set mutual TLS on WebClient and WebServer.
  */
 @ServerTest
-public class MutualTlsTest {
+class MutualTlsTest {
 
     private static final Config CONFIG = Config.just(() -> ConfigSources.classpath("application-test.yaml").build());
 
@@ -86,8 +91,22 @@ public void testAccessSuccessful() {
     public void testNoClientCert() {
         Http1Client client = createWebClient(CONFIG.get("no-client-cert"));
 
-        RuntimeException ex = assertThrows(RuntimeException.class, () -> exec(client, "https", server.port("secured")));
-        assertThat(ex.getMessage(), containsString("Received fatal alert: bad_certificate"));
+        // as the client does not have client certificate configured, it is not aware it should wait for the application
+        // data from server during SSL handshake, as a result, handshake is OK, but we fail when we try to communicate
+        // (as the client becomes aware of the problem only when it tries to read data from TLS, and by that time the
+        // server may have terminated the connection)
+        // so sometimes we get correct fatal alert about certificate, sometimes connection closed (Broken pipe or similar)
+        // it just must always fail with a socket exception or SSLHandshakeException
+        UncheckedIOException ex = assertThrows(UncheckedIOException.class, () -> exec(client, "https", server.port("secured")));
+
+        IOException cause = ex.getCause();
+        if (cause instanceof SSLHandshakeException) {
+            assertThat(cause.getMessage(), containsString("Received fatal alert: bad_certificate"));
+        } else {
+            if (!(cause instanceof SocketException)) {
+                fail("Call must fail with either SSLHandshakeException or SocketException", cause);
+            }
+        }
     }
 
     @Test

webserver/webserver/src/main/java/io/helidon/webserver/ServerListener.java

@@ -393,11 +393,28 @@ private void listen() {
                     readerExecutor.execute(handler);
                 } catch (RejectedExecutionException e) {
                     LOGGER.log(ERROR, "Executor rejected handler for new connection");
+
+                    // the socket was never handled
+                    try {
+                        socket.close();
+                    } catch (IOException ex) {
+                        LOGGER.log(TRACE, "Failed to close socket that was rejected for execution", e);
+                    }
+
                     // we never started the handler, so we must release the semaphore here
                     connectionSemaphore.release();
                 } catch (Exception e) {
                     // we may get an SSL handshake errors, which should only fail one socket, not the listener
                     LOGGER.log(TRACE, "Failed to handle accepted socket", e);
+                    // the socket was never handled
+                    try {
+                        socket.close();
+                    } catch (IOException ex) {
+                        LOGGER.log(TRACE,
+                                   "Failed to close socket that failed start execution (see previous trace for reason)",
+                                   e);
+                    }
+
                     // we never started the handler, so we must release the semaphore here
                     connectionSemaphore.release();
                 }