Code Review Request: TLS 1.3 Implementation

[Xuelei Fan]

 > 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.


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.

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