Ok to me. Xuelei
> On Feb 17, 2014, at 9:57 PM, Sean Mullan <sean.mul...@oracle.com> wrote: > >> 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 >> >
