[8u] RFR: 8076190: Customizing the generation of a PKCS12 keystore

[Martin Balao]

Hi Alexey,

Thanks for proposing this backport.

I have a few comments:

 * src/share/classes/sun/security/tools/keytool/Main.java
  * Don't we have an indentation issue when shifting "// Create new
keystore" to the left and adding the new block? Looks to me that the
code inside the function is at the same indentation as the function
'void doCommands(PrintStream out) throws Exception {'.
  * The comments "// Create new keystore" and "// Probe for keystore
type when filename is available" one after the other do not make much
sense to me. Are we dealing with an existing keystore or with a new
one?
  * Does the condition 'ksStream != null' imply 'ksfile != null'? Is
there a reason why checking for the former is not enough and we need
to check for the latter?
  * Why is 'providerName == null' a necessary condition to determine
that a keystore exists?
  * "if (!"JKS".equalsIgnoreCase(realType)) {"
   * There is an assumption that this means PKCS#12. Why not JCEKS or
a different one? Looks to me that the only safe way to decide that a
file-based keystore is of PKCS#12 type is by looking at the actual
bytes in the same way as ::engineProbe does in JDK-11. Passing a
non-PKCS12 keystore to PKCS12KeyStore::isPasswordless seems to raise
an exception which is not handled then, and this might be risky.
   * JDK-11 handles the possible 'IOException' in
'PKCS12KeyStore::isPasswordless' but that's because keystores can be
dual: specified to be 'pkcs12' but be 'JKS' in reality.
   * More on this below.
  * Why do we execute the sequence "keyStore =
KeyStore.getInstance(storetype); keyStore.load(ksStream, storePass);
ksStream.close();" at this point? I'm not sure how that relates to
determining 'isPasswordlessKeyStore' value or why it is added at this
point. This is probably related to how the whole execution flow in
JDK-8 is affected by this backport. More on that below.
  * The '!isPasswordlessKeyStore' condition in 'if
(storetype.equalsIgnoreCase("pkcs12") && !isPasswordlessKeyStore) {'
looks a bit confusing to me. What is it trying to prevent?
  * JDK-8 is not ready to determine the keystore type by probing the
file. Forcing it to do so would change the behavior in a way that is
not documented in the CSR. I'd avoid doing so. Unless 'storetype' is
set, 'storetype' must be KeyStore.getDefaultType() (which translates
to 'JKS' by default). Your changeset sets 'storetype = P12KEYSTORE;',
with the additional risk of setting an incorrect value as mentioned
above.
  * In my view, there are 2 paths:
    * If there is an existing file-based stream and it's PKCS#12, we
need to determine if it's password-less based on the actual bytes
    * If it's a new keystore of PKCS#12 type, we determine
password-less through 'keystore.pkcs12.certProtectionAlgorithm' and
'keystore.pkcs12.macAlgorithm' property values
    * JDK-8 does not split the new/existing paths in the same way that
JDK-11 does: code is mostly shared between the two. I'd try to align
the backport as much as possible to the original structure, and keep
changes to a minimum. That is: only split the path to determine
password-less status for PKCS#12 keystores and nothing else. Once we
know it's 'pkcs12' (based on 'storetype' value, either passed by
parameter or obtained from KeyStore::getDefaultType), we need to hook
a point that lets us decide if it's a new keystore or an existing one.
If it's an existing keystore we then use
PKCS12KeyStore::isPasswordless (possibly handling the exception). If
it's an existing one, we use 'keystore.pkcs12.certProtectionAlgorithm'
and 'keystore.pkcs12.macAlgorithm' properties. Please note that the
previous conditions can be swapped: check new/existing first and
PKCS#12 then.

I'm not done with the review yet but wanted to share my thoughts at
this point so we can keep the discussion going.

@8u-maintainers, I'd postpone this backport to the next cycle so we
have more time to make sure that we are all happy with it.

Thanks,
Martin.-