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
