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
