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
- The class summary should be modified to include an example using the
new getInstance methods.
* 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.
* ReadP12Test
- can you fix the typo: s/IN_KETYSTORE_TYPE/IN_KEYSTORE_TYPE/
* KeyToolTest
- you have some comments labeled with VR that look like they should be
removed
--Sean
On 12/15/2014 09:43 AM, Vincent Ryan wrote:
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.r...@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.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).
