On Fri, 2 Aug 2024 19:16:59 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). >> >> Work was begun in [another PR](https://github.com/openjdk/jdk/pull/18924). > > Kevin Driver has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 16 additional > commits since the last revision: > > - update test to include Spi updates > - Update with latest from master > > Merge remote-tracking branch 'origin/master' into kdf-jep-wip > # Please enter a commit message to explain why this merge is necessary, > # especially if it merges an updated upstream into a topic branch. > # > # Lines starting with '#' will be ignored, and an empty message aborts > # the commit. > - add engineGetKDFParameters to the KDFSpi > - code review comment fix for javadoc specification > - change course on null return values from derive methods > - code review comments > - threading refactor + code review comments > - review comments > - review comments > - update code snippet type in KDF > - ... and 6 more: https://git.openjdk.org/jdk/compare/d5bcb5a2...dd2ee48f Some comments on the general KDF API. Comments for `HKDFParameterSpec`. src/java.base/share/classes/javax/crypto/KDF.java line 48: > 46: * This class provides the functionality of a Key Derivation Function > (KDF), > 47: * which is a cryptographic algorithm for deriving additional keys from a > secret > 48: * key and other data. "From a secret key". What about passwords? I suggest just saying "input keying material". "Other data" is optional. src/java.base/share/classes/javax/crypto/KDF.java line 51: > 49: * <p> > 50: * {@code KDF} objects are instantiated with the {@code getInstance} > family of > 51: * methods. KDF algorithm names follow a naming convention of I'm not sure KDF algorithms always follow this name. For example, kdf3, scrypt, and argon2. src/java.base/share/classes/javax/crypto/KDF.java line 56: > 54: * cases the WithPRF portion of the algorithm field may be omitted if the > KDF > 55: * algorithm has a fixed or default PRF. > 56: * <p> Maybe add a very brief paragraph describing the `derive` calls. src/java.base/share/classes/javax/crypto/KDF.java line 61: > 59: * the {@code deriveKey} or {@code deriveData} method is called and a > provider > 60: * is chosen that supports the parameters passed to the {@code deriveKey} > or > 61: * {@code deriveData} method, for example the initial key material. > However, if No need for "for example". src/java.base/share/classes/javax/crypto/KDF.java line 64: > 62: * {@code getProviderName} is called before calling the {@code deriveKey} > or > 63: * {@code deriveData} methods, the first provider supporting the KDF > algorithm > 64: * is chosen which may not be the desired one; therefore it is > recommended not Not only the "KDF algorithm", but also the "KDFParameters" if the `getInstance` method has this argument. Explain what "the desired one" means. src/java.base/share/classes/javax/crypto/KDF.java line 75: > 73: * HKDFParameterSpec.ofExtract() > 74: * .addIKM(ikm) > 75: * .addSalt(salt).thenExpand(info, 42); I know 42 is a magic number, but for AES let's choose 32. src/java.base/share/classes/javax/crypto/KDF.java line 166: > 164: * Returns the {@code KDFParameters} used to initialize the object. > 165: * > 166: * @return the parameters used to initialize the object Can this be null if not provided or provided as null? Is that possible that even if there is no parameters provided but the impl might choose default parameters and return them here? src/java.base/share/classes/javax/crypto/KDF.java line 176: > 174: * > 175: * @param algorithm > 176: * the key derivation algorithm to use Add a link to a section in the Standard Named Doc. Same for all `getInstance`s below. src/java.base/share/classes/javax/crypto/KDF.java line 183: > 181: * if no additional parameters were provided > 182: */ > 183: public KDFParameters getKDFParameters() { I still want to know if this method always returns null if only getInstance(alg) is called without params. Or, when there are default params, they will be returned. src/java.base/share/classes/javax/crypto/KDF.java line 215: > 213: * list > 214: * @throws NullPointerException > 215: * if the algorithm or provider is {@code null} I suggest `if {@code algorithm} or {@code provider)...`. Same below. src/java.base/share/classes/javax/crypto/KDF.java line 265: > 263: * the key derivation algorithm to use > 264: * @param kdfParameters > 265: * the {@code KDFParameters} used to configure this KDF's > algorithm or Just say "configure this KDF". Not sure what "configure this KDF's algorithm" means exactly. src/java.base/share/classes/javax/crypto/KDF.java line 274: > 272: * the specified algorithm > 273: * @throws InvalidAlgorithmParameterException > 274: * if the {@code KDFParameters} is an invalid value Be careful here. Do you really want to throw both `NoSuchAlgorithmException` and `InvalidAlgorithmParameterException` here? Suppose there is an impl that is for the algorithm name but does not accept the parameters. Which exception do you want to throw? Is your rule always reliable? src/java.base/share/classes/javax/crypto/KDF.java line 304: > 302: * if no {@code Provider} supports a {@code KDFSpi} > implementation for > 303: * the specified algorithm > 304: * @throws InvalidAlgorithmParameterException In your current implementation, parameters are never checked. IIUC, it will only be used (i.e. passed into the constructor of implementations) in deriveXyz calls. This brings out another issue. When deriveXyz is called and and InvalidAlgorithmParameterException is thrown, do we need if it's because the constructor fails or the engineDeriveXyz call fails? This is a bigger problem. src/java.base/share/classes/javax/crypto/KDF.java line 404: > 402: * <p> > 403: * The {@code deriveKey} method may be called multiple times on a > particular > 404: * {@code KDF} instance. Similar comment as in `KDFSpi`. If we don't want to talk about thread-safety, I'd rather not mention anything. src/java.base/share/classes/javax/crypto/KDF.java line 409: > 407: * the algorithm of the resultant {@code SecretKey} object > 408: * @param kdfParameterSpec > 409: * derivation parameters Please emphasize that this is "input" and not "parameters". src/java.base/share/classes/javax/crypto/KDF.java line 412: > 410: * > 411: * @return a {@code SecretKey} object corresponding to a key built > from the > 412: * KDF output and according to the derivation parameters Similar comment as in `KDFSpi`. Can this be null? src/java.base/share/classes/javax/crypto/KDF.java line 415: > 413: * > 414: * @throws InvalidAlgorithmParameterException > 415: * if the information contained within the {@code > KDFParameterSpec} is Typo, `s/KDFParameterSpec/kdfParameterSpec/`. src/java.base/share/classes/javax/crypto/KDF.java line 416: > 414: * @throws InvalidAlgorithmParameterException > 415: * if the information contained within the {@code > KDFParameterSpec} is > 416: * invalid or incorrect for the type of key to be derived Similar comment as in `KDFSpi`. What is `alg` is not supported or the combination of `alg` and `kdfParameterSpec` is invalid? src/java.base/share/classes/javax/crypto/KDF.java line 430: > 428: + "null or empty"); > 429: } > 430: Objects.requireNonNull(kdfParameterSpec); The lines above can be put outside the `synchronized` block. src/java.base/share/classes/javax/crypto/KDFSpi.java line 38: > 36: * This class defines the <i>Service Provider Interface</i> (<b>SPI</b>) > for the > 37: * {@code KDF} class. > 38: * <p> Add a paragraph that requires "all implementations must provide a public constructor that takes a single `KDFParameters` argument. The constructor must call `super(params)` where `params` is the argument passed to it. The constructor must throw an `InvalidAlgorithmParameterException` if the argument is inappropriate". IMO it will even be better to include the pseudocode of a tiny KDF implementation here. For example, the one used by Unix to generate a key from a password. Also, I wonder if we need to add a paragraph talking about the difference between `KDFParameters` passed to the constructor and the `AlgorithmParameterSpec` passed to `derive` calls. If there is a clear guidance for implementors on which info goes into which, it will be very useful. Usually the former is configuration (Ex: underlying algorithm) for the algorithm and the latter is key input material (Ex: password) and auxiliary data (Ex: salt). src/java.base/share/classes/javax/crypto/KDFSpi.java line 38: > 36: * This class defines the <i>Service Provider Interface</i> (<b>SPI</b>) > for the > 37: * {@code KDF} class. > 38: * <p> Unfortunately I cannot unresolve a resolved conversation. For my comment above, thanks for adding the requirement on constructors. What about my other 2 suggestions? src/java.base/share/classes/javax/crypto/KDFSpi.java line 44: > 42: * > 43: * @see KDF > 44: * @see SecretKey I'd rather add a `@see KDFParameters` instead of `SecretKey`. src/java.base/share/classes/javax/crypto/KDFSpi.java line 72: > 70: * <p> > 71: * The {@code engineDeriveKey} method may be called multiple times on > a particular > 72: * {@code KDFSpi} instance. This sentence invites people asking about thread safety, but we don't want to guarantee anything here. src/java.base/share/classes/javax/crypto/KDFSpi.java line 80: > 78: * > 79: * @return a {@code SecretKey} object corresponding to a key built > from the > 80: * KDF output and according to the derivation parameters Can the return value be `null`? Same with `deriveData`. src/java.base/share/classes/javax/crypto/KDFSpi.java line 80: > 78: * > 79: * @return a {@code SecretKey} object corresponding to a key built > from the > 80: * KDF output and according to the derivation parameters Do we need to specify that if the result is extractable then the encoding should be the same as the output of `deriveData`? I'm really not sure about this. src/java.base/share/classes/javax/crypto/KDFSpi.java line 82: > 80: * KDF output and according to the derivation parameters > 81: * > 82: * @throws InvalidAlgorithmParameterException `alg` may also be invalid or not supported by the KDF. For example, if the params has length 40 but algorithm is AES, the impl may reject the combination. src/java.base/share/classes/javax/crypto/KDFSpi.java line 89: > 87: * KDF output and according to the derivation parameters or > {@code null} > 88: * in cases where an exception is not thrown but a value cannot be > 89: * returned Do you really need to say "corresponding to" and "according to"? What else can it be? It's not clear when the return value is null. This also requires users to check for multiple invalid cases. src/java.base/share/classes/javax/crypto/KDFSpi.java line 93: > 91: > 92: /** > 93: * Obtains raw data from a key derivation function. `Obtains` or `Derives`? src/java.base/share/classes/javax/crypto/KDFSpi.java line 94: > 92: * if the information contained within the {@code > kdfParameterSpec} is > 93: * invalid or incorrect for the type of key to be derived or if > 94: * {@code alg} is invalid or unsupported by the KDF implementation The current spec says this is bad or that is bad. Is it necessary to include the case where the combination is invalid? BTW, I still don't see why it's worth mentioning "invalid" and "incorrect". After all, the exception name only mentions invalid. Do you think it's not precise? src/java.base/share/classes/javax/crypto/KDFSpi.java line 103: > 101: * @return a byte array whose length matches the specified length in > the > 102: * processed {@code KDFParameterSpec} and containing the output > from the > 103: * key derivation function The `KDFParameterSpec` itself has no specified length. src/java.base/share/classes/javax/crypto/KDFSpi.java line 107: > 105: * @throws InvalidAlgorithmParameterException > 106: * if the information contained within the {@code > KDFParameterSpec} is > 107: * invalid or incorrect for the type of key to be derived Do we need both `invalid` and `incorrect`? src/java.base/share/classes/javax/crypto/KDFSpi.java line 119: > 117: * if the information contained within the {@code > kdfParameterSpec} is > 118: * invalid or incorrect for the type of key to be derived or if > 119: * {@code alg} is invalid or unsupported by the KDF > implementation No `alg` here. src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 43: > 41: * In the Extract and Extract-then-Expand cases, the {@code addIKM} and > 42: * {@code addSalt} methods may be called repeatedly (and chained). This > provides > 43: * for use-cases where a {@code SecretKey} may reside on an HSM and not be "where a portion of the IKM resides in a non-extractable SecretKey and the whole IKM cannot be provided as a single object". src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 52: > 50: * may include both byte arrays and (possibly non-extractable) secret > keys. > 51: * <p> > 52: * Examples: Add comment to each usage below. src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 58: > 56: * HKDFParameterSpec.ofExtract() > 57: * .addIKM(ikmPart1) > 58: * .addIKM(ikmPart2) Rename `ikmPart1` and `ikmPart2` to `label` and `key`. Add a comment. src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 81: > 79: */ > 80: @PreviewFeature(feature = PreviewFeature.Feature.KEY_DERIVATION) > 81: public interface HKDFParameterSpec extends AlgorithmParameterSpec { Does this class need to extend `AlgorithmParameterSpec`? It's more like a factory. src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 101: > 99: List<SecretKey> salts = new ArrayList<>(); > 100: > 101: Builder() {} Can be made private. src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 127: > 125: * @param info > 126: * the optional context and application specific information > (may be > 127: * {@code null}); the byte[] is copied to prevent subsequent "byte array". src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 290: > 288: * > 289: * @param prk > 290: * the pseudorandom key; must not be {@code null} in the Expand > case No need to say "in he Expand case". src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 297: > 295: * @param length > 296: * the length of the output key material (must be > 0 and > < 255 * > 297: * HMAC length) The maximum length can only be checked in the implementation and not here. Maybe do not mention it. src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 315: > 313: /** > 314: * Defines the input parameters of an Extract operation as defined > in <a > 315: * href="http://tools.ietf.org/html/rfc5869";>RFC 5869</a>. Unless there is a specific section for the case, I don't think you need to provide the link to the RFC in these 3 classes. src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 324: > 322: private final List<SecretKey> salts; > 323: > 324: private Extract() { Useful anywhere? src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 466: > 464: * the order they were added. > 465: * > 466: * @return the input key material values Make the spec the same as in `Extract`. ------------- PR Review: https://git.openjdk.org/jdk/pull/20301#pullrequestreview-2208777638 PR Review: https://git.openjdk.org/jdk/pull/20301#pullrequestreview-2210931145 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1698622862 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1698624603 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1698635734 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1698625536 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1698630742 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1698631750 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1698644961 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1698645792 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1700923810 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1698647892 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1698659657 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1698657943 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1700925362 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1698661060 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1698663149 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1698665806 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1698668536 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1698667819 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1698664179 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1697645623 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1700862364 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1697646175 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1698613619 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1697727038 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1697728991 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1697726871 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1700840062 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1698615228 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1700866610 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1697727954 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1698619354 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1700846456 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1698980144 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1698981519 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1698983539 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1698989594 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1698990787 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1698995897 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1698999134 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1699000160 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1699001239 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1699001855 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1699003029
