Code review request: 8025123: SNI support in Kerberos cipher suites

[Xuelei Fan]

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 at TEST.REALM
> host/https_service_2.test.machine at 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 at TEST.REALM' instead of
> 'host/https_service_1.test.machine at 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