jersey icon indicating copy to clipboard operation
jersey copied to clipboard

#3293 Pre-load default ssl socket factory in HttpUrlConnector

Open kevinconaway opened this issue 7 years ago • 1 comments

@mpotociar @petrbouda this PR is a potential fix for #3293

From my understanding of the current code, the logic for using a custom SSL socket factory is:

  1. If the HttpsUrlConnection returned by the ConnectionFactory already has a custom SSLSocketFactory set on it, do nothing
  2. Otherwise, set the socket factory on the connection to be the one provided from the SSLContext in the client config.

Currently, the first check is done via:

if (HttpsURLConnection.getDefaultSSLSocketFactory() == suc.getSSLSocketFactory()) {
    // indicates that the custom socket factory was not set
    suc.setSSLSocketFactory(sslSocketFactory.get());
}

The issue with this code is that HttpsURLConnection.getDefaultSSLSocketFactory() is not thread safe. The implementation looks like:

    public static SSLSocketFactory getDefaultSSLSocketFactory() {
        if (defaultSSLSocketFactory == null) {
            defaultSSLSocketFactory = (SSLSocketFactory)SSLSocketFactory.getDefault();
        }

        return defaultSSLSocketFactory;
    }

If there are concurrent requests, one of them may end up creating a separate SSLSocketFactory that is not the default. The comparison check above fails and the request ends up using the default SSL context instead of the provided one.

The fix in this PR is to call HttpsURLConnection.getDefaultSSLSocketFactory() in a static initializer block for the class so that the default SSLContext is present when the comparison is made in HttpUrlConnector#secureConnector

I'm open to feedback on alternate approaches to resolving this.

kevinconaway avatar Dec 19 '17 20:12 kevinconaway

@pavelbucek are you the correct person to review this?

kevinconaway avatar Jan 06 '18 00:01 kevinconaway