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

[Artem]

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/20130930/58f120bd/attachment.htm>