RFR: 8331008: KDF Implementation [v3]

[Kevin Driver]

On Thu, 9 May 2024 12:22:14 GMT, Sean Mullan <mullan at openjdk.org> wrote:

>> Kevin Driver has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   some code review comments
>
> src/java.base/share/classes/javax/crypto/KDF.java line 43:
> 
>> 41: 
>> 42: /**
>> 43:  * This class provides the functionality of a key derivation algorithm for the Java Cryptographic
> 
> We don't normally say "for the Java Cryptographic Extension framework" in our other APIs, so I would remove that part. Also, can you try to keep lines to around 80 characters - it helps with code reviews.

Done. Please resolve if satisfied.

> src/java.base/share/classes/javax/crypto/KDF.java line 46:
> 
>> 44:  * Extension (JCE) framework.
>> 45:  * <p>
>> 46:  * {@code KeyDerivation} objects will be instantiated through the {@code getInstance} family of
> 
> s/KeyDerivation/KDF/
> s/will be/are/

Done. Please resolve if satisfied.

> src/java.base/share/classes/javax/crypto/KDF.java line 47:
> 
>> 45:  * <p>
>> 46:  * {@code KeyDerivation} objects will be instantiated through the {@code getInstance} family of
>> 47:  * methods.  Key derivation algorithm names will follow a naming convention of
> 
> remove "will".

Done. Please resolve if satisfied.

> src/java.base/share/classes/javax/crypto/KDF.java line 48:
> 
>> 46:  * {@code KeyDerivation} objects will be instantiated through the {@code getInstance} family of
>> 47:  * methods.  Key derivation algorithm names will follow a naming convention of
>> 48:  * <I>algorithm</I>/<I>PRF</I>.  The algorithm field will be the KDF name
> 
> s/will be/is/
> s/name/algorithm/

Done. Please resolve if satisfied.

> src/java.base/share/classes/javax/crypto/KDF.java line 89:
> 
>> 87: 
>> 88:     /**
>> 89:      * Instantiates a KeyDerivation object.
> 
> s/KeyDerivation/KDF/

Done. Please resolve if satisfied.

> src/java.base/share/classes/javax/crypto/KDF.java line 100:
> 
>> 98:      *     the algorithm parameters
>> 99:      */
>> 100:     protected KDF(KDFSpi keyDerivSpi, Provider provider, String algorithm,
> 
> This class is final, so a protected constructor is not necessary. You should be able to make this private.

Done. Please resolve if satisfied.

> src/java.base/share/classes/javax/crypto/KDF.java line 110:
> 
>> 108: 
>> 109:     /**
>> 110:      * Returns the algorithm name of this {@code KeyDerivation} object.
> 
> s/KeyDerivation/KDF/
> 
> There are others that need to be corrected, search for all other instances in this class.

Done. Please resolve if satisfied.

> src/java.base/share/classes/javax/crypto/KDF.java line 115:
> 
>> 113:      * {@code getInstance} calls that created this {@code KeyDerivation} object.
>> 114:      *
>> 115:      * @return the algorithm name of this {@code KeyDerivation} object.
> 
> Nit: remove period, sentences that don't start with a capital letter and don't have a following sentence should not have period at end

Done. Please resolve if satisfied.

> src/java.base/share/classes/javax/crypto/KDF.java line 117:
> 
>> 115:      * @return the algorithm name of this {@code KeyDerivation} object.
>> 116:      */
>> 117:     public final String getAlgorithm() {
> 
> The class is final, so none of the methods need to have the final keyword.

Done. Please resolve if satisfied.

> src/java.base/share/classes/javax/crypto/KDF.java line 131:
> 
>> 129:     }
>> 130: 
>> 131:     private String getProviderName() {
> 
> Make this method public.

Done. Please resolve if satisfied.

> src/java.base/share/classes/javax/crypto/spec/KDFParameterSpec.java line 32:
> 
>> 30:  *
>> 31:  * <P> This interface contains no methods or constants. Its only purpose
>> 32:  * is to group (and provide type safety for) all parameter specifications. All KDF parameter
> 
> The words "all parameter specifications" is too general as this is specific to KDF. I would probably change this to "all KDF parameter specifications." Then you don't need the last sentence as it says the same thing.

Done. Please resolve if satisfied.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595927487
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595927563
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595927626
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595927676
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595927914
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595927992
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595928123
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595928198
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595928294
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595928393
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595927336