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

Sean Mullan sean.mullan at oracle.com
Tue Dec 2 15:10:33 UTC 2014


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

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

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.

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



More information about the security-dev mailing list