RFR: 8297878: KEM: Implementation

Thu Apr 13 22:22:50 UTC 2023

On Fri, 31 Mar 2023 02:25:04 GMT, Weijun Wang <weijun at 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