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