Code Review Request: TLS 1.3 Implementation

Xuelei Fan xuelei.fan at oracle.com
Mon Jun 4 15:50:12 UTC 2018


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.

> 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



More information about the security-dev mailing list