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.w...@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.mul...@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
> 

Reply via email to