Code Review Request: TLS 1.3 Implementation

[Xuelei Fan]

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