On Mon, 12 Aug 2024 23:33:42 GMT, Anthony Scarpino <ascarp...@openjdk.org> wrote:
>> Kevin Driver has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains 16 additional >> commits since the last revision: >> >> - update test to include Spi updates >> - Update with latest from master >> >> Merge remote-tracking branch 'origin/master' into kdf-jep-wip >> # Please enter a commit message to explain why this merge is necessary, >> # especially if it merges an updated upstream into a topic branch. >> # >> # Lines starting with '#' will be ignored, and an empty message aborts >> # the commit. >> - add engineGetKDFParameters to the KDFSpi >> - code review comment fix for javadoc specification >> - change course on null return values from derive methods >> - code review comments >> - threading refactor + code review comments >> - review comments >> - review comments >> - update code snippet type in KDF >> - ... and 6 more: https://git.openjdk.org/jdk/compare/2638d442...dd2ee48f > > src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java > line 245: > >> 243: } >> 244: >> 245: private static boolean isNullOrEmpty(Collection<?> c) { > > This appears to not be used. Addressed in https://github.com/openjdk/jdk/pull/20301/commits/c6f491cd05c76088e6431b2ba9d4ab42b29e4055. Please indicate if this is resolved. > src/java.base/share/classes/javax/crypto/KDF.java line 199: > >> 197: >> 198: /** >> 199: * Returns a {@code KDF} object that implements the specified >> algorithm. > > This could be considered a CSR comment as well. You may not want to say that > this object "implements the specified algorithm". Given the provider > implements the algorithm and the provider is delayed initialization. It > maybe better to say this "Returns a KDF instance initialized with the > specified algorithm." > The same applies for other `getInstance()` methods below. Addressed in https://github.com/openjdk/jdk/pull/20301/commits/c6f491cd05c76088e6431b2ba9d4ab42b29e4055. Please indicate if this is resolved. > src/java.base/share/classes/javax/crypto/KDF.java line 222: > >> 220: } catch (InvalidAlgorithmParameterException e) { >> 221: throw new NoSuchAlgorithmException( >> 222: "Received an InvalidAlgorithmParameterException. Does >> this " > > I think it would be better than to say "KDFParameters must be specified." > You already attached the initial exception and there isn't much mystery as > the cause given this method passes "null" Addressed in https://github.com/openjdk/jdk/pull/20301/commits/c6f491cd05c76088e6431b2ba9d4ab42b29e4055. Please indicate if this is resolved. > src/java.base/share/classes/javax/crypto/KDF.java line 291: > >> 289: } catch (InvalidAlgorithmParameterException e) { >> 290: throw new NoSuchAlgorithmException( >> 291: "Received an InvalidAlgorithmParameterException. Does >> this " > > same as mentioned above Addressed in https://github.com/openjdk/jdk/pull/20301/commits/c6f491cd05c76088e6431b2ba9d4ab42b29e4055. Please indicate if this is resolved. > src/java.base/share/classes/javax/crypto/KDF.java line 441: > >> 439: } >> 440: >> 441: private static KDF handleException(NoSuchAlgorithmException e) > > My comment originates more with the callers of this method. While I > appreciate that you are trying to throw correct exception for the situation, > you may have noticed that if the developer calls a `getInstance()` which only > throws `NSAE` (line 216 for example), you could be in a situation where you > unwrap the causing `IAPE` from the wrapping `NSAE`, to then rewrap it in a > `NSAE` on line 219. I may be just better to let the provider throw what > they want and not try to modify it. Nit. May address later. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1720354216 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1720353458 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1720351432 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1720351529 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1720352846
