From b2c7ba6d388cb9722f39073d7e82aa818fec49d5 Mon Sep 17 00:00:00 2001 From: jansupol <15908245+jansupol@users.noreply.github.com> Date: Mon, 2 Dec 2024 08:19:50 +0100 Subject: [PATCH] Fix possible concurrent issue with SSL & default connector (#5794) * Fix possible concurrent issue with SSL & default connector Signed-off-by: jansupol --------- Signed-off-by: jansupol --- .../client/HttpUrlConnectorProvider.java | 17 +-- .../client/internal/HttpUrlConnector.java | 6 +- tests/e2e-tls/pom.xml | 18 +++ .../ConcurrentHttpsUrlConnectionTest.java | 119 ++++++++++++++++++ .../tests/e2e/tls/connector/HttpsServer.java | 87 +++++++++++++ .../tests/e2e/tls/connector/keystore.jks | Bin 0 -> 2748 bytes 6 files changed, 231 insertions(+), 16 deletions(-) create mode 100644 tests/e2e-tls/src/test/java/org/glassfish/jersey/tests/e2e/tls/connector/ConcurrentHttpsUrlConnectionTest.java create mode 100644 tests/e2e-tls/src/test/java/org/glassfish/jersey/tests/e2e/tls/connector/HttpsServer.java create mode 100644 tests/e2e-tls/src/test/resources/org/glassfish/jersey/tests/e2e/tls/connector/keystore.jks diff --git a/core-client/src/main/java/org/glassfish/jersey/client/HttpUrlConnectorProvider.java b/core-client/src/main/java/org/glassfish/jersey/client/HttpUrlConnectorProvider.java index 66925cf551..1036105e7c 100644 --- a/core-client/src/main/java/org/glassfish/jersey/client/HttpUrlConnectorProvider.java +++ b/core-client/src/main/java/org/glassfish/jersey/client/HttpUrlConnectorProvider.java @@ -21,9 +21,6 @@ import java.net.Proxy; import java.net.URL; import java.util.Map; -import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.locks.Lock; -import java.util.concurrent.locks.ReentrantLock; import java.util.logging.Logger; import javax.ws.rs.client.Client; @@ -290,16 +287,12 @@ public interface ConnectionFactory { * @throws java.io.IOException in case the connection cannot be provided. */ default HttpURLConnection getConnection(URL url, Proxy proxy) throws IOException { - synchronized (this){ - return (proxy == null) ? getConnection(url) : (HttpURLConnection) url.openConnection(proxy); - } + return (proxy == null) ? getConnection(url) : (HttpURLConnection) url.openConnection(proxy); } } private static class DefaultConnectionFactory implements ConnectionFactory { - private final ConcurrentHashMap locks = new ConcurrentHashMap<>(); - @Override public HttpURLConnection getConnection(final URL url) throws IOException { return connect(url, null); @@ -311,13 +304,7 @@ public HttpURLConnection getConnection(URL url, Proxy proxy) throws IOException } private HttpURLConnection connect(URL url, Proxy proxy) throws IOException { - Lock lock = locks.computeIfAbsent(url, u -> new ReentrantLock()); - lock.lock(); - try { - return (proxy == null) ? (HttpURLConnection) url.openConnection() : (HttpURLConnection) url.openConnection(proxy); - } finally { - lock.unlock(); - } + return (proxy == null) ? (HttpURLConnection) url.openConnection() : (HttpURLConnection) url.openConnection(proxy); } } 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 350ad2b2a7..3433e2c8e0 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 @@ -84,7 +84,7 @@ public class HttpUrlConnector implements Connector { 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 Value DEFAULT_SSL_SOCKET_FACTORY = + private static final LazyValue DEFAULT_SSL_SOCKET_FACTORY = Values.lazy((Value) () -> HttpsURLConnection.getDefaultSSLSocketFactory()); // The list of restricted headers is extracted from sun.net.www.protocol.http.HttpURLConnection private static final String[] restrictedHeaders = { @@ -387,6 +387,10 @@ private ClientResponse _apply(final ClientRequest request) throws IOException { sniUri = request.getUri(); } + if (!DEFAULT_SSL_SOCKET_FACTORY.isInitialized() && "HTTPS".equalsIgnoreCase(sniUri.getScheme())) { + DEFAULT_SSL_SOCKET_FACTORY.get(); + } + proxy.ifPresent(clientProxy -> ClientProxy.setBasicAuthorizationHeader(request.getHeaders(), proxy.get())); uc = this.connectionFactory.getConnection(sniUri.toURL(), proxy.isPresent() ? proxy.get().proxy() : null); uc.setDoInput(true); diff --git a/tests/e2e-tls/pom.xml b/tests/e2e-tls/pom.xml index 3797bda9cf..737baa7134 100644 --- a/tests/e2e-tls/pom.xml +++ b/tests/e2e-tls/pom.xml @@ -164,6 +164,24 @@ + + JDK_17- + + [1.8,17) + + + + + org.apache.maven.plugins + maven-surefire-plugin + + + **/ConcurrentHttpsUrlConnectionTest* + + + + + diff --git a/tests/e2e-tls/src/test/java/org/glassfish/jersey/tests/e2e/tls/connector/ConcurrentHttpsUrlConnectionTest.java b/tests/e2e-tls/src/test/java/org/glassfish/jersey/tests/e2e/tls/connector/ConcurrentHttpsUrlConnectionTest.java new file mode 100644 index 0000000000..ecf2a6ee1b --- /dev/null +++ b/tests/e2e-tls/src/test/java/org/glassfish/jersey/tests/e2e/tls/connector/ConcurrentHttpsUrlConnectionTest.java @@ -0,0 +1,119 @@ +/* + * Copyright (c) 2024 Oracle and/or its affiliates. All rights reserved. + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Public License v. 2.0, which is available at + * http://www.eclipse.org/legal/epl-2.0. + * + * This Source Code may also be made available under the following Secondary + * Licenses when the conditions for such availability set forth in the + * Eclipse Public License v. 2.0 are satisfied: GNU General Public License, + * version 2 with the GNU Classpath Exception, which is available at + * https://www.gnu.org/software/classpath/license.html. + * + * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0 + */ + +package org.glassfish.jersey.tests.e2e.tls.connector; + +import org.junit.jupiter.api.Test; + +import javax.ws.rs.client.ClientBuilder; +import javax.ws.rs.client.Client; +import javax.ws.rs.core.GenericType; +import javax.ws.rs.core.MediaType; + +import java.io.InputStream; +import java.net.URL; +import java.security.KeyStore; +import java.util.Random; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.atomic.AtomicInteger; + +import javax.net.ssl.HostnameVerifier; +import javax.net.ssl.KeyManagerFactory; +import javax.net.ssl.SSLContext; +import javax.net.ssl.SSLSession; +import javax.net.ssl.TrustManagerFactory; + +/** + * Jersey client seems to be not thread-safe: + * When the first GET request is in progress, + * all parallel requests from other Jersey client instances fail + * with SSLHandshakeException: PKIX path building failed. + *

+ * Once the first GET request is completed, + * all subsequent requests work without error. + *

+ * BUG 5749 + */ +public class ConcurrentHttpsUrlConnectionTest { + private static int THREAD_NUMBER = 5; + + private static volatile int responseCounter = 0; + + private static SSLContext createContext() throws Exception { + URL url = ConcurrentHttpsUrlConnectionTest.class.getResource("keystore.jks"); + KeyStore keyStore = KeyStore.getInstance("JKS"); + try (InputStream is = url.openStream()) { + keyStore.load(is, "password".toCharArray()); + } + KeyManagerFactory kmf = KeyManagerFactory.getInstance("SunX509"); + kmf.init(keyStore, "password".toCharArray()); + TrustManagerFactory tmf = TrustManagerFactory.getInstance("PKIX"); + tmf.init(keyStore); + SSLContext context = SSLContext.getInstance("TLS"); + context.init(kmf.getKeyManagers(), tmf.getTrustManagers(), null); + return context; + } + + @Test + public void testSSLConnections() throws Exception { + if (THREAD_NUMBER == 1) { + System.out.println("\nThis is the working case (THREAD_NUMBER==1). Set THREAD_NUMBER > 1 to reproduce the error! \n"); + } + + final HttpsServer server = new HttpsServer(createContext()); + Executors.newFixedThreadPool(1).submit(server); + + // set THREAD_NUMBER > 1 to reproduce an issue + ExecutorService executorService2clients = Executors.newFixedThreadPool(THREAD_NUMBER); + + final ClientBuilder builder = ClientBuilder.newBuilder().sslContext(createContext()) + .hostnameVerifier(new HostnameVerifier() { + public boolean verify(String arg0, SSLSession arg1) { + return true; + } + }); + + AtomicInteger counter = new AtomicInteger(0); + + for (int i = 0; i < THREAD_NUMBER; i++) { + executorService2clients.submit(new Runnable() { + @Override + public void run() { + try { + Client client = builder.build(); + String ret = client.target("https://127.0.0.1:" + server.getPort() + "/" + new Random().nextInt()) + .request(MediaType.TEXT_HTML) + .get(new GenericType() { + }); + System.out.print(++responseCounter + ". Server returned: " + ret); + } catch (Exception e) { + //get an exception here, if jersey lib is buggy and THREAD_NUMBER > 1: + //jakarta.ws.rs.ProcessingException: javax.net.ssl.SSLHandshakeException: PKIX path building failed: + e.printStackTrace(); + } finally { + System.out.println(counter.incrementAndGet()); + } + } + }); + } + + while (counter.get() != THREAD_NUMBER) { + Thread.sleep(100L); + } + server.stop(); + } +} diff --git a/tests/e2e-tls/src/test/java/org/glassfish/jersey/tests/e2e/tls/connector/HttpsServer.java b/tests/e2e-tls/src/test/java/org/glassfish/jersey/tests/e2e/tls/connector/HttpsServer.java new file mode 100644 index 0000000000..31214f1174 --- /dev/null +++ b/tests/e2e-tls/src/test/java/org/glassfish/jersey/tests/e2e/tls/connector/HttpsServer.java @@ -0,0 +1,87 @@ +/* + * Copyright (c) 2024 Oracle and/or its affiliates. All rights reserved. + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Public License v. 2.0, which is available at + * http://www.eclipse.org/legal/epl-2.0. + * + * This Source Code may also be made available under the following Secondary + * Licenses when the conditions for such availability set forth in the + * Eclipse Public License v. 2.0 are satisfied: GNU General Public License, + * version 2 with the GNU Classpath Exception, which is available at + * https://www.gnu.org/software/classpath/license.html. + * + * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0 + */ + +package org.glassfish.jersey.tests.e2e.tls.connector; + +import java.io.BufferedInputStream; +import java.io.IOException; +import java.io.InputStream; +import java.io.PrintWriter; + +import javax.net.ssl.SSLContext; +import javax.net.ssl.SSLServerSocket; +import javax.net.ssl.SSLSocket; + +class HttpsServer implements Runnable { + private final SSLServerSocket sslServerSocket; + private boolean closed = false; + + public HttpsServer(SSLContext context) throws Exception { + sslServerSocket = (SSLServerSocket) context.getServerSocketFactory().createServerSocket(0); + } + + public int getPort() { + return sslServerSocket.getLocalPort(); + } + + @Override + public void run() { + System.out.printf("Server started on port %d%n", getPort()); + while (!closed) { + SSLSocket s; + try { + s = (SSLSocket) sslServerSocket.accept(); + } catch (IOException e2) { + s = null; + } + final SSLSocket socket = s; + new Thread(new Runnable() { + public void run() { + try { + if (socket != null) { + InputStream is = new BufferedInputStream(socket.getInputStream()); + byte[] data = new byte[2048]; + int len = is.read(data); + if (len <= 0) { + throw new IOException("no data received"); + } + //System.out.printf("Server received: %s\n", new String(data, 0, len)); + PrintWriter writer = new PrintWriter(socket.getOutputStream()); + writer.println("HTTP/1.1 200 OK"); + writer.println("Content-Type: text/html"); + writer.println(); + writer.println("Hello from server!"); + writer.flush(); + writer.close(); + socket.close(); + } + } catch (Exception e1) { + e1.printStackTrace(); + } + } + }).start(); + } + } + + void stop() { + try { + closed = true; + sslServerSocket.close(); + } catch (IOException e) { + throw new RuntimeException(e); + } + } +} diff --git a/tests/e2e-tls/src/test/resources/org/glassfish/jersey/tests/e2e/tls/connector/keystore.jks b/tests/e2e-tls/src/test/resources/org/glassfish/jersey/tests/e2e/tls/connector/keystore.jks new file mode 100644 index 0000000000000000000000000000000000000000..130aa714826af1f436af885e065b50a1788138e2 GIT binary patch literal 2748 zcma)8c{mh`7M~e2wuWmf%(d?fnX#vsF=dHRl%*_B7pP6lW6mg$u|A zB$GI;!6Z)0AFvsM1gZTm3Q`ItL5hCBc|R(P^~8T%P);C-OoE_)faegmemWpr2z!Li zuN8`bg9T=>Q=ZMI{a*z4Wn0ZdXOaV{xiTP-JP!ycfq=5I|NA0{0|G#ZvU1#vGXc7@ zfPqS2p2CAzpFFK=9FMO^BH&&Fl3)^exOlT}>@s@Do2mj~ewqJr78{ix{r7xBnDg3)hNukmNa#?nm~ z7e1(Kjc~DFVlg2k3z7Gib{3xu-gsscYF#5MkL8w#7s-t|AtztY3yye};@H5Sdyh+A z;iLUb+2$HT=4=;O?=EhfZSk`_(bGHgh`PUZkSuvtpys_4A<8Enun``aiw~UK0k?R* zCY5<^idmdQg=X+K?_al?a}*vBeb+JNX4k@l3>=#0I39i#Oj#inY(=;CI!ugHbF7U- z#3&4@$4jX5>S)GzZ1 zf7}Nmp857}-ARJ6bq7Pe-kVvbgElc=aL_HOSWy)^}=ERZR5WTn^ffDy|r@tYlvfyGF-Ro)kNcKlKKGUf$T5*;8~m$ywq9gy-UGegQSs%v&WMlEVhL}RHan8t zXnJSKpGO}?~EKOGTbmG*(~cCV*h`_GDZ z(eYz{5LTZzPc?NIOJ2+wksg~|pvxpfXp6@)?0(D<# zb#ZTJ);rGF!X!n~X^-D1z(q=G2cTP@lYw;=!@S#tv2xNoq?M4{`p{T!YI9-eZo@z> z7rr}Z4a2b9R zbZ`@a_pt5-OF@>1deylZgXPGAOs$EL#EV@{2X!}y3E%eCWLf>63?Y*s8B3I4`_+9^ zkxdzKMWh>kHIePcqYc+DUd7I1sR$=fd385c%_rNhpH7Wq&flxrvr?(9rm0)pjA9!{ z=pQT}J2yH-&6Yv2?q5D>j#Q1&#)%1gMLF&TO_E1{Q<@$nDAp`;mNlEQpo{2-1#W>; zVfwN;ANN&wrHqx(8WsD!XNry5Tb7Y=S8B}0nu=!^29wREs+J`WMXN))9q20J0<=>k z6HBEBlv2~ux`v=PUQewuW^#*f+bfss$3fVKJ^l42e03UgieJt@`P#j<|J;do5v?X0 z9@niqQ1(pEY>=(8cqZtsHg#|`&Zbr6ouz;>gT5@*AR~yzQ169U>2%<;0<1Nn4s<$w zIZ$4;40gFe`Wry6Abkphie8=^dhGl(HI<8eNob7XLZ0A>YU!DFyBKLdT1VDsA*HUo zRw&Z3h@ds9*wMBn2tJNQ$Y!ILvlXliLAdxW+yWCB%9>|FyRNvrw14T^+v38H3!j#c z8KD-+-UTQLX}OjW`5f2kYbNdHlh*3ayM|2?b1^j2;+b`^Z2b=0XDU%+6gm}>R4rzU zPdQco=+oykwZG5@-1-EMu;3at)#RIv8xKh}1&3<$M{JdiM;I*Kusm5f)1UT+pm}I1 zXO#^o%udfMy_^(7=~!~evUk*x)NaZw~NK%>aWqxN@cW3wy7^W4>!gYWwuh^vM(8V3#9s@X-S zy=UVg6HR-R)%c@{w%56g`ZT!4a9`U8vzn6cP4t7X8-HAh;s|g^m}QpMdL8h1romI& z{8N&&cUSv&y_JNu57jqpaC0}kC~m|tPQrKFnv?=S`ZJuSzL908K%vvk23?-DqVG-E z&WQNY;Yt2M10*N&*1YMOVE^^5uY4?)?UU98BAsm0$i@AX)3C=`%ElMD&*R$)uNB(I z-T!o|CbpKV?F(#;m^Jd(yWa=yHluZ06F9L}@0IZ? z$^$j6Bt4E(xCo{%sEC!D%-Ee{OY1wA67JeBR%otzlv8s8(EsA=i;M`C(dgGbfLV_h zLuUJLi(m)Na!ch@uJjDc@p1Do+4IA5hswzCUgE-P{C*_D18(6sP;rO1HwRfUiiVzcr>-HIfe}7C#i;B+}ZR0wAK3P!)v> z%Fn-t#X221#oU_or%eauYT*=fv*IsbD3TA|%|2XQ6Tj*sWc}J>TmRA7{4gV{OkP}x zX$cPxr{CddPlb(Xk?WkBaK`g(LiaddH^NtL9VT;n``%VIqdigZjOn6SRu!zb(jhOr z#AF4RtyrzVcG&Mh;Sa_H;NI{t%T|a5ymLc>%U4TY;n{`EvUff*Og+X6-<1oWLu~BH zK*ir{Zt-ETekpMob5Iv=S9=oyO*9eesmM1%*Vzf@jPH%j=RSDhIvvC6**6RSH8Y&1+3Lfve)~_Cr2J0!GVdEOu zM@nI^gb=eHeln`bO)=9Qrm5lz$M~cOHJet(SJErZj7uTNib% zfySM+w)8RakKJb_yDqC^-afPoaD6nB3e|eA>e*YfkzmiIsI4EUurftQmyiEV87w33 zItEFZ76dT<#7}GHd6`UHqN$^;c19<=6yOmi9>)*&6xSGGG&}3Iei(&&9q%lv+6AXi zpkc$Vo3=Z!5tPMU`-PYRbmC|%<$ULVIwQ_Q+ik?cuiWWfrzuRZ9n#cS zpWV2OFh?K}Tt7cL5D)?YOSm)$EPUed;T}6_^okdm;(WycDIq?s#`4Xkzh|Tc3Iq#e bwnZ9qSY3#Ld?i<+V&;)Ma&Kb(iJ*T0av%r= literal 0 HcmV?d00001