RFR: 8331008: Implement JEP 478: Key Derivation Function API (Preview) [v10]
Kevin Driver
kdriver at openjdk.org
Fri Aug 2 19:19:58 UTC 2024
On Wed, 31 Jul 2024 19:28:33 GMT, Weijun Wang <weijun at 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
More information about the security-dev
mailing list