[9] request for review 8044445: Create PKCS12 Keystores by Default

Sean Mullan sean.mullan at oracle.com
Wed Dec 17 18:59:54 UTC 2014


On 12/16/2014 07:12 PM, Vincent Ryan wrote:
> I’ve generated a new webrev to address your comments below:
>      http://cr.openjdk.java.net/~vinnie/8044445/webrev.04/
>
> Thanks.
>
>
> On 16 Dec 2014, at 21:11, Sean Mullan <sean.mullan at oracle.com> wrote:
>
>> Here are my comments on the latest webrev:
>>
>> * General
>>
>> - for the engineProbe methods, if a DataInputStream is not specified, I think you should wrap the stream in a DataInputStream, rather than returning false.
>
> KeyStore.getInstance(File,…) always supplies a DataInputStream to KeyStoreSpi.engineProbe
> so it is not necessary to wrap.

That's an implementation detail though. Another vendor's implementation 
of the new KeyStore.getInstance methods may not do that. If you want to 
guarantee that, you should change the InputStream parameter of the 
engineProbe method to a DataInputStream. Unless you think engineProbe 
may be called/used in other circumstances where a DataInputStream is not 
appropriate, which in that case you change the subclasses to do an 
instanceof and  wrap it.

>> * keytool/Main
>>
>> - can you explain more about why the provider is ignored on lines 812-817, 1919-1925? This doesn't seem like it is compliant with keytool -- if a provider is specified, it should be used.
>
> The probing mechanism is better equipped to determine the exact keystore type.
>
> For example, if an application specifies no -storetype option then it implicitly gets the
> default keystore type which now differs between JDK 9 and earlier releases.
> The new compatibility mode can handle this scenario so that the application doesn’t
> break. However, in compatibility mode the instantiated keystore type might not match
> the keystore format.
>
> Whereas when probing is employed the instantiated keystore type always matches
> the keystore format.

Ok, but I think you should do that only if both a provider and storetype 
are not specified. If the -providerName option is specified, you need to 
honor that. It looks like the code is not doing that right now.

--Sean



More information about the security-dev mailing list