RFR: 8301553: Support Password-Based Cryptography in SunPKCS11

[Martin Balao]

On Sat, 18 Mar 2023 07:53:15 GMT, Martin Balao <mbalao at openjdk.org> wrote:

>> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Mac.java line 223:
>> 
>>> 221:             }
>>> 222:             p11Key = P11SecretKeyFactory.convertKey(token, key, algorithm);
>>> 223:         }
>> 
>> Is this meant to handle Non-PBE-related case? The logic here seems conflicting with the above code block where the params must be null. Also if key is instanceof P11Key already, why are you calling P11SecretKeyFactory.convertKey(...) again? Strange.
>
> The quoted block handles non-PBE cases (svcPbeKi == null) and PBE cases in which the derivation was not required (svcPbeKi != null && key instanceof P11Key). The only cases left out of the block are PBE ones in which the key had to be derived because no conversion is needed.
> 
> If we enter the block in the non-PBE case, params must be null like before to this enhancement (see [here](https://github.com/openjdk/jdk/blob/jdk-21%2B14/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Mac.java#L195)).
> 
> If we enter the block in the PBE case (derivation not required), params is what PBEUtil::checkKeyParams returned. This function returns the PBEParameterSpec::paramSpec field of the passed PBEParameterSpec instance, which is always null except for the PBE Cipher cases in which the IV can be specified. Given that this is a Mac service, a non-null value there is not expected. We check this for consistency.
> 
> We are calling P11SecretKeyFactory::convertKey if the key is a P11Key because it might be a P11Key from a token different than the P11Mac service one (the condition [here](https://github.com/openjdk/jdk/blob/ab7ffd56bb8b93d513023d0136df55a6375c3286/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11SecretKeyFactory.java#L294) could be false). If that's the case, a new derivation in the P11Mac service token will be done. Notice that this is a little caveat to my previous comment [here](https://github.com/openjdk/jdk/pull/12396#discussion_r1140958830): a P11Key might still require a derivation.
> 
> I'll add comments to the code to make this more clear.

We made a subtle modification to this code that does not change execution paths —except for checking params against null in derivation cases, as commented [here](https://github.com/openjdk/jdk/pull/12396#discussion_r1142945131)— but hopefully helps with clarity. In particular:

  * The condition to try conversion is now p11Key == null. Conversion is tried in non-PBE cases and PBE cases in which derivation was not done, as commented [here](https://github.com/openjdk/jdk/pull/12396#discussion_r1140963493).
  * The check of params against null is now previous to the conversion block, and applies to all cases (including those in which we derived).

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/12396#discussion_r1142948975