RFR: 8297878: KEM: Implementation [v7]

[Anthony Scarpino]

On Tue, 18 Apr 2023 22:07:11 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:
> 
>   fine tuning spec

src/java.base/share/classes/com/sun/crypto/provider/DHKEM.java line 175:

> 173:             this.keyAlgorithm = keyAlgorithm;
> 174:             this.hkdfAlgorithm = hkdfAlgorithm;
> 175:             suiteId = concat("KEM".getBytes(StandardCharsets.UTF_8),

This is a general comment for all the `getBytes()` calls.  I think these should be static final values.  Each one of these usages is allocating a new String and byte[] every time.

src/java.base/share/classes/com/sun/crypto/provider/DHKEM.java line 202:

> 200:             } else {
> 201:                 byte[] uArray = ((XECPublicKey) k).getU().toByteArray();
> 202:                 return Arrays.copyOf(reverse(uArray), Npk);

You could just return the reversed array.  It is already a copy of the BigInteger 'u'.

src/java.base/share/classes/com/sun/crypto/provider/DHKEM.java line 206:

> 204:         }
> 205: 
> 206:         private static byte[] reverse(byte[] arr) {

It would be better to swap the bytes than allocating another array.
DeserializePublicKey() may need to copy 'data' or have two different reverse methods

src/java.base/share/classes/com/sun/crypto/provider/DHKEM.java line 346:

> 344:     private Params paramsFromKey(Key k) throws InvalidKeyException {
> 345:         if (k instanceof ECKey eckey) {
> 346:             if (ECUtil.equals(eckey.getParams(), CurveDB.lookup("secp256r1"))) {

These lookup calls look like they could be static final values

src/java.base/share/classes/com/sun/crypto/provider/DHKEM.java line 366:

> 364:     private static byte[] concat(byte[]... inputs) {
> 365:         ByteArrayOutputStream o = new ByteArrayOutputStream();
> 366:         Arrays.stream(inputs).forEach(o::writeBytes);

Unless I'm missing something there is no `stream(byte[])` support, so I'm not sure how this is compiling.  I didn't think  the generics would work with this.
More importantly, `forEach()` the API states that stream is in a non-deterministic order.  I think you want `forEachOrdered()`

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1175614103
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1174211748
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1174224442
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1175620516
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1175630598