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

Wed Oct 10 12:47:56 UTC 2012

new webrev: http://cr.openjdk.java.net./~xuelei/7068321/webrev.13/

On 10/10/2012 8:33 AM, Brad Wetmore wrote:
> Hi again,
> 
> On 10/9/2012 8:56 AM, Xuelei Fan wrote:
>> Thanks for the comments. The new comments for
>> SSLEngine/SSLSocket/SSLServerSocket do make the spec much more simple
>> and clear.
> 
> Thanks, that looks so much cleaner, and I think developers will
> appreciate it.
> 
>> 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.
> 
> Very minor comments in the API which don't need CCC modification.
> 
> javax/net/ssl/SNIHostName.java
> ==============================
> Changes look good.  Didn't review everything.
> 
> javax/net/ssl/SNIMatcher.java
> =============================
> Changes look good.  Didn't review everything.
> 
> javax/net/ssl/SNIServerName.java
> ================================
> Changes look good.  Didn't review everything.
> 
> javax/net/ssl/StandardConstants.java
> ====================================
> Didn't review.
> 
> javax/net/ssl/ExtendedSSLSession.java
> =====================================
> Didn't review.
> 
> javax/net/ssl/SSLParameters.java
> ================================
> 60:  Wow, good catch!
> 
> 314:  Looks great, thanks!
> 
> javax/net/ssl/SSLSocketFactory.java
> ===================================
> Change look good.
> 
> 216:  I think you need a period at end of sentence here.
> 
> 231:  Might suggest some reason text for the UnsupportedOperationException.
> 
I think the exception name has say the exception clearly. I think it
does not make sense to add additional message for it.  I had a search in
the packages of java.lang and java.uitl, most of the codes (39 of 42)
use the default non-parameter constructor of
UnsupportedOperationException. I will prefer to use the current style.

> 
> javax/net/ssl/SSLEngine.java
> ============================
> Minor nit:  Copyright.
> 
> 1218/1220/1225:  You are not closing the <li> properly, I think you are
> effectively adding another empty bullet.
> 
> 1227:  Copy/paste error.  You said socket, but probably meant engine.  :)
> 
> javax/net/ssl/SSLServerSocket.java
> ==================================
> Minor nit:  Copyright.
> 
> 488/490/495:  You are not closing the <li> properly.
> 
> 499:  Similar comment to above.  You could say server socket or leave as
> is, since it is a kind of a socket.
> 
> javax/net/ssl/SSLSocket.java
> ============================
> Minor nit:  Copyright.
> 
> sun/security/ssl/Utilities.java
> ===============================
> 46:  Minor comment on method name, you might want to use
> "addToSNIServerNameList" since you are adding.
> 
> 84:  Just wondering why you added this method here? This added a bit of
> overhead in extra methods calls (well, maybe not with hotspot unrolling)
> and added a few chars to the source.  So given that you have added this,
> why not update the remainder also?
> EngineOutputRecord/InputRecord/OutputRecord.
> 
Good point!

> See the below comment in SSLSocketImpl.java.  If you decide to accept
> it, you will want to remove the unmodifiable collection.
> 
See the reply in SSLSocketImpl.java

> sun/security/ssl/BaseSSLSocketImpl.java
> =======================================
> OK
> 
> sun/security/ssl/ClientHandshaker.java
> ======================================
> OK
> 
> sun/security/ssl/HandshakeInStream.java
> =======================================
> OK
> 
> sun/security/ssl/HandshakeMessage.java
> ======================================
> OK
> 
> sun/security/ssl/Handshaker.java
> ================================
> 291:  The change to allow for setting of SNI information has opened up a
> race condition.  It's probably not too bad for SSLSocket, but might be
> more for SSLEngine.  When we create ClientHandshaker/ServerHandshakers,
> we normally grab all the parameters necessary for the handshaker, and
> any future parameter modifications will apply to the next handshake.  In
> the past, our SNI information would never change, so it wasn't necessary
> to pass it in on creation.  That assumption no longer holds.
> 
> So you could see something like this:
> 
>     sslEngine.beginHandshake();
>     SSLParameters sslp = new SSLParameters();
>     sslp.setHostnames("example.com");
>     sslEngine.setSSLParameters(sslp);
>     // do handshake...
> 
> and you'll get the new value instead of old.
> 
Good catch!

Updated to store the local server name indication and matchers in
Handshaker.

