All suggestion are accepted. The new webrev:
http://cr.openjdk.java.net/~xuelei/7011497/webrev.01/
Thanks,
Xuelei
On 1/27/2011 10:20 PM, Sean Mullan wrote:
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
