On 02/13/2014 10:04 PM, Jason Uh wrote:
Hi Sean,
Looks good to me, but I'm not an official Reviewer.
I have a couple of questions, though.
Sure.
1. This isn't a part of your change, but shouldn't the comment on line
200 of AdaptableX509CertSelector.java read "As for version 3,..." and
not "As for version 2,..."?
Yes, good catch. I changed it.
2. Just curious, any reason why this wasn't just fixed with
void parseAuthorityKeyIdentifierExtension(
AuthorityKeyIdentifierExtension akidext) throws IOException {
+ super.setSubjectKeyIdentifier(null);
+ super.setSerialNumber(null);
if (akidext != null) { ... }
in AdaptableX509CertSelector.java?
I did initially consider that fix, but the problem is the
subjectKeyIdentifier and serialNumber matching rules are slightly
different for X509CertSelector. In X509CertSelector the
subjectKeyIdentifier match fails if the certificate has no subject key
identifier, whereas in AdaptableX509CertSelector it passes. With
serialNumber, the match is only performed in AdaptableX509CertSelector
if the certificate version is < 3.
The previous code for AdaptableX509CertSelector had to workaround that
by setting these criteria to null in certain cases to trick the
superclass matching rules to pass. See lines 167-171 and 182-184. After
reviewing this code, I came to the conclusion that it was much cleaner
to always override the matching rules for these criteria in
AdaptableX509CertSelector and never call the superclass methods to set
those criteria (which is why they are overridden to throw
IllegalArgumentExc).
Hope this makes sense.
--Sean
Thanks!
Jason
On 2/13/14 5:04 AM, Sean Mullan wrote:
See: http://cr.openjdk.java.net/~mullan/webrevs/8025708/webrev/
This fixes a problem with the PKIX CertPathBuilder where it wasn't able
to build a path when the Authority Key Identifier extension of an
intermediate CA cert did not contain a serial number field, and the end
entity cert did.
The problem was in the AdaptableX509CertSelector class. It was reusing
this selector without re-initializing certain fields. I changed the
implementation of this class so that it doesn't have this issue anymore.
Thanks,
Sean