> sun/security/ssl/HelloExtensions.java
> =====================================
> 35:  Are these extra imports necessary with the package import at line 31?
> 
> 307:  Can you add the "backwards compatibility" comment about the size?
> 
>    For
>    backward compatibility, all future data structures associated with
>    new NameTypes MUST begin with a 16-bit length field.
> 
> I'll forget it otherwise.
> 
> 423:  This behavior is currently underspecified.

I think the behavior is specified in RFC 6066:

   ... If the server understood the ClientHello extension but
   does not recognize the server name, the server SHOULD take one of two
   actions: either abort the handshake by sending a fatal-level
   unrecognized_name(112) alert or continue the handshake.

As means that the server will try to recognize every type of server
name. In our spec, it is required that once a SNIMatcher is defined, it
will be used to recognized the server name in the SNI extension. That's
to say, every server name should be recognizable if the corresponding
SNIMatcher is defined.  I think the current spec is clear that SNIMather
is used to match a particular server name type.

> Right now we have one
> SNI match type, but eventually we might have more.  Your current impl
> requires that *ALL* SNI matchers match.  What if you have more than one,
> and more than one SNI hostnames are sent?
> 
> SNIHostname:         "example.com"
> SNINickName:         "www.example.com"
> 
> SNIMatcher:          "example.com"
> SNINickNameMatcher:  "www1.example.com
> 
> Should this fail or succeed?  That is, should it be an AND or an OR?
> Whatever you decide, please document it at least here.  Not sure if you
> want to make the doc at the API level.
> 
If it is a "OR", we need to doc the special behaviors in API level. But
it is a "AND", I think the current API is clear that a defined
SNIMatcher is used to match a particular server name type in SNI
extension.  I think the current spec is OK.

I added some a block of comment to describe the behaviors in
HelloExtensions.

> 438:  Minor nit:  snInOther -> sniInOther?
> 
> sun/security/ssl/ProtocolList.java
> ==================================
> OK
> 
> sun/security/ssl/SSLEngineImpl.java
> ===================================
> 2084: See comments in SSLSocketImpl.java:2509.
> 
> 2100/2107:  See comments in Handshaker.
> 
> sun/security/ssl/SSLServerSocketImpl.java
> =========================================
> OK
> 
> sun/security/ssl/SSLSessionImpl.java
> ====================================
> OK
> 
> sun/security/ssl/SSLSocketFactoryImpl.java
> ==========================================
> OK
> 
> sun/security/ssl/SSLSocketImpl.java
> ===================================
> 561:  If I'm remembering and understanding this code right:
> 
>     this.host = sock.getLocalAddress().getHostName();  // in server mode
> 
> Don't ServerSockets generally creates Sockets with just local IP
> addresses (no hostnames), so this getHostname() will trigger a reverse
> hostname lookup?  Is that right?
> 
Right.

It is not really necessary to set the "host" and "serverNames"
variables, because in server mode the two variables are useless. I will
remove the setting.

> 2509:  Something to investigate: you are depending on SSLParameters
> returning an unmodifiable list.  But a subclass might not do that.  Can
> you think of any situations where this might be a bad idea?  If so, then
> we might want to change the unmodifiable language in SSLParameters and
> do regular cloning.  This will have repercussions in other areas, like
> around 2527/2532, where you'll need to pass a copy to the Handshaker.
> 
Well, I understand that something bad would happen if the subclass does
not follow the method spec while doing method implementation overriding.

That's an interesting topic that whether a none-final method can be
declared to return immutable objects, or a none-final class can be
declared as immutable class.  The logic behind is that, the new
implemented overridden method may not follow the spec of the original
spec.  Here comes other questions, can we trust the method spec of a
non-final method?  What's the general rules to override a method?  I
think it would be a very interesting topic.

There are also many non-final other APIs that requires to return
immutable object in Java APIs.  We may want to extend the discussion
more widely about what's the proper solution.

Let's have the spec and implementation as it is.  We can update the spec
and implementation if we have strong concerns or a common solution for
the issue before JDK 8 officially release.

> 2524/2541:  See comments in Handshaker.
> 
> sun/security/ssl/ServerHandshaker.java
> ======================================
> OK
> 
> sun/security/ssl/SunJSSE.java
> =============================
> OK
> 
> sun/security/ssl/X509KeyManagerImpl.java
> ========================================
> 133/150:  Thanks for the comment here.
> 
> sun/security/ssl/X509TrustManagerImpl.java
> ==========================================
> OK
> 
All of other comments are accepted.

I think there is no major concerns so far. Thank you so much for the
detailed evaluation and comments on both spec and implementation.  TLS
and its implementation is very complicated, your support is the critical
success factor to deliver quality product.

Thanks,
Xuelei

> Thanks,
> 
> Brad
> 