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
