Code Review Request: TLS 1.3 Implementation

[Anthony Scarpino]

On 06/04/2018 08:50 AM, Xuelei Fan wrote:
> On 6/4/2018 8:36 AM, Anthony Scarpino wrote:
>> Here are questions I have about the code before I or someone makes a 
>> change:
>>
>> ChangeCipherSpec.java
>> - 69 & 200:  Given this is legacy data on older version of TLS, is the 
>> exception better worded "Not supported"?  "yet" implies future additions.
> I agreed.  Would you mind update it in one of your changeset as well?
> 
>> - ChangeCipherSpec is removed in 1.3, why is there a T13 consumer ? 
> For compatibility reason, ChangeCipherSpec was back in TLS 1.3 in the 
> recent drafts.  But it is used for compatibility purposes only.  See 
> Section 5 of draft 28:
>     An implementation may receive an unencrypted record of type
>     change_cipher_spec consisting of the single byte value 0x01 at any
>     time after the first ClientHello message has been sent or received
>     and before the peer's Finished message has been received and MUST
>     simply drop it without further processing.
> 
> As JDK is using the compatibility mode, so ChangeCipherSpec has a T13 
> consumer.

Ok.. thanks. I see they talk about is as "change_cipher_spec" and not 
ChangeCipherSpec.

> 
>> Client/ServerHello are consuming these messages too, shouldn't their 
>> references be removed as well?
>>
> See above.
> 
>>
>> Below are some changes I will make:
>> Alert.java
>> - optimized imports.
>>
>> Authenticator.java
>> - 271-2: Clarifying comments about byte array ordering.  Moved 
>> sequence number to the end
>> - 406: static declaration for the interface is redundant
>>
>> ClientHandshakeContext:
>> - 3 spelling errors
>> Thanks!
> 
> Xuelei