RFR: 8331008: Implement JEP 478: Key Derivation Function API (Preview) [v9]
Sean Mullan
mullan at openjdk.org
Mon Jul 29 14:38:42 UTC 2024
On Fri, 26 Jul 2024 20:01:23 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:
>
> review comments
src/java.base/share/classes/javax/crypto/KDF.java line 54:
> 52: * <em>Algorithm</em>With<em>PRF</em>. For instance, a KDF implementation of
> 53: * HKDF using HMAC-SHA256 has an algorithm name of "HKDFWithHmacSHA256". In some
> 54: * cases the PRF portion of the algorithm field may be omitted if the KDF
s/PRF/WithPRF/
src/java.base/share/classes/javax/crypto/KDF.java line 193:
> 191: throw new NoSuchAlgorithmException(
> 192: "Received an InvalidAlgorithmParameterException. Does this "
> 193: + "algorithm require an AlgorithmParameterSpec?", e);
s/AlgorithmParameterSpec/KDFParameters/. Same comments below.
src/java.base/share/classes/javax/crypto/KDF.java line 212:
> 210: * if a provider is specified and it does not support the specified KDF
> 211: * algorithm, or if provider is {@code null} and there is no provider
> 212: * that supports a KDF implementation of the specified algorithm
provider can't be null, so the last part of this doesn't make sense. Same comment in `getInstance(String, Provider)`.
src/java.base/share/classes/javax/crypto/KDF.java line 217:
> 215: * list
> 216: * @throws NullPointerException
> 217: * if the algorithm is {@code null}
provider also can't be null.
src/java.base/share/classes/javax/crypto/KDF.java line 248:
> 246: * that supports a KDF implementation of the specified algorithm
> 247: * @throws NullPointerException
> 248: * if the algorithm is {@code null}
provider also can't be null.
src/java.base/share/classes/javax/crypto/KDF.java line 278:
> 276: * the specified algorithm
> 277: * @throws InvalidAlgorithmParameterException
> 278: * if the {@code AlgorithmParameterSpec} is an invalid value
s/AlgorithmParameterSpec/KDFParameters/. Same comment below.
src/java.base/share/classes/javax/crypto/KDF.java line 412:
> 410: * <p>
> 411: * The {@code deriveKey} method may be called multiple times at the same
> 412: * time on a particular {@code KDF} instance.
Remove "at the same time" as thread safety is no longer a requirement. Same comment for `deriveData`.
src/java.base/share/classes/javax/crypto/KDF.java line 416:
> 414: * Delayed provider selection is also supported such that the provider
> 415: * performing the derive is not selected until the method is called. Once a
> 416: * provider is selected, it cannot be changed.
I think I would remove these sentences. This doesn't discuss the case if the provider is passed into `getInstance`, and it also doesn't discuss the case if a provider is not specified and`getProviderName` is called before `deriveKey`. You already explain these scenarios in the class summary, so I think that is probably sufficient. Same comment for deriveData.
src/java.base/share/classes/javax/crypto/KDF.java line 420:
> 418: * @param alg
> 419: * the algorithm of the resultant {@code SecretKey} object (may not be
> 420: * {@code null})
You don't have to say "may not be null" since throwing NPE is specified below. Also `kdfParameterSpec` cannot be null either, so should be consistent.
src/java.base/share/classes/javax/crypto/KDFSpi.java line 54:
> 52: * <p>
> 53: * A {@code KDFParameters} object may be specified for KDF algorithms
> 54: * that require this.
Suggest replacing "this" with "initialization parameters" to avoid confusion as to what this is referring to. I would also probably say "support" instead of "require" as some parameters may be optional.
src/java.base/share/classes/javax/crypto/KDFSpi.java line 71:
> 69: * Derives a key, returned as a {@code SecretKey}.
> 70: * <p>
> 71: * The {@code deriveKey} method may be called multiple times on a particular
s/deriveKey/engineDeriveKey/
src/java.base/share/classes/javax/crypto/KDFSpi.java line 72:
> 70: * <p>
> 71: * The {@code deriveKey} method may be called multiple times on a particular
> 72: * {@code KDF} instance.
s/KDF/KDFSpi/
src/java.base/share/classes/javax/crypto/KDFSpi.java line 96:
> 94: * Obtains raw data from a key derivation function.
> 95: * <p>
> 96: * The {@code deriveData} method may be called multiple times on a
s/deriveData/engineDeriveData/
src/java.base/share/classes/javax/crypto/KDFSpi.java line 97:
> 95: * <p>
> 96: * The {@code deriveData} method may be called multiple times on a
> 97: * particular {@code KDF} instance.
s/KDF/KDFSpi/
test/jdk/com/sun/crypto/provider/KDF/Functions.java line 55:
> 53:
> 54: if (!Arrays.equals(prk.getEncoded(), expectedPrk)) {
> 55: throw new Exception();
Suggest adding details to the exception messages as to what this is testing and why these comparisons fail.
test/jdk/com/sun/crypto/provider/KDF/Functions.java line 66:
> 64: test(HKDFParameterSpec.ofExtract().extractOnly());
> 65: test(HKDFParameterSpec.ofExtract().thenExpand(new byte[0], 32));
> 66: test(HKDFParameterSpec.ofExtract().addIKM(ikm).addSalt(new byte[0]).extractOnly());
What are these calls testing? Can you add a comment for each?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1695141729
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1695164446
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1695154671
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1695155404
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1695157824
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1695162823
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1695333402
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1695346273
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1695339742
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1695114796
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1695117975
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1695118234
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1695120209
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1695120663
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1695132194
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1695135168
More information about the security-dev
mailing list