RFR: 8331008: Implement JEP 478: Key Derivation Function API (Preview) [v14]

[Valerie Peng]

On Tue, 20 Aug 2024 20:05:07 GMT, Kevin Driver <kdriver at openjdk.org> wrote:

>> Introduce an API for Key Derivation Functions (KDFs), which are cryptographic algorithms for deriving additional keys from a secret key and other data. See [JEP 478](https://openjdk.org/jeps/478).
>> 
>> Work was begun in [another PR](https://github.com/openjdk/jdk/pull/18924).
>
> Kevin Driver has updated the pull request incrementally with one additional commit since the last revision:
> 
>   addresses delayed provider selection where parameters are involved

src/java.base/share/classes/javax/crypto/KDF.java line 104:

> 102: 
> 103:     // The provider implementation (delegate)
> 104:     private KDFSpi spi;

Given these two are always updated together, maybe we can group them in a Record? Something like 

>     record Delegate(KDFSpi spi, Provider provider) {}

This way, you can merge spi+provider and firstSpi+firstProvider into 2 Delegate objects.
Also, we only synchronize on the `lock` when setting the mutable fields such as spi, we may have to mark the mutable fields volatile? I recall there is some concurrency design patterns when doing lazy initialization.

src/java.base/share/classes/javax/crypto/KDF.java line 345:

> 343:                     continue;
> 344:                 }
> 345:                 return new KDF(spiObj, s.getProvider(), t, algorithm, kdfParameters);

If there is no other services supporting the requested KDF algorithm, we can construct the KDF object without `t` and lock in the found `spiObj` and its provider? Say

            if (t.hasNext()) {
                return new KDF(d, t, algorithm, kdfParameters);
            } else { // no other choices, lock down provider
                return new KDF(d, algorithm, kdfParameters);
            }

test/jdk/com/sun/crypto/provider/KDF/Functions.java line 36:

> 34: import java.util.HexFormat;
> 35: 
> 36: public class Functions {

Since this is strictly HKDF, it'd be clearer for the test to have HKDF in its name, e.g. "HKDFFunctions.java" or "BasicHKDF,java".

test/jdk/com/sun/crypto/provider/KDF/TestHKDF.java line 55:

> 53:             testName = Objects.requireNonNull(name);
> 54:             algName = Objects.requireNonNull(algStr);
> 55:             IKM = hex2bin(Objects.requireNonNull(ikmStr));

why is "IKM" all uppercases? "ikm" matches the naming convention of other fields better.

test/jdk/com/sun/crypto/provider/KDF/TestHKDF.java line 187:

> 185:             actualPRK, testData.info,
> 186:             testData.outLen);
> 187:         actualOKM = kdfExpand.deriveKey("RAW", kdfParameterSpecExpand);

We use "RAW" for key format, but in this file, it's used as key algorithm. Maybe use a standard key algorithm name?

test/jdk/com/sun/crypto/provider/KDF/TestHKDF.java line 241:

> 239:             System.out.println("\t* Key output: FAIL");
> 240:             System.out.println("Expected:\n" +
> 241:                                dumpHexBytes(outData, 16, "\n", " "));

`outData `should be `expectedOut`.

test/jdk/com/sun/crypto/provider/KDF/TestHKDF.java line 302:

> 300:         }
> 301:         return data;
> 302:     }

What is the difference between this method and `HexFormat.of().parseHex(hex)` ? We should just reuse existing java APIs if possible.

test/jdk/javax/crypto/KDF/Threading.java line 63:

> 61:     }
> 62: 
> 63:     @Test(threadPoolSize = 50, invocationCount = 100, timeOut = 30)

When running on the group server, I observe frequent test failures due to the short `timeOut` value. Perhaps we should increase the value?

test/jdk/javax/crypto/KDF/Threading.java line 68:

> 66:         assert (HexFormat.of().formatHex(result.getEncoded()).equals(
> 67:             expectedResult));
> 68:     }

Maybe also check the deriveData() and validate the output value and rename the test to testDerive()?
For completeness sake, we should also cover deriveData(...) call.

test/jdk/security/unsignedjce/java.base/javax/crypto/ProviderVerifier.java line 1:

> 1: /*

This is exact the same code as the one in OpenJDK.
For the Delayed.java, it just needs the bare minimum, e.g. do not error out when verify() is called. For testing purpose, maybe we can just use a stub version instead of duplicating the OpenJDK one? If later, the OpenJDK one changed, people may wonder what this is for and have to read through the whole impl, If it's just the stub version, it's very clear.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1727813555
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1727824761
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1725710947
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1725721846
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1725748081
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1725754996
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1725768089
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1725483754
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1725489168
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1727779913