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

[Martin Balao]

Hi Alexey,

Thanks for proposing a new version of your patch.

If we change the default behavior for an empty "storetype" parameter (that
is: trying to get the value from the file using a SunJCE class instead of
using the default keystore type value), we would need to include that in
the CSR for 8u. However, I believe that this behavioral change is not
strictly required and have a couple of concerns:

1) We are using a SunJCE class as the only engine prober and perhaps SunJCE
is not even available/enabled in runtime. This is different in JDK-11
because all security providers are probed through a JCA API, and if one
happens to handle PKCS#12 and identify the InputStream as such, we get a
KeyStore instance of it.

2) In JDK-11, the "storetype" value is whatever the probed security
provider wants it to be. In JDK-8, we would be fixing the algorithm name to
SunJCE's "pkcs12" value -even if SunJCE is not available/enabled as
mentioned in #1-. This can break backward-compatibility in my view. The
scenario that I have in mind is for example: SunJCE disabled, a 3rd party
security provider that handles PKCS#12 and calls the algorithm "P12", a
java.security property set with "keystore.type=P12" and an invocation to
Keytool without a "storetype" parameter. In that case, SunJCE will jump-in,
say that it's a PKCS#12 keystore and rename "storetype" from the default
"P12" to "pkcs12". The invocation will later fail at "keyStore =
KeyStore.getInstance(storetype);".

In general, I'm a bit concerned about the 8u backport of this enhancement.
Can you please elaborate on the reasons why it would be justified for such
an old release? It looks to me that we are looking into password-less
PKCS#12 keystores and being more flexible with the algorithms used to
encrypt certificates and keystore integrity (note that private keys
encryption is not under the scope of this enhancement, as there is already
flexibility to choose the algorithms). While the latter could be seen as a
security concern, we should be able to leverage on the file-system/OS
permissions for that. Let me know of your thoughts. I'd like the 8u
maintainers to be involved in this discussion as well.

Thanks,
Martin.-


On Wed, Sep 8, 2021 at 1:54 PM Alexey Bakhtin <alexey at azul.com> wrote:

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