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

[Weijun Wang]

On Wed, 11 Sep 2024 22:49:12 GMT, Kevin Driver <kdriver at openjdk.org> wrote:

>> src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 143:
>> 
>>> 141:         throws InvalidAlgorithmParameterException {
>>> 142:         List<SecretKey> ikms, salts;
>>> 143:         byte[] inputKeyMaterial, salt, pseudoRandomKey, info;
>> 
>> It's always nice to clean up these intermediate bytes related to secret keys.
>
> @wangweij: Addressed in https://github.com/openjdk/jdk/pull/20301/commits/82791ac01fb7f597a7e814403261c7a50e8a08df.

We usually clean up the array in a `finally` block if an exception could be thrown in between.

>> src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 274:
>> 
>>> 272:             }
>>> 273:         } else if(keys != null) {
>>> 274:                 return null;
>> 
>> Isn't an empty array better here? The null return is unexpected and has to be handled specially.
>
> @wangweij: Resolved in https://github.com/openjdk/jdk/pull/20301/commits/82791ac01fb7f597a7e814403261c7a50e8a08df.

Thanks. And you don't need to check `if (salt == null` and `if (inputKeyMaterial == null)` in `hkdfExtract` now. Also, change the javadoc for `hkdfExtract` about nulls.

>> src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 299:
>> 
>>> 297:         throws InvalidKeyException, NoSuchAlgorithmException {
>>> 298: 
>>> 299:         if (salt == null) {
>> 
>> Also cover the empty `salt` case here. The `SecretKeySpec` creation below would fail.
>> 
>> Hint: when people call `addSalt(k)`, `k` can be empty. It doesn't have to be a `SecretKeySpec`. This is worth a test.
>
> @wangweij: Addressed in https://github.com/openjdk/jdk/pull/20301/commits/82791ac01fb7f597a7e814403261c7a50e8a08df.

I understand `addSalt(new byte[0])` is not a problem, but what about

addSalt(new SecretKey() {
    public byte[] getEncoded() { return new byte[0]; }
    ....
};

Anyway, with the new length check, this will be detected.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1756008449
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1756012745
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1756011616