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

Wed Oct 10 00:33:31 UTC 2012

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.

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.

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

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.

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.  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.

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?

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.

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

Thanks,

Brad