RFR 8076190: Customizing the generation of a PKCS12 keystore

Tue Sep 25 18:22:26 UTC 2018

Update is looking good, just a few more comments ...

* java.security

1066 # The following parameters, if configured, are used by the PKCS12 
KeyStore
1067 # implementation during the creation of a new keystore. Several of the
1068 # properties may also be used when modifying an existing keystore.

If you use the KeyStore API and specify your own algorithms, the API 
algorithms supersede these properties. Maybe you should say something 
about that, that these properties can be overridden by the API. 
Otherwise it may sound like they are always used.

1094 # iteration count not an integer, or an unknown algorithm name, an 
exception

Slight wording suggestion: "an iteration count that is not an integer, ..."

1098 # It is not guaranteed to be examined and used by other 
implementations.

s/It is/They are/

1100 # The iteration count used by the PBE algorithm encrypting a 
certificate.

Slight wording suggestion: s/encrypting/when encrypting/. Same comment 
on line 1104.

1104 # The iteration count used by the PBE algorithm encrypting a 
private key.

should this be "private key or secret key"?

1122 keystore.pkcs12.certProtectionAlgorithm = PBEWithSHA1AndRC2_40

Shouldn't this be named certPbeAlgorithm so that it matches 
certPbeIterationCount? Same comment about keyProtectionAlgorithm.

1124 # The Mac algorithm. This can be any HmacPBE algorithm defined in 
the Mac

I would add a little bit more description as to what this Mac algorithm 
is used for.

Also, I would probably order these differently. I think it makes more 
sense to have the algorithms listed first, followed by parameters that 
affect them:

keystore.pkcs12.certProtectionAlgorithm
keystore.pkcs12.certPbeIterationCount
keystore.pkcs12.keyProtectionAlgorithm
keystore.pkcs12.keyPbeIterationCount
keystore.pkcs12.macAlgorithm
keystore.pkcs12.macIterationCount

* 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?

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.

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 :)

--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
> 