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