Code Review Request: TLS 1.3 Implementation

[Anthony Scarpino]

Xuelei,

 From looking at your code change at 151 and 219  when a single 
KeyUpdate handshake is sent, it appears you are changing all 4 keys, 
both send and receive from the client and send and receive from the server.

It was my interpretation of the spec that only the send and receive keys 
were for one side were updated.  After many rereading of 4.6.3 I finally 
see where you think all 4 keys are updated.

spec:
   If the request_update field is set to “update_requested” then the
   receiver MUST send a KeyUpdate of its own with request_update set to
   “update_not_requested” prior to sending its next application data

I can see where you believe "send a KeyUpdate of its own" means update 
the send and receive keys from the opposite direction.  Where I 
interpreted this as an ACK by the receiver to updating the receiving key.

I believe your interpretation is probably right and the spec could be 
worded better.

The other changes are fine.

Tony

On 06/02/2018 09:26 PM, Xuelei Fan wrote:
>  > 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