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
