Jersey update from 3.1.3 to 3.1.4 slows down our external service res…

[jbescos]

#5746 to 2.x branch

[dtbaum]

Hello @jansupol, @jbescos, @senivam,
First of all, thank you for your effort. However, from my perspective, this PR seems to replace a performance issue by the security issue.
The configured SSL settings and configured keystore/truststore of second REST call are ignored as far 2 criteria are met:

1. The REST URL is different from the URL of previous request
2. The second REST call starts during the first request is still in progress

Please, take a look & run the reproducer code under: https://github.com/dtbaum/jersey-bug-report.

[fwippe]

Thanks for addressing the performance issue, @jbescos!

However, allow me to elaborate on a few concerns I have with your approach:

1. You effectively created a memory leak. If a client is using many different URLs (for instance, if they, for some reason, include query parameters in the URNs), the ConcurrentHashMap will grow accordingly, even if the old URLs might never be reused again.
2. Why synchronize at all? Can't we just go forward with calling HttpsURLConnection.getDefaultSSLSocketFactory() eagerly somewhere, so it's guaranteed to be initialized at getConnection() time? As per my understanding, that would resolve the race condition @dtbaum observed, right?

[jansupol]

@fwippe
There is a long discussion regarding the getDefaultSSLSocketFactory(). While some want to have it initialized eagerly, the others do not, for various purposes, such as testing, proxying or mocking. See #4815 which references multiple related issues.

I welcome any good idea, the current state is not optimal.

[jansupol]

@dtbaum

second REST call are ignored

Why ignored?

[jansupol]

The synchronization explanation reference.

[dtbaum]

@dtbaum

second REST call are ignored

Why ignored?

@jansupol
My full sentence above was:
The configured SSL settings and configured keystore/truststore of second REST call are ignored.
Admittedly, not elegant English. :-)
My old comment contains a better explanation, see #5359 (comment)
Occasionally ignoring of configured SSL settings and using system defaults instead, can lead to exploitable vulnerability in certain scenarios.
It would be great to find a clean solution that solves the performance issue pointed out by @jbescos, without creating a security issue.

[jbescos]

I knew there is no cleanup in the map, as long as the factory is alive. I was not concerned about it, because there should be anyway a pool of opened connections. Regarding the query parameters:

Anyways, it can be easily solved in this way:

        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 {
                locks.remove(url);
                lock.unlock();
            }
        }

That prevents that 1+ connections are opened at the same time and it makes a cleanup.

I will use host:port as a key instead of URL, because 1+ URLs to the same host:port could happen.

[dtbaum]

I will use host:port as a key instead of URL, because 1+ URLs to the same host:port could happen.

That would only shift the issue to the next level: If the first REST request targets host1 and the second parallel request targets host2, the race condition will reoccur.
In the case of a race condition, the second REST call loses critical SSL settings such as mutual authentication, customized key/trust stores, and other configurations - in best case this leads to SSLHandshakeException.
The worst case would be a candidate for a CVE..

The workaround replaces a performance issue with a more serious problem: In my opinion, we shouldn't improve this workaround, but rather eliminate it and find a clean solution.

For many applications, security and the absence of race conditions are more important than a fixed performance issue..

[jbescos]

I will use host:port as a key instead of URL, because 1+ URLs to the same host:port could happen.

That would only shift the issue to the next level: If the first REST request targets host1 and the second parallel request targets host2, the race condition will reoccur. In the case of a race condition, the second REST call loses critical SSL settings such as mutual authentication, customized key/trust stores, and other configurations - in best case this leads to SSLHandshakeException. The worst case would be a candidate for a CVE..

The workaround replaces a performance issue with a more serious problem: In my opinion, we shouldn't improve this workaround, but rather eliminate it and find a clean solution.

For many applications, security and the absence of race conditions are more important than a fixed performance issue..

Does it make an issue with different host in parallel?. If this is the case, then the fix I did will not help. I think in your example I saw that all the requests were happening in the same host:port.

It is really strange that issue happens with different host:port, because those are different connections and different SSL handshakes.

Were you able to reproduce the concurrent issue with my fix?.

[dtbaum]

Does it make an issue with different host in parallel?

Yes. For simplicity, I reproduced it on the same host with different ports in parallel, but that should be enough to confirm my point.

Were you able to reproduce the concurrent issue with my fix?

Yes

[jansupol]

@dtbaum I utilized your test case. It should be enough to create the instance of the DEFAULT_SSL_SOCKET_FACTORY before url.openConnection. That should not violate #4757. I put together a PR with your test #5794.

[dtbaum]

Thank you @jansupol!
I have two small points regarding #5794 - commented in the code.