Skip to content
This repository has been archived by the owner on May 28, 2018. It is now read-only.

#3293 Pre-load default ssl socket factory in HttpUrlConnector #3738

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kevinconaway
Copy link

@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.

…y() to avoid a race condition in HttpUrlConnector#secureConnection
@kevinconaway
Copy link
Author

@pavelbucek are you the correct person to review this?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant