Hi Brad,
Good points! Here is the updated webrev:
http://cr.openjdk.java.net/~xuelei/8207009/webrev.06/
Please let me know if you have more comments by 11:30AM today.
Thanks,
Xuelei
On 8/13/2018 4:43 PM, Bradford Wetmore wrote:
Hi Xuelei,
> Let's use two to emphasize the behaviors:
> 1. both input and output streams should be closed in each side, and
> 2. both client and server should perform #1.
SSLEngine.java
--------------
159: Both sides (i.e. the peer) may not be a SSLEngine:
both the client and server applications should close the {@code
SSLEngine} by calling {@link SSLEngine#closeInbound} and {@link
SSLEngine#closeOutbound} and should send/receive any...
->
the application should close the {@code SSLEngine} by calling {@link
SSLEngine#closeInbound} and {@link SSLEngine#closeOutbound} and should
send/receive any...
SSLSocket.java
--------------
With all of the ways to close, trying to say anything here is going to
get really ugly really fast. SSLSocket.close(), InputStream.close() &
OutputStream.close, and now Socket.shutdownInput() &
Socket.shutdownOutput(). Even shutdownInput()/shutdownOutput() still
throw UnsupportedOperationExceptions in JDK 10 all the way back to 1.4.
Can we just leave it at SSLSocket.close() and skip the
Input/OutputStream.close() and shutdownInput/Output? Or something like:
@apiNote
The TLS standard has evolved over the years, especially when it comes to
closing Sockets and streams. The most straightforward way to close a
connection is for an application to call SSLSocket.close(). Some
versions of the standard allow for independently closing the input or
output sides of a connection, but may throw Exceptions if certain
conditions aren't met. If such a connection state is desired, the
output stream should generally be closed before the input stream.
Once an {@code SSLSocket} is closed, it is not reusable: a new
{@code SSLSocket} must be created.
Brad
On 8/13/2018 11:50 AM, Xue-Lei Fan wrote:
Hi Jamil,
Thanks for review. One more step close to the integration.
On 8/13/2018 11:45 AM, Jamil Nimeh wrote:
Hi Xuelei,
* SSLSocket.java
o 134: Nit - You can remove the first "both" in this sentence
since you use it later with the input/output stream closure.
Let's use two to emphasize the behaviors:
1. both input and output streams should be closed in each side, and
2. both client and server should perform #1.
Thanks,
Xuelei
Looks good to me otherwise.
--Jamil
On 8/13/2018 11:31 AM, Xue-Lei Fan wrote:
One more update:
http://cr.openjdk.java.net/~xuelei/8207009/webrev.05/
It is desired to make a note in SSLSocket and SSLEngine
specification, so that users have a good sense that an application
should close the input and output stream always.
Updated for SSLEngine.java and SSLSocket.java only. No changes in
other files.
Please let me know your concerns by the end of today.
Thanks,
Xuelei
On 8/10/2018 4:02 PM, Jamil Nimeh wrote:
I'm good with the changes.
--Jamil
On 8/7/2018 5:24 PM, Xuelei Fan wrote:
Hi Jamil,
Thanks for comments. Here is the updated webrev:
http://cr.openjdk.java.net/~xuelei/8207009/webrev.04/
Thanks,
Xuelei
On 8/7/2018 3:12 PM, Jamil Nimeh wrote:
Hi Xuelei, mostly small stuff:
* SSLEngineImpl.java
o 717: Nit, inbout --> inbound
* SSLEngineOutputRecord.java
o 162, 169: Nit, applicatoin --> application
o Same section: It looks like the "if" and "else if"
clauses take
the same actions with the same message. Maybe just do "if
(isClosed())" ? Or were you planning to have different
messages
here?
* SSLSocketOutputRecord.java
o 58, 97, 204: Typo, closedd --> closed
* TLSEngineClosureTest.java
o 2: Copyright date fix
o 26: If this is a new test should the bug ID be different
than
8085979?
--Jamil
On 08/07/2018 07:46 AM, Xuelei Fan wrote:
New webrev:
http://cr.openjdk.java.net/~xuelei/8207009/webrev.03/
Thanks for a find of Tim Brooks, that the SSLEngine
inbound/outbound status is incorrect if closing during
handshake. The above webrev is trying to fix the problems. See
more in the OpenJDK thread:
http://mail.openjdk.java.net/pipermail/security-dev/2018-August/017778.html
Please let me know your concerns before this Wednesday.
Thanks,
Xuelei
On 8/3/2018 1:55 PM, Xuelei Fan wrote:
Update: http://cr.openjdk.java.net/~xuelei/8207009/webrev.02/
In webrev.01, the socket close may be blocked by super class
close synchronization. Updated the SSLSocketImpl.java to use
handshake only lock in the startHandshake() implementation.
Thanks,
Xuelei
On 8/1/2018 7:27 PM, Xuelei Fan wrote:
Update: http://cr.openjdk.java.net/~xuelei/8207009/webrev.01/
Integrated the fix for JDK-8208642, "Server initiated TLSv1.2
renegotiation fails if Java client allows TLSv1.3".
SSLHandshake.java is updated to use negotiated version so that
TLS 1.2 HelloRequest is acceptable in TLS 1.3 client side.
Thanks,
Xuelei
On 7/30/2018 10:24 AM, Xuelei Fan wrote:
<loop in net-dev as well>
Please let me know your concerns by the end of August 1st, 2018.
Thanks,
Xuelei
On 7/30/2018 9:59 AM, Xuelei Fan wrote:
Hi,
Please review the update for the TLS 1.3 half-close and
synchronization implementation:
http://cr.openjdk.java.net/~xuelei/8207009/webrev.00/
Unlike TLS 1.2 and prior versions, for TLS 1.3, the
close_notify is use to close the local write side and peer
read side only. After the close_notify get handles, the
local read side and peer write side may still be open.
In this update, if an application calls
SSLEngine.closeInbound/Outbound() or
SSLSocket.shutdownInput/Output(), half-close will be used.
For compatibility, if SSLSocket.close() get called, a duplex
close will be tried. In order to support duplex close, JDK
will use the user_canceled warning alert even the handshake
complete.
In practice, an application may only close outbound even it
is intended to close the inbound as well, or close the
connection completely. It works for TLS 1.2 and prior
versions. But no more for TLS 1.3 because of the
close_notify behavior change in the TLS 1.3 specification.
The application may be hung and dead-waiting for read/close.
It could be solved by closing the inbound explicitly. In
order to mitigate the impact, a new System Property is
introduced, "jdk.tls.acknowledgeCloseNotify" if source code
update is not available. If the System Property is set to
"true", if receiving the close_notify, a close_notify alert
will be responded. It is a countermeasure of the TLS 1.3
half-close issues.
Thanks,
Xuelei
