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

[Kevin Driver]

On Sun, 22 Sep 2024 13:37:11 GMT, Weijun Wang <weijun at openjdk.org> wrote:

>> Kevin Driver has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   spec wording changes and a few tweaks to DPS
>
> src/java.base/share/classes/javax/crypto/KDF.java line 141:
> 
>> 139:     // @param kdfParameters the parameters
>> 140:     private KDF(Delegate delegate, String algorithm,
>> 141:                 KDFParameters kdfParameters) {
> 
> There is really no need to pass in `kdfParameters` in this case. Set `this.kdfParameters` to `null`.

Addressed in https://github.com/openjdk/jdk/pull/20301/commits/315f90e6e8572dfaea0fdb4f5d94452637a7e423.

> src/java.base/share/classes/javax/crypto/KDF.java line 554:
> 
>> 552:             } catch (NoSuchAlgorithmException e) {
>> 553:                 // this will never be thrown in the deriveData case
>> 554:                 throw new RuntimeException(e);
> 
> If this will never happen, throw an `AssertionError`.

Addressed in https://github.com/openjdk/jdk/pull/20301/commits/315f90e6e8572dfaea0fdb4f5d94452637a7e423.

> src/java.base/share/classes/javax/crypto/KDF.java line 584:
> 
>> 582:         boolean isDeriveData = (algorithm == null);
>> 583: 
>> 584:         if (checkSpiNonNull(theOne)) {
> 
> This check is still needed inside the `synchronized` block below, otherwise another thread waiting on the same lock with perform DPS again after the first one exits.
> 
> In fact, since you already check for the same condition before each `chooseProvider` call, it is only necessary inside the `synchronized` block.

Addressed in https://github.com/openjdk/jdk/pull/20301/commits/315f90e6e8572dfaea0fdb4f5d94452637a7e423.

> src/java.base/share/classes/javax/crypto/KDF.java line 593:
> 
>> 591:             Exception lastException = null;
>> 592:             if (!checkSpiNonNull(candidate)) {
>> 593:                 throw new RuntimeException("Unexpected Error: candidate is null!");
> 
> This should not happen. Make it an `AssertionError`.

Addressed in https://github.com/openjdk/jdk/pull/20301/commits/315f90e6e8572dfaea0fdb4f5d94452637a7e423.

> src/java.base/share/classes/javax/crypto/KDF.java line 607:
> 
>> 605:                         return result;
>> 606:                     } catch (Exception e) {
>> 607:                         if (lastException == null) {
> 
> Add some debug output since `e` can be silently discarded.

Addressed in https://github.com/openjdk/jdk/pull/20301/commits/315f90e6e8572dfaea0fdb4f5d94452637a7e423.

> src/java.base/share/classes/javax/crypto/KDF.java line 618:
> 
>> 616:                 throw e; // getNext reached end and have seen IAPE
>> 617:             } catch (NoSuchAlgorithmException e) {
>> 618:                 // getNext reached end without finding an implementation
> 
> Print debug info on `e`. Each time an exception is not added as a cause, it makes sense to print it out in the debug log.

Addressed in https://github.com/openjdk/jdk/pull/20301/commits/315f90e6e8572dfaea0fdb4f5d94452637a7e423.

> src/java.base/share/classes/javax/crypto/KDF.java line 653:
> 
>> 651:                     pdebug.println(
>> 652:                             "The evaluated provider does not support the "
>> 653:                             + "specified algorithm and parameters");
> 
> `nsae.printStackTrace(pdebug.getPrintStream());`

Addressed in https://github.com/openjdk/jdk/pull/20301/commits/315f90e6e8572dfaea0fdb4f5d94452637a7e423.

> src/java.base/share/classes/javax/crypto/KDF.java line 662:
> 
>> 660:                     + "specified algorithm and parameters");
>> 661:         }
>> 662:         if (hasOne) throw new InvalidAlgorithmParameterException();
> 
> Since this is an IAPE, there should be a reason what "AP" is "I".

Addressed in https://github.com/openjdk/jdk/pull/20301/commits/315f90e6e8572dfaea0fdb4f5d94452637a7e423.

> src/java.base/share/classes/javax/crypto/KDF.java line 669:
> 
>> 667:         return (d != null && d.spi() != null);
>> 668:     }
>> 669: }
> 
> Add an empty line at the end of this file.

Addressed in https://github.com/openjdk/jdk/pull/20301/commits/315f90e6e8572dfaea0fdb4f5d94452637a7e423.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1771850266
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1771850411
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1771850571
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1771852068
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1771852332
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1771852496
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1771852776
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1771852666
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1771852908