On Wed, 31 Jul 2024 21:07:49 GMT, Kevin Driver <kdri...@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
