Code Review Request: TLS 1.3 Implementation

[Xuelei Fan]

Update: http://hg.openjdk.java.net/jdk/sandbox/rev/1cc2f6afa943

On 6/19/2018 12:19 PM, Valerie Peng wrote:
> Hi Xuelei,
> 
> Just questions and some nits.
> 
> <src/java.base/share/classes/sun/security/ssl/SSLKeyDerivation.java>
> looks good
> 
> <src/java.base/share/classes/sun/security/ssl/SSLBasicKeyDerivation.java>
> looks good
> 
> <src/java.base/share/classes/sun/security/ssl/SSLSecretDerivation.java>
> - line 98: for unsupported digest, maybe we should consider throw 
> exceptions. It's easier to find out where the support needs to be added 
> if we fail early.
> 
Good point!

> <src/java.base/share/classes/sun/security/ssl/SSLMasterKeyDerivation.java>
> - Just wondering why we need difference classes of 
> SecretKeyDerivationGenerator (i.e. 
> S30/T10/T12MaterSecretKeyDerivationGenerator instances), it seems that 
> they all use LegacyMasterKeyDerivation class. The enum already has name 
> which indicates the protocol version, is the SSLKeyDerivationGenerator 
> passed around by itself and you are using its class to find out the 
> protocol version? Or, is there other reason for this?
> 
It was designed to use the new key derivation APIs.  It looks unusual 
now as there is no key derivation APIs yet.

Updated by removing the S30/T10/T12MaterSecretKeyDerivationGenerator 
classes.

> <src/java.base/share/classes/sun/security/ssl/SSLTrafficKeyDerivation.java>
> - line 71: add a default to catch the unsupported ProtocolVersion case?
The default case is considered in the final "return null" clause.

> - line 319: check params or assert params to be null? I assume it'll be 
> null as it seems to be not used.
The caller might use non-null params, potentially.  I would like to 
ignore the params and checking, as if it is not used in this implementation.

Thanks,
Xuelei

> 
> Thanks,
> Valerie
> 
> 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
>