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

Tue Feb 8 15:06:54 UTC 2022

The CSR for the JDK-8076190 is updated with the section, describing the differences from JDK11.

CSR: https://bugs.openjdk.java.net/browse/JDK-8267040 <https://bugs.openjdk.java.net/browse/JDK-8267040>

Regards
Alexey

> On 8 Feb 2022, at 17:05, Alexey Bakhtin <alexey at azul.com> wrote:
> 
> Hello Martin,
> 
> Thank you a lot for detailed review, description and new proposal.
> The changes are OK for me except of PKCS12 keystore provider.
> Right now PKCS12 Keystore is defined in the SunJSSE provider only (not SunJCE):
> * https://hg.openjdk.java.net/jdk8u/jdk8u-dev/jdk/file/7fcf35286d52/src/share/classes/sun/security/ssl/SunJSSE.java#l233 <https://hg.openjdk.java.net/jdk8u/jdk8u-dev/jdk/file/7fcf35286d52/src/share/classes/sun/security/ssl/SunJSSE.java#l233>
> I was thinking to add PKCS12 into the SUN provider similar to JDK11+  but it could affect custom providers with PKCS12 support ( PKCS12 keystore implementation could be changed because of high SUN provider priority)
> 
> Also I have updated sun/security/pkcs12/ParamsTest.java test to explicitly specify PKCS12 storetype for passwordless keystores.
> All sun/security/pkcs12 and sun/security/tools/keytool tests are passed
> 
> New version of the patch is available at :
> https://cr.openjdk.java.net/~abakhtin/8076190_8u/webrev.v3 <https://cr.openjdk.java.net/~abakhtin/8076190_8u/webrev.v3>
> 
> 
> Regards
> Alexey
> 
> 
>> On 8 Feb 2022, at 08:18, Martin Balao <mbalao at redhat.com> wrote:
>> 
>> 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/ <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 <mailto:mbalao at redhat.com>> wrote:
>> Hi Alexey,
>> 
>> On Thu, Feb 3, 2022 at 4:29 PM Alexey Bakhtin <alexey at azul.com <mailto: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 <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.-
>> 
> 