On Wed, 6 Jan 2021 23:56:35 GMT, Xue-Lei Andrew Fan <[email protected]> wrote:
>> This PR aims to revert some more cases where SocketExceptions are improperly >> being wrapped as SSLException. Some work for this was done in >> [JDK-8235263](https://bugs.openjdk.java.net/browse/JDK-8235263), but that >> change did not cover all the cases. >> >> As it was mentioned in JDK-8235263, some applications rely on receiving >> SocketException to decide if the connection should be retried. An example of >> this would be Apache HTTP client. This PR should ideally fix >> https://issues.apache.org/jira/browse/HTTPCLIENT-2032 > > Thank you for take care of this issue. Looks good to me. While looking into the TrustTrustedCert.java test, we found out this change may cause the test to fails intermittently. This is because it's is not expecting a SocketException in Line 134. The root cause for this intermittent failure seems to be that when the server processes the the ClientHello message, it iterates through a list of producers to create the response message. These response messages are written to the socket by each producer, and the client can start processing these messages before all of them are sent. If one of the earlier messages causes the client to reject the TLS handshake (in this example, a bad certificate), the client will send a TLS fatal alert and close the socket. In most scenarios, by the time the TLS fatal alert is received by the server, all the pending TLS handshake messages would´ve been written into the socket, but if the client response is fast enough, this TLS alert can reach the server before it has finished sending all the messages. The server will not wait to check if it has received a fatal alert from the client and it will attempt to send the next message anyway. Because in this scenario the socket has already been closed by the client, a SocketException (Broken Pipe) will be generated, instead of the more appropriate SSLHandshakeException. Before this patch, those SocketExceptions were at least being wrapped as SSLException, but now, the SocketException will go all the way up. Most of the time, this might not be a problem, but there are scenarios that come to mind where this could become relevant. The simpler one to explain is MTLS, where a very similar scenario can happen but this time in the client side if the server sends the fatal while the client is still writing (for example, rejecting the client certificate before this has sent the Client Key Exchange). Again, instead of an SSLHanshakeException, we have SSLException before this patch and SocketException after. In this case, seeing a SocketException the client may decide to retry the connection (while for an SSLException or SSLHandshakeException, most likely it wouldn't). How bad this is, and how bad the potential for an extra retry is compared to the current situation when retries are not happening for transient errors, we'll leave for the discussion. Additionally, although the step between Certificate and Server/Client Key Exchange is the most likely to trigger this, every time multiple producers are being called there is a chance for this issue to arise. The good news is the issue requires round-trip communication between server and client to be faster than processing all the producers in a batch. The aforementioned test triggers this behavior quite often as both sides of the TLS connection are on the same machine. ------------- PR: https://git.openjdk.java.net/jdk/pull/1968
