Code Review Request: TLS 1.3 Implementation

[Jamil Nimeh]

Some comments in-line:


On 05/31/2018 10:04 PM, Xuelei Fan wrote:
> > http://cr.openjdk.java.net/~xuelei/8196584/webrev-full.00
>
> AlpnExtension.java
> ------------------
> Looks fine to me.
>
>
> CertificateRequest.java
> -----------------------
> Looks fine to me.
>
>
> CertificateStatus.java
> ----------------------
> - 42  * Pack of the CertificateStatus handshake message.
> + 42  * Pack of the CertificateStatus handshake message. [RFC 6066]
>
> May be nice if adding the RFC number that defines this handshake 
> message.  Otherwise, looks fine to me.
JJN: Yeah, we can do that.  And probably reference both 6066 and 6961 
since both are represented in this handshake message.  Perhaps a full 
header comment that describes the structure of the message might be 
appropriate here, too.  It would help maintainers perhaps.
>
>
> CertificateVerify.java
> ----------------------
> -129  if (x509Credentials == null) {
> +129  if (x509Credentials == null ||
>              x509Credentials.popPublicKey == null) {
>  130      shc.conContext.fatal(Alert.HANDSHAKE_FAILURE,
>  131         "No X509 credentials negotiated for CertificateVerify");
>  132  }
>
> May be safe to check the x509Credentials.popPublicKey as well. Similar 
> to line 357-360, 607-610, 916-919.
>
>
> -233  if (x509Possession == null) {
> +233  if (x509Possession == null ||
>               x509Possession.popPrivateKey == null) {
>  234      if (SSLLogger.isOn && SSLLogger.isOn("ssl,handshake")) {
>  235         SSLLogger.fine(
>  236         "No X.509 credentials negotiated for CertificateVerify");
>  237      }
>  238
>  239     return null;
>  240  }
>
> May be safe to check the x509Possession.popPrivateKey as well. Similar 
> to line 458-466, 697-704, 1021-1027.
>
> Otherwise, looks fine to me.
>
> CertStatusExtension.java
> ------------------------
>  326         private final int encodedLen;
>
> Looks like this field is not used.  Remove it?
>
>  425       if (!responderIds.isEmpty()) {
>  426           ridStr = responderIds.toString();
> -427
>  428      }
> One unnecessary blank line.  Otherwise, looks fine to me.
JJN: For the last two comments, yes to both.  That encodedLen field is a 
remnant from the JDK 9 implementation and it was used there.
>
> Xuelei
>
> On 5/25/2018 4:45 PM, Xuelei Fan wrote:
>> Hi,
>>
>> I'd like to invite you to review the TLS 1.3 implementation.  I 
>> appreciate it if I could have compatibility and specification 
>> feedback before May 31, 2018, and implementation feedback before June 
>> 7, 2018.
>>
>> Here is the webrev:
>>      http://cr.openjdk.java.net/~xuelei/8196584/webrev-full.00
>>
>> The formal TLS 1.3 specification is not finalized yet, although it 
>> had been approved to be a standard.  The implementation is based on 
>> the draft version 28:
>>      https://tools.ietf.org/html/draft-ietf-tls-tls13-28
>>
>> For the overall description of this enhancement, please refer to JEP 
>> 332:
>>      http://openjdk.java.net/jeps/332
>>
>> For the compatibility and specification update, please refer to CSR 
>> 8202625:
>>      https://bugs.openjdk.java.net/browse/JDK-8202625
>>
>> Note that we are using the sandbox for the development right now.  
>> For more information, please refer to Bradford's previous email:
>>
>> http://mail.openjdk.java.net/pipermail/security-dev/2018-May/017139.html
>>
>> Thanks & Regards,
>> Xuelei