<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Hi Max,<br>
<br>
I updated the fix according to your comments:<br>
<br>
<meta http-equiv="content-type" content="text/html;
charset=ISO-8859-1">
<a href="http://cr.openjdk.java.net/%7Ekshefov/8025123/webrev.02/">http://cr.openjdk.java.net/~kshefov/8025123/webrev.02/</a><br>
<br>
Artem<br>
<br>
On 09/30/2013 06:54 PM, Weijun Wang wrote:<br>
</div>
<blockquote cite="mid:5249908B.9090804@oracle.com" type="cite">Now
that we only check "localhost" and "localhost.localdomain", the
following hack in the SSL.java test is not necessary any more:
<br>
<br>
92 // Run this after KDC, so our own DNS service can be
started
<br>
93 try {
<br>
94 server =
InetAddress.getLocalHost().getHostName().toLowerCase();
<br>
95 } catch (java.net.UnknownHostException e) {
<br>
96 server = "localhost";
<br>
97 }
<br>
<br>
Just write
<br>
<br>
server = "host." + OneKDC.REALM.toLowerCase(Locale.US);
<br>
<br>
There is another coding style thing in
KerberosClientKeyExchangeImpl.java:
<br>
<br>
+ if ("localhost".equals(serverName) ||
<br>
+ "localhost.localdomain".equals(serverName)) {
<br>
<br>
When a long time is broken to two, the second line should be
indented twice the normal width, i.e.
<br>
<br>
+ if ("localhost".equals(serverName) ||
<br>
+ "localhost.localdomain".equals(serverName)) {
<br>
<br>
Everything nice is fine.
<br>
<br>
Thanks
<br>
Max
<br>
<br>
On 9/30/13 10:32 PM, Artem wrote:
<br>
<blockquote type="cite">Updated webrev:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~kshefov/8025123/webrev.01/">http://cr.openjdk.java.net/~kshefov/8025123/webrev.01/</a>
<br>
<a class="moz-txt-link-rfc2396E" href="http://cr.openjdk.java.net/%7Ekshefov/8025123/webrev.01/"><http://cr.openjdk.java.net/%7Ekshefov/8025123/webrev.01/></a>
<br>
<br>
- broke lines exceed 80 characters
<br>
- the code updated accrding to
<br>
<a class="moz-txt-link-freetext" href="http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-142311.html#449">http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-142311.html#449</a>
<br>
- updated copy right dates
<br>
<br>
Artem
<br>
<br>
On 09/30/2013 06:04 PM, Xuelei Fan wrote:
<br>
<blockquote type="cite">It's a good catch and wonderful fix.
Thanks, Artem!
<br>
<br>
The fix looks fine to me, except three very minor coding style
comments.
<br>
1. we used to have only 80 characters in each line.
<br>
I think there might some lines exceed 80 characters, would
you mind
<br>
break these lines to keep the style consistent? It is
especially
<br>
friendly to users like me that use vi terminal.
<br>
<br>
2. KerberosClientKeyExchangeImpl.java
<br>
<br>
+ if(localHost != null) serverName = localHost;
<br>
<br>
Personally, I prefer to use (with white space after "if", and
"{}" over
<br>
the statement) (see section 7.4 of [1]).
<br>
<br>
+ if (localHost != null) {
<br>
+ serverName = localHost;
<br>
+ }
<br>
<br>
3. Not necessary, but I normally update the copy right date if
it is not
<br>
up-to-date. Still a very personal preference.
<br>
<br>
Thanks,
<br>
Xuelei
<br>
<br>
[1]:
<br>
<a class="moz-txt-link-freetext" href="http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-142311.html#449">http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-142311.html#449</a>
<br>
<br>
On 9/30/2013 9:27 PM, Artem wrote:
<br>
<blockquote type="cite">Hello,
<br>
<br>
please review this fix for 8:
<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~kshefov/8025123/webrev.00/">http://cr.openjdk.java.net/~kshefov/8025123/webrev.00/</a>
<br>
<a class="moz-txt-link-rfc2396E" href="http://cr.openjdk.java.net/%7Ekshefov/8025123/webrev.00/"><http://cr.openjdk.java.net/%7Ekshefov/8025123/webrev.00/></a>
<br>
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8025123">https://bugs.openjdk.java.net/browse/JDK-8025123</a>
<br>
<br>
SNI APIs were introduced in JDK 8, but TLS Kerberos client
<br>
implementation does not take into account SNI host name when
it requests
<br>
TGS.
<br>
<br>
For example, there are two HTTPS sites at the same machine:
<br>
<br>
https_service_1.test.machine
<br>
https_service_2.test.machine
<br>
<br>
KDC contains records for both HTTPS services:
<br>
<br>
<a class="moz-txt-link-abbreviated" href="mailto:host/https_service_1.test.machine@TEST.REALM">host/https_service_1.test.machine@TEST.REALM</a>
<br>
<a class="moz-txt-link-abbreviated" href="mailto:host/https_service_2.test.machine@TEST.REALM">host/https_service_2.test.machine@TEST.REALM</a>
<br>
<br>
Client wants to request 'https_service_1.test.machine'
service, and it
<br>
sets SNI host name 'https_service_1.test.machine' during
handshaking.
<br>
Currently TLS Kerberos client implementation requests TGS
for
<br>
'<a class="moz-txt-link-abbreviated" href="mailto:host/test.machine@TEST.REALM">host/test.machine@TEST.REALM</a>' instead of
<br>
'<a class="moz-txt-link-abbreviated" href="mailto:host/https_service_1.test.machine@TEST.REALM">host/https_service_1.test.machine@TEST.REALM</a>'
<br>
<br>
Changes:
<br>
- ClientHandshaker uses SNI host name if it is specified.
<br>
- If client gets server name extension in server hello then
it is
<br>
considered as SNI confirmation, so SNI hostname must be used
to build
<br>
Kerberos service principal name. If there is no SNI
confirmation, client
<br>
uses SNI first and then fallback to getHostSE().
<br>
- KerberosClientKeyExchangeImpl.getServiceTicket() method
used to change
<br>
a hostname for service principal if loopback address was
used. But since
<br>
we introduced SNI, using IP address to make the decision
does not work
<br>
any more. For compatibility reasons, the method checks that
"localhost"
<br>
or "localhost.localdomain" are passed (they are two known
loopback
<br>
hostname). If so, it still tries to get the local hostname.
<br>
- Added a test case for test/sun/security/krb5/auto/SSL.java
<br>
<br>
I have tested this with available reg/jck/sqe tests, no
issues found.
<br>
<br>
Artem
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
</blockquote>
<br>
</body>
</html>