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

[Artem]

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20131001/839bc344/attachment.htm>