On Tue, 23 Apr 2024 20:42:51 GMT, Kevin Driver <kdri...@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
