I’ve made that change at l.970 and updated webrev.02 in-place.
Thanks.
> On 12 Aug 2016, at 19:53, Sean Mullan <sean.mul...@oracle.com> wrote:
>
> Looks fine, although I would probably avoid calling checkX509Certs on line
> 970 and just checking the cert right there to avoid creating the array which
> is not needed.
>
> --Sean
>
> On 08/12/2016 11:07 AM, Vincent Ryan wrote:
>> I’ve moved the X.509 check to earlier in the code and reverted the changes
>> to the validateChain method.
>> Updated webrev is at:
>> http://cr.openjdk.java.net/~vinnie/8163503/webrev.02/
>>
>>
>>
>>> On 10 Aug 2016, at 21:15, Sean Mullan <sean.mul...@oracle.com> wrote:
>>>
>>> On 08/10/2016 12:39 PM, Vincent Ryan wrote:
>>>> Yes they could be merged but the first loop iterates over all the certs
>>>> and the second one iterates over all but the final cert.
>>>> And the special case of a 1-cert chain also needs to be handled. I think
>>>> it’s a little clearer to leave them separate.
>>>
>>> Agreed, but it's probably better to check these earlier and bail out if
>>> they are not all X509Certificates, for example, right at the beginning of
>>> engineSetKeyEntry. There is no need to proceed with decoding keys, etc
>>> since it is already a violation of the supported API parameters.
>>>
>>> --Sean
>>>
>>>>
>>>> An updated webrev is at:
>>>> http://cr.openjdk.java.net/~vinnie/8163503/webrev.01/
>>>>
>>>> Thanks.
>>>>
>>>>> On 10 Aug 2016, at 02:04, Xuelei Fan <xuelei....@oracle.com> wrote:
>>>>>
>>>>> The for loop at line 1507 and 1520 may be merged together.
>>>>>
>>>>> Xuelei
>>>>>
>>>>> On 8/10/2016 8:38 AM, Weijun Wang wrote:
>>>>>> I thought I've seen this webrev before.
>>>>>>
>>>>>> Why not just throw a KeyStoreException in validateChain()?
>>>>>>
>>>>>> --Max
>>>>>>
>>>>>> On 8/10/2016 2:14, Vincent Ryan wrote:
>>>>>>> Please review this fix to improve the error handling for attempts to
>>>>>>> store a Certificate object in PKCS12 keystore.
>>>>>>> The PKCS12 keystore implementation supports storing only
>>>>>>> X509Certificate objects but the KeyStore API allows Certificate objects.
>>>>>>> This fix rejects attempts to store non-X.509 certificates and throws a
>>>>>>> KeyStoreException.
>>>>>>>
>>>>>>> Thanks.
>>>>>>>
>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8163503
>>>>>>> Webrev: http://cr.openjdk.java.net/~vinnie/8163503/webrev.00/
>>>>>>>
>>>>>>>
>>>>>
>>>>
>>
