RFR: 8331008: Implement JEP 478: Key Derivation Function API (Preview) [v10]

Fri Aug 2 19:19:58 UTC 2024

On Wed, 31 Jul 2024 21:07:49 GMT, Kevin Driver <kdriver at openjdk.org> wrote:

>> 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>`

The PRF or any other config can be put in a `KDFParameters`. The algorithm name does not necessarily takes this form.

>> 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.

I don't have strong opinions now. 2) is fine now. You can still consider 1) if you can write a short one. Not all details need to be implemented. Just the structure.

>> 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?

Not sure. Maybe your sentence is OK.

>> 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?

Something like: If the result key is extractable, then its getEncoded value should have the same content as the result of deriveData.

>> 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.

In the era of `Cipher` and `Mac`, we don't treat nullability as a big issue and does not always specify it. Now it's called Java's Billion Dollar Mistake.

In fact, I don't think `doFinal` has ever intended to allow returning null.

>> 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.

One example is asking for a 42-byte AES key. We cannot say "AES" is unsupported, we also cannot say length 42 is not a valid params set itself, but the combination is invalid.

>> 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.

OK.

>> 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`.

OK. As long as the exception does not cover it.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1700825281
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1700937179
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1700863644
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1700872274
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1700936295
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1700941300
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1700942655
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1700943869