On Sat, 11 Jan 2025 01:44:06 GMT, Valerie Peng <valer...@openjdk.org> wrote:
>> 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_KE...
>
> 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.
May be a matter of taste and trade-offs, but I personally lean more towards the
object-oriented/polymorphic design. While a bit more verbose, I like the
separation of responsibilities, the closed-world type of transitions in the
states machine and the potential for extensibility. The procedural approach
bases transitions in the state of a record that is not self-explanatory, opens
the world to the reader for meaningless instantiations/states (e.g. with a key
and a byte array, or with `isBytes = false` and a byte array) and is less
extensible. That type of aggregation could be just fields in the merger class,
and I don't see the point of doing `keyMerger = keyMerger.merge(key);` for a
method that can only return `this`. @franferrax ?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/22215#discussion_r1911874222
