RFR: 8328119: Support HKDF in SunPKCS11 (Preview) [v10]

[Martin Balao]

On Sat, 11 Jan 2025 01:44:06 GMT, Valerie Peng <valeriep at 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