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

[Alexey Bakhtin]

Hi Martin,

Thank you a lot for review.
I’ve simplified keytool code almost as you suggested but I still think PKCS12 probing is useful here.
I’m agree, the first version of kstype detection was not correct, so I’ve implemented it via PKCS12KeyStore.probe as you suggested in the first mail.
Without such probing the passwordless feature looks strange: "keytool -list” cmd prints PKCS12 store type and warning about password for the passwordless keystore (if storetype is not indicated explicitly). The same is for other commands.
Also, I have removed changes in the test/lib/jdk/test/lib/SecurityTools.java and updated tests to use  jdk/test/lib/jdk/test/lib/SecurityTools.java

All sun/security/pkcs12  and sun/security/tools/keytool tests passed

New version of the patch is available at : http://cr.openjdk.java.net/~abakhtin/8076190_8u/webrev.v1/

Regards
Alexey

> On 24 Aug 2021, at 18:50, Martin Balao <mbalao at redhat.com> wrote:
> 
> Hi Alexey,
> 
> Final comments for this first round:
> 
> * src/share/classes/sun/security/tools/keytool/Main.java
>  * @@ -2025,7 +2058,14 @@
>   * Seems to have an issue similar to the one before:
>    * 'srcstoretype' is changed based on probing (which was not a
> JDK-8 behavior)
>    * the value set is based on the assumption that
> '!"JKS".equalsIgnoreCase(realType)' is PKCS#12 and that may not be
> correct on some cases
>    * there might be some redundancy in the checks 'srcksfile != null
> && is != null && srcProviderName == null'
>   * I'd consider a similar approach to the one proposed earlier: take
> the 'srcstoretype' value for certain, compare against 'pkcs12' and
> handle the existing-keystore scenario. In this case, 'is' seems to be
> the variable to decide whether there is an existing keystore or not.
> 
> * test/lib/jdk/test/lib/SecurityTools.java
>  * For some reason, there are 2 SecurityTools files in JDK-8:
>   * jdk/test/lib/testlibrary/jdk/testlibrary/SecurityTools.java
>   * jdk/test/lib/jdk/test/lib/SecurityTools.java
>  * Can we avoid this change by making the test use the other one?
>   * Looks like
> 'jdk/test/lib/testlibrary/jdk/testlibrary/SecurityTools.java' is the
> one that you want for the test
>   * If we apply your proposed change, then the 2 libs will be equal
> in regards to the affected method and I'm in general concerned about
> the side-effects of modifying libraries; the set of regression tests
> that you ran may not be enough to make sure that we don't introduce a
> regression.
> 
> Thanks,
> Martin.-
>