Code Review Request: TLS 1.3 Implementation

[Valerie Peng]

On 6/20/2018 9:58 AM, Xuelei Fan wrote:
> On 6/19/2018 7:07 PM, Valerie Peng wrote:
>> Hi Xuelei,
>>
>>
>> <src/java.base/share/classes/sun/security/ssl/SSLCredentials.java>
>> <src/java.base/share/classes/sun/security/ssl/SSLPossessionGenerator.java> 
>>
>> <src/java.base/share/classes/sun/security/ssl/SSLPossession.java>
>> <src/java.base/share/classes/sun/security/ssl/SSLKeyDerivationGenerator.java> 
>>
>> These look fine.
>>
>> <src/java.base/share/classes/sun/security/ssl/SSLKeyAgreement.java>
>> - kind of strange to see SSLKeyAgreement extends 
>> SSLKeyAgreementGenerator... Normally, the naming convention implies 
>> one generates the other.
>>
> The name should be more like SSLKeyAgreementKeyDerivationGenerator.  
> It is too long, so I used a short one, SSLKeyAgreementGenerator.  But 
> yes, it is confusing. Maybe, SSLKAKDGenerator?

I am not too good at naming either. Given the complexity of TLS 
implementation, maybe we don't concatenate KeyAgreementKeyDerivation, 
but just use a new term like KeyNegotiation or KeyExchange? Just food 
for thought.
Thanks,
Valerie
>
>> <src/java.base/share/classes/sun/security/ssl/SSLKeyAgreementGenerator.java> 
>>
>> - same method name as in SSLKeyDerivationGenerator. I assume that 
>> this is intentional as both are meant to derive keys, but with 
>> different parameters?
>>
> Yes.  SSLKeyAgreementGenerator is used for key agreement key 
> generation for public keys (DHE/ECDHE/RSA).  While 
> SSLKeyDerivationGenerator is used for secret key derivation with 
> shared secrets (with an additional 'secretKey' parameter).  Two 
> classes are defined just in case the two different functions are mixed.
>
> Xuelei
>
>> Still have a few more left to review.
>> 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
>>