Hi Brad, just one follow-up to your CipherSuite comment, please see below.
--Jamil
On 06/08/2018 07:55 PM, Bradford Wetmore wrote:
CipherSuite.java
----------------
74: There is a mix of ciphersuite initialization styles, which I find
confusing. In the *_OF_12, you pass the MAC of M_NULL, with H_* being
used for PRF/HASH, which is also used for the MAC value for new
suites, IIUC. Would you consider specifying both in the enum
constructor rather than M_NULL?
KeyExchange keyExchange, SSLCipher cipher,
MacAlg macAlg, HashAlg hashAlg)
to_12
0x0018, false, "SSL_DH_anon_WITH_RC4_128_MD5", "",
ProtocolVersion.PROTOCOLS_TO_T12,
K_DH_ANON, B_RC4_128, M_MD5, H_SHA256),
of_12
0x00A2, true, "TLS_DHE_DSS_WITH_AES_128_GCM_SHA256", "",
ProtocolVersion.PROTOCOLS_OF_12,
K_DHE_DSS, B_AES_128_GCM, M_NULL, H_SHA256),
->
K_DHE_DSS, B_AES_128_GCM, H_SHA256, H_SHA256),
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.
124-287: All of a sudden the indention style changed, and then
corrected itself back a little later at the "non-enabled" suites. Can
you remove these extra spaces here?
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.
377: Maybe provide the RFC numbers (e.g. 4346, 5469, 5246, etc) for
reference?
455: This "other values" info is way out of date. If anything, I
would suggest we simply provide a link at the top of this file to the
https (not http) page, and be done with it.
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.
840: Is this else/break needed?
863: This @throws is incorrect. 869 returns an emptyList instead of
an Exception.
891: "...with currently installed providers"? This wording is
strange, since it's all part of the SunJSSE provider. Can you just
say "name unsupported"?
910: Can you please add a quick comment as to why/when you want a
keyExchange == null. i.e. TLSv1.3. It took me a while to figure out
when you would ever have a keyExchange==null.
995: For readability, you can line this up using a 80 char line.
K_RSA ("RSA", true, false, NAMED_GROUP_NONE),
...deleted...
SSLSessionImpl.java
-------------------
90: I think we need to revisit this decision, but not now.
SSLServerSocketImpl.java
------------------------
160: You should allow multiple changes between server to client, and
switch enabled protocols/ciphersuites accordingly.
SSLConfiguration.java
---------------------
99: This is a followup to the comment from last night. tls13VN is
pretty much set, let's get this temporary stuff out of here.
108: Should we reverse the two tests here? Checking for System
property and then not checking for a SunTlsExtendedMasterSecret would
be a faster option in the false case, vs driving the JCE lookup
machinery and then checking whether we'll be using it or not.
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.
76: Are you set on the name conContext? I'm still not 100% sure what
it stands for. Alternate transportContext or connectionContext?
92: trustNameService could be private and all in caps.
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.
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.
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.
682-683: These can be final.
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
