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

[Xuelei Fan]

The new webrev with the updated spec and implementation:
    http://cr.openjdk.java.net./~xuelei/7068321/webrev.12/

Xuelei

On 10/9/2012 11:56 PM, Xuelei Fan wrote:
> Thanks for the comments. The new comments for
> SSLEngine/SSLSocket/SSLServerSocket do make the spec much more simple
> and clear.
> 
> BTW, I removed the restriction on SSLSocketFactory.createSocket(), the
> socket parameter can be an instance of SSLSocket.
> 
> The spec review is closed. Please file new bugs if you have other concerns.
> 
> Thanks,
> Xuelei
> 
> On 10/9/2012 9:03 AM, Brad Wetmore wrote:
>>
>> 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
>