On Fri, 20 Sep 2024 20:11:17 GMT, Kevin Driver <kdri...@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
