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

[Weijun Wang]

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 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
>