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 >