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