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
