<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<p>Hi Xuelei,<br>
</p>
<p><sun/security/ssl/SSLSessionImpl.java><span class="changed"><br>
</span></p>
<p><span class="changed">Will this.requestedServerNames ever be
null? This field is initialized in constructors with
Collections.xxxList(...) method.</span><br>
<span class="changed"></span></p>
<span class="changed">Are the two private fields "peerPrincipal" and
"localPrincipal" really used? There are two pkg private methods,
setPeer/LocalPrincipal(...) which sets their values, however, I
don't see these two fields used inside this class. <br>
</span>
<p>line 232, 240, 248, 264, - 273: Are we sure that we want to throw
RuntimeException? It seems too generic?<br>
</p>
Quite some more fields are initialized in one constructor (line 191)
than the other one (line 175), e.g. peerCerts, compressionMethod,
negotiatedMaxFragLen (and more). Is such difference expected? The
ordering of initialization is also different which may be error
prone for future maintenance.<br>
<br>
That's all I have for today. Will continue review next Monday.<br>
Valerie<br>
<br>
<div class="moz-cite-prefix">On 5/25/2018 4:45 PM, Xuelei Fan wrote:<br>
</div>
<blockquote type="cite"
cite="mid:9a98c066-d214-5d47-f8ca-473adefe9d42@oracle.com">Hi,
<br>
<br>
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.
<br>
<br>
Here is the webrev:
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~xuelei/8196584/webrev-full.00">http://cr.openjdk.java.net/~xuelei/8196584/webrev-full.00</a>
<br>
<br>
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:
<br>
<a class="moz-txt-link-freetext" href="https://tools.ietf.org/html/draft-ietf-tls-tls13-28">https://tools.ietf.org/html/draft-ietf-tls-tls13-28</a>
<br>
<br>
For the overall description of this enhancement, please refer to
JEP 332:
<br>
<a class="moz-txt-link-freetext" href="http://openjdk.java.net/jeps/332">http://openjdk.java.net/jeps/332</a>
<br>
<br>
For the compatibility and specification update, please refer to
CSR 8202625:
<br>
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8202625">https://bugs.openjdk.java.net/browse/JDK-8202625</a>
<br>
<br>
Note that we are using the sandbox for the development right now.
For more information, please refer to Bradford's previous email:
<br>
<br>
<a class="moz-txt-link-freetext" href="http://mail.openjdk.java.net/pipermail/security-dev/2018-May/017139.html">http://mail.openjdk.java.net/pipermail/security-dev/2018-May/017139.html</a>
<br>
<br>
Thanks & Regards,
<br>
Xuelei
<br>
</blockquote>
<br>
</body>
</html>