Code review request, JEP 114, 7068321 Support TLS Server Name Indication (SNI) Extension in JSSE Server

[Brad Wetmore]

On 9/26/2012 8:05 AM, Xuelei Fan wrote:
> Hi,
>
> Please review the implementation of JEP 114/CR 7068321, Support TLS
> Server Name Indication (SNI) Extension in JSSE Server.
>
> JEP 114: http://openjdk.java.net/jeps/114
> BuG    : http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7068321
> webrev : http://cr.openjdk.java.net./~xuelei/7068321/webrev.10/

Not a lot of comments since we've already done so much over the last few 
months.  :)

If you modified anything else that we hadn't talked about previously, 
please call it out.  I didn't do a complete line/line review, just 
looking at the main points and previous comments.


javax/net/ssl/SSLSocket.java
javax/net/ssl/SSLEngine.java
javax/net/ssl/SSLServerSocket.java
==================================
(Xuelei and I have been having internal discussions about how to rework 
some of the lengthy verbage currently in the SSLParameters class, so 
this comment builds on that discussion.)

I just noticed three classes missing from the webrev.  We are updating 
the SSLParameters class with two new data structures (Host 
names/Matchers), but the actual discussion of what happens to the 
underlying SSLSockets/SSLEngines/SSLServerSocket is missing.  Note we 
did that for the other values like protocols/cipherSuites.

Rather than the current discussion in SSLParameters about "previously 
configured/default", I would like to suggest we follow the simple 
discussion already in the javadocs, and it can shorten the verbiage in 
SSLParameters very significantly!  Even though we don't have equivalent 
methods (setEnabledHostNames()) in SSLSocket/SSLEngine/SSLServer, we 
should still talk about what happens when called:

Add:

     This means:

     o ...deleted...
     o if params.getServerNames() is non-null, the socket will configure
       its server names with that value.
     o if params.getSNIMatchers() is non-null, the socket will configure
       its SNI matchers with that value.

This places the behavior explanation better where it should be, and not 
in the configuration discussion.

Now in the SSLParameters we simply talk about the effects these methods 
have on the SSLParameters class and what the values represent.  We 
already have the link to 
SSLSocket/SSLServerSocket/SSLEngine.setSSLParameters().  So...

setServerNames() remains as is.
setSNIHostNames() remains as is.

getServerNames():
=================
316:  Add to the constructor
", or null if none has been set."

321-326:  We can leave this discussion if desired, I think it sets up 
the discussion for 327-341 well.  I might suggest reordering the points 
slightly:

    * It is recommended that providers initialize default Server Name
    * Indications when creating
    * <code>SSLSocket</code>/<code>SSLEngine</code>s.  In the following
    * examples, the server name could be represented by an instance of
    * {@link SNIHostName} which has been initialized with the hostname
    * "www.example.com" and type {@link StandardConstants#SNI_HOST_NAME}.
    *
    * <pre>
    *     Socket socket =
    *         sslSocketFactory.createSocket("www.example.com", 443);
    * </pre>
    * or
    * <pre>
    *     SSLEngine engine =
    *         sslContext.createSSLEngine("www.example.com", 443);
    * </pre>
    * <P>

342-367:  Remove these lines.  All the necessary info is in the 
setSSLParameters methods.

getSNIHostNames():
==================
435: Add to the constructor
", or null if none has been set."

440-471:  Remove these lines.

Nice and clean.  All the discussion about default/current goes away.

javax/net/ssl/SNIHostName.java
==============================
52:  Do you want to remind folks here that this class is supposed to be 
immutable?

64:  Not sure if caching this hash value will actually add much to 
performance vs. volatile adds overhead.  There will only a few of these 
(likely 1).

85-86:  Should these fragments be separated "," instead of "." as you 
did below in 143-149.  You probably need an "or" before the last item.

147:  You probably need an "or" before the last item.

javax/net/ssl/SNIMatcher.java
=============================
32:  Do you want to spell out Server Name Indication before you use the 
abbreviation?  Or link to RFC 6066?  Just a thought, not strictly necessary.

105:  Empty line at end of file.

javax/net/ssl/SNIServerName.java
================================
56:  Same comment about hashCode/volatile.

136:  Might want to add a <p> between sentences.

javax/net/ssl/StandardConstants.java
====================================
OK

javax/net/ssl/ExtendedSSLSession.java
=====================================
OK

javax/net/ssl/SSLParameters.java
================================
See above.  No other comments.

javax/net/ssl/SSLSocketFactory.java
===================================
Ok.


So that you can update and submit to CCC, I'll stop here, but the 
implementation review will be continued in next email.

Hope this helps,

Brad