RFR: 8331008: KDF Implementation
Weijun Wang
weijun at openjdk.org
Thu May 9 15:32:57 UTC 2024
On Tue, 23 Apr 2024 20:42:51 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).
Some comments on `HKDFParameterSpec`.
src/java.base/share/classes/javax/crypto/KDF.java line 58:
> 56:
> 57: public final class KDF {
> 58: private static final Debug debug = Debug.getInstance("jca", "KeyDerivation");
Change to a shorter option name "kdf". Also, add it into the help output of `sun.security.util.Debug`.
src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 36:
> 34: * Parameters for the combined Extract-Only, Expand-Only, or Extract-then-Expand operations of the
> 35: * HMAC-based Key Derivation Function (HKDF). The HKDF function is defined in <a
> 36: * href="http://tools.ietf.org/html/rfc5869">RFC 5869</a>.
Please give as many examples as you can here. This is the only public API for HKDF. Show people how to construct `HKDFParameterSpec` for all 3 operations.
Describe the accumulation feature of `addIKM` and `addSalt` here, and point out why this is necessary (for labeled unextractable key). Then there is no need to mention this in all 4 `add` methods.
src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 40:
> 38: * @since 23
> 39: */
> 40: public interface HKDFParameterSpec extends KDFParameterSpec {
Make it `sealed` and its 3 child `final`.
src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 47:
> 45: final class Builder {
> 46:
> 47: Extract extract = null;
No need to store an `extract` field. Just create one and return it in `extractOnly`.
src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 50:
> 48: List<SecretKey> ikms = new ArrayList<>();
> 49: List<SecretKey> salts = new ArrayList<>();
> 50: SecretKey prk = null;
No `prk` here. In fact, maybe rename `Builder` to `ExtractBuilder`?
src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 70:
> 68: */
> 69: public Extract extractOnly() {
> 70: if (this.ikms.isEmpty() && this.salts.isEmpty()) {
I don't think this check is necessary? While it's probably unsafe to provide no IKM, providing no salt is quite common. Anyway, no need to restrict on both, IMHO
src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 96:
> 94: + "been called");
> 95: }
> 96: return extractExpand(new Extract(List.copyOf(ikms), List.copyOf(salts)), info, length);
You can reuse `extractOnly` method here.
src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 108:
> 106: *
> 107: * @param ikm
> 108: * the ikm value (null values will not be added)
Are you sure about allowing `null`? Any valid use case? Same question for the other `add` methods.
src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 134:
> 132: public Builder addIKM(byte[] ikm) {
> 133: if (ikm != null && ikm.length != 0) {
> 134: return addIKM(new SecretKeySpec(ikm, "RAW"));
Usually "RAW" is for format. I'd rather use "IKM" or "Generic". Same for `addSalt`.
src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 201:
> 199: * the PRK (may be null)
> 200: * @param info
> 201: * the info (may be null)
I know you use a null `prk` in `ExtractExpand`, but this method is public available for the Expand-Only mode and we don't want end users to provide a null here.
For `info`, I'd rather allow empty input and reject null.
src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 213:
> 211: /**
> 212: * Static helper-method that may be used to initialize an {@code ExtractExpand} object
> 213: * <p>
No we really need this method since user can already call `extract()....andExpand()`?
If this method is removed, then we don't need to check null for `ext` in `new ExtractExpand`.
Or, you want to create one `Extract` object and then reuse it multiple times?
src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 241:
> 239: private final List<SecretKey> salts;
> 240:
> 241: private Extract() {
This method is useless if we remove the `extract` object in `Builder`.
src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 255:
> 253: * @return the unmodifiable {@code List} of IKM values
> 254: */
> 255: public List<SecretKey> ikms() {
The `ikms` is already unmodifiable when this object is created back in `Builder.extractOnly`. Or, you may move the `copyOf` methods from that method to the constructor in this class.
src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 277:
> 275:
> 276: // HKDF-Expand(PRK, info, L) -> OKM
> 277: private SecretKey pseudoRandomKey = null;
Name too long. Just use `prk`.
src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 316:
> 314: */
> 315: public byte[] info() {
> 316: return info;
return a clone.
src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 352:
> 350: */
> 351: private ExtractExpand(Extract ext, byte[] info, int length) {
> 352: if (ext == null) {
Could this happen?
-------------
PR Review: https://git.openjdk.org/jdk/pull/18924#pullrequestreview-2048046661
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595609929
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595506656
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595508230
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595572683
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595571413
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595576901
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595592404
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595579692
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595581335
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595585145
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595587361
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595589388
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595590959
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595593365
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595593711
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595594155
More information about the security-dev
mailing list