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