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

[Xuelei Fan]

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