Looks fine to me. BTW, you may also want to consider Weijun's comments.
Thanks, Xuelei On 9/30/2013 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 >
