code review 7011497: new CertPathValidatorException.BasicReason enum constant for constrained algorithm

[Sean Mullan]

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