RFR: 8301553: Support Password-Based Cryptography in SunPKCS11

[Martin Balao]

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

>> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Mac.java line 204:
>> 
>>> 202:         if (svcPbeKi != null) {
>>> 203:             if (key instanceof P11Key) {
>>> 204:                 params = PBEUtil.checkKeyParams(key, params, algorithm);
>> 
>> It seems strange that you check the key to be instanceof P11Key but then inside PBEUtil.checkKeyParams, it errors out if the key instanceof PBEKey. Maybe you meant to check if the key is an instanceof P11PBEKey? Could the key be a PBEKey but not P11Key and contains more than just password? I don't quite follow the logic here.
>
> We can only pass PBEKey keys to PBE Mac services.
> 
> If the key is a P11Key (PBEKey + P11Key == P11PBEKey), derivation is not needed. We just check that the P11Key came from a PBE derivation inside PBEUtil.checkKeyParams and that's it. This is for consistency enforcement only: the token does not care the P11Key origin as long as it has the right attributes to calculate the HMAC.
> 
> If the key is not a P11Key, we have to derive it. We just check that the key implements the PBEKey interface, so there is data for derivation. The derived key will be a P11Key (in particular, a P11PBEKey).
> 
> Thus, the ```if``` [here](https://github.com/openjdk/jdk/blob/ab7ffd56bb8b93d513023d0136df55a6375c3286/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Mac.java#L203) should be read as: are we receiving an already derived key (true) or do we have to derive it (false)? In any case, apply the corresponding checks and move forward.
> 
> I'll add a comment to the code to make this logic more clear.

In addition to the new comments, we finally implemented a couple of minor changes for clarity and to enforce a check that was previously omitted:

  * PBEUtil.checkKeyParams was renamed to PBEUtil.checkKeyAndParams and does not longer return the underlying service params (PBEParameterSpec.paramSpec). It's only checking that the key implements the PBEKey interface and that params, if an instance of PBEParameterSpec, are consistent with the key's derivation data. We added a comment in PBEUtil.java too, where PBEUtil.checkKeyParams is defined.
  * For all PBE cases in which params is an instance of PBEParameterSpec, the underlying service params value (PBEParameterSpec.paramSpec) is obtained and assigned to params. Notice that this was not done before in derivation cases.
  * Now, we enforce the check that the underlying service params is null even in those cases in which we derived.

We modified P11PBECipher to be aligned to these changes, and added equivalent comments there.

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

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