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
>
