Code Review Request: TLS 1.3 Implementation

[Xuelei Fan]

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

MaxFragExtension.java
---------------------
Looks fine to me.


KeyUpdate.java
--------------
- 74  new IOException("KeyUpdate message value invalid: " + status);
+ 74  throw new IOException("KeyUpdate message value invalid: " + status);
Or
+ 74  context.conContext.fatal(...);
The exception is not thrown.  Personally, I may prefer a 
RuntimeException as it is used in generation side, or send a fatal alert 
to indicate the problem.


   81  if (m.remaining() != 1) {
   82    throw new IOException("KeyUpdate has an unexpected length of "+
   83        m.remaining());
   84  }
Personally, I'd prefer to send a fatal alert.


   87  if (status > 1) {
   88     new IOException("KeyUpdate message value invalid: " + status);
   89  }
The exception is not thrown.  I may use a fatal alert.


  108  @Override
  109  public String toString() {
  110      throw new UnsupportedOperationException("Not supported yet.");
  111  }
For debugging, please implement this method and dump debug information 
in the consumer and producer accordingly.


  145  public void consume(ConnectionContext context,
  146          ByteBuffer message) throws IOException {

I think we need to catch up with the following spec in this method:
    Implementations that receive a KeyUpdate message prior to receiving
    a Finished message MUST terminate the connection with an
    unexpected_message" alert.


  151  if (km.getStatus() == KeyUpdateMessage.NOTREQUSTED) {
  152      return;
  153  }

If I'm correct, the "update_not_requested" status means "recipient of 
the KeyUpdate" may not respond with its own KeyUpdate", but still need 
to update its receiving keys.  Am I missing something?


  165 SSLKeyDerivation skd = kdg.createKeyDerivation(hc,
  166        hc.conContext.inputRecord.readCipher.baseSecret);
  167 SecretKey nplus1 = skd.deriveKey("TlsUpdateNplus1", null);
  168 if (skd == null) {
  169      // unlikely
  170       hc.conContext.fatal(Alert.INTERNAL_ERROR,
  171             "no key derivation");
  172       return;
  173 }
Move line 167 bellow 173? he skd non-null checking can be performed  198 
      new KeyUpdateMessage(hc, KeyUpdateMessage.NOTREQUSTED));earlier.


The blank line 191 may be not necessary.


  196  // Send Reply
  197  handshakeProducer.produce(hc,
  198      new KeyUpdateMessage(hc, KeyUpdateMessage.NOTREQUSTED));
If one update the 151-153 block, please don't forgot to update this 
block as well if needed: send reply if the status is REQUSTED.


  219  if (km.getStatus() == KeyUpdateMessage.REQUSTED) {
  220      secret = hc.conContext.outputRecord.writeCipher.baseSecret;
  221  } else {
  222      km.write(hc.handshakeOutput);
  223      hc.handshakeOutput.flush();
  224      return null;
  225  }
Similar comment as 151-153.  For NOTREQUSTED status, may still need to 
update the write keys.


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