Re: Code Review Request: TLS 1.3 full handshake (JDK-8196584)

[Jamil Nimeh]

Hello, comments below, minor stuff for the most part:

 * KeyUpdate.java
     o 128-129: Spelling nit: REQUSTED --> REQUESTED
     o That change will have to get sprinkled throughout the code
 * OutputRecord.java
     o 291-295: It seems like using an absolute put would simplify this
       code a bit:  Just advance the destination limit by one like
       you're already doing, then destination.put(destination.limit() -
       1, contentType);  Or you can set an int variable to
       destination.limit() before you make the limit one larger and
       then use that as the first argument to the absolute put.  Either
       way you don't need to move your position marker forward and
       backward.
         + I know absolute puts are not guaranteed to be implemented,
           but they are used further down on line 312, 317-318 so I
           think we're safe here.
 * SSLEngineOutputRecord.java
     o Looks good to me
 * SSLSocketOutputRecord.java
     o 315: REQUSTED -> REQUESTED
     o Looks okay otherwise
 * DTLSOutputRecord
     o 68: Do we need to hang onto this for now, or can we remove this
       commented out line?
     o Otherwise looks OK.

More on the way...

--Jamil


On 02/22/2018 12:29 PM, Xuelei Fan wrote:

Updated to use package private HKDF implementation.

webrev (based on JDK-8185576):
      http://cr.openjdk.java.net/~xuelei/8196584/webrev-step.01

webrev (including JDK-8185576):
      http://cr.openjdk.java.net/~xuelei/8196584/webrev-full.01

Thanks,
Xuelei

On 2/20/2018 11:57 AM, Xuelei Fan wrote:

Hi,

I'd like to invite you to review the TLS 1.3 full handshake implementation.  I appreciate it if I could have feedback before March 9, 2018.

In the "JDK-8185576: New handshake implementation" [1] code review around, I was trying to re-org the TLS handshaking implementation in the SunJSSE provider.  If you had reviewed that part, you can start from the following webrev that based on the update of JDK-8185576:

     http://cr.openjdk.java.net/~xuelei/8196584/webrev-step.00

If you would like start from earlier, here is the webrev that contains the handshaking implementation re-org in JDK-8185576:

     http://cr.openjdk.java.net/~xuelei/8196584/webrev-full.00

This changeset only implements the full handshake of TLS 1.3, rather then a fully implementation of the latest TLS 1.3 draft [2].

In this implementation, I removed:
1. the KRB5 cipher suite implementation.

Please let me know if you are still using KRB5 cipher suite.  I may not add them back if no objections.

2. OCSP stapling.
This feature will be added back later.

Resumption and key update, and more features may be added later.

Thanks & Regards,
Xuelei

[1]: http://mail.openjdk.java.net/pipermail/security-dev/2017-December/016642.html

[2]: https://tools.ietf.org/html/draft-ietf-tls-tls13-24