I think the fix is adequate and necessary.
One problem: lines 367-373 adds a new IAE to ToUnicode but the method
should not fail forever.
And some small comments on styles etc.
On 8/12/13 9:09 AM, Xuelei Fan wrote:
new webrev: http://cr.openjdk.java.net/~xuelei/8020842/webrev.05/
Lines 123 and 185:
184 p = q + 1;
185 if (p < input.length() || q == (input.length() - 1)) {
186 // has more labels, or keep the trailing dot as at
present
187 out.append('.');
188 }
I prefer
if (q < input.length()) { // Ah, a dot!
out.append('.');
}
p = q + 1;
Lines 282, 335, 270: Insert a blank after "if".
Lines 284 and 372: nslookup uses "empty label", which I like better.
Lines 453 and 460: Personally I don't like the parenthesis for the whole
return value, but you have your choice.
Lines 280 and 333: How about we call them steps 8a and 8b?
Added a new test to test illegal hostname in SNIHostName.
Excellent. Otherwise I will be wondering why the fix in IDN could solve
the problem as described in the bug description.
Thanks
Max
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
