RFR 8076190: Customizing the generation of a PKCS12 keystore

[Sean Mullan]

On 9/23/18 9:42 AM, Weijun Wang wrote:
>> On Sep 22, 2018, at 2:49 AM, Sean Mullan<sean.mullan at 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