HttpUrlConnector.secureConnection not always setting custom SSL socket factory

[jerseyrobot]

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]

[jerseyrobot]

@glassfishrobot Commented
Reported by benpiper

[jerseyrobot]

@glassfishrobot Commented
nkoterba said:
Issue is still present on Version 2.23.1.

I spent 2+ days trying to understand why simultaneous JERSEY client SSL REST requests to our server were failing. As mentioned, out of N requests, N-1 would ALWAYS fail, 1 request would always succeed. If I added a delay between REST requests of around 100-200 ms, all requests would succeed indicating to me a threading/deadlock/resource competition in the JERSEY client logic.

On the recommendation of a colleague, I switched from using the default JERSEY REST client connector (Based on HttpUrlConnector) to the Apache REST client connector (maven: org.apache.httpcomponents:httpclient:4.5.2), and immediately all N requests were successfully sent and received.

This seems like a MASSIVE bug and it sounds like the OP has isolated the exact location and cause of this issue.

I am commenting here versus opening up a new ticket because I'm hoping this will get addressed very soon in future Jersey versions. Until then, we will be using the Apache Connector.

[jerseyrobot]

@glassfishrobot Commented
nkoterba said:
I think this issue is entirely related: https://java.net/jira/browse/JERSEY-3124

[jerseyrobot]

@glassfishrobot Commented
nkoterba said:
I switched to using the ApacheConnector and found since that doesn't use HttpUrlConnector, multi-threaded requests once again started working.

However, I did encounter two additional issues that I'm posting here for others:
(if using SSL): https://java.net/jira/browse/JERSEY-3051
(if using JERSEY Server and it sends Response objects back): https://java.net/jira/browse/JERSEY-3149

[jerseyrobot]

@glassfishrobot Commented
This issue was imported from java.net JIRA JERSEY-3021

[jerseyrobot]

• Issue Imported From: https://github.com/jersey/jersey/issues/3293
• Original Issue Raised By:@glassfishrobot
• Original Issue Assigned To: @mpotociar

[johannesherr]

The issue is still present in version 2.27.

Just spent a day finding it. If the original reporter is correct in his assumption, that the cost of the assignment is insignificant, the check should be removed.

[bmarwell]

+1 I need a reliable socket binding.

[mduft]

This issue took me DAYS of debugging to find out... :| There must be an easy way to fix this... (other than manually, statically calling getDefaultSSLSocketFactory on application startup...).