RFR 8076190: Customizing the generation of a PKCS12 keystore

[Weijun Wang]

Webrev updated at http://cr.openjdk.java.net/~weijun/8076190/webrev.04/.

Thanks
Max

> On Sep 27, 2018, at 1:58 PM, Weijun Wang <weijun.wang at oracle.com> wrote:
> 
> All others accepted.
> 
>> 1122 keystore.pkcs12.certProtectionAlgorithm = PBEWithSHA1AndRC2_40
>> 
>> Shouldn't this be named certPbeAlgorithm so that it matches certPbeIterationCount? Same comment about keyProtectionAlgorithm.
> 
> Unfortunately we already had "keystore.pkcs12.keyProtectionAlgorithm" and it also accepts "PKCS12". See http://cr.openjdk.java.net/~weijun/8076190/webrev.03/src/java.base/share/classes/sun/security/pkcs12/PKCS12KeyStore.java-.html:
> 
> 147     private static final String[] KEY_PROTECTION_ALGORITHM = {
> 148         "keystore.pkcs12.keyProtectionAlgorithm",
> 149         "keystore.PKCS12.keyProtectionAlgorithm"
> 150     };
> 
> But you are right the names are not consistent and supporting both "pkcs12" and "PKCS12" is awkward (and cannot make use of the new SecurityProperties::privilegedGetOverridable method). Now I decide to name the new properties as (key|cert)PbeAlgorithm names with only "pkcs12", and only support the old names as a fallback.
> 
>> * PKCS12KeyStore.java
>> 
>> 145     private static final int PBE_ITERATION_COUNT
>> 146             = getPkcs12Setting("pbeIterationCount", 50000);
>> 
>> Shouldn't there be 2 constants above, one for certPbeIterationCount and one for keyPbeIterationCount?
> 
> Oops. Will fix.
> 
>> 
>> 208                         String name2 = "keystore.PKCS12." + type;
>> 
>> I'm not sure checking the property name with uppercase PKCS12 is necessary. I think we should assume that the property name is case-sensitive, as most properties are.
> 
> See above. 
> 
>> 
>> 2108                     // What shall we do if there is ENCRYPTED_DATA_OID but
>> 2109                     // we do not have a password?
>> 
>> This section of comments (thru line 2123) should really be reworded to say what the current implementation does and perhaps allude to why it is not perfect but is probably the best option. Right now it reads too much like a brainstorming session :)
> 
> I *am* brainstorming inside my brain. I had long thought about throwing an exception here.
> 
> With the password-less cacerts, hopefully this will not be a problem for many people and we can just keep the current behavior.
> 
> Thanks
> Max
> 
>> 
>> --Sean
>> 
>> On 9/25/18 6:49 AM, Weijun Wang wrote:
>>> Webrev updated at http://cr.openjdk.java.net/~weijun/8076190/webrev.03/.
>>> Mostly spec changes. The test is enhanced a little to check for macAlg interop.
>>>> On Sep 24, 2018, at 11:15 PM, Sean Mullan <sean.mullan at oracle.com> wrote:
>>>> 
>>>> 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 ...
>>> Good.
>>>> 
>>>>>> 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?
>>> When not set, there is a default value. It happens to be the same as what the out-of-box java.security shows.
>>> When empty, there will be an error. Say, NumberFormatException, NoSuchAlgorithmException.
>>>> Are the algorithm names case-insensitive?
>>> Should be case-insensitive. For each one, I've specified it as "algorithm defined in the XYZ section of standard names". Is that enough to show it's 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.
>>> Throw an exception. I cannot guarantee when it will be thrown so I just say "when it's used".
>>>> 
>>>> * 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.
>>> Interoperability. I generate pkcs12 keystores from openssl using various settings and make sure keytool can read them, vice versa. Maybe I should move it to the infra test group.
>>> Thanks
>>> Max
>>>> 
>>>> --Sean
>