From 7dc7d89abf37df9f13e9e789db2e61615669ab06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adrian=20Haasler=20Garc=C3=ADa?= Date: Tue, 15 Sep 2020 18:15:50 +0200 Subject: [PATCH 1/2] Avoid unsafe lazy-initialization for SSL sockets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This prevents calling HttpsURLConnection.getDefaultSSLSocketFactory() in an unsafe manner due to the poorly implemented lazy-initialization on the JDK. When multiple threads call that method concurrently (calling secureConnection()) the SSLSocketFactory is instantiated two times, making one thread fail the check and overriding the custom socket factory with the default one. Signed-off-by: Adrian Haasler García --- .../glassfish/jersey/client/internal/HttpUrlConnector.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/core-client/src/main/java/org/glassfish/jersey/client/internal/HttpUrlConnector.java b/core-client/src/main/java/org/glassfish/jersey/client/internal/HttpUrlConnector.java index a4255a7f7c0..51aa7d72c42 100644 --- a/core-client/src/main/java/org/glassfish/jersey/client/internal/HttpUrlConnector.java +++ b/core-client/src/main/java/org/glassfish/jersey/client/internal/HttpUrlConnector.java @@ -71,6 +71,9 @@ public class HttpUrlConnector implements Connector { private static final Logger LOGGER = Logger.getLogger(HttpUrlConnector.class.getName()); private static final String ALLOW_RESTRICTED_HEADERS_SYSTEM_PROPERTY = "sun.net.http.allowRestrictedHeaders"; + // Avoid multi-thread uses of HttpsURLConnection.getDefaultSSLSocketFactory() because it does not implement a + // proper lazy-initialization. See https://github.com/jersey/jersey/issues/3293 + private static final SSLSocketFactory DEFAULT_SSL_SOCKET_FACTORY = HttpsURLConnection.getDefaultSSLSocketFactory(); // The list of restricted headers is extracted from sun.net.www.protocol.http.HttpURLConnection private static final String[] restrictedHeaders = { "Access-Control-Request-Headers", @@ -299,7 +302,7 @@ protected void secureConnection(final JerseyClient client, final HttpURLConnecti suc.setHostnameVerifier(verifier); } - if (HttpsURLConnection.getDefaultSSLSocketFactory() == suc.getSSLSocketFactory()) { + if (DEFAULT_SSL_SOCKET_FACTORY == suc.getSSLSocketFactory()) { // indicates that the custom socket factory was not set suc.setSSLSocketFactory(sslSocketFactory.get()); } From 6a8e80ec625f2ce9a7585dc45e645fab61aa504c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adrian=20Haasler=20Garc=C3=ADa?= Date: Tue, 15 Sep 2020 18:32:24 +0200 Subject: [PATCH 2/2] Add test for concurrent custom SSLSocketFactory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Test made by Kevin Conaway @kevinconaway Also-by: Kevin Conaway Signed-off-by: Adrian Haasler García --- .../ssl/SslHttpUrlConnectorTest.java | 63 +++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/tests/e2e-client/src/test/java/org/glassfish/jersey/tests/e2e/client/connector/ssl/SslHttpUrlConnectorTest.java b/tests/e2e-client/src/test/java/org/glassfish/jersey/tests/e2e/client/connector/ssl/SslHttpUrlConnectorTest.java index 7a823214a91..d7857bbc38d 100644 --- a/tests/e2e-client/src/test/java/org/glassfish/jersey/tests/e2e/client/connector/ssl/SslHttpUrlConnectorTest.java +++ b/tests/e2e-client/src/test/java/org/glassfish/jersey/tests/e2e/client/connector/ssl/SslHttpUrlConnectorTest.java @@ -17,10 +17,18 @@ package org.glassfish.jersey.tests.e2e.client.connector.ssl; import java.io.IOException; +import java.io.PrintWriter; +import java.io.StringWriter; import java.net.HttpURLConnection; import java.net.InetAddress; import java.net.Socket; import java.net.URL; +import java.util.List; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.CyclicBarrier; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; import javax.ws.rs.client.Client; import javax.ws.rs.client.ClientBuilder; @@ -79,6 +87,61 @@ public HttpURLConnection getConnection(final URL url) throws IOException { assertTrue(socketFactory.isVisited()); } + /** + * Test for https://github.com/jersey/jersey/issues/3293 + * + * @author Kevin Conaway + */ + @Test + public void testConcurrentRequestsWithCustomSSLContext() throws Exception { + final SSLContext sslContext = getSslContext(); + + final Client client = ClientBuilder.newBuilder() + .sslContext(sslContext) + .register(HttpAuthenticationFeature.basic("user", "password")) + .register(LoggingFeature.class) + .build(); + + int numThreads = 5; + CyclicBarrier barrier = new CyclicBarrier(numThreads); + ExecutorService service = Executors.newFixedThreadPool(numThreads); + List exceptions = new CopyOnWriteArrayList<>(); + + for (int i = 0; i < numThreads; i++) { + service.submit(() -> { + try { + barrier.await(1, TimeUnit.MINUTES); + for (int call = 0; call < 10; call++) { + final Response response = client.target(Server.BASE_URI).path("/").request().get(); + assertEquals(200, response.getStatus()); + } + } catch (Exception ex) { + exceptions.add(ex); + } + }); + } + + service.shutdown(); + + assertTrue( + service.awaitTermination(1, TimeUnit.MINUTES) + ); + + assertTrue( + toString(exceptions), + exceptions.isEmpty() + ); + } + + private String toString(List exceptions) { + StringWriter writer = new StringWriter(); + PrintWriter printWriter = new PrintWriter(writer); + + exceptions.forEach(e -> e.printStackTrace(printWriter)); + + return writer.toString(); + } + public static class CustomSSLSocketFactory extends SSLSocketFactory { private boolean visited = false;