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

[Vincent Ryan]

One further update: introduced a new security property to explicitly disable compatibility mode
for JKS and PKCS12 keystores. By default, compatibility mode is enabled for JKS and PKCS12
to aid applications that expect the previous default keystore type (JKS).

http://cr.openjdk.java.net/~vinnie/8044445/webrev.03/



On 12 Dec 2014, at 14:59, Vincent Ryan <vincent.x.ryan at oracle.com> wrote:

> Thanks for those comments Sean. I’ve addressed most of them in the webrev below.
> 
> Other notable changes since the previous webrev include:
> 
> - modified the two getInstance(File,…) methods to also load the keystore entries
> - PKCS12 keystore support now available in both SUN and SunJSSE providers
> - test fixes
> 
> http://cr.openjdk.java.net/~vinnie/8044445/webrev.02/
> 
> 
> On 2 Dec 2014, at 15:10, Sean Mullan <sean.mullan at oracle.com> wrote:
> 
>> Comments:
>> 
>> * JceKeyStore
>> 
>> [906] add @Override to engineProbe method
> 
>> 
>> * KeyStore
>> 
>> [1615] use @throws instead of @exception. Should probably also add an @throws NullPointerException if keystore is null.
>> 
>> You need to define what happens if the File does not exist. You could either throw a KeyStoreException, or you could instantiate a KeyStore using the default keystore type. Each have their pros/cons. The latter seems a little better as this method could then be used to open existing keystores or for creating new keystores.
>> 
>> [1638] This is a provider configuration error, but there still may be other KeyStoreSpis that can parse the file. I would probably just log this and continue to check other providers.
>> 
>> [1654] Same here. I would log this and continue to check other providers. Method currently say it throws KeyStoreExc only if no Provider supports the file, but we have not necessarily checked all providers yet.
>> 
>> * KeyStoreSpi
>> 
>> [606] s/@exception/@throws/. Should probably also add an @throws NullPointerException if stream is null.
>> 
>> You need to define that the method returns false by default.
>> 
>> * PKCS12KeyStore
>> 
>> [2340] @Override
>> [2342-51] indentation should be 4 spaces
>> [2353,2364] these should be static fields
>> 
>> * JavaKeyStore
>> 
>> [54] why did you make this public?
> 
> Because its JKS subclass is now called from the sun.security.pkcs12 package
> 
> 
>> [816] @Override
>> 
>> * keytool/Main
>> 
>> [814, 1919] I'm not sure if it is correct to ignore the storetype. I think getInstance(File) should only be called if the storetype is not specified.
> 
> I wanted to take advantage of the new keystore probing mechanism.
> Maybe we should honour the storetype if it’s neither JKS nor PKCS12, otherwise probe?
> 
> 
>> 
>> * KeyStoreDelegator
>> 
>> overridden methods should have @Override tag
>> 
>> Can you make this class final with a private ctor, and instead have a static method to return instances?
> 
> Then I have to duplicate each of the KeyStoreSpi methods in both PKCS12KeyStore.DualFormatPKCS12
> and JavaKeyStore.DualFormatJKS.
> 
> 
>> 
>> The engine methods throw NPE if the keystore has not been loaded. This is not consistent with the current JKS Keystore and PKCS12 Keystore impls and looks like it is not compliant with the KeyStoreSpi spec. I think you need to modify the methods to return null, etc instead if the keystore has not been loaded yet.
> 
> The KeyStore methods handle the test for an uninitialised keystore (before the KeyStoreSpi methods are called).
> 
> 
>> 
>> --Sean
>> 
>> On 12/02/2014 06:23 AM, Vincent Ryan wrote:
>>> 
>>> Please review the following enhancement to improve keystore security by creating PKCS12 keystores by default.
>>> Previously, JKS keystores were created by default. PKCS12 has the advantage of supporting stronger crypto
>>> and hashing algorithms. It is also an open, extensible format and supports associating arbitrary attributes with
>>> keystore entries.
>>> 
>>> Webrev: http://cr.openjdk.java.net/~vinnie/8044445/webrev.00/
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8044445
>>> 
>>> 
>>> To assist with compatibility across JDK releases, both JKS and PKCS12 keystore implementations have been
>>> extended to support both file formats. Applications that access keystores created by earlier releases should
>>> require no code changes.
>>> 
>>> This changeset also includes a new convenience method for instantiating a file-based keystore: KeyStore.getInstance
>>> - it takes a File argument. The specified file is probed by each supported keystore implementation to determine its
>>> keystore type. KeyStoreSpi has been enhanced with a boolean engineProbe method to perform the actual probing.
>>> 
>>> Finally, to improve performance, the PKCS12 keystore implementation has been moved from the SunJSSE provider
>>> to the SUN provider (as it appears earlier in the default list of installed JCE providers).
>>> 
>