On Sat, 18 Mar 2023 07:18:31 GMT, Martin Balao <mba...@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
