On 9/23/18 9:42 AM, Weijun Wang wrote:
On Sep 22, 2018, at 2:49 AM, Sean Mullan<sean.mul...@oracle.com> wrote:
Still reviewing but here are some initial comments.
It seems this is more than a fix for JDK-8076190. It also adds configuration
properties for the PKCS12 algorithms. I think you should expand the
scope/description of the issue to include that.
The title of the bug is already "Customizing the generation of a PKCS12
keystore", I'll update the description.
I also changed the Subject of the email to the new name.
These properties are also for existing keystores so I would change the first
sentence to mention that, ex:
"... during the creation of a new keystore or modification of an existing
keystore."
Those (esp certProtectionAlgorithm and macAlgorithm) are only used when
generating a keystore. When an existing keystore is modified the properties are
not used. I made this choice so that after we create the initial cacerts as
password-less (with system properties on the command line), the user can add
new cert into the file without any special system property setting and keep it
password-less.
I've tried my best to describe this in the java.security.
Right, I understand their usage and the properties are well documented.
My comment is that if you just read the first sentence which is
typically a summary sentence, it gives no indication that some of these
properties may also be used when modifying a keystore. You have to read
further to figure that out, and that might be missed. Perhaps if you
added a second sentence to the first paragraph: "Several of the
properties may also be used when modifying an existing keystore." Then
the next paragraph starts ...
The default alg values seem somewhat weak. Can we upgrade them or is there a
compatibility issue/risk?
It will be addressed in a different RFE and is not related to migrating cacerts
to password-less.
I haven't studied it yet. Need to investigate how current releases of various
tools (openssl, browsers...) support it.
Ok.
Still need to review PKCS12KeyStore, but here are some more comments:
* java.security
You should probably say that these properties are specific to the JDK
PKCS12 KeyStore implementation and may not be supported by other PKCS12
implementations.
What happens if the properties are not set or are set to the empty String?
Are the algorithm names case-insensitive?
What if illegal values, or unknown algorithms are set for the
properties? Are they ignored? Do they throw an Exception? Or do they
fallback to hard-coded defaults? This behavior should be specified.
* Passwordless
Can you add some comments as to why openssl is needed? Aren't there some
tests you can still do if it is not there? And maybe add some comments
in the class description as to what the test is generally testing as it
hard to discern that from just scanning the code.
--Sean
