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.ni...@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 >>>>> >
