Code Review Request: TLS 1.3 Implementation

[Xuelei Fan]

 > KeyUpdate.java
 > --------------
Update: http://hg.openjdk.java.net/jdk/sandbox/rev/66c803c3ce32

Fixed two test issues and the following KeyUpdate implementation.  This 
update will be included in the next webrev for further review.

Xuelei

On 6/1/2018 12:56 PM, Xuelei Fan wrote:
>  > 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