Re: RFR, JDK-8212885: TLS 1.3 resumed session does not retain peer certificate chain

2018-11-13 Thread Xuelei Fan
Looks fine to me. Thanks, Xuelei On 11/12/2018 11:46 PM, Jamil Nimeh wrote: Hello all, This is an update that addresses a few additional fields that needed to be carried over into the new SSLSession, as well as adding some more testing on the retrieved resumed SSLSession object. http://cr.

Re: RFR, JDK-8212885: TLS 1.3 resumed session does not retain peer certificate chain

2018-11-12 Thread Jamil Nimeh
Hello all, This is an update that addresses a few additional fields that needed to be carried over into the new SSLSession, as well as adding some more testing on the retrieved resumed SSLSession object. http://cr.openjdk.java.net/~jnimeh/reviews/8212885/webrev.04/ Thanks, --Jamil

Re: RFR, JDK-8212885: TLS 1.3 resumed session does not retain peer certificate chain

2018-11-06 Thread Xuelei Fan
Thanks for the update! No more comments from me. Xuelei On 11/6/2018 11:38 AM, Jamil Nimeh wrote: Hi Xuelei, I've made the change.  I think in this specific case CipherSuite.hashAlg.toString is just a simple return of the name field so it should be no less reliable than hitting the name fie

Re: RFR, JDK-8212885: TLS 1.3 resumed session does not retain peer certificate chain

2018-11-06 Thread Jamil Nimeh
Hi Xuelei, I've made the change.  I think in this specific case CipherSuite.hashAlg.toString is just a simple return of the name field so it should be no less reliable than hitting the name field directly.  Changing it does make it more consistent with other places in the method, so that's go

Re: RFR, JDK-8212885: TLS 1.3 resumed session does not retain peer certificate chain

2018-11-06 Thread Xuelei Fan
Looks fine to me. As you are already there, would you mind have an additional improvement in PreSharedKeyExtension.java? - MessageDigest md = MessageDigest.getInstance(hashAlg.toString());; + MessageDigest md = MessageDigest.getInstance(hashAlg.toString()); Normally, the toString() is not

Re: RFR, JDK-8212885: TLS 1.3 resumed session does not retain peer certificate chain

2018-11-06 Thread Jamil Nimeh
Hi Xuelei, updated review here: http://cr.openjdk.java.net/~jnimeh/reviews/8212885/webrev.02 I followed your suggestions and also cleaned up some remnant comments and removed a double-semicolon...just cosmetic stuff. --Jamil On 11/6/18 10:11 AM, Jamil Nimeh wrote: Okay, I can move this into

Re: RFR, JDK-8212885: TLS 1.3 resumed session does not retain peer certificate chain

2018-11-06 Thread Jamil Nimeh
Okay, I can move this into PreSharedKeyExtension.java and re-run the local tests that were having issues with it.  Should work pretty well. I'll put out another code review shortly. Thanks, --Jamil On 11/6/2018 7:36 AM, Xuelei Fan wrote: Nice update! For the update in ClientHello.java, I may

Re: RFR, JDK-8212885: TLS 1.3 resumed session does not retain peer certificate chain

2018-11-06 Thread Xuelei Fan
Nice update! For the update in ClientHello.java, I may suggest moving it to pre_shared_key extension class. It may be a little bit safer if the extension can be loaded in other places. Thanks, Xuelei On 11/5/2018 11:51 PM, Jamil Nimeh wrote: Hello all, This fixes an issue where TLS 1.3 re