Re: Code Review Request, JDK-8228757 : Fail fast if the handshake type is unknown

[Jamil Nimeh]

Looks good to me.

--Jamil

On 8/19/19 9:02 AM, Xuelei Fan wrote:

Hi,

Could I have the following code cleanup reviewed?

   http://cr.openjdk.java.net/~xuelei/8228757/webrev.00/

It is trying to fail fast if unknown handshake type get requested. 
Simple fix and hard to capture the fail point, no new regression test.


Thanks,
Xuelei

Code Review Request, JDK-8228757 : Fail fast if the handshake type is unknown

[Xuelei Fan]

Hi,

Could I have the following code cleanup reviewed?

   http://cr.openjdk.java.net/~xuelei/8228757/webrev.00/

It is trying to fail fast if unknown handshake type get requested. 
Simple fix and hard to capture the fail point, no new regression test.


Thanks,
Xuelei

Re: Code Review Request, 8221253: TLSv1.3 may generate TLSInnerPlainText longer than 2^14+1 bytes

[Xuelei Fan]

Hi Jamil,

Thank you for the review.

On 5/10/2019 9:22 AM, Jamil Nimeh wrote:
This looks good to me.  One question, more for my curiosity than 
anything else: Is the way you loaded the appData array in the test code 
done for any specific reason?  Or did you just want to make sure you had 
printable ASCII that wasn't all just the same character, so it looked 
"random-ish"?


I used the printable ASCII bytes for debug log checking.  It is easier 
to analysis the SSL record log with printable code.


Thanks,
Xuelei


--Jamil

On 5/9/2019 1:28 PM, Xuelei Fan wrote:

Hi,

Could I get the following update reviewed?

   http://cr.openjdk.java.net/~xuelei/8221253/webrev.00/

Because of the padding impact, the TLS 1.3 record in the JDK Reference 
implementation could exceed the limit.  It is not the expected behavior.


Thanks,
Xuelei

Re: Code Review Request, 8221253: TLSv1.3 may generate TLSInnerPlainText longer than 2^14+1 bytes

[Jamil Nimeh]

This looks good to me.  One question, more for my curiosity than 
anything else: Is the way you loaded the appData array in the test code 
done for any specific reason?  Or did you just want to make sure you had 
printable ASCII that wasn't all just the same character, so it looked 
"random-ish"?


--Jamil

On 5/9/2019 1:28 PM, Xuelei Fan wrote:

Hi,

Could I get the following update reviewed?

   http://cr.openjdk.java.net/~xuelei/8221253/webrev.00/

Because of the padding impact, the TLS 1.3 record in the JDK Reference 
implementation could exceed the limit.  It is not the expected behavior.


Thanks,
Xuelei

Code Review Request, 8221253: TLSv1.3 may generate TLSInnerPlainText longer than 2^14+1 bytes

[Xuelei Fan]

Hi,

Could I get the following update reviewed?

   http://cr.openjdk.java.net/~xuelei/8221253/webrev.00/

Because of the padding impact, the TLS 1.3 record in the JDK Reference 
implementation could exceed the limit.  It is not the expected behavior.


Thanks,
Xuelei

Re: Code Review Request, JDK-8219991 : New fix of the deadlock in sun.security.ssl.SSLSocketImpl

[Daniel Fuchs]

Hi Xuelei,

looks good to me as well.

best regards,

-- daniel

On 05/05/2019 16:18, Xuelei Fan wrote:

All good catches!

I made the update accordingly.  Here is the new webrev:
   http://cr.openjdk.java.net/~xuelei/8219991/webrev.03/

Thanks,
Xuelei

On 5/3/2019 11:27 PM, Alan Bateman wrote:

On 04/05/2019 02:23, Xuelei Fan wrote:

Hi,

There is a surprise in the regression test.  I made the update, and 
now the testing passed.  Here is the new webrev:

    http://cr.openjdk.java.net/~xuelei/8219991/webrev.01/
I assume you can save two volatile writes by not initializing 
isClosing and hasDepleted to false (their initial value).


If readLockedDeplete were to throw an exception (and there several 
opportunities for exceptions in that metho) then read would unwind 
without unlocking. You may need to structure it to reduce that 
possibility.


-Alan

Re: Code Review Request, JDK-8219991 : New fix of the deadlock in sun.security.ssl.SSLSocketImpl

[Alan Bateman]

On 05/05/2019 16:18, Xuelei Fan wrote:

All good catches!

I made the update accordingly.  Here is the new webrev:
  http://cr.openjdk.java.net/~xuelei/8219991/webrev.03/
This update looks okay to me (an aternative for read is a nested 
try/finally but what you have is okay).


-Alan

Re: Code Review Request, JDK-8219991 : New fix of the deadlock in sun.security.ssl.SSLSocketImpl

[Xuelei Fan]

All good catches!

I made the update accordingly.  Here is the new webrev:
  http://cr.openjdk.java.net/~xuelei/8219991/webrev.03/

Thanks,
Xuelei

On 5/3/2019 11:27 PM, Alan Bateman wrote:

On 04/05/2019 02:23, Xuelei Fan wrote:

Hi,

There is a surprise in the regression test.  I made the update, and 
now the testing passed.  Here is the new webrev:

    http://cr.openjdk.java.net/~xuelei/8219991/webrev.01/
I assume you can save two volatile writes by not initializing isClosing 
and hasDepleted to false (their initial value).


If readLockedDeplete were to throw an exception (and there several 
opportunities for exceptions in that metho) then read would unwind 
without unlocking. You may need to structure it to reduce that possibility.


-Alan

Re: Code Review Request, JDK-8219991 : New fix of the deadlock in sun.security.ssl.SSLSocketImpl

[Alan Bateman]

On 04/05/2019 02:23, Xuelei Fan wrote:

Hi,

There is a surprise in the regression test.  I made the update, and 
now the testing passed.  Here is the new webrev:

    http://cr.openjdk.java.net/~xuelei/8219991/webrev.01/
I assume you can save two volatile writes by not initializing isClosing 
and hasDepleted to false (their initial value).


If readLockedDeplete were to throw an exception (and there several 
opportunities for exceptions in that metho) then read would unwind 
without unlocking. You may need to structure it to reduce that possibility.


-Alan

Re: Code Review Request, JDK-8219991 : New fix of the deadlock in sun.security.ssl.SSLSocketImpl

[Xuelei Fan]

Oops, forgot to update the link of the webrev.  It should be:
   http://cr.openjdk.java.net/~xuelei/8219991/webrev.02/

Xuelei

On 5/3/2019 6:23 PM, Xuelei Fan wrote:

Hi,

There is a surprise in the regression test.  I made the update, and now 
the testing passed.  Here is the new webrev:

     http://cr.openjdk.java.net/~xuelei/8219991/webrev.01/

http://cr.openjdk.java.net/~xuelei/8219991/webrev.02/




Thanks,
Xuelei

On 5/3/2019 6:45 AM, Daniel Fuchs wrote:

Hi Xuelei,

I agree this should fix the issue I was speaking of.
Looks good to me - but maybe you'll want a second
reviewer from the security-dev team :-)

best regards,

-- daniel

On 03/05/2019 14:03, Xuelei Fan wrote:

Hi Daniel,

Good catch!

Here is a new webrev that's trying to address the problem.
   http://cr.openjdk.java.net/~xuelei/8219991/webrev.01/

The isClosing field update is moving ahead, and a new filed 
hasDepleted was added for threads competition.


The external test described in JDK-8219658 passed, and the regressing 
testing jobs are still in progress.  I will update if there is a 
surprise in the regression testing.


Thanks,
Xuelei

On 5/3/2019 4:11 AM, Daniel Fuchs wrote:

Hi Xuelei,

I believe there is still a small window of opportunity
for which `readLockedDeplete();` will never be called.

The issue is in `deplete()`:

If tryLock() fails to lock at line
1046 if (readLock.tryLock()) {
then there's no guarantee that the reading
thread will not release the readLock before
the else { } statement is executed at line:
1054 isClosing = true;

Thread 1:  enters `read`, locks `readLock`
Thread 2:  enters `deplete`, fails to lock
Thread 1:  finishes reading, find `isClosing` is false,
    releases `readLock`.
Thread 2:  `deplete` sets `isClosing` to true and returns

then any further call to `read` will find `isClosing == true`
and return EOF.

If that happens and I'm not mistaken, then
`readLockedDeplete();` will never be called.

best regards,

-- daniel



On 02/05/2019 21:11, Xuelei Fan wrote:

Hi,

Could I get the following update reviewed?

    http://cr.openjdk.java.net/~xuelei/8219991/webrev.00/

The March5 test looks good, and the external test described in 
JDK-8219658 passed.  No new regression test.


Thanks,
Xuelei

Re: Code Review Request, JDK-8219991 : New fix of the deadlock in sun.security.ssl.SSLSocketImpl

[Xuelei Fan]

Hi,

There is a surprise in the regression test.  I made the update, and now 
the testing passed.  Here is the new webrev:

http://cr.openjdk.java.net/~xuelei/8219991/webrev.01/

Thanks,
Xuelei

On 5/3/2019 6:45 AM, Daniel Fuchs wrote:

Hi Xuelei,

I agree this should fix the issue I was speaking of.
Looks good to me - but maybe you'll want a second
reviewer from the security-dev team :-)

best regards,

-- daniel

On 03/05/2019 14:03, Xuelei Fan wrote:

Hi Daniel,

Good catch!

Here is a new webrev that's trying to address the problem.
   http://cr.openjdk.java.net/~xuelei/8219991/webrev.01/

The isClosing field update is moving ahead, and a new filed 
hasDepleted was added for threads competition.


The external test described in JDK-8219658 passed, and the regressing 
testing jobs are still in progress.  I will update if there is a 
surprise in the regression testing.


Thanks,
Xuelei

On 5/3/2019 4:11 AM, Daniel Fuchs wrote:

Hi Xuelei,

I believe there is still a small window of opportunity
for which `readLockedDeplete();` will never be called.

The issue is in `deplete()`:

If tryLock() fails to lock at line
1046 if (readLock.tryLock()) {
then there's no guarantee that the reading
thread will not release the readLock before
the else { } statement is executed at line:
1054 isClosing = true;

Thread 1:  enters `read`, locks `readLock`
Thread 2:  enters `deplete`, fails to lock
Thread 1:  finishes reading, find `isClosing` is false,
    releases `readLock`.
Thread 2:  `deplete` sets `isClosing` to true and returns

then any further call to `read` will find `isClosing == true`
and return EOF.

If that happens and I'm not mistaken, then
`readLockedDeplete();` will never be called.

best regards,

-- daniel



On 02/05/2019 21:11, Xuelei Fan wrote:

Hi,

Could I get the following update reviewed?

    http://cr.openjdk.java.net/~xuelei/8219991/webrev.00/

The March5 test looks good, and the external test described in 
JDK-8219658 passed.  No new regression test.


Thanks,
Xuelei

Re: Code Review Request, JDK-8219991 : New fix of the deadlock in sun.security.ssl.SSLSocketImpl

[Daniel Fuchs]

Hi Xuelei,

I agree this should fix the issue I was speaking of.
Looks good to me - but maybe you'll want a second
reviewer from the security-dev team :-)

best regards,

-- daniel

On 03/05/2019 14:03, Xuelei Fan wrote:

Hi Daniel,

Good catch!

Here is a new webrev that's trying to address the problem.
   http://cr.openjdk.java.net/~xuelei/8219991/webrev.01/

The isClosing field update is moving ahead, and a new filed hasDepleted 
was added for threads competition.


The external test described in JDK-8219658 passed, and the regressing 
testing jobs are still in progress.  I will update if there is a 
surprise in the regression testing.


Thanks,
Xuelei

On 5/3/2019 4:11 AM, Daniel Fuchs wrote:

Hi Xuelei,

I believe there is still a small window of opportunity
for which `readLockedDeplete();` will never be called.

The issue is in `deplete()`:

