On Fri, 16 Aug 2024 18:16:07 GMT, Valerie Peng <valer...@openjdk.org> wrote:
>> Kevin Driver has updated the pull request incrementally with one additional >> commit since the last revision: >> >> addressed several review comments, namely: - renaming the getParameters >> method - renaming the AlgorithmParameterSpec object - address some javadoc >> exception messages - add some information to KDF class private constructor >> javadocs - other general cleanup > > src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java > line 202: > >> 200: salts = anExtractThenExpand.salts(); >> 201: // we should be able to combine these Lists of keys into >> single >> 202: // SecretKey Objects, > > "Single SecretKey objects" => "a byte[]" > "List of keys" is really "a list of key segments" which are combined into one > key. Same goes for salts. The API is designed with the protocol usage in > mind, but the naming we have here does not directly line up with RFC5869 > which only mentions singular "IKM" and "SALT". Addressed in https://github.com/openjdk/jdk/pull/20301/commits/48395b86ba8e1cda663ae326e06ae2556f4b905a. Please indicate if this is resolved. > src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java > line 328: > >> 326: * >> 327: * @throws InvalidKeyException >> 328: * if an invalid key was provided through the {@code >> HkdfParameterSpec} > > Clarify "key" with "{@code prk}" and get rid of the trailing description. Addressed in https://github.com/openjdk/jdk/pull/20301/commits/48395b86ba8e1cda663ae326e06ae2556f4b905a. Please indicate if this is resolved. > src/java.base/share/classes/javax/crypto/KDF.java line 124: > >> 122: >> 123: int DERIVE_KEY = 0; >> 124: int DERIVE_DATA = 1; > > These two variables are a waste of space... `chooseProvider()` impl can just > use a local boolean variable to remember whether the derivation is for key or > for data. Addressed in https://github.com/openjdk/jdk/pull/20301/commits/48395b86ba8e1cda663ae326e06ae2556f4b905a. Please indicate if this is resolved. > test/jdk/com/sun/crypto/provider/KDF/TestHKDF.java line 79: > >> 77: >> 78: public static final List<TestData> testList = new >> LinkedList<TestData>() {{ >> 79: add(new TestData("RFC 5689 Test Case 1", "HKDFWithHmacSHA256", > > typo: "5689" should be "5869". Sames goes to other test datum. Addressed in https://github.com/openjdk/jdk/pull/20301/commits/48395b86ba8e1cda663ae326e06ae2556f4b905a. Please indicate if this is resolved. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1722376679 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1722376314 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1722376862 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1722376789
