RFR: 8297878: KEM: Implementation [v9]

[Sean Mullan]

On Wed, 26 Apr 2023 14:29:25 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.
>
> Weijun Wang has updated the pull request incrementally with one additional commit since the last revision:
> 
>   small spec change

src/java.base/share/classes/javax/crypto/KEMSpi.java line 111:

> 109:          * The key encapsulation function.
> 110:          * <p>
> 111:          * Each invocation of this method must generates a new secret key and key

s/generates/generate/

src/java.base/share/classes/javax/crypto/KEMSpi.java line 115:

> 113:          * <p>
> 114:          * An implementation must support the case where {@code from} is 0,
> 115:          * {@code from} is the same as the output of {@code secretSize()},

Should "from" be "to"?

Maybe also say "return value" instead of "output".

src/java.base/share/classes/javax/crypto/KEMSpi.java line 125:

> 123:          * @return an {@link KEM.Encapsulated} object containing a portion of
> 124:          *          the shared secret as a key with the specified algorithm,
> 125:          *          the key encapsulation message, and optional parameters.

Remove "the".

src/java.base/share/classes/javax/crypto/KEMSpi.java line 127:

> 125:          *          the key encapsulation message, and optional parameters.
> 126:          * @throws ArrayIndexOutOfBoundsException if {@code from < 0},
> 127:          *     {@code from > to}, or {@code to > secretSize()}

Should this throw an `IndexOutOfBoundsException` instead here? That's what the DHKEM impl does and this seems more reasonable since you aren't actually trying to access an array.

src/java.base/share/classes/javax/crypto/KEMSpi.java line 168:

> 166:          * <p>
> 167:          * An implementation must support the case where {@code from} is 0,
> 168:          * {@code from} is the same as the output of {@code secretSize()},

Same comment as above.

src/java.base/share/classes/javax/crypto/KEMSpi.java line 184:

> 182:          * @throws DecapsulateException if an error occurs during the
> 183:          *          decapsulation process
> 184:          * @throws ArrayIndexOutOfBoundsException if {@code from < 0},

Same comment as above.

src/java.base/share/classes/javax/crypto/KEMSpi.java line 242:

> 240:      * @see KEM#newDecapsulator(PrivateKey, AlgorithmParameterSpec)
> 241:      */
> 242:     DecapsulatorSpi engineNewDecapsulator(PrivateKey sk, AlgorithmParameterSpec spec)

Bit of a nit, but suggest changing variable names to be more descriptive: `privKey` (or `privateKey`) and `pubKey` (or `publicKey`), for the `engineEncapsulate` method. I assume `sk` is taken from RFC 9180, but I don't think we need to use the same variable names.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1178210665
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1178212708
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1178214239
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1178219972
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1178221846
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1178220363
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1178207872