Code Review Request: TLS 1.3 Implementation

[Xuelei Fan]

Hi Tony,

Thanks for the quick review!

Xuelei

On 6/3/2018 11:02 AM, Anthony Scarpino wrote:
> 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
>