Re: RFR: 8282600: SSLSocketImpl should not use user_canceled workaround when not necessary
On Wed, 4 May 2022 19:22:05 GMT, Bradford Wetmore wrote: >> When testing compatibility of jdk TLS implementation with gnutls, I have >> found a problem. The problem is, that gnutls does not like use of >> user_canceled alert when closing TLS-1.3 connection from duplexCloseOutput() >> (used by socket.close() unless shutdownOutput was called explicitly) and >> considers it error. (For more details see: [1]) >> >> As I understand it, usage of user_canceled alert before close is workaround >> for an issue of not being able to cleanly initialize full (duplex) close of >> TLS-1.3 connection (other side is not required to immediately close the >> after receiving close_notify, unlike in earlier TLS versions). Some legacy >> programs could probably hang or something, expecting socket.close to perform >> immediate duplex close. Problem is this is not what user_canceled alert is >> intended for [2] and it is therefore undefined how the other side handles >> this. (JDK itself replies to close_notify preceded by user_canceled alert by >> immediately closing its output [3].) >> >> This fix disables this workaround when it is not necessary (connection is >> already half-closed by the other side). This way it fixes my case (gnutls >> client connected to jdk server initiates close) and it should be safe. (As >> removing workaround altogether could probably reintroduce issues for legacy >> apps... ) >> >> I also ran jdk_security tests locally, which passed for me. >> >> [1] https://bugzilla.redhat.com/show_bug.cgi?id=1918473 >> [2] https://datatracker.ietf.org/doc/html/rfc8446#section-6.1 >> [3] >> https://github.com/openjdk/jdk/blob/b6c35ae44aeb31deb7a15ee2939156ed00b3df52/src/java.base/share/classes/sun/security/ssl/Alert.java#L243 > > Infra + JCK passed. Looks good. @bradfordwetmore Thank you - PR: https://git.openjdk.java.net/jdk/pull/7664
Re: RFR: 8282600: SSLSocketImpl should not use user_canceled workaround when not necessary
On Wed, 2 Mar 2022 19:04:26 GMT, zzambers wrote: > When testing compatibility of jdk TLS implementation with gnutls, I have > found a problem. The problem is, that gnutls does not like use of > user_canceled alert when closing TLS-1.3 connection from duplexCloseOutput() > (used by socket.close() unless shutdownOutput was called explicitly) and > considers it error. (For more details see: [1]) > > As I understand it, usage of user_canceled alert before close is workaround > for an issue of not being able to cleanly initialize full (duplex) close of > TLS-1.3 connection (other side is not required to immediately close the after > receiving close_notify, unlike in earlier TLS versions). Some legacy programs > could probably hang or something, expecting socket.close to perform immediate > duplex close. Problem is this is not what user_canceled alert is intended for > [2] and it is therefore undefined how the other side handles this. (JDK > itself replies to close_notify preceded by user_canceled alert by immediately > closing its output [3].) > > This fix disables this workaround when it is not necessary (connection is > already half-closed by the other side). This way it fixes my case (gnutls > client connected to jdk server initiates close) and it should be safe. (As > removing workaround altogether could probably reintroduce issues for legacy > apps... ) > > I also ran jdk_security tests locally, which passed for me. > > [1] https://bugzilla.redhat.com/show_bug.cgi?id=1918473 > [2] https://datatracker.ietf.org/doc/html/rfc8446#section-6.1 > [3] > https://github.com/openjdk/jdk/blob/b6c35ae44aeb31deb7a15ee2939156ed00b3df52/src/java.base/share/classes/sun/security/ssl/Alert.java#L243 Infra + JCK passed. Looks good. - PR: https://git.openjdk.java.net/jdk/pull/7664
Re: RFR: 8282600: SSLSocketImpl should not use user_canceled workaround when not necessary
On Wed, 2 Mar 2022 19:04:26 GMT, zzambers wrote: > When testing compatibility of jdk TLS implementation with gnutls, I have > found a problem. The problem is, that gnutls does not like use of > user_canceled alert when closing TLS-1.3 connection from duplexCloseOutput() > (used by socket.close() unless shutdownOutput was called explicitly) and > considers it error. (For more details see: [1]) > > As I understand it, usage of user_canceled alert before close is workaround > for an issue of not being able to cleanly initialize full (duplex) close of > TLS-1.3 connection (other side is not required to immediately close the after > receiving close_notify, unlike in earlier TLS versions). Some legacy programs > could probably hang or something, expecting socket.close to perform immediate > duplex close. Problem is this is not what user_canceled alert is intended for > [2] and it is therefore undefined how the other side handles this. (JDK > itself replies to close_notify preceded by user_canceled alert by immediately > closing its output [3].) > > This fix disables this workaround when it is not necessary (connection is > already half-closed by the other side). This way it fixes my case (gnutls > client connected to jdk server initiates close) and it should be safe. (As > removing workaround altogether could probably reintroduce issues for legacy > apps... ) > > I also ran jdk_security tests locally, which passed for me. > > [1] https://bugzilla.redhat.com/show_bug.cgi?id=1918473 > [2] https://datatracker.ietf.org/doc/html/rfc8446#section-6.1 > [3] > https://github.com/openjdk/jdk/blob/b6c35ae44aeb31deb7a15ee2939156ed00b3df52/src/java.base/share/classes/sun/security/ssl/Alert.java#L243 tier1/tier2 tests pass. Did not try infra or JCK yet. - PR: https://git.openjdk.java.net/jdk/pull/7664
Re: RFR: 8282600: SSLSocketImpl should not use user_canceled workaround when not necessary
On Wed, 2 Mar 2022 19:04:26 GMT, zzambers wrote: > When testing compatibility of jdk TLS implementation with gnutls, I have > found a problem. The problem is, that gnutls does not like use of > user_canceled alert when closing TLS-1.3 connection from duplexCloseOutput() > (used by socket.close() unless shutdownOutput was called explicitly) and > considers it error. (For more details see: [1]) > > As I understand it, usage of user_canceled alert before close is workaround > for an issue of not being able to cleanly initialize full (duplex) close of > TLS-1.3 connection (other side is not required to immediately close the after > receiving close_notify, unlike in earlier TLS versions). Some legacy programs > could probably hang or something, expecting socket.close to perform immediate > duplex close. Problem is this is not what user_canceled alert is intended for > [2] and it is therefore undefined how the other side handles this. (JDK > itself replies to close_notify preceded by user_canceled alert by immediately > closing its output [3].) > > This fix disables this workaround when it is not necessary (connection is > already half-closed by the other side). This way it fixes my case (gnutls > client connected to jdk server initiates close) and it should be safe. (As > removing workaround altogether could probably reintroduce issues for legacy > apps... ) > > I also ran jdk_security tests locally, which passed for me. > > [1] https://bugzilla.redhat.com/show_bug.cgi?id=1918473 > [2] https://datatracker.ietf.org/doc/html/rfc8446#section-6.1 > [3] > https://github.com/openjdk/jdk/blob/b6c35ae44aeb31deb7a15ee2939156ed00b3df52/src/java.base/share/classes/sun/security/ssl/Alert.java#L243 Thanks for the extra time to review this change. I'm still wondering if there is better pattern that doesn't use user_canceled, but that doesn't need to delay this fix from going in. - Marked as reviewed by wetmore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7664
Re: RFR: 8282600: SSLSocketImpl should not use user_canceled workaround when not necessary
On Wed, 2 Mar 2022 19:04:26 GMT, zzambers wrote: > When testing compatibility of jdk TLS implementation with gnutls, I have > found a problem. The problem is, that gnutls does not like use of > user_canceled alert when closing TLS-1.3 connection from duplexCloseOutput() > (used by socket.close() unless shutdownOutput was called explicitly) and > considers it error. (For more details see: [1]) > > As I understand it, usage of user_canceled alert before close is workaround > for an issue of not being able to cleanly initialize full (duplex) close of > TLS-1.3 connection (other side is not required to immediately close the after > receiving close_notify, unlike in earlier TLS versions). Some legacy programs > could probably hang or something, expecting socket.close to perform immediate > duplex close. Problem is this is not what user_canceled alert is intended for > [2] and it is therefore undefined how the other side handles this. (JDK > itself replies to close_notify preceded by user_canceled alert by immediately > closing its output [3].) > > This fix disables this workaround when it is not necessary (connection is > already half-closed by the other side). This way it fixes my case (gnutls > client connected to jdk server initiates close) and it should be safe. (As > removing workaround altogether could probably reintroduce issues for legacy > apps... ) > > I also ran jdk_security tests locally, which passed for me. > > [1] https://bugzilla.redhat.com/show_bug.cgi?id=1918473 > [2] https://datatracker.ietf.org/doc/html/rfc8446#section-6.1 > [3] > https://github.com/openjdk/jdk/blob/b6c35ae44aeb31deb7a15ee2939156ed00b3df52/src/java.base/share/classes/sun/security/ssl/Alert.java#L243 Resettting the clock. Sorry for the delay. - PR: https://git.openjdk.java.net/jdk/pull/7664
Re: RFR: 8282600: SSLSocketImpl should not use user_canceled workaround when not necessary
On Wed, 2 Mar 2022 19:04:26 GMT, zzambers wrote: > When testing compatibility of jdk TLS implementation with gnutls, I have > found a problem. The problem is, that gnutls does not like use of > user_canceled alert when closing TLS-1.3 connection from duplexCloseOutput() > (used by socket.close() unless shutdownOutput was called explicitly) and > considers it error. (For more details see: [1]) > > As I understand it, usage of user_canceled alert before close is workaround > for an issue of not being able to cleanly initialize full (duplex) close of > TLS-1.3 connection (other side is not required to immediately close the after > receiving close_notify, unlike in earlier TLS versions). Some legacy programs > could probably hang or something, expecting socket.close to perform immediate > duplex close. Problem is this is not what user_canceled alert is intended for > [2] and it is therefore undefined how the other side handles this. (JDK > itself replies to close_notify preceded by user_canceled alert by immediately > closing its output [3].) > > This fix disables this workaround when it is not necessary (connection is > already half-closed by the other side). This way it fixes my case (gnutls > client connected to jdk server initiates close) and it should be safe. (As > removing workaround altogether could probably reintroduce issues for legacy > apps... ) > > I also ran jdk_security tests locally, which passed for me. > > [1] https://bugzilla.redhat.com/show_bug.cgi?id=1918473 > [2] https://datatracker.ietf.org/doc/html/rfc8446#section-6.1 > [3] > https://github.com/openjdk/jdk/blob/b6c35ae44aeb31deb7a15ee2939156ed00b3df52/src/java.base/share/classes/sun/security/ssl/Alert.java#L243 Thanks, been working on another close issue. I should be finishing that shortly, - PR: https://git.openjdk.java.net/jdk/pull/7664
Re: RFR: 8282600: SSLSocketImpl should not use user_canceled workaround when not necessary
On Wed, 9 Mar 2022 21:12:06 GMT, Bradford Wetmore wrote: >> When testing compatibility of jdk TLS implementation with gnutls, I have >> found a problem. The problem is, that gnutls does not like use of >> user_canceled alert when closing TLS-1.3 connection from duplexCloseOutput() >> (used by socket.close() unless shutdownOutput was called explicitly) and >> considers it error. (For more details see: [1]) >> >> As I understand it, usage of user_canceled alert before close is workaround >> for an issue of not being able to cleanly initialize full (duplex) close of >> TLS-1.3 connection (other side is not required to immediately close the >> after receiving close_notify, unlike in earlier TLS versions). Some legacy >> programs could probably hang or something, expecting socket.close to perform >> immediate duplex close. Problem is this is not what user_canceled alert is >> intended for [2] and it is therefore undefined how the other side handles >> this. (JDK itself replies to close_notify preceded by user_canceled alert by >> immediately closing its output [3].) >> >> This fix disables this workaround when it is not necessary (connection is >> already half-closed by the other side). This way it fixes my case (gnutls >> client connected to jdk server initiates close) and it should be safe. (As >> removing workaround altogether could probably reintroduce issues for legacy >> apps... ) >> >> I also ran jdk_security tests locally, which passed for me. >> >> [1] https://bugzilla.redhat.com/show_bug.cgi?id=1918473 >> [2] https://datatracker.ietf.org/doc/html/rfc8446#section-6.1 >> [3] >> https://github.com/openjdk/jdk/blob/b6c35ae44aeb31deb7a15ee2939156ed00b3df52/src/java.base/share/classes/sun/security/ssl/Alert.java#L243 > > I am looking at a few things in this PR. Please ping me before integrating. > > I believe this code is correct, but checking on the overall USER_CANCELED > strategy. @bradfordwetmore, it is ready from my side, but please take your time to do any checks, which you find necessary to make sure there is no problem here. - PR: https://git.openjdk.java.net/jdk/pull/7664
Re: RFR: 8282600: SSLSocketImpl should not use user_canceled workaround when not necessary
On Wed, 2 Mar 2022 19:04:26 GMT, zzambers wrote: > When testing compatibility of jdk TLS implementation with gnutls, I have > found a problem. The problem is, that gnutls does not like use of > user_canceled alert when closing TLS-1.3 connection from duplexCloseOutput() > (used by socket.close() unless shutdownOutput was called explicitly) and > considers it error. (For more details see: [1]) > > As I understand it, usage of user_canceled alert before close is workaround > for an issue of not being able to cleanly initialize full (duplex) close of > TLS-1.3 connection (other side is not required to immediately close the after > receiving close_notify, unlike in earlier TLS versions). Some legacy programs > could probably hang or something, expecting socket.close to perform immediate > duplex close. Problem is this is not what user_canceled alert is intended for > [2] and it is therefore undefined how the other side handles this. (JDK > itself replies to close_notify preceded by user_canceled alert by immediately > closing its output [3].) > > This fix disables this workaround when it is not necessary (connection is > already half-closed by the other side). This way it fixes my case (gnutls > client connected to jdk server initiates close) and it should be safe. (As > removing workaround altogether could probably reintroduce issues for legacy > apps... ) > > I also ran jdk_security tests locally, which passed for me. > > [1] https://bugzilla.redhat.com/show_bug.cgi?id=1918473 > [2] https://datatracker.ietf.org/doc/html/rfc8446#section-6.1 > [3] > https://github.com/openjdk/jdk/blob/b6c35ae44aeb31deb7a15ee2939156ed00b3df52/src/java.base/share/classes/sun/security/ssl/Alert.java#L243 I am looking at a few things in this PR. Please ping me before integrating. I believe this code is correct, but checking on the overall USER_CANCELED strategy. - PR: https://git.openjdk.java.net/jdk/pull/7664
Re: RFR: 8282600: SSLSocketImpl should not use user_canceled workaround when not necessary
On Thu, 3 Mar 2022 15:40:31 GMT, Xue-Lei Andrew Fan wrote: >> When testing compatibility of jdk TLS implementation with gnutls, I have >> found a problem. The problem is, that gnutls does not like use of >> user_canceled alert when closing TLS-1.3 connection from duplexCloseOutput() >> (used by socket.close() unless shutdownOutput was called explicitly) and >> considers it error. (For more details see: [1]) >> >> As I understand it, usage of user_canceled alert before close is workaround >> for an issue of not being able to cleanly initialize full (duplex) close of >> TLS-1.3 connection (other side is not required to immediately close the >> after receiving close_notify, unlike in earlier TLS versions). Some legacy >> programs could probably hang or something, expecting socket.close to perform >> immediate duplex close. Problem is this is not what user_canceled alert is >> intended for [2] and it is therefore undefined how the other side handles >> this. (JDK itself replies to close_notify preceded by user_canceled alert by >> immediately closing its output [3].) >> >> This fix disables this workaround when it is not necessary (connection is >> already half-closed by the other side). This way it fixes my case (gnutls >> client connected to jdk server initiates close) and it should be safe. (As >> removing workaround altogether could probably reintroduce issues for legacy >> apps... ) >> >> I also ran jdk_security tests locally, which passed for me. >> >> [1] https://bugzilla.redhat.com/show_bug.cgi?id=1918473 >> [2] https://datatracker.ietf.org/doc/html/rfc8446#section-6.1 >> [3] >> https://github.com/openjdk/jdk/blob/b6c35ae44aeb31deb7a15ee2939156ed00b3df52/src/java.base/share/classes/sun/security/ssl/Alert.java#L243 > > Did you want to add a test? Otherwise, please add a noreg- label in the bug > JDK-8282600. @XueleiFan thank you for review. I have added noreg-external tag and some explanation to Openjdk bug. - PR: https://git.openjdk.java.net/jdk/pull/7664
Re: RFR: 8282600: SSLSocketImpl should not use user_canceled workaround when not necessary
On Wed, 2 Mar 2022 19:04:26 GMT, zzambers wrote: > When testing compatibility of jdk TLS implementation with gnutls, I have > found a problem. The problem is, that gnutls does not like use of > user_canceled alert when closing TLS-1.3 connection from duplexCloseOutput() > (used by socket.close() unless shutdownOutput was called explicitly) and > considers it error. (For more details see: [1]) > > As I understand it, usage of user_canceled alert before close is workaround > for an issue of not being able to cleanly initialize full (duplex) close of > TLS-1.3 connection (other side is not required to immediately close the after > receiving close_notify, unlike in earlier TLS versions). Some legacy programs > could probably hang or something, expecting socket.close to perform immediate > duplex close. Problem is this is not what user_canceled alert is intended for > [2] and it is therefore undefined how the other side handles this. (JDK > itself replies to close_notify preceded by user_canceled alert by immediately > closing its output [3].) > > This fix disables this workaround when it is not necessary (connection is > already half-closed by the other side). This way it fixes my case (gnutls > client connected to jdk server initiates close) and it should be safe. (As > removing workaround altogether could probably reintroduce issues for legacy > apps... ) > > I also ran jdk_security tests locally, which passed for me. > > [1] https://bugzilla.redhat.com/show_bug.cgi?id=1918473 > [2] https://datatracker.ietf.org/doc/html/rfc8446#section-6.1 > [3] > https://github.com/openjdk/jdk/blob/b6c35ae44aeb31deb7a15ee2939156ed00b3df52/src/java.base/share/classes/sun/security/ssl/Alert.java#L243 It looks like a nice change to me. Thank you! Did you want to add a test? Otherwise, please add a noreg- label in the bug JDK-8282600. - Marked as reviewed by xuelei (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7664
Re: RFR: 8282600: SSLSocketImpl should not use user_canceled workaround when not necessary
On Thu, 3 Mar 2022 10:45:02 GMT, Severin Gehwolf wrote: >> When testing compatibility of jdk TLS implementation with gnutls, I have >> found a problem. The problem is, that gnutls does not like use of >> user_canceled alert when closing TLS-1.3 connection from duplexCloseOutput() >> (used by socket.close() unless shutdownOutput was called explicitly) and >> considers it error. (For more details see: [1]) >> >> As I understand it, usage of user_canceled alert before close is workaround >> for an issue of not being able to cleanly initialize full (duplex) close of >> TLS-1.3 connection (other side is not required to immediately close the >> after receiving close_notify, unlike in earlier TLS versions). Some legacy >> programs could probably hang or something, expecting socket.close to perform >> immediate duplex close. Problem is this is not what user_canceled alert is >> intended for [2] and it is therefore undefined how the other side handles >> this. (JDK itself replies to close_notify preceded by user_canceled alert by >> immediately closing its output [3].) >> >> This fix disables this workaround when it is not necessary (connection is >> already half-closed by the other side). This way it fixes my case (gnutls >> client connected to jdk server initiates close) and it should be safe. (As >> removing workaround altogether could probably reintroduce issues for legacy >> apps... ) >> >> I also ran jdk_security tests locally, which passed for me. >> >> [1] https://bugzilla.redhat.com/show_bug.cgi?id=1918473 >> [2] https://datatracker.ietf.org/doc/html/rfc8446#section-6.1 >> [3] >> https://github.com/openjdk/jdk/blob/b6c35ae44aeb31deb7a15ee2939156ed00b3df52/src/java.base/share/classes/sun/security/ssl/Alert.java#L243 > > @zzambers I've create https://bugs.openjdk.java.net/browse/JDK-8282600 for > this issue. Please reference it in the PR title. @jerboaa thank you - PR: https://git.openjdk.java.net/jdk/pull/7664
Re: RFR: 8282600: SSLSocketImpl should not use user_canceled workaround when not necessary
On Wed, 2 Mar 2022 19:04:26 GMT, zzambers wrote: > When testing compatibility of jdk TLS implementation with gnutls, I have > found a problem. The problem is, that gnutls does not like use of > user_canceled alert when closing TLS-1.3 connection from duplexCloseOutput() > (used by socket.close() unless shutdownOutput was called explicitly) and > considers it error. (For more details see: [1]) > > As I understand it, usage of user_canceled alert before close is workaround > for an issue of not being able to cleanly initialize full (duplex) close of > TLS-1.3 connection (other side is not required to immediately close the after > receiving close_notify, unlike in earlier TLS versions). Some legacy programs > could probably hang or something, expecting socket.close to perform immediate > duplex close. Problem is this is not what user_canceled alert is intended for > [2] and it is therefore undefined how the other side handles this. (JDK > itself replies to close_notify preceded by user_canceled alert by immediately > closing its output [3].) > > This fix disables this workaround when it is not necessary (connection is > already half-closed by the other side). This way it fixes my case (gnutls > client connected to jdk server initiates close) and it should be safe. (As > removing workaround altogether could probably reintroduce issues for legacy > apps... ) > > I also ran jdk_security tests locally, which passed for me. > > [1] https://bugzilla.redhat.com/show_bug.cgi?id=1918473 > [2] https://datatracker.ietf.org/doc/html/rfc8446#section-6.1 > [3] > https://github.com/openjdk/jdk/blob/b6c35ae44aeb31deb7a15ee2939156ed00b3df52/src/java.base/share/classes/sun/security/ssl/Alert.java#L243 @zzambers I've create https://bugs.openjdk.java.net/browse/JDK-8282600 for this issue. Please reference it in the PR title. - PR: https://git.openjdk.java.net/jdk/pull/7664
RFR: 8282600: SSLSocketImpl should not use user_canceled workaround when not necessary
When testing compatibility of jdk TLS implementation with gnutls, I have found a problem. The problem is, that gnutls does not like use of user_canceled alert when closing TLS-1.3 connection from duplexCloseOutput() (used by socket.close() unless shutdownOutput was called explicitly) and considers it error. (For more details see: [1]) As I understand it, usage of user_canceled alert before close is workaround for an issue of not being able to cleanly initialize full (duplex) close of TLS-1.3 connection (other side is not required to immediately close the after receiving close_notify, unlike in earlier TLS versions). Some legacy programs could probably hang or something, expecting socket.close to perform immediate duplex close. Problem is this is not what user_canceled alert is intended for [2] and it is therefore undefined how the other side handles this. (JDK itself replies to close_notify preceded by user_canceled alert by immediately closing its output [3].) This fix disables this workaround when it is not necessary (connection is already half-closed by the other side). This way it fixes my case (gnutls client connected to jdk server initiates close) and it should be safe. (As removing workaround altogether could probably reintroduce issues for legacy apps... ) I also ran jdk_security tests locally, which passed for me. [1] https://bugzilla.redhat.com/show_bug.cgi?id=1918473 [2] https://datatracker.ietf.org/doc/html/rfc8446#section-6.1 [3] https://github.com/openjdk/jdk/blob/b6c35ae44aeb31deb7a15ee2939156ed00b3df52/src/java.base/share/classes/sun/security/ssl/Alert.java#L243 - Commit messages: - SSLSocketImpl: Do not use user_canceled workaround if the other side has already half-closed the connection. Changes: https://git.openjdk.java.net/jdk/pull/7664/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7664=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8282600 Stats: 6 lines in 1 file changed: 4 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7664.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7664/head:pull/7664 PR: https://git.openjdk.java.net/jdk/pull/7664
