I see what you're doing, but I'm not understanding why we're not
checking the other direction also?
e.g. if (p < SSLv3 && not SSLv2) || (p > TLSv1.3 ) {
Per TLS 1.2 version negotiation specification, the higher number is not
limited. For example, if client request for TLS 1.9, the server can
response with TLS 1.2. If the version if 1.9, this method does not
reject it here. However, it may be not necessary in practice.
Ah, Appendix E.1 of RFC 5246.
If a TLS server receives a ClientHello containing a version number
greater than the highest version supported by the server, it MUST
reply according to the highest version supported by the server.
Thanks.
Brad
Thanks,
Xuelei
Thanks,
Brad
289: You already checked this condition in 285-287 above. And then
304 is no longer needed.
Good catch!
Thanks,
Xuelei
Brad
On 6/7/2018 10:28 AM, Bradford Wetmore wrote:
Also reviewed SSLSocketFactoryImpl.java
BaseSSLSocketImpl.java
----------------------
I don't see where requireCloseNotify is actually being used in the
remaining code. If you are removing this functionality, you should
probably be calling that out in your CSR.
SSLSessionContextImpl.java
--------------------------
The only change here was to reorder the import of Vector (obsolete)
and update the copyright? May I ask why? If this was somehow a
netbeans suggestion, there were several other things that could be
updated that seem more important. e.g. 37-39 could be made final.
207: unnecessary unboxing.
Brad
On 6/6/2018 5:45 PM, Bradford Wetmore wrote:
Today, I looked over SunJSSE.java, Utilities.java, and
module-info.java. Mostly nits below, some style things.
Utilities.java
--------------
39: hexDigits can be private.
42: Extra level of indent. 4 or 8.
39-41: Nits: When variables are static finals, they are usually
written uppercase.
115: Extra " " and can we get SNI cap'd?
HostName for server name indication
->
HostName for Server Name Indication
150: Minor Nit: You're concating ("+") and then using
StringBuilder.append(), instead of simply append chaining which
might be slightly more efficient.
167-217: The code looks ok, but this might be reworked to take
better advantage of StringBuilder.append() chaining, rather than
the mix of concat and append here now. e.g. what you did with
toHexString(long). I see much of the same code repeated over and
over here. Can be done later.
Brad
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
