On Mon, 1 Dec 2025 17:25:34 GMT, Sean Mullan <[email protected]> wrote:
>> 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/Hybrid.java line 55:
>
>> (failed to retrieve contents of file, check the PR for context)
> Add a few comments describing this class.
Done.
> src/java.base/share/classes/sun/security/ssl/KAKeyDerivation.java line 54:
>
>> 52: private final PublicKey peerPublicKey;
>> 53: private final byte[] keyshare;
>> 54: private final java.security.Provider provider;
>
> Add `java.security.Provider` to imports.
Done.
> src/java.base/share/classes/sun/security/ssl/KAKeyDerivation.java line 162:
>
>> 160: * to encapsulate a shared secret and returns the encapsulated
>> message.
>> 161: */
>> 162: public KEM.Encapsulated encapsulate(String algorithm)
>
> Can this be package-private?
Yes, done.
> 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.
Done.
> 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.
Can not make this line `private` for `namedGroup` as
`KeyShareExtension.SHKeyShareProducer::produce()` needs access. Made `private`
for the 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".
Renaming done.
> 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.
Fixed.
> 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.
Fixed.
> 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 ":".
Removed the extra space.
> 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.
Fixed.
> 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.
Added.
> 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`?
Yes, fixed.
> 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`.
Done.
> 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`.
Fixed.
> 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`.
Done.
> 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.
Done.
> 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?
No, fixed.
> src/java.base/share/classes/sun/security/ssl/KeyShareExtension.java line 599:
>
>> 597: xp.setKey(encaped.key());
>> 598: keyShare = new KeyShareEntry(ng.id,
>> 599: encaped.encapsulation());
>
> Should there be a break after this line?
No, we have a break at line #606 for this.
> 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.
Removed the call.
> src/java.base/share/classes/sun/security/util/Hybrid.java line 60:
>
>> 58: @Override
>> 59: public String getAlgorithm() {
>> 60: return "Hybrid";
>
> Should we just return "Generic" here to align with the KEM spec?
Yes, fixed.
> src/java.base/share/classes/sun/security/util/Hybrid.java line 430:
>
>> 428: @Override
>> 429: public byte[] getEncoded() {
>> 430: return new byte[0];
>
> Should return `null` instead (according to `Key.getEncoded` spec).
Fixed.
> src/java.base/share/classes/sun/security/util/Hybrid.java line 444:
>
>> 442: }
>> 443:
>> 444: public static final NamedParameterSpec X25519_MLKEM768 =
>
> Move these constants to the top of the class so they are more prominent.
Done.
> test/jdk/javax/net/ssl/TLSv13/HRRKeyShares.java line 1:
>
>> 1: /*
>
> Is the comment on line 352-353 accurate?:
>
>
> // Now we're expecting to reissue the ClientHello, this time
> // with a secp384r1 share.
>
> Shouldn't this be whatever the hrrNamedGroup parameter passed in is?
Fixed the comment.
> test/jdk/sun/security/ssl/CipherSuite/NamedGroupsWithCipherSuite.java line 84:
>
>> 82: };
>> 83:
>> 84: private static final String[] HYBRID_NAMEDGROUPS = {
>
> Somewhat of a nit, but you only access this as a `List` in the code below, so
> it would be better to just use `List.of` here. In fact, these 3 arrays could
> all be lists and initialized with `List.of`.
Done as suggested.
> test/jdk/sun/security/ssl/CipherSuite/SupportedGroups.java line 51:
>
>> 49:
>> 50: private static volatile int index;
>> 51: private static final String[][][] protocolForClassic = {
>
> Nit: suggest naming this `protocolsForClassic`.
Done.
> test/jdk/sun/security/ssl/CipherSuite/SupportedGroups.java line 58:
>
>> 56: };
>> 57:
>> 58: private static final String[][][] protocolForHybrid = {
>
> Nit: suggest naming this `protocolsForHybrid`.
Done.
> test/micro/org/openjdk/bench/javax/crypto/full/KeyPairGeneratorBench.java
> line 59:
>
>> 57: try {
>> 58: Class<?> dhClazz =
>> Class.forName("com.sun.crypto.provider.DH");
>> 59: return (Provider) (dhClazz.getField("PROVIDER").get(null));
>
> Nit: don't think you need the parens around
> `dhClazz.getField("PROVIDER").get(null)`
Removed parens.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2584270431
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2584271259
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2584271412
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2584267054
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2584267257
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2584267463
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2584267949
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2584268196
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2584267671
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2584268630
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2584268449
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2584269726
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2584269450
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2584268879
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2584269098
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2584269260
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2584269932
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2584270154
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2584266540
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2584271070
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2584270863
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2584270663
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2584261901
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2584264922
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2584264409
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2584264641
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2584264204
