RFR: 8331008: KDF Implementation [v3]

[Weijun Wang]

On Thu, 9 May 2024 19:46:39 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).
>
> Kevin Driver has updated the pull request incrementally with one additional commit since the last revision:
> 
>   some code review comments

For `HkdfKeyDerivation`.

src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 53:

> 51:  * and Expand-only variants.
> 52:  */
> 53: abstract class HkdfKeyDerivation extends KDFSpi {

How about just name it `HKDF`?

src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 54:

> 52:  */
> 53: abstract class HkdfKeyDerivation extends KDFSpi {
> 54: 

All fields below should be local variables inside methods for thread-safety, except for `hmacLen` and `hmacAlgName`, which can be made final.

src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 59:

> 57:     protected String hmacAlgName;
> 58:     protected List<SecretKey> ikms;
> 59:     protected List<SecretKey> salts;

Not sure if the 3 below should be `SecretKey` or `byte[]`. The latter has a benefit that when empty can be empty instead of null.

src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 66:

> 64:     protected int length;
> 65: 
> 66:     protected enum HKDFTYPES {

We don't usually use plural. make it `HKDF_TYPE`.

src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 70:

> 68:     }
> 69: 
> 70:     protected HKDFTYPES HKDFTYPE;

This is not a constant, make it `hldfType`.

src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 80:

> 78:      * @throws InvalidAlgorithmParameterException
> 79:      *     if the initialization parameters are inappropriate for this {@code KDFSpi}
> 80:      */

This constructor should contain `hmacAlgName` as a parameter, and its assignment moved here, thus make it final.

src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 101:

> 99:     protected SecretKey engineDeriveKey(String alg, KDFParameterSpec kdfParameterSpec)
> 100:         throws InvalidParameterSpecException {
> 101: 

Too many duplicate code with `engineDerivedata`. Please consider let one call the other, or, you can create a `deriveInternal` method which is then called by the 2 engine methods.

If you do this, there is also no need for a separate `inspectKDFParameterSpec` method, which then make it easy to only have local variables.

src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 189:

> 187:             // perform expand
> 188:             try {
> 189:                 return Arrays.copyOf(hkdfExpand(this.pseudoRandomKey, this.info, this.length),

If length is already correct, there is no need to call `copyOf`.

src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 223:

> 221:         // A switch would be nicer, but we may need to backport this before JDK 17
> 222:         // Also, JEP 305 came out in JDK 14, so we can't declare a variable in instanceof either
> 223:         if (kdfParameterSpec instanceof HKDFParameterSpec.Extract) {

`kdfParameterSpec instanceof HKDFParameterSpec.Extract anExtract`.

src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 227:

> 225:             this.ikms = anExtract.ikms();
> 226:             this.salts = anExtract.salts();
> 227:             if (isNullOrEmpty(ikms) && isNullOrEmpty(salts)) {

As I said earlier, I think this restriction is not necessary.

src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 244:

> 242:             HKDFParameterSpec.Expand anExpand = (HKDFParameterSpec.Expand) kdfParameterSpec;
> 243:             // set this value in the "if"
> 244:             if ((pseudoRandomKey = anExpand.prk()) == null) {

This could not happen here.

src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 249:

> 247:             }
> 248:             // set this value in the "if"
> 249:             if ((info = anExpand.info()) == null) {

I would disallow null when the parameter is created.

src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 365:

> 363:         // Calculate the number of rounds of HMAC that are needed to
> 364:         // meet the requested data.  Then set up the buffers we will need.
> 365:         hmacObj.init(prk);

prk size must be checked somewhere.

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

PR Review: https://git.openjdk.org/jdk/pull/18924#pullrequestreview-2048746355
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595929751
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595932007
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595933601
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595931078
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595930649
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595935623
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595943274
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595944111
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595948410
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595948979
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595949546
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595949980
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595951294