Code review of JDK-8072385, Only the first DNSName entry is checked for endpoint identification

[Wang Weijun]

Looks fine to me too.

The isEquivalent() method name is not very precise IMHO, maybe hasSame()? Of course, if has same is what isEquivalent means then it's OK. :-)

Thanks
Max

> On Mar 21, 2015, at 06:00, Jamil Nimeh <jamil.j.nimeh at oracle.com> wrote:
> 
> Hi Xuelei, this looks good to me.
> 
> --Jamil
> 
> On 3/9/2015 10:52 PM, Xuelei Fan wrote:
>> On 3/10/2015 12:27 PM, Jamil Nimeh wrote:
>>> Hi Xuelei, I can't be an official reviewer, but I did look over the code
>>> and it looks pretty good to me.
>> I think it is OK to push if you reviewed the code.
>> 
>>> I did have a couple questions though:
>>> 
>>>   * Line 1570
>>>       o Since the getSubjectAltNames() method is generic enough to take
>>>         any int type it might be worthwhile to put some comments into
>>>         the header stating that only those subjectAltName types that use
>>>         String data should be passed into this function (see follow-on
>>>         question below).
>> Good point.  I added a note comment and updated the webrev.
>> 
>>>   * Line 1580
>>>       o If, for some reason, a maintainer were to ever pass in a
>>>         different altname value, like 4 (X400Address), wouldn't the code
>>>         cause a ClassCastException when trying to cast the returned
>>>         byte[] to a String?  Would it be worthwhile to either do a
>>>         instanceof check on the returned value before the cast or catch
>>>         the CCE before it goes up the stack?  Or is it your desire to
>>>         let the CCE be thrown?  I ask only because earlier in
>>>         isIdentityEquivalent() you take the time to catch parsing
>>>         exceptions and handle it with a log message rather than let it
>>>         unwind the stack.  Would it be worthwhile to do something
>>>         similar with a CCE down in getSubjectAltNames()?
>>> 
>> It's better to check the instance for a good coding experiences.
>> 
>> It's safe here as for subjectAltName type of DNS and IPAddress, the data
>> type is String.  See the spec of
>> X509Certificate.getSubjectAlternativeNames().
>> 
>> We have the similar code in sun/security/util/HostnameChecker.java.
>> 
>> Thanks,
>> Xuelei
>> 
>>> But even as-is, this still looks good to me.
>>> 
>>> --Jamil
>>> 
>>> On 03/09/2015 05:24 PM, Xuelei Fan wrote:
>>>> Ping ...
>>>> 
>>>> wbrev: http://cr.openjdk.java.net/~xuelei/8072385/webrev.00/
>>>> 
>>>> On 3/4/2015 10:51 PM, Xuelei Fan wrote:
>>>>> Hi,
>>>>> 
>>>>> Please review the fix for:
>>>>>    https://bugs.openjdk.java.net/browse/JDK-8072385
>>>>> 
>>>>> In SunJSSE implementation, during endpoint identification, there is a
>>>>> bug in SubjectAlternativeName matching that only the fist DNSName are
>>>>> checked. As may impact some business where host-name alias are used.
>>>>> 
>>>>> The patch is attached.
>>>>> 
>>>>> Thanks,
>>>>> Xuelei
>>>>> 
>