HttpUrlConnector.secureConnection not always setting custom SSL socket factory #3293
Description
To be more precise, due to changes introduced by #3171 (jersey/jersey@57f5daf#diff-2af8c9e0f0f9963b8b1ce356cf17ecb7), a custom SSL socket factory is not set on an HttpsURLConnection 'suc' in HttpUrlConnector unless HttpsURLConnection.getDefaultSSLSocketFactory returns the exact same object as suc.getSSLSocketFactory(). This is not a robust way of determining if the custom socket factory has already been set because HttpsURLConnection.getDefaultSSLSocketFactory() can be called simultaneously by multiple threads, returning a different object for each invocation, thus resulting in up to n-1 threads getting a mismatch in HttpUrlConnector.secureConnection, even though suc.getSSLSocketFactory() is actually returning a default SSL socket factory (just not necessarily the exact instance we expected, which is ultimately irrelevant).
This is fairly easy to reproduce, given an HTTPS server with a self-signed certificate. The following code is not a self-contained unit test, but will demonstrate the issue nevertheless, by spitting out at least one SSLHandshakeException (caused by the custom SSLContext not being used). If numThreads is set to 1, then it will never fail due to a certificate validation error. The code will also run without error on Jersey 2.21 or earlier.
import javax.net.ssl.*;
import javax.ws.rs.client.Client;
import javax.ws.rs.client.ClientBuilder;
import java.security.SecureRandom;
import java.security.cert.CertificateException;
import java.security.cert.X509Certificate;
public class HttpsTest {
private static SSLContext sslContext = null;
private static synchronized SSLContext getSslContext() {
if (sslContext == null) {
try {
sslContext = SSLContext.getInstance("TLS");
sslContext.init(null, new TrustManager[]{new X509TrustManager() {
public void checkClientTrusted(X509Certificate[] arg0, String arg1) throws CertificateException {
}
public void checkServerTrusted(X509Certificate[] arg0, String arg1) throws CertificateException {
}
public X509Certificate[] getAcceptedIssuers() {
return new X509Certificate[0];
}
}}, new SecureRandom());
} catch (Exception e) {
throw new RuntimeException(e);
}
}
return sslContext;
}
private Client client;
public HttpsTest() {
// Create a Client instance that won't complain about self-signed SSL certs
client = ClientBuilder.newBuilder().sslContext(getSslContext()).hostnameVerifier(new LenientHostnameVerifier())
.build();
}
private void runTest() {
try {
client.target("https://lists.gno.org/self-signed.html").request().get();
} catch (Exception e) {
e.printStackTrace();
}
}
public static void main(String... args) throws InterruptedException {
final int numThreads = 4;
for (int i = 1; i <= numThreads; i++) {
new Thread(() -> new HttpsTest().runTest()).start();
}
Thread.sleep(10000L);
}
/**
* A HostnameVerifier that always returns true
*/
private class LenientHostnameVerifier implements HostnameVerifier {
@Override
public boolean verify(String s, SSLSession sslSession) {
return true;
}
}
}
I would suggest that the change to HttpUrlConnector.secureConnection could be reverted, or at least if the purpose of it was to avoid redundant calls to setSSLSocketFactory (the cost of which is arguably insignificant), that comparing suc.getSSLSocketFactory to sslSocketFactory.get() might be more appropriate than comparing it to HttpsUrlConnection.getDefaultSSLSocketFactory().
Environment
Ubuntu 14.04, JDK 1.8.0_45
Affected Versions
[2.22]