Bug 1918473

Created attachment 1749156 [details]
ssl-tests-ojdk11-gnutls-user-canceled.txt

I have tried to add this call which is suggested as workaround in Filezilla bug [3]:
socket.shutdownOutput();

I added it to server code here [4] (before end of try block is reached), then it passes.

Interesting however, that closing outputstream at the very same point does not fix the issue.
( Nor does closing both inputstream and outputstream (in any order))

[3] https://trac.filezilla-project.org/ticket/12099#comment:10
[4] https://github.com/zzambers/ssl-tests/blob/574608989efb8bad93e61cb1140698bf934ad841/ssl-tests/src/SSLSocketServer.java#L135

Javadoc does not give very clear instructions:

"API Note" in SSLSocket javadoc says [5]:
"When the connection is no longer needed, the client and server applications should each close both sides of their respective connection."

Than it continues, that it can be done in one of following ways:
- calling Socket.shutdownOutput()  (Socket.shutdownInput())
  works in my case
- OutputStream.close() (OutputStream.close())
  does not work in my case

But javadoc also says, that calling Socket.close() should have done this implicitly [6],
so closing streams explicitly should not be needed, as I understand it.

[5] https://docs.oracle.com/en/java/javase/11/docs/api/java.base/javax/net/ssl/SSLSocket.html
[6] https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/net/Socket.html#close()

Java sending user_cancelled alert after handshake completed is a bug: https://tools.ietf.org/html/rfc8446#section-6.1 this alert is applicable only during a handshake.

I think the confusion between shutdowns is because TCP allows for a half-close, TLS before 1.3 does not, TLS 1.3 introduced that behaviour.
So for TLS 1.3, the implementation should call OutputStream.close(), and still attempt reads from the other side. Same with receiving EOF on InputStream, it still needs to explicitly call OutputStream.close() (possibly only after finishing writing queued data); if the API is consistent with protocol support (see section 6.1).

I have studied OpenJDK code a bit and also have done some reading of TLS RFCs, and as I understand it problem is, that with TLSv1.3 is that connection can be half-closed unlike in prior versions, where other side was required to immediately close the connection after receiving close_notify.

New behaviour of TLSv1.3 reportedly shown problematic for some java programs, expecting old behaviour. This was addressed by [1][2] and some code changes were made. This includes that duplexCloseOutput method [3], which sends user cancelled alert in case of TLSv1.3 as workaround (seems like trying to push the other side to immediately close the connection, in case it was not intending to do so. Also notice this change, which seem to be reacting by immediate close after close notify in case user cancelled was received prior to that [4]). Other than that "API Note" was added to javadoc recommending client and server applications to "close both sides of their respective connection". As stated in the comment section of that openjdk bug [1], these methods then performed half-close:
  InputStream.close()
  OutputStream.close()
  Socket.shutdownInput()
  Socket.shutdownOutput()
while this method performed duplex-close (this does user alert workaround, in case output is still open)
  Socket.close()

Behaviour however changed again by this bug [4] which made InputStream and OutputStream closure equivalent to Socket.close() (duplex-close). But "API note" was not updated and is therefore currently incorrect. This also explains, why closing OutputStream did not solve the issue for me, but Socket.shutdownOutput() did.

[1] https://bugs.openjdk.java.net/browse/JDK-8208526
[2] http://hg.openjdk.java.net/jdk/jdk11/rev/d0e2e34eec65
[3] http://hg.openjdk.java.net/jdk/jdk11/file/1ddf9a99e4ad/src/java.base/share/classes/sun/security/ssl/SSLSocketImpl.java#l501
[4] http://hg.openjdk.java.net/jdk/jdk11/diff/d0e2e34eec65/src/java.base/share/classes/sun/security/ssl/Alert.java
[5] https://bugs.openjdk.java.net/browse/JDK-8216326

