Code Review Request: TLS 1.3 full handshake (JDK-8196584)

[Valerie Peng]

Sure. Changes look fine.

Valerie


On 6/8/2018 12:54 PM, Xuelei Fan wrote:
> > http://cr.openjdk.java.net/~xuelei/8196584/webrev-full.02
>
> Update: http://hg.openjdk.java.net/jdk/sandbox/rev/f4c7a97a1275
>
> On 6/8/2018 12:19 PM, Valerie Peng wrote:
>> Hi Xuelei,
>>
>> <sun/security/ssl/SignatureScheme.java>
>>
>> Some typos, line 139: minial -> minimal, line 144: caculate -> 
>> calculate, minial -> minimal
>> When constructing PSSParameterSpec object on line 174, I think it's 
>> better to specify new MGF1ParameterSpec(hash) instead of null. 
>> Although current RSASSA-PSS signature algorithm impl in SunRsaSign 
>> provider will use the same message digest algorithm if no 
>> MGF1ParameterSpec is specified. However, this is somewhat a gray area 
>> and maybe better to be explicit
>>
> Good catches!  Fixed in the above changeset.
>
>> Do we need a constructor with both NamedGroup and SigAlgParamSpec? 
>> NamedGroup is for EC and SigAlgParamSpec is for RSASSA-PSS, they 
>> shouldn't co-exist in the same scheme, right?
>>
> So far, they cannot co-exist in the same scheme.  I would like to make 
> an update together with the following coding style.
>
>> There are currently 5 constructors in SignatureScheme class, I find 
>> the ordering of arguments somewhat confusing. The required arguments 
>> should appear first and we can add optional ones after them. Easier 
>> to read this way, at least for me...
>>
> It makes senses to me.
>
> I plan to do it later, maybe after we merge to the mainline.  Here is 
> the JBS bug I used to track the issues:
>      https://bugs.openjdk.java.net/browse/JDK-8204636
>
> Thanks,
> Xuelei
>
>> That's it for this class. More to review.
>> Thanks,
>> Valerie
>>
>> On 2/20/2018 11:57 AM, Xuelei Fan wrote:
>>> Hi,
>>>
>>> I'd like to invite you to review the TLS 1.3 full handshake 
>>> implementation.  I appreciate it if I could have feedback before 
>>> March 9, 2018.
>>>
>>> In the "JDK-8185576: New handshake implementation" [1] code review 
>>> around, I was trying to re-org the TLS handshaking implementation in 
>>> the
>>> SunJSSE provider.  If you had reviewed that part, you can start from 
>>> the following webrev that based on the update of JDK-8185576:
>>> http://cr.openjdk.java.net/~xuelei/8196584/webrev-step.00
>>>
>>> If you would like start from earlier, here is the webrev that 
>>> contains the handshaking implementation re-org in JDK-8185576:
>>> http://cr.openjdk.java.net/~xuelei/8196584/webrev-full.00
>>>
>>>
>>> This changeset only implements the full handshake of TLS 1.3, rather 
>>> then a fully implementation of the latest TLS 1.3 draft [2].
>>>
>>> In this implementation, I removed:
>>> 1. the KRB5 cipher suite implementation.
>>> Please let me know if you are still using KRB5 cipher suite. I may 
>>> not add them back if no objections.
>>>
>>> 2. OCSP stapling.
>>> This feature will be added back later.
>>>
>>> Resumption and key update, and more features may be added later.
>>>
>>> Thanks & Regards,
>>> Xuelei
>>>
>>> [1]: 
>>> http://mail.openjdk.java.net/pipermail/security-dev/2017-December/016642.html 
>>>
>>> [2]: https://tools.ietf.org/html/draft-ietf-tls-tls13-24
>>