Code Review Request: TLS 1.3 Implementation

[Xuelei Fan]

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

Xuelei

On 6/11/2018 6:36 PM, Xuelei Fan wrote:
> 
> 
> On 6/11/2018 5:56 PM, Bradford Wetmore wrote:
> <...skipped...>
>>>>> 262:  What is the point of the aliases argument in the constructor? 
>>>>> Was the idea to provide a mapping between suites we originally 
>>>>> created with the SSL_ prefix vs the more current TLS_ prefix we 
>>>>> used in the later TLS protocols?  There is only an empty string in 
>>>>> every constructor, so this code doesn't do anything.
>>>>>
>>> Added the aliases.
>>
>> Great, thanks.  Once minor formatting comment which would help 
>> comparability/readability.  Take or leave it.
>>
>>      SSL_DHE_RSA_WITH_3DES_EDE_CBC_SHA(
>>              0x0016, true, "SSL_DHE_RSA_WITH_3DES_EDE_CBC_SHA",
>>              "TLS_DHE_RSA_WITH_3DES_EDE_CBC_SHA",
>>              ProtocolVersion.PROTOCOLS_TO_12,
>>              K_DHE_RSA, B_3DES, M_SHA, H_SHA256),
>> ->
>>      SSL_DHE_RSA_WITH_3DES_EDE_CBC_SHA(
>>              0x0016, true, "SSL_DHE_RSA_WITH_3DES_EDE_CBC_SHA",
>>                            "TLS_DHE_RSA_WITH_3DES_EDE_CBC_SHA",
>>              ProtocolVersion.PROTOCOLS_TO_12,
>>              K_DHE_RSA, B_3DES, M_SHA, H_SHA256),
>>
> It looks really nice, and I will take it.  Updated in my local 
> workspace, will push later in my next changeset.
> 
> <...skipped...>
>>>>> 840:  Is this else/break needed?
>>>>>
>>> Yes.  It is mostly for performance by ignoring unsupported cipher 
>>> suites look up.
>>
>> Ah, because they are coming out of .values() ordered, and the 
>> unsupported are at the end.  Can you change the comment:
>>
>> the following cipher suites are not supported.
>> ->
>> values() is ordered, remaining cipher suites are not supported.
>>
> Updated.
> 
>>
>>>>> SSLServerSocketImpl.java
>>>>> ------------------------
>>>>> 160:  You should allow multiple changes between server to client, 
>>>>> and switch enabled protocols/ciphersuites accordingly.
>>>>>
>>> Yes, multiple changes are allowed.
>>
>> I didn't see a change here.  If you go from server (default) to 
>> client, and then back again, shouldn't you reset to the original 
>> server values? And set sslConfig.useClientMode()
>>
>> e.g.
>>      sslServerSocket.setUseClientMode(true);
>>      sslServerSocket.setUseClientMode(false);
>>
>> The current code won't change back to server.
>>
> I may miss something.  I think, it is able to change back.
> 
> sslServerSocket.setUseClientMode(true) ->
>    if it is server mode, set to use client mode; and if using default 
> protocols, set to use client default protocols; if using default cipher 
> suites, set to use client default cipher suites.  sslConfig is set to 
> use client mode.
> 
> sslServerSocket.setUseClientMode(false);
>    if it is client mode, set to use server mode; and id using default 
> protocols, set to use server default protocols; if using default cipher 
> suites, set to use server default cipher suites.  sslConfig is set to 
> use server mode.
> 
> Thanks,
> Xuelei