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

[Xuelei Fan]

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