On Sat, 11 Jan 2025 01:40:39 GMT, Valerie Peng <valer...@openjdk.org> wrote:
>> Martin Balao has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Renaming of P11KDF fix.
>>
>> Co-authored-by: Martin Balao Alonso <mba...@redhat.com>
>> Co-authored-by: Francisco Ferrari Bihurriet <fferr...@redhat.com>
>
> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11HKDF.java line
> 230:
>
>> 228: }
>> 229:
>> 230: private abstract sealed class KeyMaterialMerger permits
>
> Hmm, instead of all these different sub-classes which handle the various
> combination of key/data when merging the key materials, how about encapsulate
> the key/data into a record and handling the merging in one class? Something
> like:
>
>
> record KeyMaterial(boolean isBytes, byte[] data, P11Key.P11SecretKey key)
> {
> }
>
> private final class KeyMaterialMerger {
>
> private KeyMaterial curr;
>
> private KeyMaterial toKeyMaterial(SecretKey key) {
> if (key instanceof SecretKeySpec) {
> return new KeyMaterial(true, key.getEncoded(), null);
> } else {
> return new KeyMaterial(false, null,
> convertKey(key, "Failure merging key material"));
> }
> }
>
> KeyMaterialMerger merge(SecretKey nextKey) {
> if (curr == null) {
> curr = toKeyMaterial(nextKey);
> } else {
> KeyMaterial next = toKeyMaterial(nextKey);
> int combination = (curr.isBytes ? 2 : 0) |
> (next.isBytes ? 1 : 0);
> switch (combination) {
> case 3: // 'curr' and 'next' both are byte[]
> int newLen = curr.data.length + next.data.length;
> var resData = Arrays.copyOf(curr.data, newLen);
> System.arraycopy(next.data, 0, resData, newLen -
> next.data.length, next.data.length);
> curr = new KeyMaterial(true, resData, null);
> break;
> case 2: // 'curr' is byte[], 'next' is key
> var resKey = p11Merge(next.key, new CK_MECHANISM(
> CKM_CONCATENATE_DATA_AND_BASE,
> new CK_KEY_DERIVATION_STRING_DATA(curr.data)),
> curr.data.length + next.key.keyLength);
> curr = new KeyMaterial(false, null, resKey);
> break;
> case 1: // 'curr' is key, 'next' is byte[]
> resKey = p11Merge(curr.key, new CK_MECHANISM(
> CKM_CONCATENATE_BASE_AND_DATA,
> new CK_KEY_DERIVATION_STRING_DATA(next.data)),
> curr.key.keyLength + next.data.le...
This way, there is no need for the 3 XXXKeyMaterialMerger (where XXX:
Any/Data/Key) and the merging code is in 1 method instead of handled over 4
different methods.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/22215#discussion_r1911796182
