Code Review Request: TLS 1.3 Implementation
Bradford Wetmore
bradford.wetmore at oracle.com
Tue Jun 12 00:56:47 UTC 2018
>>> CipherSuite.java
>>> ----------------
>>> 74: There is a mix of ciphersuite initialization styles, which I
>>> find confusing.
>>>
>> These two suites may not be the best comparison, since you have a
>> non-AEAD stream cipher up against AES-GCM. For the latter, I think
>> M_MULL in the MacAlg field is the right thing because GCM doesn't have
>> a separate HMAC. The authentication is performed through GHASH. The
>> only place for SHA-256 in this suite is for the PRF, IIUC. As a
>> second data point ChaCha20-Poly1305 suites (when we eventually do
>> them) would also use M_NULL for the MacAlg since Poly1305 takes care
>> of the authentication internally and SHA-256 is just used with PRF
>> and/or HKDF in the TLS 1.3 case.
>>
> I agreed with Jamil.
Makes perfect sense now, thanks.
>>> 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),
>>> 803/814/824/867: I'm concerned of the performance impact of
>>> repeatedly iterating over 300+ ciphersuite ids/names using values().
>>> You should benchmark and see if it makes sense to switch to using a
>>> HashMap (or even TreeMap) here. For the limited number of Protocols
>>> (< 10 for TLSv1.x), this approach is fine, but this has quite a bit
>>> larger search space.
>>>
> Good point! This enhancement now is tracked with
> https://bugs.openjdk.java.net/browse/JDK-8204636
Good, thanks.
>>> 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.
>>> 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.
>>>
>>> SSLConfiguration.java
>>> ---------------------
40: There is a new unused import here.
>>> SSLSocketImpl.java
>>> ------------------
>>> 28: I'm not a fan of unnecessarily expanding the imports. Netbeans
>>> has/had a default of 5, but apparently turned it off by default in
>>> favor of single class imports.
>>>
> I'm a fan of expanding the imports now. It is more clear about the
> imported classes and the packet of a particular class.
>
> For example, we can use:
> import java.net.*;
> import java.io.*;
>
> Or
> import java.net.Socket;
> import java.io.StringReader;
>
> If using the "*" option, we don't actually know if Socket is in packet
> java.net or java.io. So if IDE has made the update, I'm not included to
> update them back any more.
Just seems like a lot of extra necessary cruft when you have lots of
specifics (e.g. >5 like in SSLEngineImpl.java), but that's just my
personal preference. And IDEs can be set to do all kinds of formatting. :)
>>> 76: Are you set on the name conContext? I'm still not 100% sure
>>> what it stands for. Alternate transportContext or connectionContext?
>>>
> The name is implying connection context, but it is an instance of
> TransportContext. Do you like, traContext, for transport context? I'd
> like to use a short name as it is used a lot that it is easy to exceed
> the 80 character per line limit.
Nothing really jump at me.
Brad
>>> 92: trustNameService could be private and all in caps.
>>>
> Updated.
>
>>> 102-269: Any chance of combining the commonalities of these
>>> different constructors into 1, and then do the constructor-specific
>>> things at the end? It will help with future maintenance to not have
>>> to make the same changes in multiple places.
>>>
> It is not clear to me now. The issue is tracked with:
> https://bugs.openjdk.java.net/browse/JDK-8204636
>
>>> 484: A couple comments here would be appreciated about what these
>>> App* classes do. The old comments in App[InputStream would be
>>> sufficient, plus some of the other comments stripped out inside the
>>> class:
>>>
>>> Read data if needed ... notice that the connection guarantees
>>> that handshake, alert, and change cipher spec data streams are
>>> handled as they arrive, so we never see them here.
>>>
> Updated.
>
>>> Nice job on this refactoring in this class, there was a heck of a lot
>>> of cruft in this file after 20+ years. I am anxious to see how the
>>> rest of the code is laid out.
>>>
>>>
>>> TransportContext.java
>>> ---------------------
>>> 90: Any chance of combining these 3 constructors into 1? Almost
>>> identical duplicate code here.
>>>
> This issue had been addressed in one of Tony's changesets:
> http://hg.openjdk.java.net/jdk/sandbox/rev/b152d06ed6a9
>
>>> 682-683: These can be final.
>>>
> Updated.
>
> Thanks,
> Xuelei
>
>>>
>>> Thanks,
>>>
>>> Brad
>>>
>>>
>>>
>>> On 6/8/2018 10:21 AM, Xuelei Fan wrote:
>>>> Here is the 3rd full webrev:
>>>> http://cr.openjdk.java.net/~xuelei/8196584/webrev-full.02
>>>>
>>>> and the delta update to the 1st webrev:
>>>> http://cr.openjdk.java.net/~xuelei/8196584/webrev-delta.01
>>>>
>>>> Xuelei
>>>>
>>>> On 6/3/2018 9:43 PM, Xuelei Fan wrote:
>>>>> Hi,
>>>>>
>>>>> Here it the 2nd full webrev:
>>>>> http://cr.openjdk.java.net/~xuelei/8196584/webrev-full.01
>>>>>
>>>>> and the delta update to the 1st webrev:
>>>>> http://cr.openjdk.java.net/~xuelei/8196584/webrev-delta.00/
>>>>>
>>>>> Xuelei
>>>>>
>>>>> 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
>>
More information about the security-dev
mailing list