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

Thu Jan 27 11:43:52 UTC 2011

webrev: http://cr.openjdk.java.net/~xuelei/7011497/webrev/

On 1/27/2011 1:41 AM, Sean Mullan wrote:
> On 1/24/11 9:28 PM, Xuelei Fan wrote:
>> webrev: http://cr.openjdk.java.net/~xuelei/7011497/webrev/
>>
>> Changed to use X509CertSelector to select the trust certificate.
>> Comparing with the previous webrev, the following files are updated to
>> use X509CertSelector:
>> DistributionPointFetcher.java
>> ForwardBuilder.java
>> PKIXCertPathValidator.java
>> AdaptableX509CertSelector.java
>>
>> You can only review the above updates.
>
> * 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"

>
> [240,248]: s/trust/trusted
>
Updated.

> [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?

> * 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.

> * DistributionPointFetcher
>
> [703-707] Same comment as PKIXCertPathValidator[234-238] above.
>
> [711] Same comment as PKIXCertPathValidator[242] above.
>
Updated as the updates in PKIXCertPathValidator.

> 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.

Thanks,
Xuelei
> --Sean
>
>
>> Thanks,
>> Xuelei
>>
>> On 1/15/2011 5:37 AM, Sean Mullan wrote:
>>> Hi Andrew,
>>>
>>> Did you consider using the existing X509CertSelector class to match on
>>> the authority key identifier? I actually think this should work, and it
>>> will avoid having to create the AKIDMatchState class. Take a look at 
>>> the
>>> ForwardBuilder.getMatchingCACerts method towards the end, where it gets
>>> the AuthorityKeyIdentifierExtension. Can you create a similar
>>> X509CertSelector to select the proper trust anchor?
>>>
>>> --Sean
>>>
>>>
>>> On 1/14/11 12:32 PM, Xuelei Fan wrote:
>>>> On 1/15/2011 1:30 AM, Xuelei Fan wrote:
>>>>> Hi Sean,
>>>>>
>>>>> webrev:
>>>> http://cr.openjdk.java.net/~xuelei/7011497/webrev/
>>>>
>>>>> Would you please review the update again. I integrate the fix for
>>>>> 7011497 and 7012357 together.
>>>>>
>>>>> Comparing with previous webrev, the following updates are unchanged:
>>>>> src/share/classes/java/security/cert/CertPathValidatorException.java
>>>>> src/share/classes/sun/security/provider/certpath/AlgorithmChecker.java 
>>>>>
>>>>> src/share/classes/sun/security/validator/SimpleValidator.java
>>>>> other test files.
>>>>>
>>>>>
>>>>> The following are new changes for CR 7012357:
>>>>> src/share/classes/sun/security/provider/certpath/ForwardBuilder.java
>>>>> src/share/classes/sun/security/provider/certpath/PKIXCertPathValidator.java 
>>>>>
>>>>>
>>>>> test/sun/security/provider/certpath/DisabledAlgorithms/CPValidatorEndEntity.java 
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Thanks,
>>>>> Xuelei
>>>>>
>>>>> On 1/14/2011 11:10 AM, Xuelei Fan wrote:
>>>>>> We don't checking the SKID and AKID during searching for the trust
>>>>>> anchor.
>>>>>>
>>>>>> I have filled a new CR for the issue, 7012357, Improve trust anchor
>>>>>> searching method during cert path validation.
>>>>>>
>>>>>> I will have this commented out block in CPValidatorEndEntity.java. I
>>>>>> will use this test case for CR 7012357.
>>>>>>
>>>>>> Thanks,
>>>>>> Xuelei
>>>>>>
>>>>>> On 1/14/2011 12:44 AM, Xuelei Fan wrote:
>>>>>>> I just realized, if subject KID and issuer KID works, the cert path
>>>>>>> validation should be able to find the proper trust anchor.  I will
>>>>>>> look
>>>>>>> into the issue tomorrow.
>>>>>>>
>>>>>>> Xuelei
>>>>>>>
>>>>>>> On 1/14/2011 12:27 AM, Xuelei Fan wrote:
>>>>>>>> On 1/14/2011 12:05 AM, Sean Mullan wrote:
>>>>>>>>> On 1/13/11 6:38 AM, Xuelei Fan wrote:
>>>>>>>>>> Hi Sean,
>>>>>>>>>>
>>>>>>>>>> Would you please review the fix for CR 7011497?
>>>>>>>>>>
>>>>>>>>>> http://cr.openjdk.java.net/~xuelei/7011497/webrev/
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Xuelei
>>>>>>>>>
>>>>>>>>> CPValidatorEndEntity.java:
>>>>>>>>>
>>>>>>>>>    307         /* coment out useless trust anchor
>>>>>>>>>    308         is = new
>>>>>>>>> ByteArrayInputStream(trustAnchor_SHA1withRSA_512.getBytes());
>>>>>>>>>    309         cert = cf.generateCertificate(is);
>>>>>>>>>    310         anchor = new TrustAnchor((X509Certificate)cert, 
>>>>>>>>> null);
>>>>>>>>>    311         anchors.add(anchor);
>>>>>>>>>    312         */
>>>>>>>>>
>>>>>>>>> Why do you leave this code in with this comment?
>>>>>>>>>
>>>>>>>> If I have this block. The cert path validation cannot find the 
>>>>>>>> proper
>>>>>>>> trust anchor. As there are two trusted certificates, they are
>>>>>>>> almost the
>>>>>>>> same except the key size (one key size is 1024, another one is 
>>>>>>>> 512).
>>>>>>>>
>>>>>>>> In cert path validation, once a trust anchor found, if the
>>>>>>>> signature is
>>>>>>>> not valid, I think no more effort to test more trust anchors.
>>>>>>>>
>>>>>>>> I was wondering whether it is worthy to try more trust anchors. 
>>>>>>>> It's
>>>>>>>> expensive!
>>>>>>>>
>>>>>>>> Thanks for the review.
>>>>>>>>
>>>>>>>> Xuelei
>>>>>>>>
>>>>>>>>> Otherwise, looks good.
>>>>>>>>>
>>>>>>>>> --Sean
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>