On Wed, 26 Apr 2023 17:56:12 GMT, Sean Mullan <mul...@openjdk.org> wrote:
>> 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 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.
Yes, you're right. I actually didn't realize the difference between IOOBE and
AIOOBE at all. In fact, I checked for IOOBE in the `Compliance.java` test.
> 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.
Will fix.
> 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.
Will fix.
> 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.
OK, I'll change them.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1178254497
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1178256035
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1178255927
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1178247018
