On Wed, 31 Jul 2024 19:28:33 GMT, Weijun Wang <wei...@openjdk.org> wrote:
>> 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 > > Comments for `HKDFParameterSpec`. @wangweij: Commit https://github.com/openjdk/jdk/pull/20301/commits/30dd26e2382486b6aaec6fd0798aa77c934fcb7a contains the `synchronized` block refactor as well as changes to address your other code review comments, except where I replied or asked a follow-up. > 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. The existing sentence seems to cover this scenario. Let me know if you disagree. `In some cases the WithPRF portion of the algorithm field may be omitted if the KDF algorithm has a fixed or default PRF.` However, if Argon2 (or another relevant example) were to require a PRF specifier at some point in the future, we would still want it to follow that form, right? `<em>Algorithm</em>With<em>PRF</em>` > 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. Will discuss "offline" with the other `KDFParameters` & DPS discussion. > 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? In that case, it should be an `InvalidAlgorithmParameterException` because the parameters provided were invalid. > 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. ~`KDFParameters` is an empty interface **and** optional. There is nothing to validate, yet. The parameters need only be passed to the implementation. The HKDF implementation does not require them.~ After discussion, I understand the first concern better. It is not related to the HKDF implementation. We will discuss this further "offline". Your second concern is relevant if `getInstance` and `deriveX` happen in the same try/catch but not otherwise. In that case, the exception message can do the work of informing the user what occurred. An implementation could also create a subclass of `InvalidAlgorithmParameterException` to indicate by type instead of message. > 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. Same as above: I was trying to convey that deriveKey|Data are not like doFinal in that they can be called more than once on a single instance. Is there a different way to word this? Or is it not important to mention here? > 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? 1) An example of a KDF implementation within the class header seems a bit excessive to me. 2) I felt that changes to the wording describing `KDFParameters` elsewhere accomplished this goal. > 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. I was trying to convey that deriveKey|Data are not like doFinal in that they can be called more than once on a single instance. Is there a different way to word this? Or is it not important to mention 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 > > 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. Can you add some more detail here? > 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. I looked at `Cipher` and `Mac`. They do not specify in `doFinal` whether or not the result may be `null`. I think this specification is sufficient for implementations (more precise than the above mentioned classes) and allows the implementors of `KDFSpi` to return `null` in cases which we may not yet have anticipated. > 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`? I think "obtains ... derivation ..." reads better than "derives ... derivation ...". Personal opinion maybe. > 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? I am open to changing this wording, but here is my logic on the above: `or if {@code alg} is invalid or unsupported by the KDF implementation` Since the kdfParameterSpec (the `AlgorithmParameterSpec`) determines the KDF implementation, my feeling is that **if** the implementation cannot return a key of the desired algorithm, this could be because it is: `unsupported by the KDF implementation`. To me, this covers the combination case. > 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`? IMO they mean different things. Invalid may suggest input that is out of a particular range, for example. Incorrect input may suggest input that passes "sanitation" checks but may not make sense in a particular scenario. > 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. Each of the inner classes extend `HKDFParameterSpec` and therefore `AlgorithmParameterSpec`. I like the inheritance model here. > 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. I disagree. I think this is a helpful bit of info for the developer who may be surprised later by an `Exception`. ------------- PR Comment: https://git.openjdk.org/jdk/pull/20301#issuecomment-2263878241 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1699108651 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1702170605 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1700429103 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1700971254 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1699087939 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1700878089 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1698680917 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1700779973 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1700875137 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1699086646 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1700886443 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1699084170 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1700726226 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1700733297
