On Mon, 24 Nov 2025 07:51:40 GMT, Hai-May Chao <[email protected]> wrote:
>> Implement hybrid key exchange support for TLS 1.3 by adding three >> post-quantum hybrid named groups: X25519MLKEM768, SecP256r1MLKEM768, and >> SecP384r1MLKEM1024. >> Please see [JEP 527](https://openjdk.org/jeps/527) for details about this >> change. > > Hai-May Chao has updated the pull request incrementally with three additional > commits since the last revision: > > - Update names to uppercase > - Remove fallback in engineGeneratePublic > - Change default named group list to have only X25519MLKEM768 src/java.base/share/classes/sun/security/ssl/KEMKeyExchange.java line 28: > 26: > 27: import java.io.IOException; > 28: import java.security.*; Probably clearer not to use wildcard imports so we can see all the classes needed, same comment on line 32. src/java.base/share/classes/sun/security/ssl/KEMKeyExchange.java line 35: > 33: import sun.security.x509.X509Key; > 34: > 35: import javax.crypto.SecretKey; Move this import up, after line 29. src/java.base/share/classes/sun/security/ssl/KEMKeyExchange.java line 47: > 45: static final class KEMCredentials implements NamedGroupCredentials { > 46: > 47: final NamedGroup namedGroup; Make private, also line 50. src/java.base/share/classes/sun/security/ssl/KEMKeyExchange.java line 67: > 65: } > 66: > 67: public byte[] getKeyshare() { Nit, suggest renaming as "getKeyShare()" since RFC refers to these as two words: "key share". src/java.base/share/classes/sun/security/ssl/KEMKeyExchange.java line 77: > 75: > 76: /** > 77: * Parse the encoded Point into the KEMCredentials using the This doesn't do any parsing, it just instantiates a `KEMCredentials` object. src/java.base/share/classes/sun/security/ssl/KEMKeyExchange.java line 82: > 80: static KEMCredentials valueOf(NamedGroup namedGroup, > 81: byte[] encodedPoint) throws IOException, > 82: GeneralSecurityException { This doesn't throw either of these exceptions AFAICT. src/java.base/share/classes/sun/security/ssl/KEMKeyExchange.java line 86: > 84: if (namedGroup.spec != NamedGroupSpec.NAMED_GROUP_KEM) { > 85: throw new RuntimeException( > 86: "Credentials decoding: Not KEM named group"); Nit: only one space after ":". src/java.base/share/classes/sun/security/ssl/KEMKeyExchange.java line 97: > 95: } > 96: > 97: static class KEMPossession implements SSLPossession { Looks like this can be private. src/java.base/share/classes/sun/security/ssl/KEMKeyExchange.java line 98: > 96: > 97: static class KEMPossession implements SSLPossession { > 98: private final NamedGroup namedGroup; Nit, add empty lines between variable declarations and method names. src/java.base/share/classes/sun/security/ssl/KEMKeyExchange.java line 128: > 126: } catch (GeneralSecurityException e) { > 127: throw new RuntimeException( > 128: "Could not generate XDH keypair", e); XDH? Should this be `algName`? src/java.base/share/classes/sun/security/ssl/KEMKeyExchange.java line 142: > 140: } > 141: > 142: public PublicKey getPublicKey() { This can be package-private, also `getPrivateKey`. src/java.base/share/classes/sun/security/ssl/KEMKeyExchange.java line 153: > 151: static final class KEMSenderPossession extends KEMPossession { > 152: > 153: public SecretKey getKey() { This method can be package-private, also `setKey`. src/java.base/share/classes/sun/security/ssl/KEMKeyExchange.java line 161: > 159: } > 160: > 161: private SecretKey key; Not - move this declaration to before `getKey`. src/java.base/share/classes/sun/security/ssl/KEMKeyExchange.java line 163: > 161: private SecretKey key; > 162: > 163: KEMSenderPossession(NamedGroup namedGroup) { Move this ctor up to before the methods. src/java.base/share/classes/sun/security/ssl/KEMKeyExchange.java line 200: > 198: } > 199: context.conContext.fatal(Alert.HANDSHAKE_FAILURE, > 200: "No sufficient XDHE key agreement " Is this always XDHE? src/java.base/share/classes/sun/security/ssl/NamedGroup.java line 317: > 315: // Skip AlgorithmParameters for KEMs (not supported) > 316: if (namedGroupSpec == NamedGroupSpec.NAMED_GROUP_KEM) { > 317: Provider p = getProvider(); Nit: you can just use `defaultProvider`, no need to call `getProvider()` or define another variable. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2577168986 PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2577172087 PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2577185252 PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2577203089 PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2577219207 PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2577220264 PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2577207226 PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2577242023 PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2577235201 PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2577290498 PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2577263772 PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2577252219 PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2577254497 PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2577258290 PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2577305844 PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2566394510
