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

[Kevin Driver]

On Mon, 12 Aug 2024 23:33:42 GMT, Anthony Scarpino <ascarpino at 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