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.

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

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

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

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