One other observation is that user_canceled workaround is used even in case when close_notify was received and connection is therfore half-closed (other side has already closed it's output), which is my scenario.

So conservative (safe) approach to fix this bug could be not using user_canceled workaround, when it is not necessary (connection is already half-closed by the other side), as it can actually be harmful as demonstrated. This should be safe and fixes my scenario with gnutls-cli, where server does not explicitly call Socket.shutdownOutput() prior to Socket.close(), but it has already reached the end of input. But there would still likely be a scenario with error on server side when java tls-1.3 client duplex-closes the connection (without first calling Socket.shutdownOutput()) and where tls server does not tolerate user_canceled workaround.

Less conservative approach  (but probably correct one, when it comes to TLS 1.3 RFC) would be to disable/remove user_canceled workaround altogether, but this could reintroduce some issues addressed by JDK-8208526 (See [1] higher).

I have also made patch for latest jdk (upstream) implementing conservative approach. I have built patched jdk and tested it locally and it indeed fixes bug for my scenario.

Created attachment 1750999 [details]
tls-1-3-user-canceled-conservative.patch

Created attachment 1751000 [details]
sslsocket-javadoc-fix.patch

Javadoc fix (API Note) for SSLSocket class to reflect changes by JDK-8216326.

This seems to be related to:
https://bugs.openjdk.java.net/browse/JDK-8253368

which is in latest jdk11u-dev/jdk8u-dev trees. Are you able to reproduce with those trees?

I reproduced this even on latest jdk built locally, so my build included JDK-82553368. However I think JDK-82553368 is little different issue, as I do not see relation to problem of user_cancelled alert being sent by duplexCloseOutput method for TLSv1.3.

(In reply to zzambers from comment #11)
> I reproduced this even on latest jdk built locally, so my build included
> JDK-82553368. However I think JDK-82553368 is little different issue, as I
> do not see relation to problem of user_cancelled alert being sent by
> duplexCloseOutput method for TLSv1.3.

OK. Sorry for the noise then.

Btw: As it turned out workaround with explicit call to Socket.shutdownOutput() is also not that great,
as older JDK versions throw this exception (seen on java-1.8.0-openjdk-1.8.0.265.b01-4.el8.x86_64):

java.lang.UnsupportedOperationException: The method shutdownOutput() is not supported in SSLSocket
	at sun.security.ssl.BaseSSLSocketImpl.shutdownOutput(BaseSSLSocketImpl.java:228)
	at SSLSocketServer.serverLoop(SSLSocketServer.java:138)
	at SSLSocketServer$1.run(SSLSocketServer.java:75)
	at java.lang.Thread.run(Thread.java:748)

I guess same would happen on any version JDK7.

So current situation would force server developers to use really ugly workarounds
to be both portable and tls1.3 (+ gnutls) compatible:

try (Socket s = ...) {
    ...
    try {
        s.shutdownOutput();
    } catch (UnsupportedOperationException ex) {
        // ignored
    }
}

zzambers, is this issue still present? Is your patch at a stage we should look at taking it upstream?

I can still reproduce it with current rpms:
java-1.8.0-openjdk-1.8.0.292.b10-1.el8_4.x86_64
java-11-openjdk-11.0.11.0.9-2.el8_4.x86_64
java-latest-openjdk-16.0.1.0.9-1.rolling.el8.x86_64
+
gnutls-3.6.14-8.el8_3.x86_64

(I can later try on upstream)

tls-1-3-user-canceled-conservative.patch:
I don't plan any changes to this one (as it is already very simple). I believe it should be safe. But I have done only simple testing using ssl-tests.

sslsocket-javadoc-fix.patch:
This one is only about javadoc, as earlier it was possible to do half-close connection (TLSv1.3), by calling InputStream/OutputStream close method, but this behaviour was changed by JDK-8216326. However api note in javadoc was not updated.

I have proposed fix for this to upstream:
https://github.com/openjdk/jdk/pull/7664

After evaluating this issue, there are no plans to address it further or fix it in an upcoming release.  Therefore, it is being closed.  If plans change such that this issue will be fixed in an upcoming release, then the bug can be reopened.