Hi Xuelei,
I like this style much better than your earlier versions. Thanks for
considering them.
We should still do a little more wordsmithing, some of the passages are
a little awkward. The following are mainly content questions.
ExtendedSSLSession.java
=======================
91: Having the SNI acronym will help people who are specifically
looking for this option. (i.e. find) Suggest updating to caps and
include acronym.
values of the Server Name Indication (SNI) capability.
94: "to to"->"to"
98: "of enabled server name"->"of requested server name"
99: SNI
SSLParameters.java
==================
One general question before we get to specifics. Your current default
behavior of the SunJSSE is to add a SNI extension if we have the value
available. So if we call:
sslSocket = socketFactory.createSocket("www.example.com", 443);
sslp = sslParameters.getSSLParameters();
will this sslParameters ever contain a map with preinstalled "host_name"
set to "www.example.com", or will it be empty? I think the answer will
be empty. This API is just a way to force setting the value if an
implementation select an unwanted value.
But here's my point. These are brand new methods. Have you considered
just having the SSLSocket.getSSLParameters() return the already fully
populated SNI field, that is, the SSLSocketImpl's default values are
already set in the SSLParameters? That way, if someone sets value to
null, then we know they don't want the provider to send SNI fields of
that type.
e.g.
sslp = sslParameters.getSSLParameters();
// sslp has host_name already set to www.example.com
sslp.setAccessibleServerName("host_name", null);
this makes it less confusing in that you get rid of all that ambigious
"default" discussions in your API, and the user can easily find out
exactly what will be sent. Something to consider.
72: Might suggest minor rewording:
// As "hostname" is the only known server name indication type,
to
// As "host_name" is the only known Server Name Indication (SNI) type,
RFC 6066 uses "host_name" and as previously mentioned, having the SNI
acronym will help people who are specifically looking for this. (i.e. find)
76: Not sure why you want/need a LinkedHashMap with only one currently
defined NameType. Even if there were multiple types, I don't think that
SNI requires an ordering. You also mention this in
setAccessibleServerName, but not sure what purpose this serves. I'm not
strongly against it, just wondering.
271: Same comment in the APIs about SNI acronym.
295: So what does value = "" mean now? Will that send a SNI extension
with an empty string, or will no SNI entry be sent? If null is passed,
then it's up to the provider to decide what to do, but there doesn't
seem to be a way to shut off SNI like in your old proposal.
308: What kinds of things are you thinking of writing in the Additional
Standard Names doc for "value"? Would it be sufficient to just say
"this is the SNI hostname value"?
365: alternatice -> alternate
368: "www\\.example\\.com|www\\.example\\.org"
-> "www\\.example\\.(com,org)"
This is a little more illuminating of the power of these patterns.
Even though you have a link to java.util.regex.Pattern in the actual
API, you might also mention here in textual form that these are
java.util.regex patterns, and add a @link java.util.regex. Makes it
more clear where to go to learn about how to specify patterns.
379: So what does "" mean now? Will an empty string SNI extension be
accepted? Or we will
379: "will not try to recognize the server name value..."
I'm thinking of IETF's "WILL/SHALL/MAY" here. What do you mean by "will
not try"? Do you mean that if this is null, we will not use the SNI
extension of this type? Or that we could possibly try?
387: This is a pattern and not a String, so only listing
setAccessibleServerName() isn't quite complete. You might also list
"and also see the Pattern page for details on specifying regular
expression patterns".
SSLSocketFactory.java
=====================
187: I would suggest following the style in the SocketFactory page
which describes the functionality differences in the various
createSocket methods.
A new second paragraph really should give some motivation for this
method. "allow for inspection of inbound data, then provide that data
to the SSLSocket's normal IO streams in the consumed variable, etc."
Since you're leaving the door open for this socket being either client
or server, shouldn't the API then be similar to the existing layered
socket one, just including the additional InputStream here:
public abstract Socket createSocket(Socket s, InputStream is,
String host, int port, boolean autoClose)
We might be able to glean SNI information from the host here.
171: Interesting, the existing layered createSocket doesn't mention
anything about whether client or server mode, and that you must set the
mode or suffer with the default. Might want to file a bug and CCC this
also.
197: You're not planning to process (e.g.
ServerHandshaker/ClientHandshaker.process_message) the consumed
handshaking bytes immediately during the createSocket call, are you?
You still need to allow the user time to set the socket
options/SSLParameters/etc. I was expecting in this method you'd just
suck in the consumed bytes into temporary storage, then create/return
the socket, and then when the handshaking is started, you then read out
from the temporary storage until you run out, then you switch to the
Socket's InputStream.
197: This needs some wordsmithing here. This method will produce the
SSLSocket that will be consuming this data, so of course it has to be
called first.
204: This description is too implementation specific, and can probably
be left out.
206: "comsumed"->"consumed"
221: this should probably have the same behavior as the other layered
SSLSocket at 183: This should be an IOException. If not, then this
javadoc does not describe why an IOException might be thrown.
Hope this helps. We can wordsmith next week.
Brad
On 8/9/2012 9:04 AM, Xuelei Fan wrote:
The new webrev:
http://cr.openjdk.java.net./~xuelei/7068321/webrev_spec.02/
Diffs from the previous webrev:
1. changed the method names
setServername->setAccessibleServerName
getServername->getAccessibleServerNames
setServernamePattern->setRecognizableServername
getServernamePatterns->getRecognizableServernames
2. behaviors changes:
set/getAccessibleServerName(s) will only make sense in client mode,
and set/getRecognizableServername(s) will only make sense in server mode.
Have not merge all comments from Sean, will do it tomorrow.
Thanks,
Xuelei
On 8/9/2012 9:57 AM, Xuelei Fan wrote:
Please review the design of JEP 114, TLS Server Name Indication (SNI)
Extension.
The webrev: http://cr.openjdk.java.net./~xuelei/7068321/webrev_spec.01/
The following is a list of typical user cases, and the demo code using
this spec for each case.
Typical user cases for client side:
Case 1: I want to access "www.example.com"
sslParameters.setServerName("host_name", "www.example.com");
Set the host name explicit.
It is recommend that the client always specify the host name.
Case 2: The server has some compatibility issues, I do not want
to use server name indication for hostname because of the compatibility
concerns.
sslParameters.setServerName("host_name", "");
I will not use server name indication for host_name.
Case 3: I want to access URL, "https://www.example.com";.
Doing nothing, the provider default behaviors will set the hostname
for me. I don't care about what's the real server name indication.
Note that it is the compatible behaviors of JDK 7.
Case 4: the parameter was previously set to "www.example.com" (see
Case 1), but I want to use the provider default value. I need to remove
the previous set value.
sslParameters.setServerName("host_name", null);
Typical user cases for application server side:
Case 1: I want to accept all kind of server name indication
Doing nothing, the server will ignore the server name indication
Case 2: I want to deny all server name indication of type host name
sslParameters.setServerName("hostname", "");
Case 3: I want to accept all kind of server name indication, but
previously, I have set the server name indication to other values. I
need to reset the value
sslParameters.setServerName("hostname", null);
Case 4: I want to accept server name indication of "www.example.com".
sslParameters.setServerName("host_name", "www.example.com");
Case 5: I want to accept server name indication of
"www.xuelei.com", but I also have alternative names of "www.example.edu"
and "www.example.org".
sslParameters.setServerName("host_name", "www.example.com");
sslParameters.setServernamePattern("host_name",
Pattern.compile("www\\.example\\.(edu|org)"));
Case 6: I want to accept server name indication of
"www.example.com", and I have no alternative name. But I need to make
sure that other component does not set alternative name for me in
previous calling.
sslParameters.setServerName("host_name", "www.example.com");
sslParameters.setServernamePattern("host_name", null);
Typical user cases for dispatch server:
A dispatch server is one server who parsers the server name indication,
and dispatches the connection to the right/real server on a virtual
hosting environment.
See section 2, "The How-To and Scenarios in Example" of the README:
http://cr.openjdk.java.net./~xuelei/7068321/README
I appreciate any feedback about the spec.
Thanks & Regards,
Xuelei