If tryLock() fails to lock at line
1046 if (readLock.tryLock()) {
then there's no guarantee that the reading
thread will not release the readLock before
the else { } statement is executed at line:
1054 isClosing = true;

Thread 1:  enters `read`, locks `readLock`
Thread 2:  enters `deplete`, fails to lock
Thread 1:  finishes reading, find `isClosing` is false,
    releases `readLock`.
Thread 2:  `deplete` sets `isClosing` to true and returns

then any further call to `read` will find `isClosing == true`
and return EOF.

If that happens and I'm not mistaken, then
`readLockedDeplete();` will never be called.

best regards,

-- daniel



On 02/05/2019 21:11, Xuelei Fan wrote:

Hi,

Could I get the following update reviewed?

    http://cr.openjdk.java.net/~xuelei/8219991/webrev.00/

The March5 test looks good, and the external test described in 
JDK-8219658 passed.  No new regression test.


Thanks,
Xuelei

Re: Code Review Request, JDK-8219991 : New fix of the deadlock in sun.security.ssl.SSLSocketImpl

[Xuelei Fan]

Hi Daniel,

Good catch!

Here is a new webrev that's trying to address the problem.
  http://cr.openjdk.java.net/~xuelei/8219991/webrev.01/

The isClosing field update is moving ahead, and a new filed hasDepleted 
was added for threads competition.


The external test described in JDK-8219658 passed, and the regressing 
testing jobs are still in progress.  I will update if there is a 
surprise in the regression testing.


Thanks,
Xuelei

On 5/3/2019 4:11 AM, Daniel Fuchs wrote:

Hi Xuelei,

I believe there is still a small window of opportunity
for which `readLockedDeplete();` will never be called.

The issue is in `deplete()`:

If tryLock() fails to lock at line
1046 if (readLock.tryLock()) {
then there's no guarantee that the reading
thread will not release the readLock before
the else { } statement is executed at line:
1054 isClosing = true;

Thread 1:  enters `read`, locks `readLock`
Thread 2:  enters `deplete`, fails to lock
Thread 1:  finishes reading, find `isClosing` is false,
    releases `readLock`.
Thread 2:  `deplete` sets `isClosing` to true and returns

then any further call to `read` will find `isClosing == true`
and return EOF.

If that happens and I'm not mistaken, then
`readLockedDeplete();` will never be called.

best regards,

-- daniel



On 02/05/2019 21:11, Xuelei Fan wrote:

Hi,

Could I get the following update reviewed?

    http://cr.openjdk.java.net/~xuelei/8219991/webrev.00/

The March5 test looks good, and the external test described in 
JDK-8219658 passed.  No new regression test.


Thanks,
Xuelei

Code Review Request, JDK-8219991 : New fix of the deadlock in sun.security.ssl.SSLSocketImpl

[Xuelei Fan]

Hi,

Could I get the following update reviewed?

   http://cr.openjdk.java.net/~xuelei/8219991/webrev.00/

The March5 test looks good, and the external test described in 
JDK-8219658 passed.  No new regression test.


Thanks,
Xuelei

Re: Code Review Request, JDK-8217835, Remove the experimental SunJSSE FIPS compliant mode

[Sean Mullan]

Looks good, pretty straightforward cleanup.

--Sean

On 2/5/19 1:44 PM, Xuelei Fan wrote:

Hi,

Could I have the update reviewed?
    http://cr.openjdk.java.net/~xuelei/8217835/webrev.00/

With this update, the experimental FIPS 140 compliant mode is removed 
from the SunJSSE provider. As the SunJSSE provider uses the JDK default 
cryptography providers, alternatively applications can configure the 
Java Security Property to use FIPS 140 compliant cryptography providers.


More details,  please refer to CSR:
    https://bugs.openjdk.java.net/browse/JDK-8217907

Thanks & Regards,
Xuelei

Re: Code Review Request, JDK-8218580, endpoint identification algorithm should be case-insensitive

[Jamil Nimeh]

Looks fine to me.

--Jamil

On 2/8/2019 8:16 AM, Xuelei Fan wrote:

Hi,

Please review this update:
   http://cr.openjdk.java.net/~xuelei/8218580/webrev.00/

Objects.equals is case-sensitive, and should not be used to recognize 
case-insensitive objects.


Trivial update, no new regression test.

Thanks,
Xuelei

Code Review Request, JDK-8218580, endpoint identification algorithm should be case-insensitive

[Xuelei Fan]

Hi,

Please review this update:
   http://cr.openjdk.java.net/~xuelei/8218580/webrev.00/

Objects.equals is case-sensitive, and should not be used to recognize 
case-insensitive objects.


Trivial update, no new regression test.

Thanks,
Xuelei

Code Review Request, JDK-8217835, Remove the experimental SunJSSE FIPS compliant mode

[Xuelei Fan]

Hi,

Could I have the update reviewed?
   http://cr.openjdk.java.net/~xuelei/8217835/webrev.00/

With this update, the experimental FIPS 140 compliant mode is removed 
from the SunJSSE provider. As the SunJSSE provider uses the JDK default 
cryptography providers, alternatively applications can configure the 
Java Security Property to use FIPS 140 compliant cryptography providers.


More details,  please refer to CSR:
   https://bugs.openjdk.java.net/browse/JDK-8217907

Thanks & Regards,
Xuelei

Re: Code Review Request, JDK-8217820 Useless cast in ECUtil.java

[Jamil Nimeh]

You sure can!  Looks good.

--Jamil

On 1/25/2019 12:02 PM, Xuelei Fan wrote:

Hi,

Can I have a code review for a trivial code cleanup?

  https://bugs.openjdk.java.net/browse/JDK-8217820

Thanks,
Xuelei

diff -r 1262a93634c2 
src/java.base/share/classes/sun/security/util/ECUtil.java
--- a/src/java.base/share/classes/sun/security/util/ECUtil.java Thu 
Jan 24 12:52:37 2019 -0500
+++ b/src/java.base/share/classes/sun/security/util/ECUtil.java Fri 
Jan 25 09:26:40 2019 -0800

@@ -31,7 +31,6 @@
 import java.security.interfaces.*;
 import java.security.spec.*;
 import java.util.Arrays;
-import sun.security.x509.X509Key;

 public class ECUtil {

@@ -103,7 +102,7 @@
 ECParameterSpec params) throws InvalidKeySpecException {
 KeyFactory keyFactory = getKeyFactory();
 ECPublicKeySpec keySpec = new ECPublicKeySpec(w, params);
-    X509Key key = (X509Key)keyFactory.generatePublic(keySpec);
+    Key key = keyFactory.generatePublic(keySpec);

 return key.getEncoded();
 }

Code Review Request, JDK-8217820 Useless cast in ECUtil.java

[Xuelei Fan]

Hi,

Can I have a code review for a trivial code cleanup?

  https://bugs.openjdk.java.net/browse/JDK-8217820

Thanks,
Xuelei

diff -r 1262a93634c2 
src/java.base/share/classes/sun/security/util/ECUtil.java
--- a/src/java.base/share/classes/sun/security/util/ECUtil.java Thu Jan 
24 12:52:37 2019 -0500
+++ b/src/java.base/share/classes/sun/security/util/ECUtil.java Fri Jan 
25 09:26:40 2019 -0800

@@ -31,7 +31,6 @@
 import java.security.interfaces.*;
 import java.security.spec.*;
 import java.util.Arrays;
-import sun.security.x509.X509Key;

 public class ECUtil {

@@ -103,7 +102,7 @@
 ECParameterSpec params) throws InvalidKeySpecException {
 KeyFactory keyFactory = getKeyFactory();
 ECPublicKeySpec keySpec = new ECPublicKeySpec(w, params);
-X509Key key = (X509Key)keyFactory.generatePublic(keySpec);
+Key key = keyFactory.generatePublic(keySpec);

 return key.getEncoded();
 }

Re: Code Review Request, JDK-8216045 The size of key_exchange may be wrong on FFDHE

[Jamil Nimeh]

Hi Xuelei, this looks good to me.

--Jamil

On 1/15/2019 7:45 AM, Xue-Lei Fan wrote:

Hi,

Could I have the update reviewed?
   http://cr.openjdk.java.net/~xuelei/8216045/webrev.00/

While getting the encoded public key for DH key exchange,  the leading 
zeros of the key are not trimmed and the key bit size is used for byte 
size.


John helped to verify the fix with the infra testing and fuzzing 
testing.  I did not add a new unit test as it is not straightforward.


Thanks,
Xuelei

Code Review Request, JDK-8216045 The size of key_exchange may be wrong on FFDHE

[Xue-Lei Fan]

Hi,

Could I have the update reviewed?
   http://cr.openjdk.java.net/~xuelei/8216045/webrev.00/

While getting the encoded public key for DH key exchange,  the leading 
zeros of the key are not trimmed and the key bit size is used for byte size.


John helped to verify the fix with the infra testing and fuzzing 
testing.  I did not add a new unit test as it is not straightforward.


Thanks,
Xuelei

Re: Code Review Request, JDK-8214418 HttpClient falls in running with 100% cpu usage after an error signalled on channel

[Daniel Fuchs]

Hi Xuelei,

This is not my area of expertise - so I'm going to rephrase
what I understand (which may be wrong):

SSLEngineImpl.java:
  This change makes sure that the SSLEngineResult::getStatus()
  returns Status.CLOSED when closure has been initiated, even
  if the engine is only half-closed at that time.
  This allows the application to complete the closure by looking
  at the state of the inbound/outbound.

TransportContext.java:
   This change makes sure that the appropriate
   HandshakeStatus is returned to allow the completion of the
   closure: reading/sending the closure acknowledgement.

If my understanding is correct - then I think this change is good.
As far as I can see this is exactly what we are expecting in
the java.net.http HttpClient SSLFlowDelegate, so that looks good
to me!

As Chris mentioned, it would be good to have a deterministic test
to verify the behavior.

best regards,

-- daniel

On 22/12/2018 17:20, Xue-Lei Fan wrote:

Hi,

Could I get the update reviewed?
    http://cr.openjdk.java.net/~xuelei/8214418/webrev.00/

The reproducing testing case passed with the update.

The issue is caused by the handshake status "NEED_WRAP" while the 
connection is half-closed. An application may just call wrap() when the 
handshake status is "NEED_WRAP". For compatibility, I changed the 
handshake status from NEED_WRAP back to NOT_HANDSHAKING for inbound 
half-closed connection.  An application can use 
SSLEngine.isOutboundDone() for the determination if SSLEngine.wrap() 
should be called.


Thanks,
Xuelei

Re: Code Review Request, JDK-8214418 HttpClient falls in running with 100% cpu usage after an error signalled on channel

[Xue-Lei Fan]

On 1/9/2019 10:58 AM, Daniel Fuchs wrote:

Hi Xuelei,

On 22/12/2018 17:20, Xue-Lei Fan wrote:
The issue is caused by the handshake status "NEED_WRAP" while the 
connection is half-closed. An application may just call wrap() when 
the handshake status is "NEED_WRAP". For compatibility, I changed the 
handshake status from NEED_WRAP back to NOT_HANDSHAKING for inbound 
half-closed connection.  An application can use 
SSLEngine.isOutboundDone() for the determination if SSLEngine.wrap() 
should be called.


For clarification: the logic for the application would be to call
SSLEngine.isOutboundDone(), and if that is false, to call
SSLEngine.wrap() in order to trigger the sending of the close notify
response. Is my understanding correct?


Yes.

Thanks,
Xuelei

Re: Code Review Request, JDK-8214418 HttpClient falls in running with 100% cpu usage after an error signalled on channel

[Daniel Fuchs]

Hi Xuelei,

On 22/12/2018 17:20, Xue-Lei Fan wrote:
The issue is caused by the handshake status "NEED_WRAP" while the 
connection is half-closed. An application may just call wrap() when the 
handshake status is "NEED_WRAP". For compatibility, I changed the 
handshake status from NEED_WRAP back to NOT_HANDSHAKING for inbound 
half-closed connection.  An application can use 
SSLEngine.isOutboundDone() for the determination if SSLEngine.wrap() 
should be called.


For clarification: the logic for the application would be to call
SSLEngine.isOutboundDone(), and if that is false, to call
SSLEngine.wrap() in order to trigger the sending of the close notify
response. Is my understanding correct?

best regards,

-- daniel

Re: Code Review Request, JDK-8214418 HttpClient falls in running with 100% cpu usage after an error signalled on channel

[Jamil Nimeh]

Code changes look good to me.

--Jamil

On 1/8/2019 3:00 PM, Xue-Lei Fan wrote:

ping ...

Xuelei

On 12/22/2018 9:20 AM, Xue-Lei Fan wrote:

Hi,

Could I get the update reviewed?
    http://cr.openjdk.java.net/~xuelei/8214418/webrev.00/

The reproducing testing case passed with the update.

The issue is caused by the handshake status "NEED_WRAP" while the 
connection is half-closed. An application may just call wrap() when 
the handshake status is "NEED_WRAP". For compatibility, I changed the 
handshake status from NEED_WRAP back to NOT_HANDSHAKING for inbound 
half-closed connection.  An application can use 
SSLEngine.isOutboundDone() for the determination if SSLEngine.wrap() 
should be called.


Thanks,
Xuelei

Re: Code Review Request, JDK-8214418 half-closed SSLEnigne status may cause application dead loop

[Xue-Lei Fan]

On 1/9/2019 6:10 AM, Chris Hegarty wrote:

Xuelei,

Is it possible to update the synopsis of this bug to better
reflect the underlying issue ( rather than one particular
symptom )?


Updated.


Also, it is possible to construct a small, non-HTTP related,
targeted test that verifies the fix?


There was an test update that covered the fix.

Thanks,
Xuelei


-Chris.

On 08/01/2019 23:00, Xue-Lei Fan wrote:

ping ...

Xuelei

On 12/22/2018 9:20 AM, Xue-Lei Fan wrote:

Hi,

Could I get the update reviewed?
    http://cr.openjdk.java.net/~xuelei/8214418/webrev.00/

The reproducing testing case passed with the update.

The issue is caused by the handshake status "NEED_WRAP" while the 
connection is half-closed. An application may just call wrap() when 
the handshake status is "NEED_WRAP". For compatibility, I changed the 
handshake status from NEED_WRAP back to NOT_HANDSHAKING for inbound 
half-closed connection.  An application can use 
SSLEngine.isOutboundDone() for the determination if SSLEngine.wrap() 
should be called.


Thanks,
Xuelei

Re: Code Review Request, JDK-8214418 HttpClient falls in running with 100% cpu usage after an error signalled on channel

[Chris Hegarty]

Xuelei,

Is it possible to update the synopsis of this bug to better
reflect the underlying issue ( rather than one particular
symptom )?

Also, it is possible to construct a small, non-HTTP related,
targeted test that verifies the fix?

-Chris.

On 08/01/2019 23:00, Xue-Lei Fan wrote:

ping ...

Xuelei

On 12/22/2018 9:20 AM, Xue-Lei Fan wrote:

Hi,

Could I get the update reviewed?
    http://cr.openjdk.java.net/~xuelei/8214418/webrev.00/

The reproducing testing case passed with the update.

The issue is caused by the handshake status "NEED_WRAP" while the 
connection is half-closed. An application may just call wrap() when 
the handshake status is "NEED_WRAP". For compatibility, I changed the 
handshake status from NEED_WRAP back to NOT_HANDSHAKING for inbound 
half-closed connection.  An application can use 
SSLEngine.isOutboundDone() for the determination if SSLEngine.wrap() 
should be called.


Thanks,
Xuelei

Re: Code Review Request, JDK-8214418 HttpClient falls in running with 100% cpu usage after an error signalled on channel

[Xue-Lei Fan]

ping ...

Xuelei

On 12/22/2018 9:20 AM, Xue-Lei Fan wrote:

Hi,

Could I get the update reviewed?
    http://cr.openjdk.java.net/~xuelei/8214418/webrev.00/

The reproducing testing case passed with the update.

The issue is caused by the handshake status "NEED_WRAP" while the 
connection is half-closed. An application may just call wrap() when the 
handshake status is "NEED_WRAP". For compatibility, I changed the 
handshake status from NEED_WRAP back to NOT_HANDSHAKING for inbound 
half-closed connection.  An application can use 
SSLEngine.isOutboundDone() for the determination if SSLEngine.wrap() 
should be called.


Thanks,
Xuelei

Code Review Request, JDK-8214418 HttpClient falls in running with 100% cpu usage after an error signalled on channel

[Xue-Lei Fan]

Hi,

Could I get the update reviewed?
   http://cr.openjdk.java.net/~xuelei/8214418/webrev.00/

The reproducing testing case passed with the update.

The issue is caused by the handshake status "NEED_WRAP" while the 
connection is half-closed. An application may just call wrap() when the 
handshake status is "NEED_WRAP". For compatibility, I changed the 
handshake status from NEED_WRAP back to NOT_HANDSHAKING for inbound 
half-closed connection.  An application can use 
SSLEngine.isOutboundDone() for the determination if SSLEngine.wrap() 
should be called.


Thanks,
Xuelei

Re: Code Review Request JDK-8215443: The use of TransportContext.fatal() leads to bad coding style

[Anthony Scarpino]

On 12/17/18 5:26 PM, Xue-Lei Fan wrote:

On 12/17/2018 1:17 PM, Anthony Scarpino wrote:
It looks like in TransportContext.java:68, you had a mistype that 
added "fa" to the end of a comment.



Oops, I will update it.

Also in fatal():267, did you plan to return the exception and have the 
calling method throw the exception?  As is, the exception is never 
return and fatal() continues to throw the exceptions.


I considered to return the exception.  I'm not very confident with if I 
searched out all use of the fatal() methods.  For safe, I kept to use 
thrown exception instead. If the method is updated later that there is a 
case that now exception get thrown, there will be a compiling issue. Are 
you OK with it?


Since the stacktrace is based on the exception's creation and not where 
it's thrown, I guess there's no reason to return the value.  As you have 
it now should be fine.


Tony



Thanks,
Xuelei


Tony

On 12/15/18 7:51 AM, Xue-Lei Fan wrote:

Hi,

Could I have the update reviewed?
    http://cr.openjdk.java.net/~xuelei/8215443/webrev.00/

The TransportContext.fatal() methods always throw exception. While 
the compiler does not aware of it, and may not happy without 
following a return statement.  Currently, a lot never executable 
return statements are inserted.  As make the code hard to read 
(thanks for Jamil and Tony's points).  For example:


 shc.conContext.fatal(Alert.UNEXPECTED_MESSAGE, ...);
 return null;    // fatal() always throws, make the compiler happy.

In this update, I changed the fatal() method with a return value:

-    void fatal(Alert alert, ...
+    SSLException fatal(Alert alert, ...

Then we can change the use of method as:

-    shc.conContext.fatal(Alert.UNEXPECTED_MESSAGE, ...);
-    return null;    // fatal() always throws, make the compiler happy.
+    throw shc.conContext.fatal(Alert.UNEXPECTED_MESSAGE, ...);

The changeset is mostly about removing the never executed return 
statements and add the 'throw' keyword to lines that use the fatal() 
methods.


Thanks,
Xuelei

Re: Code Review Request JDK-8215443: The use of TransportContext.fatal() leads to bad coding style

[Xue-Lei Fan]

On 12/17/2018 1:17 PM, Anthony Scarpino wrote:
It looks like in TransportContext.java:68, you had a mistype that added 
"fa" to the end of a comment.



Oops, I will update it.

Also in fatal():267, did you plan to return the exception and have the 
calling method throw the exception?  As is, the exception is never 
return and fatal() continues to throw the exceptions.


I considered to return the exception.  I'm not very confident with if I 
searched out all use of the fatal() methods.  For safe, I kept to use 
thrown exception instead. If the method is updated later that there is a 
case that now exception get thrown, there will be a compiling issue. 
Are you OK with it?


Thanks,
Xuelei


Tony

On 12/15/18 7:51 AM, Xue-Lei Fan wrote:

Hi,

Could I have the update reviewed?
    http://cr.openjdk.java.net/~xuelei/8215443/webrev.00/

The TransportContext.fatal() methods always throw exception. While the 
compiler does not aware of it, and may not happy without following a 
return statement.  Currently, a lot never executable return statements 
are inserted.  As make the code hard to read (thanks for Jamil and 
Tony's points).  For example:


 shc.conContext.fatal(Alert.UNEXPECTED_MESSAGE, ...);
 return null;    // fatal() always throws, make the compiler happy.

In this update, I changed the fatal() method with a return value:

-    void fatal(Alert alert, ...
+    SSLException fatal(Alert alert, ...

Then we can change the use of method as:

-    shc.conContext.fatal(Alert.UNEXPECTED_MESSAGE, ...);
-    return null;    // fatal() always throws, make the compiler happy.
+    throw shc.conContext.fatal(Alert.UNEXPECTED_MESSAGE, ...);

The changeset is mostly about removing the never executed return 
statements and add the 'throw' keyword to lines that use the fatal() 
methods.


Thanks,
Xuelei

Re: Code Review Request, JDK-8209333 Socket reset issue for TLS 1.3 socket close

[Jamil Nimeh]

Hi Xuelei, comments in-line.

On 12/17/2018 12:11 PM, Xue-Lei Fan wrote:

On 12/17/2018 11:28 AM, Jamil Nimeh wrote:

Hi Xuelei, just a couple questions:

  * SSLSocketImpl
  o 611: Are you sure conContext.inputRecord should go into a
    try-with-resources?  As far as I can tell, the inheritence chain
    is SSLSocketInputRecord->InputRecord and that directly or by
    extension implements the SSLRecord, Record and Closeable
    interfaces.  I thought that you needed to implement
    AutoCloseable in order to get that automatic closure benefit
    from leaving the try block.  Am I missing something?
Closeable was updated to extend AutoCloseable, so using either 
AutoCloseable or Closeable is fine for try-with-resources statement.

    public interface Closeable extends AutoCloseable
Yep, I see what you're talking about.  I completely missed that 
interface relationship.



  * SSLContextTemplate
  o 505: Is there any benefit to initializing the SSLContext using a
    PKIXParameters object with the date fixed to sometime in between
    2018 and 2028 (the soonest expiration date of all the certs in
    the test).  This way you'd never have to worry about things
    expiring.  I know we don't do this in most of our tests, dunno
    why it just occurred to me now.


Good point!  It's a really nice enhancement!

I had pushed this template in another bug fix. Let's do it separately. 
I filed a new bug for tracking this enhancement.

    https://bugs.openjdk.java.net/browse/JDK-8215509

Sounds good to me.  The review as a whole looks good.

--Jamil



Thanks,
Xuelei



The rest looks OK to me.

--Jamil

On 12/17/2018 9:52 AM, Xue-Lei Fan wrote:

ping ...

On 12/10/2018 1:14 PM, Xue-Lei Fan wrote:

Hi,

Please review the TLS 1.3 half-close issue in JDK.

http://cr.openjdk.java.net/~xuelei/8209333/webrev.00/

While trying to duplex close a TLS connection upon the half-close 
policy, there might be pending receiving data in the closing side, 
and result in a TCP RST during closing. The TCP RST may then cause 
the peer reading failure.  For example:

1. client and server establish a TLS 1.3 connection.
2. server sending the post-handshake NewSessionTicket message.
3. client send the application data, and then close the connection.
4. as the client does not call to read the post-handshake message, 
the connection close will cause a TCP RST.
5. server trying to read the client application data, but the 
socket may be impacted by the TCP RST, and the reading can fail.


It would not be an issue any more if the client could read the 
post-handshake message, explicit or implicit.


I would like applications consider to use half-close policy, and 
moving away from the duplex-close policy.


The basic idea of the fix is trying to use up buffered network 
input before close the socket.  As is an implicit behavior to 
consume the post-handshake message, and mitigate the impact of it.


This fix is not a perfect one.  It is just a workaround for 
duplex-close.  I'm open to hear more ideas.


Thanks,
Xuelei

Re: Code Review Request JDK-8215443: The use of TransportContext.fatal() leads to bad coding style

[Anthony Scarpino]

It looks like in TransportContext.java:68, you had a mistype that added 
"fa" to the end of a comment.


Also in fatal():267, did you plan to return the exception and have the 
calling method throw the exception?  As is, the exception is never 
return and fatal() continues to throw the exceptions.


Tony

On 12/15/18 7:51 AM, Xue-Lei Fan wrote:

Hi,

Could I have the update reviewed?
    http://cr.openjdk.java.net/~xuelei/8215443/webrev.00/

The TransportContext.fatal() methods always throw exception. While the 
compiler does not aware of it, and may not happy without following a 
return statement.  Currently, a lot never executable return statements 
are inserted.  As make the code hard to read (thanks for Jamil and 
Tony's points).  For example:


     shc.conContext.fatal(Alert.UNEXPECTED_MESSAGE, ...);
     return null;    // fatal() always throws, make the compiler happy.

In this update, I changed the fatal() method with a return value:

-    void fatal(Alert alert, ...
+    SSLException fatal(Alert alert, ...

Then we can change the use of method as:

-    shc.conContext.fatal(Alert.UNEXPECTED_MESSAGE, ...);
-    return null;    // fatal() always throws, make the compiler happy.
+    throw shc.conContext.fatal(Alert.UNEXPECTED_MESSAGE, ...);

The changeset is mostly about removing the never executed return 
statements and add the 'throw' keyword to lines that use the fatal() 
methods.


Thanks,
Xuelei

Re: Code Review Request, JDK-8209333 Socket reset issue for TLS 1.3 socket close

[Xue-Lei Fan]

On 12/17/2018 11:28 AM, Jamil Nimeh wrote:

Hi Xuelei, just a couple questions:

  * SSLSocketImpl
  o 611: Are you sure conContext.inputRecord should go into a
try-with-resources?  As far as I can tell, the inheritence chain
is SSLSocketInputRecord->InputRecord and that directly or by
extension implements the SSLRecord, Record and Closeable
interfaces.  I thought that you needed to implement
AutoCloseable in order to get that automatic closure benefit
from leaving the try block.  Am I missing something?
Closeable was updated to extend AutoCloseable, so using either 
AutoCloseable or Closeable is fine for try-with-resources statement.

public interface Closeable extends AutoCloseable


  * SSLContextTemplate
  o 505: Is there any benefit to initializing the SSLContext using a
PKIXParameters object with the date fixed to sometime in between
2018 and 2028 (the soonest expiration date of all the certs in
the test).  This way you'd never have to worry about things
expiring.  I know we don't do this in most of our tests, dunno
why it just occurred to me now.


Good point!  It's a really nice enhancement!

I had pushed this template in another bug fix. Let's do it separately. I 
filed a new bug for tracking this enhancement.

https://bugs.openjdk.java.net/browse/JDK-8215509

Thanks,
Xuelei



The rest looks OK to me.

--Jamil

On 12/17/2018 9:52 AM, Xue-Lei Fan wrote:

ping ...

On 12/10/2018 1:14 PM, Xue-Lei Fan wrote:

Hi,

Please review the TLS 1.3 half-close issue in JDK.

http://cr.openjdk.java.net/~xuelei/8209333/webrev.00/

While trying to duplex close a TLS connection upon the half-close 
policy, there might be pending receiving data in the closing side, 
and result in a TCP RST during closing.  The TCP RST may then cause 
the peer reading failure.  For example:

1. client and server establish a TLS 1.3 connection.
2. server sending the post-handshake NewSessionTicket message.
3. client send the application data, and then close the connection.
4. as the client does not call to read the post-handshake message, 
the connection close will cause a TCP RST.
5. server trying to read the client application data, but the socket 
may be impacted by the TCP RST, and the reading can fail.


It would not be an issue any more if the client could read the 
post-handshake message, explicit or implicit.


I would like applications consider to use half-close policy, and 
moving away from the duplex-close policy.


The basic idea of the fix is trying to use up buffered network input 
before close the socket.  As is an implicit behavior to consume the 
post-handshake message, and mitigate the impact of it.


This fix is not a perfect one.  It is just a workaround for 
duplex-close.  I'm open to hear more ideas.


Thanks,
Xuelei

Re: Code Review Request, JDK-8209333 Socket reset issue for TLS 1.3 socket close

[Jamil Nimeh]

Hi Xuelei, just a couple questions:

 * SSLSocketImpl
 o 611: Are you sure conContext.inputRecord should go into a
   try-with-resources?  As far as I can tell, the inheritence chain
   is SSLSocketInputRecord->InputRecord and that directly or by
   extension implements the SSLRecord, Record and Closeable
   interfaces.  I thought that you needed to implement
   AutoCloseable in order to get that automatic closure benefit
   from leaving the try block.  Am I missing something?
 * SSLContextTemplate
 o 505: Is there any benefit to initializing the SSLContext using a
   PKIXParameters object with the date fixed to sometime in between
   2018 and 2028 (the soonest expiration date of all the certs in
   the test).  This way you'd never have to worry about things
   expiring.  I know we don't do this in most of our tests, dunno
   why it just occurred to me now.

The rest looks OK to me.

--Jamil

On 12/17/2018 9:52 AM, Xue-Lei Fan wrote:

ping ...

On 12/10/2018 1:14 PM, Xue-Lei Fan wrote:

Hi,

Please review the TLS 1.3 half-close issue in JDK.

 http://cr.openjdk.java.net/~xuelei/8209333/webrev.00/

While trying to duplex close a TLS connection upon the half-close 
policy, there might be pending receiving data in the closing side, 
and result in a TCP RST during closing.  The TCP RST may then cause 
the peer reading failure.  For example:

1. client and server establish a TLS 1.3 connection.
2. server sending the post-handshake NewSessionTicket message.
3. client send the application data, and then close the connection.
4. as the client does not call to read the post-handshake message, 
the connection close will cause a TCP RST.
5. server trying to read the client application data, but the socket 
may be impacted by the TCP RST, and the reading can fail.


It would not be an issue any more if the client could read the 
post-handshake message, explicit or implicit.


I would like applications consider to use half-close policy, and 
moving away from the duplex-close policy.


The basic idea of the fix is trying to use up buffered network input 
before close the socket.  As is an implicit behavior to consume the 
post-handshake message, and mitigate the impact of it.


This fix is not a perfect one.  It is just a workaround for 
duplex-close.  I'm open to hear more ideas.


Thanks,
Xuelei

Re: Code Review Request, JDK-8209333 Socket reset issue for TLS 1.3 socket close

[Xue-Lei Fan]

ping ...

On 12/10/2018 1:14 PM, Xue-Lei Fan wrote:

Hi,

Please review the TLS 1.3 half-close issue in JDK.

     http://cr.openjdk.java.net/~xuelei/8209333/webrev.00/

While trying to duplex close a TLS connection upon the half-close 
policy, there might be pending receiving data in the closing side, and 
result in a TCP RST during closing.  The TCP RST may then cause the peer 
reading failure.  For example:

1. client and server establish a TLS 1.3 connection.
2. server sending the post-handshake NewSessionTicket message.
3. client send the application data, and then close the connection.
4. as the client does not call to read the post-handshake message, the 
connection close will cause a TCP RST.
5. server trying to read the client application data, but the socket may 
be impacted by the TCP RST, and the reading can fail.


It would not be an issue any more if the client could read the 
post-handshake message, explicit or implicit.


I would like applications consider to use half-close policy, and moving 
away from the duplex-close policy.


The basic idea of the fix is trying to use up buffered network input 
before close the socket.  As is an implicit behavior to consume the 
post-handshake message, and mitigate the impact of it.


This fix is not a perfect one.  It is just a workaround for 
duplex-close.  I'm open to hear more ideas.


Thanks,
Xuelei

Re: Code Review Request, JDK-8214339 : SSLSocketImpl erroneously wraps SocketException

[Anthony Scarpino]

Just the complete the thread.  Says "fatal() throws exception" is fine.

If this all only trying to make the compiler happy, you could just have 
one "return null" at the bottom of the method.  It is not necessary to 
put a return after each fatal().  I will admit it could be less readable.


Or fatal could return the exception, then the calling method would throw 
it:   throw shc.conContext.fatal();

But that's a larger change that I don't expected changed here.

Tony

On 12/14/18 1:46 PM, Xue-Lei Fan wrote:

Hi,

The purpose of combination of the two lines together:

     shc.conContext.fatal(Alert.UNEXPECTED_MESSAGE, "...");
     return null;    // make the complier happy

is actually for the reading of the code.  If one don't know the fatal() 
always throw an exception (compiler is one of them), he may continue 
reading the following code, and may be confusing about the code logic.


The "return" statement could never be executed actually.  It could be 
easier to catch the idea if using a format like:


    throw new SSLException("there is an alert")
-  return null;

But we need the fatal() method to wrap something more.

We used a lot comments similar to:

    return; // fatal() always throws, make the compiler happy.

and the abbreviate comment:
    return;    // make the compiler happy

misses the part "fatal() always throws", and then it may look weird.

I'm fine to remove the comment, or use the full comment instead "fatal() 
always throws, make the compiler happy".  Which one is your preference?



On 12/14/2018 11:49 AM, Jamil Nimeh wrote:
Looks pretty good.  I did have one question about a few of the methods 
in KeyShareExtension and PreSharedKeyExtension, specifically where you 
return nulls on error branches with the make-compiler-happy comments. 
In those cases would it be a bit cleaner to set a byte[] variable to 
null at the beginning of the method and then in the successful code 
path set the value to whatever byte array comes back from hmacs or 
hkdf operations?  At the end of the method you only need one return 
statement, rather than a return null on every error branch, many of 
which won't be executed due to method calls which always throw 
exceptions.
Hm, I see your points.  As the return statement could never be executed, 
it may not worthy to declare the byte[] variable earlier.


See above. Is it a reasonable coding style to you as well?

Thanks,
Xuelei

Not a big deal if you wish to leave things as they are since I know 
the current approach is used in many places in the code that this 
review doesn't touch.


--Jamil

On 12/14/2018 8:14 AM, Xue-Lei Fan wrote:

Hi,

Please review the update:
   http://cr.openjdk.java.net/~xuelei/8214339/webrev.00/

In some cases, the SSLProtocolException or SSLHandshakeException may 
be thrown if the underlying socket run into problems.  An application 
may depends on the exception class for further action, for example 
retry the connection with different parameters.


This update is trying to separate the socket problem from the TLS 
protocol or handshake problem, by using different exception classes.


Thanks,
Xuelei

Code Review Request JDK-8215443: The use of TransportContext.fatal() leads to bad coding style

[Xue-Lei Fan]

Hi,

Could I have the update reviewed?
   http://cr.openjdk.java.net/~xuelei/8215443/webrev.00/

The TransportContext.fatal() methods always throw exception. While the 
compiler does not aware of it, and may not happy without following a 
return statement.  Currently, a lot never executable return statements 
are inserted.  As make the code hard to read (thanks for Jamil and 
Tony's points).  For example:


shc.conContext.fatal(Alert.UNEXPECTED_MESSAGE, ...);
return null;// fatal() always throws, make the compiler happy.

In this update, I changed the fatal() method with a return value:

-void fatal(Alert alert, ...
+SSLException fatal(Alert alert, ...

Then we can change the use of method as:

-shc.conContext.fatal(Alert.UNEXPECTED_MESSAGE, ...);
-return null;// fatal() always throws, make the compiler happy.
+throw shc.conContext.fatal(Alert.UNEXPECTED_MESSAGE, ...);

The changeset is mostly about removing the never executed return 
statements and add the 'throw' keyword to lines that use the fatal() 
methods.


Thanks,
Xuelei

Re: Code Review Request, JDK-8214339 : SSLSocketImpl erroneously wraps SocketException

[Jamil Nimeh]

Hi Xuelei, thanks for the clarification.  If you want to keep the 
structure as-is, then the comment "fatal() always throws, make the 
compiler happy" is fine.  I think that's a little more helpful to a 
future maintainer who might be new to the code.


Thanks,
--Jamil

On 12/14/2018 1:56 PM, Xue-Lei Fan wrote:

On 12/14/2018 1:46 PM, Xue-Lei Fan wrote:

Hi,

The purpose of combination of the two lines together:

 shc.conContext.fatal(Alert.UNEXPECTED_MESSAGE, "...");
 return null;    // make the complier happy

is actually for the reading of the code.  If one don't know the 
fatal() always throw an exception (compiler is one of them), he may 
continue reading the following code, and may be confusing about the 
code logic.


The "return" statement could never be executed actually.  It could be 
easier to catch the idea if using a format like:


    throw new SSLException("there is an alert")
-  return null;

But we need the fatal() method to wrap something more.

We used a lot comments similar to:

    return; // fatal() always throws, make the compiler happy.

and the abbreviate comment:
    return;    // make the compiler happy

misses the part "fatal() always throws", and then it may look weird.

I'm fine to remove the comment, or use the full comment instead 
"fatal() always throws, make the compiler happy".  Which one is your 
preference?



On 12/14/2018 11:49 AM, Jamil Nimeh wrote:
Looks pretty good.  I did have one question about a few of the 
methods in KeyShareExtension and PreSharedKeyExtension, specifically 
where you return nulls on error branches with the 
make-compiler-happy comments. In those cases would it be a bit 
cleaner to set a byte[] variable to null at the beginning of the 
method and then in the successful code path set the value to 
whatever byte array comes back from hmacs or hkdf operations?  At 
the end of the method you only need one return statement, rather 
than a return null on every error branch, many of which won't be 
executed due to method calls which always throw exceptions.
Hm, I see your points.  As the return statement could never be 
executed, it may not worthy to declare the byte[] variable earlier.


I mean that we can remove the "return" statement as it is never 
executed, if not for code reading or compiler.


In some other places, the compiler may not happy because it cannot 
tell that the fatal() throws exception.  So we may still need the 
"return" statement a little bit.  But I think most of them could be 
removed if we don't like it.


Xuelei


See above. Is it a reasonable coding style to you as well?

Thanks,
Xuelei

Not a big deal if you wish to leave things as they are since I know 
the current approach is used in many places in the code that this 
review doesn't touch.


--Jamil

On 12/14/2018 8:14 AM, Xue-Lei Fan wrote:

Hi,

Please review the update:
   http://cr.openjdk.java.net/~xuelei/8214339/webrev.00/

In some cases, the SSLProtocolException or SSLHandshakeException 
may be thrown if the underlying socket run into problems.  An 
application may depends on the exception class for further action, 
for example retry the connection with different parameters.


This update is trying to separate the socket problem from the TLS 
protocol or handshake problem, by using different exception classes.


Thanks,
Xuelei

Re: Code Review Request, JDK-8214339 : SSLSocketImpl erroneously wraps SocketException

[Xue-Lei Fan]

On 12/14/2018 1:46 PM, Xue-Lei Fan wrote:

Hi,

The purpose of combination of the two lines together:

     shc.conContext.fatal(Alert.UNEXPECTED_MESSAGE, "...");
     return null;    // make the complier happy

is actually for the reading of the code.  If one don't know the fatal() 
always throw an exception (compiler is one of them), he may continue 
reading the following code, and may be confusing about the code logic.


The "return" statement could never be executed actually.  It could be 
easier to catch the idea if using a format like:


    throw new SSLException("there is an alert")
-  return null;

But we need the fatal() method to wrap something more.

We used a lot comments similar to:

    return; // fatal() always throws, make the compiler happy.

and the abbreviate comment:
    return;    // make the compiler happy

misses the part "fatal() always throws", and then it may look weird.

I'm fine to remove the comment, or use the full comment instead "fatal() 
always throws, make the compiler happy".  Which one is your preference?



On 12/14/2018 11:49 AM, Jamil Nimeh wrote:
Looks pretty good.  I did have one question about a few of the methods 
in KeyShareExtension and PreSharedKeyExtension, specifically where you 
return nulls on error branches with the make-compiler-happy comments. 
In those cases would it be a bit cleaner to set a byte[] variable to 
null at the beginning of the method and then in the successful code 
path set the value to whatever byte array comes back from hmacs or 
hkdf operations?  At the end of the method you only need one return 
statement, rather than a return null on every error branch, many of 
which won't be executed due to method calls which always throw 
exceptions.
Hm, I see your points.  As the return statement could never be executed, 
it may not worthy to declare the byte[] variable earlier.


I mean that we can remove the "return" statement as it is never 
executed, if not for code reading or compiler.


In some other places, the compiler may not happy because it cannot tell 
that the fatal() throws exception.  So we may still need the "return" 
statement a little bit.  But I think most of them could be removed if we 
don't like it.


Xuelei


See above. Is it a reasonable coding style to you as well?

Thanks,
Xuelei

Not a big deal if you wish to leave things as they are since I know 
the current approach is used in many places in the code that this 
review doesn't touch.


--Jamil

On 12/14/2018 8:14 AM, Xue-Lei Fan wrote:

Hi,

Please review the update:
   http://cr.openjdk.java.net/~xuelei/8214339/webrev.00/

In some cases, the SSLProtocolException or SSLHandshakeException may 
be thrown if the underlying socket run into problems.  An application 
may depends on the exception class for further action, for example 
retry the connection with different parameters.


This update is trying to separate the socket problem from the TLS 
protocol or handshake problem, by using different exception classes.


Thanks,
Xuelei

Re: Code Review Request, JDK-8214339 : SSLSocketImpl erroneously wraps SocketException

[Xue-Lei Fan]

Hi,

The purpose of combination of the two lines together:

shc.conContext.fatal(Alert.UNEXPECTED_MESSAGE, "...");
return null;// make the complier happy

is actually for the reading of the code.  If one don't know the fatal() 
always throw an exception (compiler is one of them), he may continue 
reading the following code, and may be confusing about the code logic.


The "return" statement could never be executed actually.  It could be 
easier to catch the idea if using a format like:


   throw new SSLException("there is an alert")
-  return null;

But we need the fatal() method to wrap something more.

We used a lot comments similar to:

   return; // fatal() always throws, make the compiler happy.

and the abbreviate comment:
   return;// make the compiler happy

misses the part "fatal() always throws", and then it may look weird.

I'm fine to remove the comment, or use the full comment instead "fatal() 
always throws, make the compiler happy".  Which one is your preference?



On 12/14/2018 11:49 AM, Jamil Nimeh wrote:
Looks pretty good.  I did have one question about a few of the methods 
in KeyShareExtension and PreSharedKeyExtension, specifically where you 
return nulls on error branches with the make-compiler-happy comments. In 
those cases would it be a bit cleaner to set a byte[] variable to null 
at the beginning of the method and then in the successful code path set 
the value to whatever byte array comes back from hmacs or hkdf 
operations?  At the end of the method you only need one return 
statement, rather than a return null on every error branch, many of 
which won't be executed due to method calls which always throw 
exceptions.
Hm, I see your points.  As the return statement could never be executed, 
it may not worthy to declare the byte[] variable earlier.


See above. Is it a reasonable coding style to you as well?

Thanks,
Xuelei

Not a big deal if you wish to leave things as they are 
since I know the current approach is used in many places in the code 
that this review doesn't touch.


--Jamil

On 12/14/2018 8:14 AM, Xue-Lei Fan wrote:

Hi,

Please review the update:
   http://cr.openjdk.java.net/~xuelei/8214339/webrev.00/

In some cases, the SSLProtocolException or SSLHandshakeException may 
be thrown if the underlying socket run into problems.  An application 
may depends on the exception class for further action, for example 
retry the connection with different parameters.


This update is trying to separate the socket problem from the TLS 
protocol or handshake problem, by using different exception classes.


Thanks,
Xuelei

Re: Code Review Request, JDK-8213782: NullPointerException in sun.security.ssl.OutputRecord.changeWriteCiphers

[Xue-Lei Fan]

On 12/14/2018 12:26 PM, Anthony Scarpino wrote:

Other than my nit about the “make the compiler happy”, this all looks fine.


It makes sense to me.  I will remove the comment while pushing.


For KeyUpdate, shouldn’t it never be null given the suite and protocol are 
already known good?  I have not problem with the check to be cautious even if 
it should never happen.

Right, it should never be null in some cases. I added them is in case I 
missed some scenarios I'm not aware of.  I would like to keep them for safe.


Thanks for the review.

Xuelei


Tony


On Dec 14, 2018, at 9:00 AM, Xue-Lei Fan  wrote:

Hi,

Could I have the fix reviewed?
   http://cr.openjdk.java.net/~xuelei/8213782/webrev.00/

The SSLCipher.createReadCipher() and createWriteCipher() could return null if the cipher 
is not supported or the cipher is not available for a certain protocol version.  The 
caller should check the null value, and send back a "illegal_parameter" alert 
for such cases.

I did not add new regression test. The update is straightforward, while 
constructing an illegal handshake message for such cases is complicated.

Thanks,
Xuelei

Re: Code Review Request, JDK-8213782: NullPointerException in sun.security.ssl.OutputRecord.changeWriteCiphers

[Anthony Scarpino]

Other than my nit about the “make the compiler happy”, this all looks fine.

For KeyUpdate, shouldn’t it never be null given the suite and protocol are 
already known good?  I have not problem with the check to be cautious even if 
it should never happen. 

Tony 

> On Dec 14, 2018, at 9:00 AM, Xue-Lei Fan  wrote:
> 
> Hi,
> 
> Could I have the fix reviewed?
>   http://cr.openjdk.java.net/~xuelei/8213782/webrev.00/
> 
> The SSLCipher.createReadCipher() and createWriteCipher() could return null if 
> the cipher is not supported or the cipher is not available for a certain 
> protocol version.  The caller should check the null value, and send back a 
> "illegal_parameter" alert for such cases.
> 
> I did not add new regression test. The update is straightforward, while 
> constructing an illegal handshake message for such cases is complicated.
> 
> Thanks,
> Xuelei

Re: Code Review Request, JDK-8214339 : SSLSocketImpl erroneously wraps SocketException

[Jamil Nimeh]

Looks pretty good.  I did have one question about a few of the methods 
in KeyShareExtension and PreSharedKeyExtension, specifically where you 
return nulls on error branches with the make-compiler-happy comments.  
In those cases would it be a bit cleaner to set a byte[] variable to 
null at the beginning of the method and then in the successful code path 
set the value to whatever byte array comes back from hmacs or hkdf 
operations?  At the end of the method you only need one return 
statement, rather than a return null on every error branch, many of 
which won't be executed due to method calls which always throw 
exceptions.  Not a big deal if you wish to leave things as they are 
since I know the current approach is used in many places in the code 
that this review doesn't touch.


--Jamil

On 12/14/2018 8:14 AM, Xue-Lei Fan wrote:

Hi,

Please review the update:
   http://cr.openjdk.java.net/~xuelei/8214339/webrev.00/

In some cases, the SSLProtocolException or SSLHandshakeException may 
be thrown if the underlying socket run into problems.  An application 
may depends on the exception class for further action, for example 
retry the connection with different parameters.


This update is trying to separate the socket problem from the TLS 
protocol or handshake problem, by using different exception classes.


Thanks,
Xuelei

Code Review Request, JDK-8213782: NullPointerException in sun.security.ssl.OutputRecord.changeWriteCiphers

[Xue-Lei Fan]

Hi,

Could I have the fix reviewed?
   http://cr.openjdk.java.net/~xuelei/8213782/webrev.00/

The SSLCipher.createReadCipher() and createWriteCipher() could return 
null if the cipher is not supported or the cipher is not available for a 
certain protocol version.  The caller should check the null value, and 
send back a "illegal_parameter" alert for such cases.


I did not add new regression test. The update is straightforward, while 
constructing an illegal handshake message for such cases is complicated.


Thanks,
Xuelei

Code Review Request, JDK-8214339 : SSLSocketImpl erroneously wraps SocketException

[Xue-Lei Fan]

Hi,

Please review the update:
   http://cr.openjdk.java.net/~xuelei/8214339/webrev.00/

In some cases, the SSLProtocolException or SSLHandshakeException may be 
thrown if the underlying socket run into problems.  An application may 
depends on the exception class for further action, for example retry the 
connection with different parameters.


This update is trying to separate the socket problem from the TLS 
protocol or handshake problem, by using different exception classes.


Thanks,
Xuelei

Code Review Request, JDK-8209333 Socket reset issue for TLS 1.3 socket close

[Xue-Lei Fan]

Hi,

Please review the TLS 1.3 half-close issue in JDK.

http://cr.openjdk.java.net/~xuelei/8209333/webrev.00/

While trying to duplex close a TLS connection upon the half-close 
policy, there might be pending receiving data in the closing side, and 
result in a TCP RST during closing.  The TCP RST may then cause the peer 
reading failure.  For example:

1. client and server establish a TLS 1.3 connection.
2. server sending the post-handshake NewSessionTicket message.
3. client send the application data, and then close the connection.
4. as the client does not call to read the post-handshake message, the 
connection close will cause a TCP RST.
5. server trying to read the client application data, but the socket may 
be impacted by the TCP RST, and the reading can fail.


It would not be an issue any more if the client could read the 
post-handshake message, explicit or implicit.


I would like applications consider to use half-close policy, and moving 
away from the duplex-close policy.


The basic idea of the fix is trying to use up buffered network input 
before close the socket.  As is an implicit behavior to consume the 
post-handshake message, and mitigate the impact of it.


This fix is not a perfect one.  It is just a workaround for 
duplex-close.  I'm open to hear more ideas.


Thanks,
Xuelei

Re: Code Review Request, JDK-8213577, Update the default SSL session cache size to 20480

[Sean Mullan]

Looks good. My only question is whether the apiNote should be an 
implNote instead since it refers to what the JDK Implementation does. 
But either way seems ok.


--Sean

On 11/26/18 1:14 PM, Xue-Lei Fan wrote:

I made the update accordingly:

   http://cr.openjdk.java.net/~xuelei/8210985/webrev.04/

Thanks,

Xuelei

On 11/19/2018 7:39 AM, Sean Mullan wrote:

On 11/16/18 3:19 PM, Xuelei Fan wrote:

Thanks for the review, Jmail & Sean.

New webrev:
 http://cr.openjdk.java.net/~xuelei/8210985/webrev.03/

I will update CSR when we come to an agreement.

On 11/16/2018 11:33 AM, Sean Mullan wrote:
  122  * @apiNote Both the session timeout and cache size impact 
performance
  123  *  of future connections.  It is not recommended 
to use too big
  124  *  or too small timeout or cache size limit. 
Applications should
  125  *  carefully weigh the limits and performance for 
the specific

  126  *  circumstances.

I am not really sure if the @apiNote is that useful or appropriate,

I worry about the default value actually.


Then maybe the default is what you should be discussing in this 
apiNote. Right now I don't think the apiNote adds much. To me, all you 
are really saying is that these are methods that can be used to tune 
performance, which I think should be obvious from their name and 
description. Maybe the apiNote should say something like:


"Note that the JDK Implementation uses default values for both the 
session cache size and timeout. See getSessionCacheSize and 
getSessionTimeout for more information. Applications should consider 
their performance requirements and override the defaults if necessary."


Also I think you should add a similar @implNote for getSessionTimeout 
which describes the default value (86400 seconds or 24 hours), ex:


@implNote The JDK implementation returns the session timeout as set by 
   the {@code setSessionTimeout} method, or if not set, a default 
value of 86400 seconds (24 hours).


  A new bug may be filed again and argue if the default value is not 
a proper one.  The default value of session timeout and cache size 
really depends on the real world circumstances.  I think we'd better 
make a clarification in the spec, and remind application tune them 
accordingly.


Ok, but the apiNote above says nothing about the default value.

--Sean



but it seems a bit odd to talk about the session timeout in this 
method. 
The performance impact is a combination of the session timeout limit 
and cache size.  I would like application consider them together if 
need to tune the values, but not individually.


If you still want to add this, I would add an @apiNote to each of 
the setSessionCacheSize and setSessionTimeout methods and just 
discuss each property separately.


I added the update spec to both setSessionCacheSize and 
setSessionTimeout.


Also, unless you say what "too big" or "too small" is, I would avoid 
giving this advice.



It makes sense to me.

Thanks,
Xuelei


--Sean


On 11/16/18 1:30 PM, Xuelei Fan wrote:

It's time to use the systemProperty tag as it is ready.

As we are already there, I also update the setSessionCacheSize() 
for more clarification.


Please review both CSR and webrev:
 https://bugs.openjdk.java.net/browse/JDK-8213577
 http://cr.openjdk.java.net/~xuelei/8210985/webrev.02/

Thanks,
Xuelei

On 11/16/2018 8:19 AM, Sean Mullan wrote:

On 11/15/18 3:37 PM, Xuelei Fan wrote:

Hi Sean,

Are you OK if we do it later?  I'm waiting for the 
@systemProperty tag, proposed within JDK-5076751.  I will file a 
bug to use the tag for more cleanup.


JDK-5076751 is completed and pushed to JDK 12, so you can use the 
new tag now.


I think it would be easier to do it now, it seems pretty simple 
and that way there is no need to worry about it later.


--Sean



Thanks,
Xuelei

On 11/15/2018 11:55 AM, Sean Mullan wrote:
This is a good opportunity to document the 
javax.net.ssl.sessionCacheSize system property in the 
SSLSessionContext API (and use the new @systemProperty tag) in 
an @implNote, for example:


 /**
  * Returns the size of the cache used for storing
  * SSLSession objects grouped under this
  * SSLSessionContext.
  *
  * @implNote The JDK implementation returns the cache size 
as set by
  * the {@code setSessionCacheSize method}, or if not set, 
the value
  * of the {@systemProperty javax.net.ssl.sessionCacheSize} 
system
  * property. If neither is set, it returns a default value 
of 20480.

  *
  * @return size of the session cache; zero means there is 
no size limit.

  * @see #setSessionCacheSize
  */
 public int getSessionCacheSize();

On 11/14/18 11:59 AM, Xuelei Fan wrote:

Hi,

Please review this simple update:
http://cr.openjdk.java.net/~xuelei/8210985/webrev.00/

The default value for the maximum number of entries in the SSL 
session cache (SSLSessionContext.getSessionCacheSize()) is 
infinite now.  In the 

Re: Code Review Request, JDK-8214321: Misleading code in SSLCipher

[Anthony Scarpino]

On 11/26/18 4:14 PM, Xue-Lei Fan wrote:

Hi,

Please review this code cleanup in SSLCipher.java:
    http://cr.openjdk.java.net/~xuelei/8214321/webrev.00/

The code should be fine as readCipherGenerators.length and 
writeCipherGenerators.length are the same value in the implementation. 
However, it is misleading to use readCipherGenerators when it is 
intended to use writeCipherGenerators.  It's nice to have a code cleanup 
for readability.


Thanks,
Xuelei


looks fine.. you'll probably want to put noreg-trivial on the bug

Tony

Code Review Request, JDK-8214321: Misleading code in SSLCipher

[Xue-Lei Fan]

Hi,

Please review this code cleanup in SSLCipher.java:
   http://cr.openjdk.java.net/~xuelei/8214321/webrev.00/

The code should be fine as readCipherGenerators.length and 
writeCipherGenerators.length are the same value in the implementation. 
However, it is misleading to use readCipherGenerators when it is 
intended to use writeCipherGenerators.  It's nice to have a code cleanup 
for readability.


Thanks,
Xuelei

Re: Code Review Request, JDK-8213577, Update the default SSL session cache size to 20480

[Xue-Lei Fan]

I made the update accordingly:

  http://cr.openjdk.java.net/~xuelei/8210985/webrev.04/

Thanks,

Xuelei

On 11/19/2018 7:39 AM, Sean Mullan wrote:

On 11/16/18 3:19 PM, Xuelei Fan wrote:

Thanks for the review, Jmail & Sean.

New webrev:
 http://cr.openjdk.java.net/~xuelei/8210985/webrev.03/

I will update CSR when we come to an agreement.

On 11/16/2018 11:33 AM, Sean Mullan wrote:
  122  * @apiNote Both the session timeout and cache size impact 
performance
  123  *  of future connections.  It is not recommended 
to use too big
  124  *  or too small timeout or cache size limit. 
Applications should
  125  *  carefully weigh the limits and performance for 
the specific

  126  *  circumstances.

I am not really sure if the @apiNote is that useful or appropriate,

I worry about the default value actually.


Then maybe the default is what you should be discussing in this 
apiNote. Right now I don't think the apiNote adds much. To me, all you 
are really saying is that these are methods that can be used to tune 
performance, which I think should be obvious from their name and 
description. Maybe the apiNote should say something like:


"Note that the JDK Implementation uses default values for both the 
session cache size and timeout. See getSessionCacheSize and 
getSessionTimeout for more information. Applications should consider 
their performance requirements and override the defaults if necessary."


Also I think you should add a similar @implNote for getSessionTimeout 
which describes the default value (86400 seconds or 24 hours), ex:


@implNote The JDK implementation returns the session timeout as set by 
   the {@code setSessionTimeout} method, or if not set, a default 
value of 86400 seconds (24 hours).


  A new bug may be filed again and argue if the default value is not 
a proper one.  The default value of session timeout and cache size 
really depends on the real world circumstances.  I think we'd better 
make a clarification in the spec, and remind application tune them 
accordingly.


Ok, but the apiNote above says nothing about the default value.

--Sean



but it seems a bit odd to talk about the session timeout in this 
method. 
The performance impact is a combination of the session timeout limit 
and cache size.  I would like application consider them together if 
need to tune the values, but not individually.


If you still want to add this, I would add an @apiNote to each of 
the setSessionCacheSize and setSessionTimeout methods and just 
discuss each property separately.


I added the update spec to both setSessionCacheSize and 
setSessionTimeout.


Also, unless you say what "too big" or "too small" is, I would avoid 
giving this advice.



It makes sense to me.

Thanks,
Xuelei


--Sean


On 11/16/18 1:30 PM, Xuelei Fan wrote:

It's time to use the systemProperty tag as it is ready.

As we are already there, I also update the setSessionCacheSize() 
for more clarification.


Please review both CSR and webrev:
 https://bugs.openjdk.java.net/browse/JDK-8213577
 http://cr.openjdk.java.net/~xuelei/8210985/webrev.02/

Thanks,
Xuelei

On 11/16/2018 8:19 AM, Sean Mullan wrote:

On 11/15/18 3:37 PM, Xuelei Fan wrote:

Hi Sean,

Are you OK if we do it later?  I'm waiting for the 
@systemProperty tag, proposed within JDK-5076751.  I will file a 
bug to use the tag for more cleanup.


JDK-5076751 is completed and pushed to JDK 12, so you can use the 
new tag now.


I think it would be easier to do it now, it seems pretty simple 
and that way there is no need to worry about it later.


--Sean



Thanks,
Xuelei

On 11/15/2018 11:55 AM, Sean Mullan wrote:
This is a good opportunity to document the 
javax.net.ssl.sessionCacheSize system property in the 
SSLSessionContext API (and use the new @systemProperty tag) in 
an @implNote, for example:


 /**
  * Returns the size of the cache used for storing
  * SSLSession objects grouped under this
  * SSLSessionContext.
  *
  * @implNote The JDK implementation returns the cache size 
as set by
  * the {@code setSessionCacheSize method}, or if not set, 
the value
  * of the {@systemProperty javax.net.ssl.sessionCacheSize} 
system
  * property. If neither is set, it returns a default value 
of 20480.

  *
  * @return size of the session cache; zero means there is 
no size limit.

  * @see #setSessionCacheSize
  */
 public int getSessionCacheSize();

On 11/14/18 11:59 AM, Xuelei Fan wrote:

Hi,

Please review this simple update:
http://cr.openjdk.java.net/~xuelei/8210985/webrev.00/

The default value for the maximum number of entries in the SSL 
session cache (SSLSessionContext.getSessionCacheSize()) is 
infinite now.  In the request, the default value is updated to 
20480 for performance consideration.


For the detailed behavior update, please refer to CSR:
https://bugs.openjdk.java.net/browse/JDK-8213577

Thanks,
Xuelei

Re: Code Review Request, JDK-8213577, Update the default SSL session cache size to 20480

[Sean Mullan]

On 11/16/18 3:19 PM, Xuelei Fan wrote:

Thanks for the review, Jmail & Sean.

New webrev:
     http://cr.openjdk.java.net/~xuelei/8210985/webrev.03/

I will update CSR when we come to an agreement.

On 11/16/2018 11:33 AM, Sean Mullan wrote:
  122  * @apiNote Both the session timeout and cache size impact 
performance
  123  *  of future connections.  It is not recommended to 
use too big
  124  *  or too small timeout or cache size limit. 
Applications should
  125  *  carefully weigh the limits and performance for 
the specific

  126  *  circumstances.

I am not really sure if the @apiNote is that useful or appropriate,

I worry about the default value actually.


Then maybe the default is what you should be discussing in this apiNote. 
Right now I don't think the apiNote adds much. To me, all you are really 
saying is that these are methods that can be used to tune performance, 
which I think should be obvious from their name and description. Maybe 
the apiNote should say something like:


"Note that the JDK Implementation uses default values for both the 
session cache size and timeout. See getSessionCacheSize and 
getSessionTimeout for more information. Applications should consider 
their performance requirements and override the defaults if necessary."


Also I think you should add a similar @implNote for getSessionTimeout 
which describes the default value (86400 seconds or 24 hours), ex:


@implNote The JDK implementation returns the session timeout as set by 
   the {@code setSessionTimeout} method, or if not set, a default 
value of 86400 seconds (24 hours).


  A new bug may be filed again 
and argue if the default value is not a proper one.  The default value 
of session timeout and cache size really depends on the real world 
circumstances.  I think we'd better make a clarification in the spec, 
and remind application tune them accordingly.


Ok, but the apiNote above says nothing about the default value.

--Sean



but it seems a bit odd to talk about the session timeout in this method. 
The performance impact is a combination of the session timeout limit and 
cache size.  I would like application consider them together if need to 
tune the values, but not individually.


If you still want to add this, I would add an @apiNote to each of the 
setSessionCacheSize and setSessionTimeout methods and just discuss 
each property separately.



I added the update spec to both setSessionCacheSize and setSessionTimeout.

Also, unless you say what "too big" or "too small" is, I would avoid 
giving this advice.



It makes sense to me.

Thanks,
Xuelei


--Sean


On 11/16/18 1:30 PM, Xuelei Fan wrote:

It's time to use the systemProperty tag as it is ready.

As we are already there, I also update the setSessionCacheSize() for 
more clarification.


Please review both CSR and webrev:
 https://bugs.openjdk.java.net/browse/JDK-8213577
 http://cr.openjdk.java.net/~xuelei/8210985/webrev.02/

Thanks,
Xuelei

On 11/16/2018 8:19 AM, Sean Mullan wrote:

On 11/15/18 3:37 PM, Xuelei Fan wrote:

Hi Sean,

Are you OK if we do it later?  I'm waiting for the @systemProperty 
tag, proposed within JDK-5076751.  I will file a bug to use the tag 
for more cleanup.


JDK-5076751 is completed and pushed to JDK 12, so you can use the 
new tag now.


I think it would be easier to do it now, it seems pretty simple and 
that way there is no need to worry about it later.


--Sean



Thanks,
Xuelei

On 11/15/2018 11:55 AM, Sean Mullan wrote:
This is a good opportunity to document the 
javax.net.ssl.sessionCacheSize system property in the 
SSLSessionContext API (and use the new @systemProperty tag) in an 
@implNote, for example:


 /**
  * Returns the size of the cache used for storing
  * SSLSession objects grouped under this
  * SSLSessionContext.
  *
  * @implNote The JDK implementation returns the cache size as 
set by
  * the {@code setSessionCacheSize method}, or if not set, the 
value
  * of the {@systemProperty javax.net.ssl.sessionCacheSize} 
system
  * property. If neither is set, it returns a default value of 
20480.

  *
  * @return size of the session cache; zero means there is no 
size limit.

  * @see #setSessionCacheSize
  */
 public int getSessionCacheSize();

On 11/14/18 11:59 AM, Xuelei Fan wrote:

Hi,

Please review this simple update:
 http://cr.openjdk.java.net/~xuelei/8210985/webrev.00/

The default value for the maximum number of entries in the SSL 
session cache (SSLSessionContext.getSessionCacheSize()) is 
infinite now.  In the request, the default value is updated to 
20480 for performance consideration.


For the detailed behavior update, please refer to CSR:
 https://bugs.openjdk.java.net/browse/JDK-8213577

Thanks,
Xuelei

Re: Code Review Request, JDK-8213577, Update the default SSL session cache size to 20480

[Xuelei Fan]

Thanks for the review, Jmail & Sean.

New webrev:
http://cr.openjdk.java.net/~xuelei/8210985/webrev.03/

I will update CSR when we come to an agreement.

On 11/16/2018 11:33 AM, Sean Mullan wrote:
  122  * @apiNote Both the session timeout and cache size impact 
performance
  123  *  of future connections.  It is not recommended to 
use too big
  124  *  or too small timeout or cache size limit. 
Applications should
  125  *  carefully weigh the limits and performance for the 
specific

  126  *  circumstances.

I am not really sure if the @apiNote is that useful or appropriate,
I worry about the default value actually.  A new bug may be filed again 
and argue if the default value is not a proper one.  The default value 
of session timeout and cache size really depends on the real world 
circumstances.  I think we'd better make a clarification in the spec, 
and remind application tune them accordingly.


but 
it seems a bit odd to talk about the session timeout in this method. 
The performance impact is a combination of the session timeout limit and 
cache size.  I would like application consider them together if need to 
tune the values, but not individually.


If 
you still want to add this, I would add an @apiNote to each of the 
setSessionCacheSize and setSessionTimeout methods and just discuss each 
property separately.



I added the update spec to both setSessionCacheSize and setSessionTimeout.

Also, unless you say what "too big" or "too small" is, I would avoid 
giving this advice.



It makes sense to me.

Thanks,
Xuelei


--Sean


On 11/16/18 1:30 PM, Xuelei Fan wrote:

It's time to use the systemProperty tag as it is ready.

As we are already there, I also update the setSessionCacheSize() for 
more clarification.


Please review both CSR and webrev:
 https://bugs.openjdk.java.net/browse/JDK-8213577
 http://cr.openjdk.java.net/~xuelei/8210985/webrev.02/

Thanks,
Xuelei

On 11/16/2018 8:19 AM, Sean Mullan wrote:

On 11/15/18 3:37 PM, Xuelei Fan wrote:

Hi Sean,

Are you OK if we do it later?  I'm waiting for the @systemProperty 
tag, proposed within JDK-5076751.  I will file a bug to use the tag 
for more cleanup.


JDK-5076751 is completed and pushed to JDK 12, so you can use the new 
tag now.


I think it would be easier to do it now, it seems pretty simple and 
that way there is no need to worry about it later.


--Sean



Thanks,
Xuelei

On 11/15/2018 11:55 AM, Sean Mullan wrote:
This is a good opportunity to document the 
javax.net.ssl.sessionCacheSize system property in the 
SSLSessionContext API (and use the new @systemProperty tag) in an 
@implNote, for example:


 /**
  * Returns the size of the cache used for storing
  * SSLSession objects grouped under this
  * SSLSessionContext.
  *
  * @implNote The JDK implementation returns the cache size as 
set by
  * the {@code setSessionCacheSize method}, or if not set, the 
value

  * of the {@systemProperty javax.net.ssl.sessionCacheSize} system
  * property. If neither is set, it returns a default value of 
20480.

  *
  * @return size of the session cache; zero means there is no 
size limit.

  * @see #setSessionCacheSize
  */
 public int getSessionCacheSize();

On 11/14/18 11:59 AM, Xuelei Fan wrote:

Hi,

Please review this simple update:
 http://cr.openjdk.java.net/~xuelei/8210985/webrev.00/

The default value for the maximum number of entries in the SSL 
session cache (SSLSessionContext.getSessionCacheSize()) is 
infinite now.  In the request, the default value is updated to 
20480 for performance consideration.


For the detailed behavior update, please refer to CSR:
 https://bugs.openjdk.java.net/browse/JDK-8213577

Thanks,
Xuelei

Re: Code Review Request, JDK-8213577, Update the default SSL session cache size to 20480

[Sean Mullan]

 122  * @apiNote Both the session timeout and cache size impact 
performance
 123  *  of future connections.  It is not recommended to 
use too big
 124  *  or too small timeout or cache size limit. 
Applications should
 125  *  carefully weigh the limits and performance for the 
specific

 126  *  circumstances.

I am not really sure if the @apiNote is that useful or appropriate, but 
it seems a bit odd to talk about the session timeout in this method. If 
you still want to add this, I would add an @apiNote to each of the 
setSessionCacheSize and setSessionTimeout methods and just discuss each 
property separately.


Also, unless you say what "too big" or "too small" is, I would avoid 
giving this advice.


--Sean


On 11/16/18 1:30 PM, Xuelei Fan wrote:

It's time to use the systemProperty tag as it is ready.

As we are already there, I also update the setSessionCacheSize() for 
more clarification.


Please review both CSR and webrev:
     https://bugs.openjdk.java.net/browse/JDK-8213577
     http://cr.openjdk.java.net/~xuelei/8210985/webrev.02/

Thanks,
Xuelei

On 11/16/2018 8:19 AM, Sean Mullan wrote:

On 11/15/18 3:37 PM, Xuelei Fan wrote:

Hi Sean,

Are you OK if we do it later?  I'm waiting for the @systemProperty 
tag, proposed within JDK-5076751.  I will file a bug to use the tag 
for more cleanup.


JDK-5076751 is completed and pushed to JDK 12, so you can use the new 
tag now.


I think it would be easier to do it now, it seems pretty simple and 
that way there is no need to worry about it later.


--Sean



Thanks,
Xuelei

On 11/15/2018 11:55 AM, Sean Mullan wrote:
This is a good opportunity to document the 
javax.net.ssl.sessionCacheSize system property in the 
SSLSessionContext API (and use the new @systemProperty tag) in an 
@implNote, for example:


 /**
  * Returns the size of the cache used for storing
  * SSLSession objects grouped under this
  * SSLSessionContext.
  *
  * @implNote The JDK implementation returns the cache size as 
set by
  * the {@code setSessionCacheSize method}, or if not set, the 
value

  * of the {@systemProperty javax.net.ssl.sessionCacheSize} system
  * property. If neither is set, it returns a default value of 
20480.

  *
  * @return size of the session cache; zero means there is no 
size limit.

  * @see #setSessionCacheSize
  */
 public int getSessionCacheSize();

On 11/14/18 11:59 AM, Xuelei Fan wrote:

Hi,

Please review this simple update:
 http://cr.openjdk.java.net/~xuelei/8210985/webrev.00/

The default value for the maximum number of entries in the SSL 
session cache (SSLSessionContext.getSessionCacheSize()) is infinite 
now.  In the request, the default value is updated to 20480 for 
performance consideration.


For the detailed behavior update, please refer to CSR:
 https://bugs.openjdk.java.net/browse/JDK-8213577

Thanks,
Xuelei

Re: Code Review Request, JDK-8213577, Update the default SSL session cache size to 20480

[Jamil Nimeh]

Hi Xuelei,

A little wordsmithing, nit picky stuff (sorry for not seeing this earlier):

 * @apiNote for setSessionCacheSize: The sentence beginning, "It is not
   recommended to use too big..." needs a slight grammatical change
 o Suggestion: "It is recommended that applications tune their
   timeout and cache size values so they are neither too small nor
   too large."
 o If you decide to change the language in the CSR then change it
   in SSLSessionContext.java, too.
 * @implNote for getSessionCacheSize: bring the closing brace in from
   "{@code setSessionCacheSize method}" to put the word method outside
   the braces.
 o This should change in the code review, SSLSessionContext.java:143

The rest is fine.

--Jamil

On 11/16/2018 10:30 AM, Xuelei Fan wrote:

It's time to use the systemProperty tag as it is ready.

As we are already there, I also update the setSessionCacheSize() for 
more clarification.


Please review both CSR and webrev:
    https://bugs.openjdk.java.net/browse/JDK-8213577
    http://cr.openjdk.java.net/~xuelei/8210985/webrev.02/

Thanks,
Xuelei

On 11/16/2018 8:19 AM, Sean Mullan wrote:

On 11/15/18 3:37 PM, Xuelei Fan wrote:

Hi Sean,

Are you OK if we do it later?  I'm waiting for the @systemProperty 
tag, proposed within JDK-5076751.  I will file a bug to use the tag 
for more cleanup.


JDK-5076751 is completed and pushed to JDK 12, so you can use the new 
tag now.


I think it would be easier to do it now, it seems pretty simple and 
that way there is no need to worry about it later.


--Sean



Thanks,
Xuelei

On 11/15/2018 11:55 AM, Sean Mullan wrote:
This is a good opportunity to document the 
javax.net.ssl.sessionCacheSize system property in the 
SSLSessionContext API (and use the new @systemProperty tag) in an 
@implNote, for example:


 /**
  * Returns the size of the cache used for storing
  * SSLSession objects grouped under this
  * SSLSessionContext.
  *
  * @implNote The JDK implementation returns the cache size as 
set by
  * the {@code setSessionCacheSize method}, or if not set, the 
value

  * of the {@systemProperty javax.net.ssl.sessionCacheSize} system
  * property. If neither is set, it returns a default value of 
20480.

  *
  * @return size of the session cache; zero means there is no 
size limit.

  * @see #setSessionCacheSize
  */
 public int getSessionCacheSize();

On 11/14/18 11:59 AM, Xuelei Fan wrote:

Hi,

Please review this simple update:
 http://cr.openjdk.java.net/~xuelei/8210985/webrev.00/

The default value for the maximum number of entries in the SSL 
session cache (SSLSessionContext.getSessionCacheSize()) is 
infinite now.  In the request, the default value is updated to 
20480 for performance consideration.


For the detailed behavior update, please refer to CSR:
 https://bugs.openjdk.java.net/browse/JDK-8213577

Thanks,
Xuelei

Re: Code Review Request, JDK-8213577, Update the default SSL session cache size to 20480

[Xuelei Fan]

It's time to use the systemProperty tag as it is ready.

As we are already there, I also update the setSessionCacheSize() for 
more clarification.


Please review both CSR and webrev:
https://bugs.openjdk.java.net/browse/JDK-8213577
http://cr.openjdk.java.net/~xuelei/8210985/webrev.02/

Thanks,
Xuelei

On 11/16/2018 8:19 AM, Sean Mullan wrote:

On 11/15/18 3:37 PM, Xuelei Fan wrote:

Hi Sean,

Are you OK if we do it later?  I'm waiting for the @systemProperty 
tag, proposed within JDK-5076751.  I will file a bug to use the tag 
for more cleanup.


JDK-5076751 is completed and pushed to JDK 12, so you can use the new 
tag now.


I think it would be easier to do it now, it seems pretty simple and that 
way there is no need to worry about it later.


--Sean



Thanks,
Xuelei

On 11/15/2018 11:55 AM, Sean Mullan wrote:
This is a good opportunity to document the 
javax.net.ssl.sessionCacheSize system property in the 
SSLSessionContext API (and use the new @systemProperty tag) in an 
@implNote, for example:


 /**
  * Returns the size of the cache used for storing
  * SSLSession objects grouped under this
  * SSLSessionContext.
  *
  * @implNote The JDK implementation returns the cache size as 
set by

  * the {@code setSessionCacheSize method}, or if not set, the value
  * of the {@systemProperty javax.net.ssl.sessionCacheSize} system
  * property. If neither is set, it returns a default value of 
20480.

  *
  * @return size of the session cache; zero means there is no 
size limit.

  * @see #setSessionCacheSize
  */
 public int getSessionCacheSize();

On 11/14/18 11:59 AM, Xuelei Fan wrote:

Hi,

Please review this simple update:
 http://cr.openjdk.java.net/~xuelei/8210985/webrev.00/

The default value for the maximum number of entries in the SSL 
session cache (SSLSessionContext.getSessionCacheSize()) is infinite 
now.  In the request, the default value is updated to 20480 for 
performance consideration.


For the detailed behavior update, please refer to CSR:
 https://bugs.openjdk.java.net/browse/JDK-8213577

Thanks,
Xuelei

Re: Code Review Request, JDK-8213577, Update the default SSL session cache size to 20480

[Sean Mullan]

On 11/15/18 3:37 PM, Xuelei Fan wrote:

Hi Sean,

Are you OK if we do it later?  I'm waiting for the @systemProperty tag, 
proposed within JDK-5076751.  I will file a bug to use the tag for more 
cleanup.


JDK-5076751 is completed and pushed to JDK 12, so you can use the new 
tag now.


I think it would be easier to do it now, it seems pretty simple and that 
way there is no need to worry about it later.


--Sean



Thanks,
Xuelei

On 11/15/2018 11:55 AM, Sean Mullan wrote:
This is a good opportunity to document the 
javax.net.ssl.sessionCacheSize system property in the 
SSLSessionContext API (and use the new @systemProperty tag) in an 
@implNote, for example:


 /**
  * Returns the size of the cache used for storing
  * SSLSession objects grouped under this
  * SSLSessionContext.
  *
  * @implNote The JDK implementation returns the cache size as set by
  * the {@code setSessionCacheSize method}, or if not set, the value
  * of the {@systemProperty javax.net.ssl.sessionCacheSize} system
  * property. If neither is set, it returns a default value of 20480.
  *
  * @return size of the session cache; zero means there is no size 
limit.

  * @see #setSessionCacheSize
  */
 public int getSessionCacheSize();

On 11/14/18 11:59 AM, Xuelei Fan wrote:

Hi,

Please review this simple update:
 http://cr.openjdk.java.net/~xuelei/8210985/webrev.00/

The default value for the maximum number of entries in the SSL 
session cache (SSLSessionContext.getSessionCacheSize()) is infinite 
now.  In the request, the default value is updated to 20480 for 
performance consideration.


For the detailed behavior update, please refer to CSR:
 https://bugs.openjdk.java.net/browse/JDK-8213577

Thanks,
Xuelei

Re: Code Review Request, JDK-8213577, Update the default SSL session cache size to 20480

[Xuelei Fan]

Hi Sean,

Are you OK if we do it later?  I'm waiting for the @systemProperty tag, 
proposed within JDK-5076751.  I will file a bug to use the tag for more 
cleanup.


Thanks,
Xuelei

On 11/15/2018 11:55 AM, Sean Mullan wrote:
This is a good opportunity to document the 
javax.net.ssl.sessionCacheSize system property in the SSLSessionContext 
API (and use the new @systemProperty tag) in an @implNote, for example:


     /**
  * Returns the size of the cache used for storing
  * SSLSession objects grouped under this
  * SSLSessionContext.
  *
  * @implNote The JDK implementation returns the cache size as set by
  * the {@code setSessionCacheSize method}, or if not set, the value
  * of the {@systemProperty javax.net.ssl.sessionCacheSize} system
  * property. If neither is set, it returns a default value of 20480.
  *
  * @return size of the session cache; zero means there is no size 
limit.

  * @see #setSessionCacheSize
  */
     public int getSessionCacheSize();

On 11/14/18 11:59 AM, Xuelei Fan wrote:

Hi,

Please review this simple update:
 http://cr.openjdk.java.net/~xuelei/8210985/webrev.00/

The default value for the maximum number of entries in the SSL session 
cache (SSLSessionContext.getSessionCacheSize()) is infinite now.  In 
the request, the default value is updated to 20480 for performance 
consideration.


For the detailed behavior update, please refer to CSR:
 https://bugs.openjdk.java.net/browse/JDK-8213577

Thanks,
Xuelei

Re: Code Review Request, JDK-8213577, Update the default SSL session cache size to 20480

[Sean Mullan]

This is a good opportunity to document the 
javax.net.ssl.sessionCacheSize system property in the SSLSessionContext 
API (and use the new @systemProperty tag) in an @implNote, for example:


/**
 * Returns the size of the cache used for storing
 * SSLSession objects grouped under this
 * SSLSessionContext.
 *
 * @implNote The JDK implementation returns the cache size as set by
 * the {@code setSessionCacheSize method}, or if not set, the value
 * of the {@systemProperty javax.net.ssl.sessionCacheSize} system
 * property. If neither is set, it returns a default value of 20480.
 *
 * @return size of the session cache; zero means there is no size 
limit.

 * @see #setSessionCacheSize
 */
public int getSessionCacheSize();

On 11/14/18 11:59 AM, Xuelei Fan wrote:

Hi,

Please review this simple update:
     http://cr.openjdk.java.net/~xuelei/8210985/webrev.00/

The default value for the maximum number of entries in the SSL session 
cache (SSLSessionContext.getSessionCacheSize()) is infinite now.  In the 
request, the default value is updated to 20480 for performance 
consideration.


For the detailed behavior update, please refer to CSR:
     https://bugs.openjdk.java.net/browse/JDK-8213577

Thanks,
Xuelei

Re: Code Review Request, JDK-8213577, Update the default SSL session cache size to 20480

[Xuelei Fan]

On 11/14/2018 9:16 AM, Jamil Nimeh wrote:

Hi Xuelei,

The fix looks fine to me.  I think it would be good to have an else 
branch off of the check on line 205 so any < 0 value has a warning log 
entry stating that an invalid value was detected and the cache is 
getting set to DEFAULT_MAX_CACHE_SIZE.



Good point!

I updated with warnings of invalid property:
http://cr.openjdk.java.net/~xuelei/8210985/webrev.01/

Xuelei


--Jamil

On 11/14/2018 8:59 AM, Xuelei Fan wrote:

Hi,

Please review this simple update:
    http://cr.openjdk.java.net/~xuelei/8210985/webrev.00/

The default value for the maximum number of entries in the SSL session 
cache (SSLSessionContext.getSessionCacheSize()) is infinite now.  In 
the request, the default value is updated to 20480 for performance 
consideration.


For the detailed behavior update, please refer to CSR:
    https://bugs.openjdk.java.net/browse/JDK-8213577

Thanks,
Xuelei

Re: Code Review Request, JDK-8213577, Update the default SSL session cache size to 20480

[Jamil Nimeh]

Hi Xuelei,

The fix looks fine to me.  I think it would be good to have an else 
branch off of the check on line 205 so any < 0 value has a warning log 
entry stating that an invalid value was detected and the cache is 
getting set to DEFAULT_MAX_CACHE_SIZE.


--Jamil

On 11/14/2018 8:59 AM, Xuelei Fan wrote:

Hi,

Please review this simple update:
    http://cr.openjdk.java.net/~xuelei/8210985/webrev.00/

The default value for the maximum number of entries in the SSL session 
cache (SSLSessionContext.getSessionCacheSize()) is infinite now.  In 
the request, the default value is updated to 20480 for performance 
consideration.


For the detailed behavior update, please refer to CSR:
    https://bugs.openjdk.java.net/browse/JDK-8213577

Thanks,
Xuelei

Code Review Request, JDK-8213577, Update the default SSL session cache size to 20480

[Xuelei Fan]

Hi,

Please review this simple update:
http://cr.openjdk.java.net/~xuelei/8210985/webrev.00/

The default value for the maximum number of entries in the SSL session 
cache (SSLSessionContext.getSessionCacheSize()) is infinite now.  In the 
request, the default value is updated to 20480 for performance 
consideration.


For the detailed behavior update, please refer to CSR:
https://bugs.openjdk.java.net/browse/JDK-8213577

Thanks,
Xuelei

Re: Code Review Request : JDK-8213694 : Test Timeout.java should run in othervm mode

[Jamil Nimeh]

Looks fine to me.

On 11/9/2018 8:43 AM, Xuelei Fan wrote:

Hi,

Could I get the following small change reviewed please?
   http://cr.openjdk.java.net/~xuelei/8213694/webrev.00/

The test SSLSessionContextImpl/Timeout.java is running in the default 
mode. As the test initializes the SSLContext with the current System 
Properties, while the SunJSSE provider does not support dynamic System 
Properties, this test may impact other test cases running in 
samevm/agentvm mode.  The impact is very hard to debugging.


I updated the test to run in othervm mode, and cleanup the code by 
removing commented out codes and close the server socket with a 
try-with-resource.


Thanks,
Xuelei

Code Review Request : JDK-8213694 : Test Timeout.java should run in othervm mode

[Xuelei Fan]

Hi,

Could I get the following small change reviewed please?
   http://cr.openjdk.java.net/~xuelei/8213694/webrev.00/

The test SSLSessionContextImpl/Timeout.java is running in the default 
mode. As the test initializes the SSLContext with the current System 
Properties, while the SunJSSE provider does not support dynamic System 
Properties, this test may impact other test cases running in 
samevm/agentvm mode.  The impact is very hard to debugging.


I updated the test to run in othervm mode, and cleanup the code by 
removing commented out codes and close the server socket with a 
try-with-resource.


Thanks,
Xuelei

Re: Code Review Request, JDK-8212738, Incorrectly named signature scheme ecdsa_secp512r1_sha512

[Anthony Scarpino]

Looks good to me

Tony

On 10/29/2018 10:41 AM, Xuelei Fan wrote:

Hi,

Please review the update:

     http://cr.openjdk.java.net/~xuelei/8212738/webrev.00/

The signature algorithm name should be ""ecdsa_secp521r1_sha512", 
instead of "ecdsa_secp512r1_sha512".


No new regression test.  Trivial update, no impact on the behaviors 
except the debug log message.


Thanks,
Xuelei

Code Review Request, JDK-8212738, Incorrectly named signature scheme ecdsa_secp512r1_sha512

[Xuelei Fan]

Hi,

Please review the update:

http://cr.openjdk.java.net/~xuelei/8212738/webrev.00/

The signature algorithm name should be ""ecdsa_secp521r1_sha512", 
instead of "ecdsa_secp512r1_sha512".


No new regression test.  Trivial update, no impact on the behaviors 
except the debug log message.


Thanks,
Xuelei

Re: Code Review Request, JDK-8210974 : No extensions debug log for ClientHello

[Bradford Wetmore]

Ditto.

Brad


On 9/20/2018 1:03 PM, Jamil Nimeh wrote:

Looks good.

On 9/20/2018 1:02 PM, Xuelei Fan wrote:

Hi,

Please review this simple fix for SunJSSE debug log:
  http://cr.openjdk.java.net/~xuelei/8210974/webrev.00/

The debug log for ClientHello message does not appear in JDK 12. 
Trivial update, no new regression test.


Thanks,
Xuelei

Re: Code Review Request, JDK-8210974 : No extensions debug log for ClientHello

[Jamil Nimeh]

Looks good.

On 9/20/2018 1:02 PM, Xuelei Fan wrote:

Hi,

Please review this simple fix for SunJSSE debug log:
  http://cr.openjdk.java.net/~xuelei/8210974/webrev.00/

The debug log for ClientHello message does not appear in JDK 12. 
Trivial update, no new regression test.


Thanks,
Xuelei

Code Review Request, JDK-8210974 : No extensions debug log for ClientHello

[Xuelei Fan]

Hi,

Please review this simple fix for SunJSSE debug log:
  http://cr.openjdk.java.net/~xuelei/8210974/webrev.00/

The debug log for ClientHello message does not appear in JDK 12. 
Trivial update, no new regression test.


Thanks,
Xuelei

Re: Code Review Request, JDK-8209916 : NPE in SupportedGroupsExtension

[Bradford Wetmore]

Looks ok to me too.

Brad


On 9/11/2018 8:43 PM, Jamil Nimeh wrote:

Looks good to me.

Thanks,
--Jamil

On 9/11/2018 7:22 PM, Xuelei Fan wrote:

Hi Jamil,

Would you please review the fix for the NPE issue:
   http://cr.openjdk.java.net/~xuelei/8209916/webrev.00/

The issue may happen if the client supports a SunJSSE provider known 
but not supported named group.


Thanks,
Xuelei

Re: Code Review Request, JDK-8209916 : NPE in SupportedGroupsExtension

[Jamil Nimeh]

Looks good to me.

Thanks,
--Jamil

On 9/11/2018 7:22 PM, Xuelei Fan wrote:

Hi Jamil,

Would you please review the fix for the NPE issue:
   http://cr.openjdk.java.net/~xuelei/8209916/webrev.00/

The issue may happen if the client supports a SunJSSE provider known 
but not supported named group.


Thanks,
Xuelei

Re: Code Review Request, JDK-8209916 : NPE in SupportedGroupsExtension

[Jamil Nimeh]

Yes I will take a look at this tonight.


--Jamil
 Original message From: Xuelei Fan  
Date: 9/11/18  7:22 PM  (GMT-08:00) To: security-dev@openjdk.java.net, Jamil 
Nimeh  Subject: Code Review Request, JDK-8209916 : 
NPE in SupportedGroupsExtension 
Hi Jamil,

Would you please review the fix for the NPE issue:
    http://cr.openjdk.java.net/~xuelei/8209916/webrev.00/

The issue may happen if the client supports a SunJSSE provider known but 
not supported named group.

Thanks,
Xuelei

Code Review Request, JDK-8209916 : NPE in SupportedGroupsExtension

[Xuelei Fan]

Hi Jamil,

Would you please review the fix for the NPE issue:
   http://cr.openjdk.java.net/~xuelei/8209916/webrev.00/

The issue may happen if the client supports a SunJSSE provider known but 
not supported named group.


Thanks,
Xuelei

Re: Code Review Request, JDK-8210334, TLS 1.3 server fails if ClientHello doesn't have pre_shared_key and psk_key_exchange_modes

[Anthony Scarpino]

Looks fine

> On Sep 5, 2018, at 11:01 AM, Xuelei Fan  wrote:
> 
> Hi,
> 
> Please review:
>http://cr.openjdk.java.net/~xuelei/8210334/webrev.00/
> 
> Simple update, no new regression test.
> 
> Thanks,
> Xuelei

Code Review Request, JDK-8210334, TLS 1.3 server fails if ClientHello doesn't have pre_shared_key and psk_key_exchange_modes

[Xuelei Fan]

Hi,

Please review:
http://cr.openjdk.java.net/~xuelei/8210334/webrev.00/

Simple update, no new regression test.

Thanks,
Xuelei

Re: Code Review Request JDK-8209965 : The "supported_groups" extension in ServerHellos

[Anthony Scarpino]

Thanks for considering the idea

Tony 

> On Aug 27, 2018, at 7:12 AM, Xuelei Fan  wrote:
> 
> Hi Tony,
> 
> I thought about to limit the workaround to TLS 1.2 and prior version. 
> However, just as you noticed that the implementation is not effective as it 
> is needed to wait and check for the supported_versions extension if it 
> presents.  As may make the workaround a lot complicated.  I would prefer to a 
> simple change for now.
> 
> Thanks,
> Xuelei
> 
>> On 8/26/2018 2:35 PM, Anthony Scarpino wrote:
>> The change looks fine but I have a question about restricting it.
>> This sounds like a problem with servers using 1.2 or before, does it make 
>> sense to throw an error for 1.3? I don’t like allowing buggy implementation 
>> to continue because we will never be able to undo this workaround.  It would 
>> be nice if when someday 1.2 is removed, this change won’t persist in our 
>> code base. I realize this maybe a lot to ask given the decision of the 
>> protocol version hasn’t been made yet I believe.  If it’s unreasonable, that 
>> is ok. I’m fine with the change as is.
>> Tony
>>> On Aug 26, 2018, at 7:39 AM, Xuelei Fan  wrote:
>>> 
>>> Hi,
>>> 
>>> Please review a compatibility fix for SunJSSE provider:
>>>http://cr.openjdk.java.net/~xuelei/8209965/webrev.00
>>> 
>>> There are servers that send the supported_groups extension in the 
>>> ServerHello handshake message.  It does not comply to the specification.  
>>> However, as there are a few deployments already with the buggy 
>>> implementation, we may want to tolerate this behavior in JDK.
>>> 
>>> Note that although this extension is allowed in the ServerHello, it should 
>>> be ignored and have no impact on the client behavior.
>>> 
>>> The problem was reported and the fix was tested in OopenJDK:
>>> http://mail.openjdk.java.net/pipermail/security-dev/2018-August/018005.html
>>> 
>>> 
>>> Thanks,
>>> Xuelei

Re: Code Review Request JDK-8209965 : The "supported_groups" extension in ServerHellos

[Xuelei Fan]

Hi Tony,

I thought about to limit the workaround to TLS 1.2 and prior version. 
However, just as you noticed that the implementation is not effective as 
it is needed to wait and check for the supported_versions extension if 
it presents.  As may make the workaround a lot complicated.  I would 
prefer to a simple change for now.


Thanks,
Xuelei

On 8/26/2018 2:35 PM, Anthony Scarpino wrote:

The change looks fine but I have a question about restricting it.

This sounds like a problem with servers using 1.2 or before, does it make sense 
to throw an error for 1.3? I don’t like allowing buggy implementation to 
continue because we will never be able to undo this workaround.  It would be 
nice if when someday 1.2 is removed, this change won’t persist in our code 
base. I realize this maybe a lot to ask given the decision of the protocol 
version hasn’t been made yet I believe.  If it’s unreasonable, that is ok. I’m 
fine with the change as is.

Tony


On Aug 26, 2018, at 7:39 AM, Xuelei Fan  wrote:

Hi,

Please review a compatibility fix for SunJSSE provider:
http://cr.openjdk.java.net/~xuelei/8209965/webrev.00

There are servers that send the supported_groups extension in the ServerHello 
handshake message.  It does not comply to the specification.  However, as there 
are a few deployments already with the buggy implementation, we may want to 
tolerate this behavior in JDK.

Note that although this extension is allowed in the ServerHello, it should be 
ignored and have no impact on the client behavior.

The problem was reported and the fix was tested in OopenJDK:
http://mail.openjdk.java.net/pipermail/security-dev/2018-August/018005.html


Thanks,
Xuelei

Re: Code Review Request JDK-8209965 : The "supported_groups" extension in ServerHellos

[Anthony Scarpino]

The change looks fine but I have a question about restricting it. 

This sounds like a problem with servers using 1.2 or before, does it make sense 
to throw an error for 1.3? I don’t like allowing buggy implementation to 
continue because we will never be able to undo this workaround.  It would be 
nice if when someday 1.2 is removed, this change won’t persist in our code 
base. I realize this maybe a lot to ask given the decision of the protocol 
version hasn’t been made yet I believe.  If it’s unreasonable, that is ok. I’m 
fine with the change as is. 

Tony

> On Aug 26, 2018, at 7:39 AM, Xuelei Fan  wrote:
> 
> Hi,
> 
> Please review a compatibility fix for SunJSSE provider:
>http://cr.openjdk.java.net/~xuelei/8209965/webrev.00
> 
> There are servers that send the supported_groups extension in the ServerHello 
> handshake message.  It does not comply to the specification.  However, as 
> there are a few deployments already with the buggy implementation, we may 
> want to tolerate this behavior in JDK.
> 
> Note that although this extension is allowed in the ServerHello, it should be 
> ignored and have no impact on the client behavior.
> 
> The problem was reported and the fix was tested in OopenJDK:
> http://mail.openjdk.java.net/pipermail/security-dev/2018-August/018005.html
> 
> 
> Thanks,
> Xuelei

Re: Code Review Request, JDK-8207009 SSLEngine#closeInbound mentions SSLException when no close_notify is received

[Xue-Lei Fan]

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.

Re: Code Review Request, JDK-8207009 SSLEngine#closeInbound mentions SSLException when no close_notify is received

[Bradford Wetmore]

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:


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 

Re: Code Review Request, JDK-8207009 SSLEngine#closeInbound mentions SSLException when no close_notify is received

[Jamil Nimeh]

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.

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:


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

Re: Code Review Request, JDK-8207009 SSLEngine#closeInbound mentions SSLException when no close_notify is received

[Xue-Lei Fan]

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:


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

Re: Code Review Request, JDK-8207009 SSLEngine#closeInbound mentions SSLException when no close_notify is received

[Jamil Nimeh]

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:


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

Re: Code Review Request, JDK-8208166, Still unable to use custom SSLEngine with default TrustManagerFactory after JDK-8207029

[Xuelei Fan]

I'm waiting for the CSR approval for JDK 11:
https://bugs.openjdk.java.net/browse/JDK-8208526

Thanks,
Xuelei

On 8/9/2018 6:50 AM, Norman Maurer wrote:

I there any idea when the patch will be merged and a release will be cut so we 
can enable testing with JDK11 again for netty ?



On 31. Jul 2018, at 07:19, Norman Maurer  wrote:

After digging more this morning I noticed the test code did made some wrong 
assumptions which just worked out of luck before. After fixing the test 
everything passes now.

So +1 from me on the patch :)

Also sorry for the false-alarm.

Niorman



On 30. Jul 2018, at 22:23, Xuelei Fan  wrote:

Would you mind send me the debug log (-Djavax.net.debug=all) and the exception stacks?  
The "renegotiation" in TLS 1.3 is different from TLS 1.2 and prior 
specifications.  It would be helpful to me to find the cause of the test failure.

Thanks,
Xuelei

On 7/30/2018 1:11 PM, Norman Maurer wrote:

Sorry but I just noticed we still have a another integration test failing which 
tests that client SSL renegotiation is failing. This seems to be not the case 
anymore with java11 + your patch (it was in ea20 tho).
https://github.com/netty/netty/blob/netty-4.1.28.Final/testsuite/src/main/java/io/netty/testsuite/transport/socket/SocketSslClientRenegotiateTest.java
Let me know if I need to dig more into it.
Bye
Norman

On 30. Jul 2018, at 21:54, Norman Maurer mailto:norman.mau...@googlemail.com>> wrote:

Hey Xuelei,

I just re-ran our testsuite with your patch and everything pass except two 
tests. After digging a bit I found that we needed to add explicit calls to 
`SSLEngine.setUSeClientMode(false)` now in these test where we did not need to 
do this before.

The tests in question are:

https://github.com/netty/netty/blob/netty-4.1.28.Final/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java#L400
https://github.com/netty/netty/blob/netty-4.1.28.Final/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java#L418

Here we use SslContext.getDefault().createSSLEngine() and did not set the mode 
explicitly before. With the following patch to netty all works when using your 
patch:

diff --git a/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java 
b/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java
index e982b6a63..40d6e7b59 100644
--- a/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java
+++ b/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java
@@ -398,7 +398,9 @@ public class SslHandlerTest {
 @Test
 public void testCloseFutureNotified() throws Exception {
-SslHandler handler = new 
SslHandler(SSLContext.getDefault().createSSLEngine());
+SSLEngine engine = SSLContext.getDefault().createSSLEngine();
+engine.setUseClientMode(false);
+SslHandler handler = new SslHandler(engine);
 EmbeddedChannel ch = new EmbeddedChannel(handler);
 ch.close();
@@ -417,6 +419,7 @@ public class SslHandlerTest {
 @Test(timeout = 5000)
 public void testEventsFired() throws Exception {
 SSLEngine engine = SSLContext.getDefault().createSSLEngine();
+engine.setUseClientMode(false);
 final BlockingQueue events = new 
LinkedBlockingQueue();
 EmbeddedChannel channel = new EmbeddedChannel(new SslHandler(engine), 
new ChannelInboundHandlerAdapter() {
 @Override


The exception we see without the patch is:

java.lang.IllegalStateException: Client/Server mode has not yet been set.
at 
java.base/sun.security.ssl.SSLEngineImpl.beginHandshake(SSLEngineImpl.java:98)
at io.netty.handler.ssl.SslHandler.handshake(SslHandler.java:1731)
at 
io.netty.handler.ssl.SslHandler.startHandshakeProcessing(SslHandler.java:1644)
at io.netty.handler.ssl.SslHandler.handlerAdded(SslHandler.java:1634)
at 
io.netty.channel.DefaultChannelPipeline.callHandlerAdded0(DefaultChannelPipeline.java:637)
at 
io.netty.channel.DefaultChannelPipeline.addLast(DefaultChannelPipeline.java:235)
at 
io.netty.channel.DefaultChannelPipeline.addLast(DefaultChannelPipeline.java:409)
at 
io.netty.channel.DefaultChannelPipeline.addLast(DefaultChannelPipeline.java:396)
at 
io.netty.channel.embedded.EmbeddedChannel$2.initChannel(EmbeddedChannel.java:203)
at io.netty.channel.ChannelInitializer.initChannel(ChannelInitializer.java:115)
at io.netty.channel.ChannelInitializer.handlerAdded(ChannelInitializer.java:107)
at 
io.netty.channel.DefaultChannelPipeline.callHandlerAdded0(DefaultChannelPipeline.java:637)
at 
io.netty.channel.DefaultChannelPipeline.access$000(DefaultChannelPipeline.java:46)
at 
io.netty.channel.DefaultChannelPipeline$PendingHandlerAddedTask.execute(DefaultChannelPipeline.java:1487)
at 
io.netty.channel.DefaultChannelPipeline.callHandlerAddedForAllHandlers(DefaultChannelPipeline.java:1161)
at 
io.netty.channel.DefaultChannelPipeline.invokeHandlerAddedIfNeeded(DefaultChannelPipeline.java:686)
at 
io.netty.channel.AbstractChannel$AbstractUnsafe.register0(AbstractChannel.java:510)
at 

Re: Code Review Request, JDK-8208166, Still unable to use custom SSLEngine with default TrustManagerFactory after JDK-8207029

[Norman Maurer]

I there any idea when the patch will be merged and a release will be cut so we 
can enable testing with JDK11 again for netty ?


> On 31. Jul 2018, at 07:19, Norman Maurer  wrote:
> 
> After digging more this morning I noticed the test code did made some wrong 
> assumptions which just worked out of luck before. After fixing the test 
> everything passes now.
> 
> So +1 from me on the patch :)
> 
> Also sorry for the false-alarm.
> 
> Niorman
> 
> 
>> On 30. Jul 2018, at 22:23, Xuelei Fan  wrote:
>> 
>> Would you mind send me the debug log (-Djavax.net.debug=all) and the 
>> exception stacks?  The "renegotiation" in TLS 1.3 is different from TLS 1.2 
>> and prior specifications.  It would be helpful to me to find the cause of 
>> the test failure.
>> 
>> Thanks,
>> Xuelei
>> 
>> On 7/30/2018 1:11 PM, Norman Maurer wrote:
>>> Sorry but I just noticed we still have a another integration test failing 
>>> which tests that client SSL renegotiation is failing. This seems to be not 
>>> the case anymore with java11 + your patch (it was in ea20 tho).
>>> https://github.com/netty/netty/blob/netty-4.1.28.Final/testsuite/src/main/java/io/netty/testsuite/transport/socket/SocketSslClientRenegotiateTest.java
>>> Let me know if I need to dig more into it.
>>> Bye
>>> Norman
 On 30. Jul 2018, at 21:54, Norman Maurer >>> > wrote:
 
 Hey Xuelei,
 
 I just re-ran our testsuite with your patch and everything pass except two 
 tests. After digging a bit I found that we needed to add explicit calls to 
 `SSLEngine.setUSeClientMode(false)` now in these test where we did not 
 need to do this before.
 
 The tests in question are:
 
 https://github.com/netty/netty/blob/netty-4.1.28.Final/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java#L400
 https://github.com/netty/netty/blob/netty-4.1.28.Final/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java#L418
 
 Here we use SslContext.getDefault().createSSLEngine() and did not set the 
 mode explicitly before. With the following patch to netty all works when 
 using your patch:
 
 diff --git 
 a/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java 
 b/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java
 index e982b6a63..40d6e7b59 100644
 --- a/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java
 +++ b/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java
 @@ -398,7 +398,9 @@ public class SslHandlerTest {
 @Test
 public void testCloseFutureNotified() throws Exception {
 -SslHandler handler = new 
 SslHandler(SSLContext.getDefault().createSSLEngine());
 +SSLEngine engine = SSLContext.getDefault().createSSLEngine();
 +engine.setUseClientMode(false);
 +SslHandler handler = new SslHandler(engine);
 EmbeddedChannel ch = new EmbeddedChannel(handler);
 ch.close();
 @@ -417,6 +419,7 @@ public class SslHandlerTest {
 @Test(timeout = 5000)
 public void testEventsFired() throws Exception {
 SSLEngine engine = SSLContext.getDefault().createSSLEngine();
 +engine.setUseClientMode(false);
 final BlockingQueue events = new 
 LinkedBlockingQueue();
 EmbeddedChannel channel = new EmbeddedChannel(new 
 SslHandler(engine), new ChannelInboundHandlerAdapter() {
 @Override
 
 
 The exception we see without the patch is:
 
 java.lang.IllegalStateException: Client/Server mode has not yet been set.
 at 
 java.base/sun.security.ssl.SSLEngineImpl.beginHandshake(SSLEngineImpl.java:98)
 at io.netty.handler.ssl.SslHandler.handshake(SslHandler.java:1731)
 at 
 io.netty.handler.ssl.SslHandler.startHandshakeProcessing(SslHandler.java:1644)
 at io.netty.handler.ssl.SslHandler.handlerAdded(SslHandler.java:1634)
 at 
 io.netty.channel.DefaultChannelPipeline.callHandlerAdded0(DefaultChannelPipeline.java:637)
 at 
 io.netty.channel.DefaultChannelPipeline.addLast(DefaultChannelPipeline.java:235)
 at 
 io.netty.channel.DefaultChannelPipeline.addLast(DefaultChannelPipeline.java:409)
 at 
 io.netty.channel.DefaultChannelPipeline.addLast(DefaultChannelPipeline.java:396)
 at 
 io.netty.channel.embedded.EmbeddedChannel$2.initChannel(EmbeddedChannel.java:203)
 at 
 io.netty.channel.ChannelInitializer.initChannel(ChannelInitializer.java:115)
 at 
 io.netty.channel.ChannelInitializer.handlerAdded(ChannelInitializer.java:107)
 at 
 io.netty.channel.DefaultChannelPipeline.callHandlerAdded0(DefaultChannelPipeline.java:637)
 at 
 io.netty.channel.DefaultChannelPipeline.access$000(DefaultChannelPipeline.java:46)
 at 
 io.netty.channel.DefaultChannelPipeline$PendingHandlerAddedTask.execute(DefaultChannelPipeline.java:1487)
 at 
 

Re: Code Review Request, JDK-8207009 SSLEngine#closeInbound mentions SSLException when no close_notify is received

[Xuelei Fan]

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:


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

Re: Code Review Request, JDK-8207009 SSLEngine#closeInbound mentions SSLException when no close_notify is received

[Jamil Nimeh]

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:


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

Re: Code Review Request, JDK-8207009 SSLEngine#closeInbound mentions SSLException when no close_notify is received

[Xuelei Fan]

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:


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

Re: Code Review Request, JDK-8207009 SSLEngine#closeInbound mentions SSLException when no close_notify is received

[Xuelei Fan]

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:


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

Re: Code Review Request, JDK-8208166, Still unable to use custom SSLEngine with default TrustManagerFactory after JDK-8207029

[Norman Maurer]

After digging more this morning I noticed the test code did made some wrong 
assumptions which just worked out of luck before. After fixing the test 
everything passes now.

So +1 from me on the patch :)

Also sorry for the false-alarm.

Niorman


> On 30. Jul 2018, at 22:23, Xuelei Fan  wrote:
> 
> Would you mind send me the debug log (-Djavax.net.debug=all) and the 
> exception stacks?  The "renegotiation" in TLS 1.3 is different from TLS 1.2 
> and prior specifications.  It would be helpful to me to find the cause of the 
> test failure.
> 
> Thanks,
> Xuelei
> 
> On 7/30/2018 1:11 PM, Norman Maurer wrote:
>> Sorry but I just noticed we still have a another integration test failing 
>> which tests that client SSL renegotiation is failing. This seems to be not 
>> the case anymore with java11 + your patch (it was in ea20 tho).
>> https://github.com/netty/netty/blob/netty-4.1.28.Final/testsuite/src/main/java/io/netty/testsuite/transport/socket/SocketSslClientRenegotiateTest.java
>> Let me know if I need to dig more into it.
>> Bye
>> Norman
>>> On 30. Jul 2018, at 21:54, Norman Maurer >> > wrote:
>>> 
>>> Hey Xuelei,
>>> 
>>> I just re-ran our testsuite with your patch and everything pass except two 
>>> tests. After digging a bit I found that we needed to add explicit calls to 
>>> `SSLEngine.setUSeClientMode(false)` now in these test where we did not need 
>>> to do this before.
>>> 
>>> The tests in question are:
>>> 
>>> https://github.com/netty/netty/blob/netty-4.1.28.Final/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java#L400
>>> https://github.com/netty/netty/blob/netty-4.1.28.Final/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java#L418
>>> 
>>> Here we use SslContext.getDefault().createSSLEngine() and did not set the 
>>> mode explicitly before. With the following patch to netty all works when 
>>> using your patch:
>>> 
>>> diff --git a/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java 
>>> b/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java
>>> index e982b6a63..40d6e7b59 100644
>>> --- a/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java
>>> +++ b/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java
>>> @@ -398,7 +398,9 @@ public class SslHandlerTest {
>>>  @Test
>>>  public void testCloseFutureNotified() throws Exception {
>>> -SslHandler handler = new 
>>> SslHandler(SSLContext.getDefault().createSSLEngine());
>>> +SSLEngine engine = SSLContext.getDefault().createSSLEngine();
>>> +engine.setUseClientMode(false);
>>> +SslHandler handler = new SslHandler(engine);
>>>  EmbeddedChannel ch = new EmbeddedChannel(handler);
>>>  ch.close();
>>> @@ -417,6 +419,7 @@ public class SslHandlerTest {
>>>  @Test(timeout = 5000)
>>>  public void testEventsFired() throws Exception {
>>>  SSLEngine engine = SSLContext.getDefault().createSSLEngine();
>>> +engine.setUseClientMode(false);
>>>  final BlockingQueue events = new 
>>> LinkedBlockingQueue();
>>>  EmbeddedChannel channel = new EmbeddedChannel(new 
>>> SslHandler(engine), new ChannelInboundHandlerAdapter() {
>>>  @Override
>>> 
>>> 
>>> The exception we see without the patch is:
>>> 
>>> java.lang.IllegalStateException: Client/Server mode has not yet been set.
>>> at 
>>> java.base/sun.security.ssl.SSLEngineImpl.beginHandshake(SSLEngineImpl.java:98)
>>> at io.netty.handler.ssl.SslHandler.handshake(SslHandler.java:1731)
>>> at 
>>> io.netty.handler.ssl.SslHandler.startHandshakeProcessing(SslHandler.java:1644)
>>> at io.netty.handler.ssl.SslHandler.handlerAdded(SslHandler.java:1634)
>>> at 
>>> io.netty.channel.DefaultChannelPipeline.callHandlerAdded0(DefaultChannelPipeline.java:637)
>>> at 
>>> io.netty.channel.DefaultChannelPipeline.addLast(DefaultChannelPipeline.java:235)
>>> at 
>>> io.netty.channel.DefaultChannelPipeline.addLast(DefaultChannelPipeline.java:409)
>>> at 
>>> io.netty.channel.DefaultChannelPipeline.addLast(DefaultChannelPipeline.java:396)
>>> at 
>>> io.netty.channel.embedded.EmbeddedChannel$2.initChannel(EmbeddedChannel.java:203)
>>> at 
>>> io.netty.channel.ChannelInitializer.initChannel(ChannelInitializer.java:115)
>>> at 
>>> io.netty.channel.ChannelInitializer.handlerAdded(ChannelInitializer.java:107)
>>> at 
>>> io.netty.channel.DefaultChannelPipeline.callHandlerAdded0(DefaultChannelPipeline.java:637)
>>> at 
>>> io.netty.channel.DefaultChannelPipeline.access$000(DefaultChannelPipeline.java:46)
>>> at 
>>> io.netty.channel.DefaultChannelPipeline$PendingHandlerAddedTask.execute(DefaultChannelPipeline.java:1487)
>>> at 
>>> io.netty.channel.DefaultChannelPipeline.callHandlerAddedForAllHandlers(DefaultChannelPipeline.java:1161)
>>> at 
>>> io.netty.channel.DefaultChannelPipeline.invokeHandlerAddedIfNeeded(DefaultChannelPipeline.java:686)
>>> at 
>>> 

Re: Code Review Request, JDK-8208166, Still unable to use custom SSLEngine with default TrustManagerFactory after JDK-8207029

[Xuelei Fan]

Would you mind send me the debug log (-Djavax.net.debug=all) and the 
exception stacks?  The "renegotiation" in TLS 1.3 is different from TLS 
1.2 and prior specifications.  It would be helpful to me to find the 
cause of the test failure.


Thanks,
Xuelei

On 7/30/2018 1:11 PM, Norman Maurer wrote:
Sorry but I just noticed we still have a another integration test 
failing which tests that client SSL renegotiation is failing. This seems 
to be not the case anymore with java11 + your patch (it was in ea20 tho).


https://github.com/netty/netty/blob/netty-4.1.28.Final/testsuite/src/main/java/io/netty/testsuite/transport/socket/SocketSslClientRenegotiateTest.java


Let me know if I need to dig more into it.

Bye
Norman


On 30. Jul 2018, at 21:54, Norman Maurer > wrote:


Hey Xuelei,

I just re-ran our testsuite with your patch and everything pass except 
two tests. After digging a bit I found that we needed to add explicit 
calls to `SSLEngine.setUSeClientMode(false)` now in these test where 
we did not need to do this before.


The tests in question are:

https://github.com/netty/netty/blob/netty-4.1.28.Final/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java#L400
https://github.com/netty/netty/blob/netty-4.1.28.Final/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java#L418

Here we use SslContext.getDefault().createSSLEngine() and did not set 
the mode explicitly before. With the following patch to netty all 
works when using your patch:


diff --git 
a/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java 
b/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java

index e982b6a63..40d6e7b59 100644
--- a/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java
+++ b/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java
@@ -398,7 +398,9 @@ public class SslHandlerTest {
     @Test
     public void testCloseFutureNotified() throws Exception {
-        SslHandler handler = new 
SslHandler(SSLContext.getDefault().createSSLEngine());

+        SSLEngine engine = SSLContext.getDefault().createSSLEngine();
+        engine.setUseClientMode(false);
+        SslHandler handler = new SslHandler(engine);
         EmbeddedChannel ch = new EmbeddedChannel(handler);
         ch.close();
@@ -417,6 +419,7 @@ public class SslHandlerTest {
     @Test(timeout = 5000)
     public void testEventsFired() throws Exception {
         SSLEngine engine = SSLContext.getDefault().createSSLEngine();
+        engine.setUseClientMode(false);
         final BlockingQueue events = new 
LinkedBlockingQueue();
         EmbeddedChannel channel = new EmbeddedChannel(new 
SslHandler(engine), new ChannelInboundHandlerAdapter() {

             @Override


The exception we see without the patch is:

java.lang.IllegalStateException: Client/Server mode has not yet been set.
at 
java.base/sun.security.ssl.SSLEngineImpl.beginHandshake(SSLEngineImpl.java:98)

at io.netty.handler.ssl.SslHandler.handshake(SslHandler.java:1731)
at 
io.netty.handler.ssl.SslHandler.startHandshakeProcessing(SslHandler.java:1644)

at io.netty.handler.ssl.SslHandler.handlerAdded(SslHandler.java:1634)
at 
io.netty.channel.DefaultChannelPipeline.callHandlerAdded0(DefaultChannelPipeline.java:637)
at 
io.netty.channel.DefaultChannelPipeline.addLast(DefaultChannelPipeline.java:235)
at 
io.netty.channel.DefaultChannelPipeline.addLast(DefaultChannelPipeline.java:409)
at 
io.netty.channel.DefaultChannelPipeline.addLast(DefaultChannelPipeline.java:396)
at 
io.netty.channel.embedded.EmbeddedChannel$2.initChannel(EmbeddedChannel.java:203)
at 
io.netty.channel.ChannelInitializer.initChannel(ChannelInitializer.java:115)
at 
io.netty.channel.ChannelInitializer.handlerAdded(ChannelInitializer.java:107)
at 
io.netty.channel.DefaultChannelPipeline.callHandlerAdded0(DefaultChannelPipeline.java:637)
at 
io.netty.channel.DefaultChannelPipeline.access$000(DefaultChannelPipeline.java:46)
at 
io.netty.channel.DefaultChannelPipeline$PendingHandlerAddedTask.execute(DefaultChannelPipeline.java:1487)
at 
io.netty.channel.DefaultChannelPipeline.callHandlerAddedForAllHandlers(DefaultChannelPipeline.java:1161)
at 
io.netty.channel.DefaultChannelPipeline.invokeHandlerAddedIfNeeded(DefaultChannelPipeline.java:686)
at 
io.netty.channel.AbstractChannel$AbstractUnsafe.register0(AbstractChannel.java:510)
at 
io.netty.channel.AbstractChannel$AbstractUnsafe.register(AbstractChannel.java:476)
at 
io.netty.channel.embedded.EmbeddedChannel$EmbeddedUnsafe$1.register(EmbeddedChannel.java:773)
at 
io.netty.channel.embedded.EmbeddedEventLoop.register(EmbeddedEventLoop.java:130)
at 
io.netty.channel.embedded.EmbeddedEventLoop.register(EmbeddedEventLoop.java:124)
at 
io.netty.channel.embedded.EmbeddedChannel.setup(EmbeddedChannel.java:208)
at 
io.netty.channel.embedded.EmbeddedChannel.(EmbeddedChannel.java:167)
at 
io.netty.channel.embedded.EmbeddedChannel.(EmbeddedChannel.java:148)
at 

Re: Code Review Request, JDK-8208166, Still unable to use custom SSLEngine with default TrustManagerFactory after JDK-8207029

[Xuelei Fan]

Hi Norman,

Thank you very much for the great help!  Glad to know it works for you.

For the SSLEngine.setUSeClientMode() issues, the 
SSLEngine.beginHandshake() spec is expected to throw 
IllegalStateException if the client/server mode has not yet been set.

https://docs.oracle.com/javase/10/docs/api/index.html?overview-summary.html

I will make more evaluation about this behavior change.

Thanks,
Xuelei

On 7/30/2018 12:54 PM, Norman Maurer wrote:

Hey Xuelei,

I just re-ran our testsuite with your patch and everything pass except 
two tests. After digging a bit I found that we needed to add explicit 
calls to `SSLEngine.setUSeClientMode(false)` now in these test where we 
did not need to do this before.


The tests in question are:

https://github.com/netty/netty/blob/netty-4.1.28.Final/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java#L400
https://github.com/netty/netty/blob/netty-4.1.28.Final/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java#L418

Here we use SslContext.getDefault().createSSLEngine() and did not set 
the mode explicitly before. With the following patch to netty all works 
when using your patch:


diff --git 
a/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java 
b/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java

index e982b6a63..40d6e7b59 100644
--- a/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java
+++ b/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java
@@ -398,7 +398,9 @@ public class SslHandlerTest {
      @Test
      public void testCloseFutureNotified() throws Exception {
-        SslHandler handler = new 
SslHandler(SSLContext.getDefault().createSSLEngine());

+        SSLEngine engine = SSLContext.getDefault().createSSLEngine();
+        engine.setUseClientMode(false);
+        SslHandler handler = new SslHandler(engine);
          EmbeddedChannel ch = new EmbeddedChannel(handler);
          ch.close();
@@ -417,6 +419,7 @@ public class SslHandlerTest {
      @Test(timeout = 5000)
      public void testEventsFired() throws Exception {
          SSLEngine engine = SSLContext.getDefault().createSSLEngine();
+        engine.setUseClientMode(false);
          final BlockingQueue events = new 
LinkedBlockingQueue();
          EmbeddedChannel channel = new EmbeddedChannel(new 
SslHandler(engine), new ChannelInboundHandlerAdapter() {

              @Override


The exception we see without the patch is:

java.lang.IllegalStateException: Client/Server mode has not yet been set.
at 
java.base/sun.security.ssl.SSLEngineImpl.beginHandshake(SSLEngineImpl.java:98)

at io.netty.handler.ssl.SslHandler.handshake(SslHandler.java:1731)
at 
io.netty.handler.ssl.SslHandler.startHandshakeProcessing(SslHandler.java:1644)

at io.netty.handler.ssl.SslHandler.handlerAdded(SslHandler.java:1634)
at 
io.netty.channel.DefaultChannelPipeline.callHandlerAdded0(DefaultChannelPipeline.java:637)
at 
io.netty.channel.DefaultChannelPipeline.addLast(DefaultChannelPipeline.java:235)
at 
io.netty.channel.DefaultChannelPipeline.addLast(DefaultChannelPipeline.java:409)
at 
io.netty.channel.DefaultChannelPipeline.addLast(DefaultChannelPipeline.java:396)
at 
io.netty.channel.embedded.EmbeddedChannel$2.initChannel(EmbeddedChannel.java:203)
at 
io.netty.channel.ChannelInitializer.initChannel(ChannelInitializer.java:115)
at 
io.netty.channel.ChannelInitializer.handlerAdded(ChannelInitializer.java:107)
at 
io.netty.channel.DefaultChannelPipeline.callHandlerAdded0(DefaultChannelPipeline.java:637)
at 
io.netty.channel.DefaultChannelPipeline.access$000(DefaultChannelPipeline.java:46)
at 
io.netty.channel.DefaultChannelPipeline$PendingHandlerAddedTask.execute(DefaultChannelPipeline.java:1487)
at 
io.netty.channel.DefaultChannelPipeline.callHandlerAddedForAllHandlers(DefaultChannelPipeline.java:1161)
at 
io.netty.channel.DefaultChannelPipeline.invokeHandlerAddedIfNeeded(DefaultChannelPipeline.java:686)
at 
io.netty.channel.AbstractChannel$AbstractUnsafe.register0(AbstractChannel.java:510)
at 
io.netty.channel.AbstractChannel$AbstractUnsafe.register(AbstractChannel.java:476)
at 
io.netty.channel.embedded.EmbeddedChannel$EmbeddedUnsafe$1.register(EmbeddedChannel.java:773)
at 
io.netty.channel.embedded.EmbeddedEventLoop.register(EmbeddedEventLoop.java:130)
at 
io.netty.channel.embedded.EmbeddedEventLoop.register(EmbeddedEventLoop.java:124)

at io.netty.channel.embedded.EmbeddedChannel.setup(EmbeddedChannel.java:208)
at 
io.netty.channel.embedded.EmbeddedChannel.(EmbeddedChannel.java:167)
at 
io.netty.channel.embedded.EmbeddedChannel.(EmbeddedChannel.java:148)
at 
io.netty.channel.embedded.EmbeddedChannel.(EmbeddedChannel.java:135)
at 
io.netty.channel.embedded.EmbeddedChannel.(EmbeddedChannel.java:100)
at 
io.netty.handler.ssl.SslHandlerTest.testCloseFutureNotified(SslHandlerTest.java:404)
at 
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native 
Method)
at 

Re: Code Review Request, JDK-8208166, Still unable to use custom SSLEngine with default TrustManagerFactory after JDK-8207029

[Norman Maurer]

Sorry but I just noticed we still have a another integration test failing which 
tests that client SSL renegotiation is failing. This seems to be not the case 
anymore with java11 + your patch (it was in ea20 tho).

https://github.com/netty/netty/blob/netty-4.1.28.Final/testsuite/src/main/java/io/netty/testsuite/transport/socket/SocketSslClientRenegotiateTest.java
 



Let me know if I need to dig more into it.

Bye
Norman


> On 30. Jul 2018, at 21:54, Norman Maurer  wrote:
> 
> Hey Xuelei,
> 
> I just re-ran our testsuite with your patch and everything pass except two 
> tests. After digging a bit I found that we needed to add explicit calls to 
> `SSLEngine.setUSeClientMode(false)` now in these test where we did not need 
> to do this before.
> 
> The tests in question are:
> 
> https://github.com/netty/netty/blob/netty-4.1.28.Final/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java#L400
>  
> 
> https://github.com/netty/netty/blob/netty-4.1.28.Final/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java#L418
>  
> 
> 
> Here we use SslContext.getDefault().createSSLEngine() and did not set the 
> mode explicitly before. With the following patch to netty all works when 
> using your patch:
> 
> diff --git a/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java 
> b/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java
> index e982b6a63..40d6e7b59 100644
> --- a/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java
> +++ b/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java
> @@ -398,7 +398,9 @@ public class SslHandlerTest {
>  
>  @Test
>  public void testCloseFutureNotified() throws Exception {
> -SslHandler handler = new 
> SslHandler(SSLContext.getDefault().createSSLEngine());
> +SSLEngine engine = SSLContext.getDefault().createSSLEngine();
> +engine.setUseClientMode(false);
> +SslHandler handler = new SslHandler(engine);
>  EmbeddedChannel ch = new EmbeddedChannel(handler);
>  
>  ch.close();
> @@ -417,6 +419,7 @@ public class SslHandlerTest {
>  @Test(timeout = 5000)
>  public void testEventsFired() throws Exception {
>  SSLEngine engine = SSLContext.getDefault().createSSLEngine();
> +engine.setUseClientMode(false);
>  final BlockingQueue events = new 
> LinkedBlockingQueue();
>  EmbeddedChannel channel = new EmbeddedChannel(new 
> SslHandler(engine), new ChannelInboundHandlerAdapter() {
>  @Override
> 
> 
> The exception we see without the patch is:
> 
> java.lang.IllegalStateException: Client/Server mode has not yet been set.
>   at 
> java.base/sun.security.ssl.SSLEngineImpl.beginHandshake(SSLEngineImpl.java:98)
>   at io.netty.handler.ssl.SslHandler.handshake(SslHandler.java:1731)
>   at 
> io.netty.handler.ssl.SslHandler.startHandshakeProcessing(SslHandler.java:1644)
>   at io.netty.handler.ssl.SslHandler.handlerAdded(SslHandler.java:1634)
>   at 
> io.netty.channel.DefaultChannelPipeline.callHandlerAdded0(DefaultChannelPipeline.java:637)
>   at 
> io.netty.channel.DefaultChannelPipeline.addLast(DefaultChannelPipeline.java:235)
>   at 
> io.netty.channel.DefaultChannelPipeline.addLast(DefaultChannelPipeline.java:409)
>   at 
> io.netty.channel.DefaultChannelPipeline.addLast(DefaultChannelPipeline.java:396)
>   at 
> io.netty.channel.embedded.EmbeddedChannel$2.initChannel(EmbeddedChannel.java:203)
>   at 
> io.netty.channel.ChannelInitializer.initChannel(ChannelInitializer.java:115)
>   at 
> io.netty.channel.ChannelInitializer.handlerAdded(ChannelInitializer.java:107)
>   at 
> io.netty.channel.DefaultChannelPipeline.callHandlerAdded0(DefaultChannelPipeline.java:637)
>   at 
> io.netty.channel.DefaultChannelPipeline.access$000(DefaultChannelPipeline.java:46)
>   at 
> io.netty.channel.DefaultChannelPipeline$PendingHandlerAddedTask.execute(DefaultChannelPipeline.java:1487)
>   at 
> io.netty.channel.DefaultChannelPipeline.callHandlerAddedForAllHandlers(DefaultChannelPipeline.java:1161)
>   at 
> io.netty.channel.DefaultChannelPipeline.invokeHandlerAddedIfNeeded(DefaultChannelPipeline.java:686)
>   at 
> io.netty.channel.AbstractChannel$AbstractUnsafe.register0(AbstractChannel.java:510)
>   at 
> io.netty.channel.AbstractChannel$AbstractUnsafe.register(AbstractChannel.java:476)
>   at 
> io.netty.channel.embedded.EmbeddedChannel$EmbeddedUnsafe$1.register(EmbeddedChannel.java:773)
>   at 
> 

Re: Code Review Request, JDK-8208166, Still unable to use custom SSLEngine with default TrustManagerFactory after JDK-8207029

[Norman Maurer]

Hey Xuelei,

I just re-ran our testsuite with your patch and everything pass except two 
tests. After digging a bit I found that we needed to add explicit calls to 
`SSLEngine.setUSeClientMode(false)` now in these test where we did not need to 
do this before.

The tests in question are:

https://github.com/netty/netty/blob/netty-4.1.28.Final/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java#L400
 

https://github.com/netty/netty/blob/netty-4.1.28.Final/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java#L418
 


Here we use SslContext.getDefault().createSSLEngine() and did not set the mode 
explicitly before. With the following patch to netty all works when using your 
patch:

diff --git a/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java 
b/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java
index e982b6a63..40d6e7b59 100644
--- a/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java
+++ b/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java
@@ -398,7 +398,9 @@ public class SslHandlerTest {
 
 @Test
 public void testCloseFutureNotified() throws Exception {
-SslHandler handler = new 
SslHandler(SSLContext.getDefault().createSSLEngine());
+SSLEngine engine = SSLContext.getDefault().createSSLEngine();
+engine.setUseClientMode(false);
+SslHandler handler = new SslHandler(engine);
 EmbeddedChannel ch = new EmbeddedChannel(handler);
 
 ch.close();
@@ -417,6 +419,7 @@ public class SslHandlerTest {
 @Test(timeout = 5000)
 public void testEventsFired() throws Exception {
 SSLEngine engine = SSLContext.getDefault().createSSLEngine();
+engine.setUseClientMode(false);
 final BlockingQueue events = new 
LinkedBlockingQueue();
 EmbeddedChannel channel = new EmbeddedChannel(new SslHandler(engine), 
new ChannelInboundHandlerAdapter() {
 @Override


The exception we see without the patch is:

java.lang.IllegalStateException: Client/Server mode has not yet been set.
at 
java.base/sun.security.ssl.SSLEngineImpl.beginHandshake(SSLEngineImpl.java:98)
at io.netty.handler.ssl.SslHandler.handshake(SslHandler.java:1731)
at 
io.netty.handler.ssl.SslHandler.startHandshakeProcessing(SslHandler.java:1644)
at io.netty.handler.ssl.SslHandler.handlerAdded(SslHandler.java:1634)
at 
io.netty.channel.DefaultChannelPipeline.callHandlerAdded0(DefaultChannelPipeline.java:637)
at 
io.netty.channel.DefaultChannelPipeline.addLast(DefaultChannelPipeline.java:235)
at 
io.netty.channel.DefaultChannelPipeline.addLast(DefaultChannelPipeline.java:409)
at 
io.netty.channel.DefaultChannelPipeline.addLast(DefaultChannelPipeline.java:396)
at 
io.netty.channel.embedded.EmbeddedChannel$2.initChannel(EmbeddedChannel.java:203)
at 
io.netty.channel.ChannelInitializer.initChannel(ChannelInitializer.java:115)
at 
io.netty.channel.ChannelInitializer.handlerAdded(ChannelInitializer.java:107)
at 
io.netty.channel.DefaultChannelPipeline.callHandlerAdded0(DefaultChannelPipeline.java:637)
at 
io.netty.channel.DefaultChannelPipeline.access$000(DefaultChannelPipeline.java:46)
at 
io.netty.channel.DefaultChannelPipeline$PendingHandlerAddedTask.execute(DefaultChannelPipeline.java:1487)
at 
io.netty.channel.DefaultChannelPipeline.callHandlerAddedForAllHandlers(DefaultChannelPipeline.java:1161)
at 
io.netty.channel.DefaultChannelPipeline.invokeHandlerAddedIfNeeded(DefaultChannelPipeline.java:686)
at 
io.netty.channel.AbstractChannel$AbstractUnsafe.register0(AbstractChannel.java:510)
at 
io.netty.channel.AbstractChannel$AbstractUnsafe.register(AbstractChannel.java:476)
at 
io.netty.channel.embedded.EmbeddedChannel$EmbeddedUnsafe$1.register(EmbeddedChannel.java:773)
at 
io.netty.channel.embedded.EmbeddedEventLoop.register(EmbeddedEventLoop.java:130)
at 
io.netty.channel.embedded.EmbeddedEventLoop.register(EmbeddedEventLoop.java:124)
at 
io.netty.channel.embedded.EmbeddedChannel.setup(EmbeddedChannel.java:208)
at 
io.netty.channel.embedded.EmbeddedChannel.(EmbeddedChannel.java:167)
at 
io.netty.channel.embedded.EmbeddedChannel.(EmbeddedChannel.java:148)
at 
io.netty.channel.embedded.EmbeddedChannel.(EmbeddedChannel.java:135)
at 
io.netty.channel.embedded.EmbeddedChannel.(EmbeddedChannel.java:100)
at 
io.netty.handler.ssl.SslHandlerTest.testCloseFutureNotified(SslHandlerTest.java:404)
at 
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at 

Re: Code Review Request, JDK-8208166, Still unable to use custom SSLEngine with default TrustManagerFactory after JDK-8207029

[Norman Maurer]

Will do and report back as soon as possible.

Thanks
Norman


> On 30. Jul 2018, at 19:57, Xuelei Fan  wrote:
> 
> Hi Norman,
> 
> Would you mind look at the code I posted in the following thread:
> http://mail.openjdk.java.net/pipermail/security-dev/2018-July/017708.html
> 
> I appreciate if you could have a test by the end of this week.
> 
> Note that with this update, a complete TLS connection should close both 
> inbound and outbound explicitly.  However, existing applications may not did 
> this way.  If the source code update is not available, please consider to use 
> the "jdk.tls.acknowledgeCloseNotify" as a workaround.
> 
> Thanks,
> Xuelei
> 
> On 7/25/2018 11:22 PM, Norman Maurer wrote:
>> Just FYI… I tested this patch via the netty ssl tests and we no longer see 
>> the class-cast-exception problems I reported before dso I think this solves 
>> the issue.
>> That said we still encounter a few test-failures for tests that test 
>> behaviour of closing outbound of the SSLEngine but I think these are more 
>> related to 
>> http://mail.openjdk.java.net/pipermail/security-dev/2018-July/017633.html 
>> and 
>> http://mail.openjdk.java.net/pipermail/security-dev/2018-July/017566.html .
>> Bye
>> Norman
>>> On 25. Jul 2018, at 20:30, Xuelei Fan >> > wrote:
>>> 
>>> Hi,
>>> 
>>> Please review the update for JDK-8208166:
>>> http://cr.openjdk.java.net/~xuelei/8208166/webrev.00/ 
>>> 
>>> https://bugs.openjdk.java.net/browse/JDK-8208166
>>> 
>>> Thanks,
>>> Xuelei

Re: Code Review Request, JDK-8208166, Still unable to use custom SSLEngine with default TrustManagerFactory after JDK-8207029

[Xuelei Fan]

Hi Norman,

Would you mind look at the code I posted in the following thread:
http://mail.openjdk.java.net/pipermail/security-dev/2018-July/017708.html

I appreciate if you could have a test by the end of this week.

Note that with this update, a complete TLS connection should close both 
inbound and outbound explicitly.  However, existing applications may not 
did this way.  If the source code update is not available, please 
consider to use the "jdk.tls.acknowledgeCloseNotify" as a workaround.


Thanks,
Xuelei

On 7/25/2018 11:22 PM, Norman Maurer wrote:
Just FYI… I tested this patch via the netty ssl tests and we no longer 
see the class-cast-exception problems I reported before dso I think this 
solves the issue.



That said we still encounter a few test-failures for tests that test 
behaviour of closing outbound of the SSLEngine but I think these are 
more related to 
http://mail.openjdk.java.net/pipermail/security-dev/2018-July/017633.html and 
http://mail.openjdk.java.net/pipermail/security-dev/2018-July/017566.html .


Bye
Norman



On 25. Jul 2018, at 20:30, Xuelei Fan > wrote:


Hi,

Please review the update for JDK-8208166:
http://cr.openjdk.java.net/~xuelei/8208166/webrev.00/ 


https://bugs.openjdk.java.net/browse/JDK-8208166

Thanks,
Xuelei

Re: Code Review Request, JDK-8207009 SSLEngine#closeInbound mentions SSLException when no close_notify is received

[Xuelei Fan]

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

Code Review Request, JDK-8207009 SSLEngine#closeInbound mentions SSLException when no close_notify is received

[Xuelei Fan]

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

Re: Code Review Request, JDK-8208166, Still unable to use custom SSLEngine with default TrustManagerFactory after JDK-8207029

[Norman Maurer]

Just FYI… I tested this patch via the netty ssl tests and we no longer see the 
class-cast-exception problems I reported before dso I think this solves the 
issue.


That said we still encounter a few test-failures for tests that test behaviour 
of closing outbound of the SSLEngine but I think these are more related to 
http://mail.openjdk.java.net/pipermail/security-dev/2018-July/017633.html 
 and 
http://mail.openjdk.java.net/pipermail/security-dev/2018-July/017566.html 
 .

Bye
Norman



> On 25. Jul 2018, at 20:30, Xuelei Fan  wrote:
> 
> Hi,
> 
> Please review the update for JDK-8208166:
>   http://cr.openjdk.java.net/~xuelei/8208166/webrev.00/
>   https://bugs.openjdk.java.net/browse/JDK-8208166
> 
> Thanks,
> Xuelei