On 02/14/2014 08:52 PM, Xuelei Fan wrote:
AdaptableX509CertSelector.java
==============================
1. for boolean value checking, I normally use
if (boolean-value) or if (!boolean-value)
I usually do too, but "== false" seemed more readable to me in this
code, perhaps because it is immediately returning false afterwards.
Anyway, it's not a big deal either way to me, so I'll change it to "!".
2. AdaptableX509CertSelector.match(Certificate)
KID can be used to facilitate the path building. I would suggest
check the SKID and SN at first, and then call super.match().
Good point, this will filter out non-matching certs faster. Changed.
3. AdaptableX509CertSelector.clone()
Also want to clone "serial" variabl?
Not necessary, it's immutable.
4. absence of SKID
Per section 4.2.1.2, RFC 5280:
To facilitate certification path construction, this extension MUST
appear in all conforming CA certificates, that is, all certificates
including the basic constraints extension (Section 4.2.1.9) where the
value of cA is TRUE. In conforming CA certificates, the value of the
subject key identifier MUST be the value placed in the key identifier
field of the authority key identifier extension (Section 4.2.1.1) of
certificates issued by the subject of this certificate.
Not a big concerns, I was just wondering we may still want to check
KID although the serial number checking may be optional. This update
allow the absence of subject key ID extension.
Yes I was wondering about that, but I am just preserving the previous
behavior: see lines 167-170 -- which I assume was done for a good reason.
--Sean
Otherwise, looks fine to me.
Xuelei
On 2/13/2014 9:04 PM, 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
