(2nd round) Proposed API Changes for JEP 114: TLS Server Name Indication (SNI) Extension

Wed Aug 15 02:45:48 UTC 2012

I only reply on the items that I may need more review.

On 8/15/2012 7:54 AM, Brad Wetmore wrote:
>>> SSLParameters.java
>>> ==================
>>> 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.
>>>
>> I am also not sure about the strong desire that the SNI should be
>> ordered. But Weijun prefers it to be ordered because he think the SNI in
>> RFC6066 is defined as an ordered sequence.
>>
>>        struct {
>>            ServerName server_name_list<1..2^16-1>
>>        } ServerNameList;
> 
> I've gone through RFC6066 pretty carefully, and I'm not seeing any
> indication that this should be ordered.
> 
> In RFC 2246, if there is an ordering required, such as
> cipher_suites/compression/certs/cert_requests, it's specifically called
> out.  For any other "lists", it is not specified.
> 
> Section 7.4.1.2
> 
>    The CipherSuite list, passed from the client to the server in the
>    client hello message, contains the combinations of cryptographic
>    algorithms supported by the client in order of the client's
>    preference (favorite choice first).
> 
>    ...deleted...
> 
>    The client hello includes a list of compression algorithms supported
>    by the client, ordered according to the client's preference.
> 
>    ...deleted...
> 
>    cipher_suites
>        This is a list of the cryptographic options supported by the
>        client, with the client's first preference first.
> 
>    ...deleted...
> 
>    compression_methods
>        This is a list of the compression methods supported by the
>        client, sorted by client preference.
> 
> Section 7.4.2
> 
>    certificate_list
>        This is a sequence (chain) of X.509v3 certificates. The sender's
>        certificate must come first in the list.
> 
> Section 7.4.4
> 
>        certificate_types
>               This field is a list of the types of certificates requested,
>               sorted in order of the server's preference.
> 
> Weijun, did you see something else in your read of the spec that
> indicates an ordering?  If not, maybe we should not put in the order
> wording now.  If it turns out we do need it, we can always add that
> wording later in a later release, but it will be impossible to remove it
> if we add it now.
> 
I think you are right. If Weijun has no other concerns, I will remove
related description.

>> The SSLParameters will not check the validity of the value. The checking
>> will be checked later during handshaking. With the revised APIs, empty
>> string is not valid. An exception will be thrown during the preparing of
>> SNI extension.
>>
>>> 308:  What kinds of things are you thinking of writing in the Additional
>>> Standard Names doc for "value"?
>>>
>> That was a hot discussion, I think we had talked about it a lot.
> 
> You missed my original point, sorry for dragging you into that
> discussion.  I was talking about the "value" field not the "name" field.
>  In that section, you indicated that the "value" field was going to be
> talked about in the Standard Algorithm Name document.  I was wondering
> what you were going to be talking about there, just that this needs to
> be an ASCII value of a DNS hostname if it's a host_name type?  For
> example, a table with:
> 
>     type        value
>     ====        =====
>     host_name   ASCII representation of a DNS hostname
> 
Sorry, I did misunderstand your point. Yes, the doc will looks like your
above example.

>>> SSLSocketFactory.java
>>> =====================
>>>
>>> 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.
>>>
>> To make it more straightforward, I closed the door for this socket being
>> in client mode in the revised APIs.
> 
> As you like.  I would think adding an InputStream to the existing API is
> just as straightforward also for learning the API, but since we would
> only expect this to be used in server mode, your point has merit.
> 
>>> 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.
>>>
>> Not really need the description of the mode. The existing description
>> has already implied that the return socket must be in client mode.
>>
>>       * @param host the server host
>>       * @param port the server port
>>       * @return a socket connected to the specified host and port
> 
> You mean because it's a "named host"?  I can kind of buy that.  But
> currently it's kind of subtle that it will be in client mode.  I was
> thinking we should make it more clear.
> 
I mean the "host" param is the *server* host, and the "port" param is
the *server* port, and the returned value is connected to the server
"host" and server "port", so it should be a client.  But, it is not
straightforward. I'm OK to make it more clear.

>>> 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.
>>>
>> You're right. It is allowed to set more options in the returned socket
>> before kick off handshake.
>>
>>> 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.
>>>
>> I'm not sure I understand your point. Please comment it again with the
>> revised APIs if you still have concerns.
> 
> I just didn't understand much of this paragraph.
> 
> 1. You have to call this method, then set up your parameters, then start
> your handshaking.  So the first half of this sentence doesn't apply.
> 
Oh, I know your concerns. What I want to express is that before the
calling to method, the caller should not do real handshaking. The logic
I concerned looks like:
    // 1. accept a socket
    // 2. read ClientHello and reply ServerHello to output stream.
    // 3. call this method
    SSLSocket sslSocket = (SSLSocket)sslSocketFactory.createSocket(
                    socke, inputStream, true);

