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