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