Code review request, 8020842 IDN do not throw IAE when hostname ends with a trailing dot

[Xuelei Fan]

new webrev: http://cr.openjdk.java.net/~xuelei/8020842/webrev.05/

Added a new test to test illegal hostname in SNIHostName.

Xuelei

On 8/10/2013 10:49 AM, Xuelei Fan wrote:
> Hi Michael,
> 
> It is pretty hard to get the issue solved in SNIHostName in a good
> sharp.  Here is my try to state why we should fix the issue in IDN.
> 
> In SNIHostName, the following hostname are not accepted as valid hostname:
> 1. empty hostname
> 2. hostname ends with a trailing dot
> 3. hostname does not comply to RFC 3490.
> 
> The process in SNIHostName looks like:
> 1. call IDN.toASCII() to convert a string hostname
> 2. check that the return value of #1 is an valid hostname (non-empty,
> non-end-with-tailing-dot).
> 
> At present, the IDN cannot handle the following IDN properly.
> 1. returns "com" for "com."
>    the trailing dot is swallowed.
> 
> 2. throws StringIndexOutOfBoundsException for "."
>     If "." is an valid IDN that comply to RFC 3490, IDN.toASCII() should
> be able to handle it; otherwise, IDN.toASCII() should throw IAE as the
> specification suggested. However, IDN.toASCII(".") throws
> StringIndexOutOfBoundsException, this behavior does not comply the the
> specification:
> 
> 3. throws StringIndexOutOfBoundsException for "example...net"
>    As #2.
> 
> We can address #1 and #2 in SNIHostName, but the checking is overloaded
> as IDN also need to address the issue. And SNIHostName has to know
> what's the separators (".", "\u3002, etc) of IDN in order to check the
> dot character. It is not a good encapsulation, and involved in too much
> about the details of domain name, I think.
> 
> It is a little big hard to address #3 in SNIHostName.
> 
> Both all of above issue can be easily addressed in IDN.  And once IDN
> addressed these issues, the current SNIHostName is able to handle
> invalid hostname (empty, trailing dot, etc) correctly.  We won't need to
> touch SNIHostName any more.
> 
> Please consider it.
> 
> The latest webrev is at:
> http://cr.openjdk.java.net/~xuelei/8020842/webrev.02/
> 
> Thanks,
> Xuelei
> 
> On 8/10/2013 9:13 AM, Xuelei Fan wrote:
>> Hi Michael,
>>
>> I plan to address this issue in SNIHostName.  I have filled another two
>> the potential bugs in IDN.
>>
>> Thank you, and other people, for the feedback.
>>
>> Thanks,
>> Xuelei
>>
>> On 8/9/2013 11:25 PM, Xuelei Fan wrote:
>>> On 8/9/2013 7:31 PM, Michael McMahon wrote:
>>>> I don't see how this fixes the original problem as the SNIHostName spec
>>>> still doesn't like hostnames with a trailing '.'
>>>>
>>> The bug description did not reflect the IDN specification correctly.  If
>>> "com." is a valid IDN, SNIHostName should accept it an an valid
>>> hostname.  The host name in SNIHostName is nothing more or less than an
>>> standard IDN.
>>>
>>> I added a comment in the bug: "com." and "." are valid IDN according the
>>> IDN and domain name specifications.  I will contact the bug reporter
>>> about this point.
>>>
>>> Xuelei
>>>
>>>> I'd prefer to check first where that requirement is coming from, if it is
>>>> actually necessary, and if not consider removing it from SNIHostName.
>>>> If it is necessary, then the check should be implemented in SNIHostName.
>>>>
>>>> Michael
>>>>
>>>> On 09/08/13 05:28, Xuelei Fan wrote:
>>>>> Thanks for your feedback and suggestions.
>>>>>
>>>>> Here is the new webrev:
>>>>>     http://cr.openjdk.java.net/~xuelei/8020842/webrev.02/
>>>>>
>>>>> "." is regarded as valid IDN in this update.
>>>>>
>>>>> Thanks,
>>>>> Xuelei
>>>>>
>>>>> On 8/9/2013 10:50 AM, Xuelei Fan wrote:
>>>>>> On 8/9/2013 10:14 AM, Weijun Wang wrote:
>>>>>>>
>>>>>>> On 8/9/13 9:37 AM, Xuelei Fan wrote:
>>>>>>>> On 8/9/2013 9:22 AM, Weijun Wang wrote:
>>>>>>>>> I tried nslookup. Those with ".." inside are illegal,
>>>>>>>>>
>>>>>>>>> $ nslookup com..
>>>>>>>>> nslookup: 'com..' is not a legal name (empty label)
>>>>>>>>>
>>>>>>>>> but
>>>>>>>>>
>>>>>>>>> $ nslookup .
>>>>>>>>> Server:        192.168.10.1
>>>>>>>>> Address:    192.168.10.1#53
>>>>>>>>>
>>>>>>>>> Non-authoritative answer:
>>>>>>>>> *** Can't find .: No answer
>>>>>>>>>
>>>>>>>> Thanks for the testing.  The behaviors are the same as this fix now.
>>>>>>> No exactly. It seems nslookup still regards "." legal but just cannot
>>>>>>> find an IP for it.
>>>>>>>
>>>>>> I'm not sure whether a root domain name can be stand alone.  Root label
>>>>>> is not considered as a label in IDN.  I think it is safe to regard that
>>>>>> "." is not a valid IDN as it contains no label.  Anyway, it is a corner
>>>>>> case.
>>>>>>
>>>>>> There are many online IDN conversion web services, some of them can
>>>>>> convert ".", some of the cannot.  In the present implementation, we
>>>>>> cannot recognize ".", and IDN.toASCII(".") throws
>>>>>> StringIndexOutOfBoundsException.  With this fix, I was wondering IAE is
>>>>>> a better exception for IDN.toASCII(".").
>>>>>>
>>>>>>>> Learn something new today to use nslookup.
>>>>>>>>
>>>>>>>>> Also, since this bug was originally about SNIHostName, do you need to
>>>>>>>>> add some extra restriction there to reject "oracle.com." things?
>>>>>>>>>
>>>>>>>> No, we cannot restrict the format of IDN in SNIHostName more than in
>>>>>>>> IDN. However, we may need to rethink about the comparing of two
>>>>>>>> IDN, for
>>>>>>>> example, "example.com." should equal to "example.com".  I want to
>>>>>>>> consider it in another bug.
>>>>>>> Not sure. Does the spec say IDN and SNIHostName are equivalent sets?
>>>>>>> And
>>>>>>> it's not one is another's subset?
>>>>>>>
>>>>>> Per TLS specification, host name in SNI is an IDN.  The spec of
>>>>>> SNIHostname says, "hostname is not a valid Internationalized Domain Name
>>>>>> (IDN) compliant with the RFC 3490 specification". The spec in
>>>>>> SNIHostName has the same means as IDN.  I won't want to add additional
>>>>>> restrict beyond the specification of an IDN.
>>>>>>
>>>>>> Xuelei
>>>>>>
>>>>>>>> Can I push the changeset?
>>>>>>> I think it's better to ask someone in the networking team to make the
>>>>>>> suggestion. From what I read Michael in this thread, he does not seem
>>>>>>> totally agreed with your code changes (at least not the 00 version).
>>>>>>>
>>>>>>> Thanks
>>>>>>> Max
>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Xuelei
>>>>>>>>
>>>>>>>>> Thanks
>>>>>>>>> Max
>>>>>>>>>
>>>>>>>>> On 8/9/13 8:41 AM, Xuelei Fan wrote:
>>>>>>>>>> Ping.
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Xuelei
>>>>>>>>>>
>>>>>>>>>> On 8/7/2013 11:17 PM, Xuelei Fan wrote:
>>>>>>>>>>> Please review the new update:
>>>>>>>>>>>
>>>>>>>>>>> http://cr.openjdk.java.net./~xuelei/8020842/webrev.01/
>>>>>>>>>>>
>>>>>>>>>>> With this update, "com." is valid (return "com."); "." and
>>>>>>>>>>> "example..com" are invalid.  And IAE will be thrown for invalid
>>>>>>>>>>> IDN.
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> Xuelei
>>>>>>>>>>>
>>>>
>>>
>>
>