because the handshaking has started in step 2, then in step 3, we cannot
get a proper SSLSocket.

> 2. "consumed network data is resumable" wasn't clear either.  To me this
> should mean that you can obtain the data which has already been read
> from "s".
> 
Yes, need wordsmithing here.

> 3. "Otherwise, the behavior of the return socket is not defined" lost
> me.  Does this mean that that SSLParameters and assorted settings are
> not otherwise defined?
> 
See above example.

> I think you could delete this paragraph.
> 
> From your second email:
> 
>> Thought more about the design, I would have to say that we cannot return
>> the default value in sslParameters.getServerNames().  Otherwise, the
>> following two block of codes look very weird to me:
>>      // case one:
>> 1   SSLparameters sslParameters = sslSocket.getSSLParameters();
>> 2   sslParameters.clearServerName("host_name");
>> 3   Map<String, String> names = sslParameters.getServerNames();
>> 4   sslSocket.setSSLParameters(sslParameters);
>> 5   sslParameters = sslSocket.getSSLParameters();
>> 6   names = sslParameters.getServerNames();
>>
>> In line 3, the returned map does not contain "host_name" entry. But in
>> line 6, it may be expected that no "host_name" in the returned map. But
>> if we want to return default values, line 6 do need to return a map
>> containing "host_name".  The behavior is pretty confusing. We may want
>> to try avoid the confusion.
> 
> I'm not following your confusion, it seemed pretty straightforward to
> me, it works much like CipherSuites.  We have a set of ciphersuites
> which are enabled by default.  We can turn some off by using
> SSLParameters.  Expanding a bit on your example here, I'll describe what
> I think would happen internally/externally:
> 
> 1    SSLSocket sslSocket = mySSLSocketFactory.createSocket(
>         "www.example.com", 443);
> 
> mySSLSocketFactory sets any initial parameters as usual.  SSLSocketImpl
> knows it's connecting to www.example.com and automatically stores
> "host_name" -> "www.example.com" in its local host data (map or separate
> variables).
> 
> 2   SSLparameters sslParameters = sslSocket.getSSLParameters();
> 
> SSLSocketImpl.getSSLParameters() creates a SSLParameters, and sets the
> hostmap to the one value "host_name" -> "www.example.com"
> 
> If the application want to get the "default values", they just pull them
> out of the SSLParameters here
> 
> 3   sslParameters.clearServerName("host_name");
> 
> Or sslParameters.setServerName("host_name", null)?
> 
> User just decided to clear it.  Ok, that's what we do.  It becomes an
> empty map in SSLParameters.
> 
> 4   Map<String, String> names = sslParameters.getServerNames();
> 
> Returns empty Map.
> 
As far as good.

> 5   sslSocket.setSSLParameters(sslParameters);
> 
> SSLSocketImpl.setSSLParameters is empty, so SSLSocketImpl takes this
> SSLParameters and as a result, clears it's internal "host_name" map to
> null, and thus won't send anything out since it's empty.
> 
We have problems here.  We need to support that if an application does
not specified host_name value, we should use default values.
  I.   SSLParameters sslParameters = new SSLParameters();
  II.  sslParameters.setCipherSuites(...);
  III. SSLSocket sslSocket =
          sslSocketFactory.createSocket("www.example.com", 443)
  IV.  sslSocket.setSSLParameters(sslParameters);

Before line IV and after line II, the sslParameters.getServerNames() are
empty. In line IV, we need to make sure the internal "host_name",
"www.example.com" is used as default value, and send it to server in
SNI.  That's the default behaviors in JDK 7.  We cannot break it without
strong desires.

I think it means that we cannot clear the internal "host_name" when the
sslParameters.getServerNames() return empty.

Does it make sense to you?

Thanks,
Xuelei

> 6   sslParameters = sslSocket.getSSLParameters();
> 
> SSLSocketImpl.getSSLParameters() creates a SSLParameters, which sees
> that there's no name indication, so it creates an empty name map and
> stores in SSLParameters.
> 
> 7   names = sslParameters.getServerNames();
> 
> returns empty.
> 
> It's no longer the default value, because they have specifically set the
> value.
> 
> HTH,
> 
> Brad