On 1/27/11 6:43 AM, Xuelei Fan wrote:
webrev: http://cr.openjdk.java.net/~xuelei/7011497/webrev/
* PKIXCertPathValidator
[234-238]: these fields are set to null by default. I don't think you need to
invoke the set methods.
Yes. But in order to remind that we don't want to check the validity, I have one
line comment:
"// no certificate and private key valid check for trusted certificate"
I don't really think this comment is necessary, since we didn't check these
fields prior to your changes. Can we just leave it out?
BTW, I recommend you post each revision of the webrev (ex: webrev.00, webrev.01,
...) This way you can still go back and get the previous version which can be
useful as you make incremental updates and review each revision.
[242]: You don't need to check the length of the array here. If
X509Certificate.getKeyUsage returned an array of the wrong length then that
would be a non-compliant implementation and you should just let the code throw
an ArrayIndexOOB exception.
Good catch.
I don't like runtime exception here, as it expose to DOS attack if the client
send improper data. I updated the code to throw CRLException or
CertPathValidatorException, "Encounter non-compliant X.509 certificate: no
enought KeyUsage bits". Does it sound reasonable to you?
I don't really think this is necessary. X509Certificate.getKeyUsage states that
it returns null or a boolean array of length 9. These X509Certificates are
instantiated from an CertificateFactory from a trusted provider. We shouldn't
have to code around potentially non-compliant implementations that don't conform
to the standard API.
* AdaptableX509CertSelector
[151-154]: this block could be moved into the previous conditional block since
version is < 3. Also, can you explain this block? It seems like you would want
to return false (as X509CertSelector.match does) if there is no subject key
id, but I assume there is an important use case that I'm missing here.
Ooops, there was a mistake in my code. The logic connector should be "||" but
not "&&":
- 151 if (version< 3&& xcert.getExtensionValue("2.5.29.14") == null) {
+ 151 if (version< 3 || xcert.getExtensionValue("2.5.29.14") == null) {
The purpose of the block is to ensure, conservatively, that if there is no
SubjectKID extension in the certificate, we don't check it by force for
compatibility.
Ok.
Why did you remove the signature verification check of the CRL that was
previously in ForwardBuilder.issues. Is that because we verify it later (line
642)?
Yes, I think the signature will be finally be verified later in line 642.
Another point that I think we can remove the check is that for version 1 and 2
certificates, the CA should not issue multiple CRL signers with the same
subject; and for version 3 certificate, the AKID and AKID should be used
instead. So we don't need to verify the signature twice.
Ok.
--Sean
