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

Tue Feb 8 05:18:21 UTC 2022

Hi Alexey,

I've gone through this 8u backport again and made some changes:

 * http://cr.openjdk.java.net/~mbalao/webrevs/8076190/8076190.webrev.8u.02/

This is the assessment for each non-test file modified:

 * src/share/classes/com/sun/crypto/provider/PBES2Parameters.java
  * Ok. Applies cleanly and fixes an actual bug when keyLength is not
specified (the field is marked as optional in the RFC).

 * src/share/classes/com/sun/crypto/provider/SunJCE.java
  * Ok. Adaptations necessary to 8u's way of registering services. All of
them are consistent to changes in 11u.

 * src/share/classes/sun/security/pkcs12/PKCS12KeyStore.java
  * Ok. Extended to handle a variable number of iterations for PBE
algorithms and custom MAC and Cipher algorithms (which includes NONE).
   * 8u new behavior
    * When loading a store, the certProtectionAlgorithm value comes from
the last encrypted certificate bag (contentType ==
ContentInfo.ENCRYPTED_DATA_OID). If certificates are seen but no encrypted
bags, certProtectionAlgorithm = "NONE". If the keystore is new or no
certificates are seen, a default value for certProtectionAlgorithm is used
when storing.
    * When loading a store, the macAlgorithm value comes from the
(optional) MacData section of the PKCS#12 keystore if a password was
provided. If no MacData section is available, then macAlgorithm = "NONE".
If there is a MacData section but a password was not provided to load
(notice that passwords when loading are optional), a default macAlgorithm
value will be used later when storing. If the keystore is new, a default
value for macAlgorithm is used when storing.
    * certProtectionAlgorithm and macAlgorithm values are set when loading,
or set to default values otherwise, and can be overwritten when storing by
means of System/Security properties.
    * certProtectionAlgorithm and macAlgorithm values that are set to NONE
do not enable data encryption or authentication when storing.
   * In my view, all these changes provide an extension of what was
previously supported in a way that is acceptable for backward-compatibility.
    * Note: creating a new passwordless keystore and trying to open it with
a previous 8u release won't work, but that is fine.
  * Removed the function to detect PKCS#12 keystores based on the file. My
proposal is not to change 8u Keytool behavior to enable this capability.
See below.

 * src/share/classes/sun/security/tools/keytool/Main.java
  * This is how my 8u proposal differs from 11u:
   * In 11u, when 'storetype' is not set, it's detected based on the file
if a file is provided. This is not fundamental to 8076190 and it's not a
natural fit because the engine probe API is not available. This is where I
had backward-compatibility concerns in Webrev.00 and 01.
   * 8u will only decide passworless status if 'storetype' is set to
case-insensitive 'pkcs12' (either by Keytool parameter explicitly or taken
from the keystore.type security property) AND if the security provider in
use is SunJCE.
    * The behavior for non-SunJCE security providers using case-insensitive
'pkcs12' algorithm name remains unchanged. They will not take advantage of
this but they will not be broken if they were not supporting passwordless
keystores.
    * If there is an existing keystore, SunJCE's PKCS12KeyStore will be
used to check passwordless status.
    * If there is not an existing keystore, passwordless status is taken
from the
keystore.pkcs12.certProtectionAlgorithm/keystore.pkcs12.macAlgorithm
properties.
    * passwordless status matters to Keytool for checks, asking for
passwords and warnings.
  * The rest of it looks good to me.

 * src/share/classes/sun/security/x509/AlgorithmId.java
  * Ok

 * src/share/lib/security/java.security-*
  * Ok

 * src/share/classes/com/sun/crypto/provider/HmacPKCS12PBECore.java
  * Ok

 * src/share/classes/com/sun/crypto/provider/HmacPKCS12PBESHA1.java
  * Ok

 * src/java.base/share/classes/java/security/KeyStore.java
  * Ok, no probing in 8u.

Tests:

 * test/sun/security/pkcs12/ParamsPreferences.java
  * Ok

 * test/sun/security/pkcs12/ParamsTest.java
  * @Alexey, can you please elaborate on why there are differences in
'.shouldContain("Enter key password for <a>")' and '.shouldContain("Enter
key password for <b>")' ?
  * Not passing.

 * test/sun/security/pkcs12/params/README
  * Ok

 * test/sun/security/pkcs12/params/kandc
  * Ok

 * test/sun/security/pkcs12/params/ks
  * Ok

 * test/sun/security/pkcs12/params/os2
  * Ok

 * test/sun/security/pkcs12/params/os3
  * Ok

 * test/sun/security/pkcs12/params/os4
  * Ok

 * test/sun/security/pkcs12/params/os5
  * Ok

 * test/jdk/sun/security/tools/keytool/ProbingFailure.java
  * Ok, no probing in 8u.

@Alexey:

 * Can you please review and fully test this proposal?
  * I've only tested 'sun/security/pkcs12' category, observing 1 failure in
'test/sun/security/pkcs12/ParamsTest.java' (see below)
  * I'm particularly interested in testing sun/security/tools/keytool

 * Can you please have a look at my
'test/sun/security/pkcs12/ParamsTest.java' comment above and make the
changes necessary for this test to pass?

 * Can you please write a CSR for 8u based on the 11u one but describing
the differences (Keytool)?

Thanks,
Martin.-

On Fri, Feb 4, 2022 at 9:27 PM Martin Balao <mbalao at redhat.com> wrote:

> Hi Alexey,
>
> On Thu, Feb 3, 2022 at 4:29 PM Alexey Bakhtin <alexey at azul.com> wrote:
>
>> The issue described in the [1] is related to HmacPBESHA256 support in the
>> PKCS12KeyStore. This issue is not covered by the JDK-8245169 but fixed by
>> another part of  JDK-8076190
>> Should it be considered as a separate issue with new Bug Id ? or should
>> we fix altogether as a
>> JDK-8076190 backport without password-less support ?
>>
>> [1] -
>> https://mail.openjdk.java.net/pipermail/jdk8u-dev/2021-December/014473.html
>
>
> Hmm... The HmacPBESHA* part means introducing
> 'keystore.pkcs12.macAlgorithm' which can be, eventually, 'NONE'. If the
> user invokes Keytool with 'storetype' equals to 'pkcs12',
> 'keystore.pkcs12.macAlgorithm=NONE' and SunJCE is handling that, it should
> be fine to generate a new keystore without a Mac. Same for certificates and
> keys probably. It looks to me that the real backward-compatibility breaker
> is changing the 'storetype' based on the file, not the whole passwordless
> thing. Perhaps we can skip those parts causing trouble and move forward
> with the rest of it. I'll have a look again next week.
>
> I suggest keeping this backport under 8076190.
>
> Martin.-
>
>