Code Review Request: TLS 1.3 Implementation

Valerie Peng valerie.peng at
Sat Jun 9 00:16:29 UTC 2018

Hi Xuelei,


Will this.requestedServerNames ever be null? This field is initialized 
in constructors with Collections.xxxList(...) method.

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.

line 232, 240, 248, 264, - 273: Are we sure that we want to throw 
RuntimeException? It seems too generic?

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.

That's all I have for today. Will continue review next Monday.

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:
> 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:
> For the overall description of this enhancement, please refer to JEP 332:
> For the compatibility and specification update, please refer to CSR 
> 8202625:
> Note that we are using the sandbox for the development right now. For 
> more information, please refer to Bradford's previous email:
> Thanks & Regards,
> Xuelei

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <>

More information about the security-dev mailing list