On Fri, 13 Sep 2024 15:30:04 GMT, Weijun Wang <wei...@openjdk.org> wrote:
>> Kevin Driver has updated the pull request incrementally with one additional >> commit since the last revision: >> >> remove explicit zeroing in favor of finally blocks > > src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 48: > >> 46: * The caller may wish to provide a label (or other components) of >> 47: * the IKM without having access to all portions. The same feature is >> 48: * available for salts. > > Maybe remove the `The caller...` sentence above? The next paragraph has more > detail on it. @wangweij: Addressed in https://github.com/openjdk/jdk/pull/20301/commits/f513525c78aaf0797446c023329d22559eb39ac1. > src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 65: > >> 63: * >> 64: * >> 65: *} > > What are these blank lines for? What do they look in the rendered HTML? @wangweij: Addressed in https://github.com/openjdk/jdk/pull/20301/commits/f513525c78aaf0797446c023329d22559eb39ac1. > src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 96: > >> 94: * {@code HKDFParameterSpec}. As stated in the class description, >> 95: * {@code addIKM} and/or {@code addSalt} may be called as needed. >> Finally, >> 96: * the object is "built" by calling either {@code extractOnly} or > > Maybe "an object is built"? @wangweij: Addressed in https://github.com/openjdk/jdk/pull/20301/commits/f513525c78aaf0797446c023329d22559eb39ac1. > src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 105: > >> 103: >> 104: List<SecretKey> ikms = new ArrayList<>(); >> 105: List<SecretKey> salts = new ArrayList<>(); > > Make the 2 above private. @wangweij: Addressed in https://github.com/openjdk/jdk/pull/20301/commits/f513525c78aaf0797446c023329d22559eb39ac1. > src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 114: > >> 112: * @return a {@code Builder} to mutate >> 113: */ >> 114: private Builder createBuilder() { > > Is this method useful? @wangweij: Addressed in https://github.com/openjdk/jdk/pull/20301/commits/f513525c78aaf0797446c023329d22559eb39ac1. > src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 129: > >> 127: >> 128: /** >> 129: * Builds an {@code ExtractThenExpand}. > > In `extractOnly`, you have "from the current state...". Make them consistent. @wangweij: Addressed in https://github.com/openjdk/jdk/pull/20301/commits/f513525c78aaf0797446c023329d22559eb39ac1. > src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 133: > >> 131: * @param info >> 132: * the optional context and application specific >> information (may be >> 133: * {@code null}); the byte array is copied to prevent >> subsequent > > The RFC says "can be a zero-length string". Do we want to somehow mention > that our null is their empty? @wangweij: Addressed in https://github.com/openjdk/jdk/pull/20301/commits/f513525c78aaf0797446c023329d22559eb39ac1. > src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 139: > >> 137: * >> 138: * @implNote HKDF implementations will enforce that the length >> is less >> 139: * than 255 * HMAC length. > > Precisely, it's "not greater than". @wangweij: Addressed in https://github.com/openjdk/jdk/pull/20301/commits/f513525c78aaf0797446c023329d22559eb39ac1. > src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 158: > >> 156: * be assembled piece-meal or if part of the IKM is to be >> supplied by a >> 157: * hardware crypto device. This method appends to the existing >> list of >> 158: * values or creates a new list if there is none yet. > > Precisely, this should be "multiple addIKM may be called when..." since even > in the simplest case you still need to call this method once. In fact, I > don't think we need to talk about this detail every time. If you really want > to emphasize this, create a section header with an anchor the class spec, and > inside each method just say "This method can be called multiple times on a > single Builder object. See class_section_name". > > Appending to or creating a list is implementation detail. @wangweij: I have changed the wording, though I will leave these descriptions in-place. See: https://github.com/openjdk/jdk/pull/20301/commits/f513525c78aaf0797446c023329d22559eb39ac1. > src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 248: > >> 246: >> 247: /** >> 248: * Returns a builder for building {@code Extract} and > > Modify `builder` to `{@code Builder} object`. @wangweij: Addressed in https://github.com/openjdk/jdk/pull/20301/commits/f513525c78aaf0797446c023329d22559eb39ac1. > src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 278: > >> 276: * if {@code prk} is {@code null} >> 277: * @throws IllegalArgumentException >> 278: * if {@code length} is not > 0 > > Modify `>` to `greater than`. @wangweij: Addressed in https://github.com/openjdk/jdk/pull/20301/commits/f513525c78aaf0797446c023329d22559eb39ac1. > src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 309: > >> 307: * <p> >> 308: * Input keying material values added by {@link >> Builder#addIKM(byte[])} >> 309: * are converted to a {@code SecretKeySpec} object. > > Do we need to say "empty arrays were discarded"? This is more precise and JCK > might test on it. @wangweij: Addressed in https://github.com/openjdk/jdk/pull/20301/commits/f513525c78aaf0797446c023329d22559eb39ac1. I kept the same verb tense that was already established. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1759288227 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1759287918 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1759287670 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1759287409 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1759287252 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1759287053 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1759286868 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1759286669 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1759286329 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1759288567 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1759288723 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1759289114
