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

[Weijun Wang]

On Fri, 20 Sep 2024 20:11:17 GMT, Kevin Driver <kdriver at openjdk.org> wrote:

>> Introduce an API for Key Derivation Functions (KDFs), which are cryptographic algorithms for deriving additional keys from a secret key and other data. See [JEP 478](https://openjdk.org/jeps/478).
>> 
>> Work was begun in [another PR](https://github.com/openjdk/jdk/pull/18924).
>
> 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

Comments on `KDF.java` implementation.

src/java.base/share/classes/javax/crypto/KDF.java line 116:

> 114:     private Delegate theOne;
> 115:     //guarded by 'lock'
> 116:     private Delegate candidate;

Since `candidate` is never modified, make it `final`.

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

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

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.

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

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.

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.

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());`

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

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.

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

PR Review: https://git.openjdk.org/jdk/pull/20301#pullrequestreview-2320856915
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1770557142
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1770557223
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1770557319
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1770557903
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1770558024
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1770558159
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1770558356
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1770558795
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1770558630
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1770558915