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

[Sean Mullan]

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.

[240,248]: s/trust/trusted

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

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

* DistributionPointFetcher

[703-707] Same comment as PKIXCertPathValidator[234-238] above.

[711] Same comment as PKIXCertPathValidator[242] above.

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

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