On Fri, 31 Mar 2023 02:25:04 GMT, Weijun Wang <wei...@openjdk.org> wrote:
> The KEM API and DHKEM impl. Note that this PR uses new methods in > https://github.com/openjdk/jdk/pull/13250. src/java.base/share/classes/javax/crypto/KEM.java line 79: > 77: > 78: /** > 79: * Type for the return value of the {@link Encapsulator#encapsulate} > method. I think the description should more generally describe what this is, maybe "The encapsulated data generated by a Key Encapsulation Mechanism (KEM), which includes the shared secret (as a SecretKey), the key encapsulation message, and additional optional parameters." src/java.base/share/classes/javax/crypto/KEM.java line 94: > 92: * @see KEM#newEncapsulator(PublicKey, AlgorithmParameterSpec, > SecureRandom) > 93: */ > 94: public record Encapsulated(SecretKey key, byte[] encapsulation, > byte[] params) { We need to decide if the encapsulation array should be defensively cloned. I would lean towards cloning it since immutability is a feature of this API, and I think it would be surprising if this type was not. We can potentially switch to frozen arrays later. src/java.base/share/classes/javax/crypto/KEM.java line 109: > 107: > 108: /** > 109: * A decapsulator, generated by {@link #newDecapsulator}. See comments on Encapsulator about adding more description here. src/java.base/share/classes/javax/crypto/KEM.java line 140: > 138: * @param encapsulation the key encapsulation message from the > sender > 139: * @return the shared secret as a {@code SecretKey} with > 140: * algorithm being "Generic", not {@code null} suggest "with an algorithm name of "Generic" ..." src/java.base/share/classes/javax/crypto/KEM.java line 150: > 148: > 149: /** > 150: * The key decapsulation function. See encapsulate for similar comments. src/java.base/share/classes/javax/crypto/KEM.java line 155: > 153: * @param from the initial index of the shared secret to be > returned, inclusive > 154: * @param to the final index of the shared secret to be > returned, exclusive. > 155: * @param algorithm the algorithm for the secret key returned Suggest: " ... algorithm name of the secret key that is returned" (or generated) src/java.base/share/classes/javax/crypto/KEM.java line 168: > 166: * is not supported by the decapsulator > 167: */ > 168: public SecretKey decapsulate(byte[] encapsulation, WDYT about throwing IllegalArgumentException if the size of encapsulation is not equal to encapsulationSize()? If not, I think you should add a sentence to the @param encapsulation such as "The size must be equal to ..., or a DecapsulateException will be thrown." src/java.base/share/classes/javax/crypto/KEM.java line 193: > 191: > 192: /** > 193: * Returns the size of the key encapsulation message. I think both the `secretSize` and `encapsulationSize` methods could use a sentence or two explaining why they are useful. This will help both users understand when they might need to call them and implementors to implement them correctly. src/java.base/share/classes/javax/crypto/KEM.java line 206: > 204: > 205: /** > 206: * An encapsulator, generated by {@link #newEncapsulator}. Can you add more description here? "An encapsulator, generated by {@link #newEncapsulator}. This class represents the key encapsulation function of a KEM. Each invocation of the {@link encapsulate} method generates a new secret key and key encapsulation message that is returned in an {@link Encapsulated} object." src/java.base/share/classes/javax/crypto/KEM.java line 223: > 221: * Returns the provider. > 222: * > 223: * @return thr provider s/thr/the/ src/java.base/share/classes/javax/crypto/KEM.java line 230: > 228: > 229: /** > 230: * The key encapsulation function. I think you should include this text in both encapsulate methods: "Each invocation of this method generates a new secret key and key encapsulation message that is returned in an {@link Encapsulated} object." src/java.base/share/classes/javax/crypto/KEM.java line 238: > 236: * @return an {@link KEM.Encapsulated} object containing the full > 237: * shared secret as a key with algorithm being > "Generic", > 238: * the key encapsulation message, and optional > parameters. Maybe break up into two sentences: "an {@link KEM.Encapsulated} object containing the shared secret, key encapsulation message, and optional parameters. The shared secret is a {@code SecretKey} containing all of the bytes of the secret, and an algorithm name of "Generic". src/java.base/share/classes/javax/crypto/KEM.java line 246: > 244: > 245: /** > 246: * The key encapsulation function. I think it would be useful to add a sentence explaining when this method would be useful. src/java.base/share/classes/javax/crypto/KEM.java line 248: > 246: * The key encapsulation function. > 247: * > 248: * @param from the initial index of the shared secret to be > returned, inclusive Should you say the index is the index of the byte array? "... the initial index of the shared secret byte array ..." src/java.base/share/classes/javax/crypto/KEM.java line 249: > 247: * > 248: * @param from the initial index of the shared secret to be > returned, inclusive > 249: * @param to the final index of the shared secret to be > returned, exclusive. omit period to be consistent src/java.base/share/classes/javax/crypto/KEM.java line 250: > 248: * @param from the initial index of the shared secret to be > returned, inclusive > 249: * @param to the final index of the shared secret to be > returned, exclusive. > 250: * @param algorithm the algorithm for the secret key returned Suggest: "the algorithm name of the secret key that is returned" (or maybe "generated") src/java.base/share/classes/javax/crypto/KEM.java line 253: > 251: * @return an {@link KEM.Encapsulated} object containing a > portion of > 252: * the shared secret as a key with the specified > algorithm, > 253: * the key encapsulation message, and optional > parameters. See comment above about splitting into 2 sentences. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1166026177 PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1166037807 PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1166009690 PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1166010704 PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1166011839 PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1166012724 PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1166020023 PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1166022013 PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1165988842 PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1165989666 PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1165991209 PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1165997956 PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1166000074 PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1165999651 PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1166004651 PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1166005900 PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1166006297
