On Sat, 31 Oct 2026 11:04:17 GMT, Hai-May Chao <[email protected]> wrote:
>> This feature enables SunJSSE to use a TLS 1.3 hybrid named group implemented >> directly by a security provider as JCA `KeyPairGenerator` and `KEM` >> services. It also moves the JDK implementations of `X25519MLKEM768`, >> SecP256r1MLKEM768, and `SecP384r1MLKEM1024` into SunJCE provider. >> >> --------- >> - [X] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Hai-May Chao has updated the pull request incrementally with one additional > commit since the last revision: > > Update with Weijun's comments Overall looks good. Some small comments. src/java.base/share/classes/com/sun/crypto/provider/SunJCE.java line 84: > 82: * - ML-KEM > 83: * > 84: * - HKDF-SHA256, HKDF-SHA384, and HKDF-SHA512 You can add the new algorithms here and in the `info` field below on line 94. src/java.base/share/classes/com/sun/crypto/provider/SunJCE.java line 800: > 798: attrs = new HashMap<>(Map.of( > 799: "ImplementedIn", "Software", > 800: "name", "X25519MLKEM768", What is the `name` attribute for? src/java.base/share/classes/sun/security/ssl/Hybrid.java line 67: > 65: // Their public key encoding is the TLS 1.3 key_share encoding, not an > 66: // X.509 SubjectPublicKeyInfo encoding, and the algorithms have no object > 67: // identifiers. Hence, the public key’s getFormat() method must return > "RAW", Please change the apostrophe in `key’s ` to a normal `'`. src/java.base/share/classes/sun/security/ssl/Hybrid.java line 163: > 161: throws InvalidAlgorithmParameterException { > 162: if (!(params instanceof NamedParameterSpec namedSpec) || > 163: !name.equals(namedSpec.getName())) { Should be `name.equalsIgnoreCase`. src/java.base/share/classes/sun/security/ssl/Hybrid.java line 171: > 169: > 170: @Override > 171: public void initialize(int keysize, SecureRandom random) { Better ensure `keySize` is -1. See https://github.com/openjdk/jdk/blob/e51ccef9cb415ed31db70971bb439ca3d96c5bce/src/java.base/share/classes/sun/security/provider/NamedKeyPairGenerator.java#L187 for example. src/java.base/share/classes/sun/security/ssl/Hybrid.java line 337: > 335: } > 336: if (publicKey instanceof PublicKeyImpl pk && > 337: name.equals(pk.getAlgorithm())) { `equalsIgnoreCase`. src/java.base/share/classes/sun/security/ssl/Hybrid.java line 337: > 335: } > 336: if (publicKey instanceof PublicKeyImpl pk && > 337: name.equals(pk.getAlgorithm())) { Also, the check should be done before `keyDecoder.decode`. Otherwise, this method will write the expected algorithm name into the result. src/java.base/share/classes/sun/security/ssl/Hybrid.java line 342: > 340: null, null); > 341: } > 342: throw new InvalidKeyException(); Want to add a reason? src/java.base/share/classes/sun/security/ssl/Hybrid.java line 354: > 352: } > 353: if (privateKey instanceof PrivateKeyImpl pk && > 354: name.equals(pk.getAlgorithm())) { `equalsIgnoreCase` src/java.base/share/classes/sun/security/ssl/KAKeyDerivation.java line 57: > 55: private final PublicKey peerPublicKey; > 56: private final byte[] keyshare; > 57: private final NamedGroup namedGroup; Seems useless now. test/jdk/sun/security/ssl/HybridKeyExchange/TestHybrid.java line 94: > 92: group.name); > 93: } > 94: kpg.initialize(256, null); If you require `keySize` to be -1, update here. ------------- PR Review: https://git.openjdk.org/jdk/pull/33433#pullrequestreview-5729036622 PR Review Comment: https://git.openjdk.org/jdk/pull/33433#discussion_r4436411227 PR Review Comment: https://git.openjdk.org/jdk/pull/33433#discussion_r4436413799 PR Review Comment: https://git.openjdk.org/jdk/pull/33433#discussion_r4436418772 PR Review Comment: https://git.openjdk.org/jdk/pull/33433#discussion_r4436425229 PR Review Comment: https://git.openjdk.org/jdk/pull/33433#discussion_r4436430349 PR Review Comment: https://git.openjdk.org/jdk/pull/33433#discussion_r4436431814 PR Review Comment: https://git.openjdk.org/jdk/pull/33433#discussion_r4436448011 PR Review Comment: https://git.openjdk.org/jdk/pull/33433#discussion_r4436433188 PR Review Comment: https://git.openjdk.org/jdk/pull/33433#discussion_r4436434328 PR Review Comment: https://git.openjdk.org/jdk/pull/33433#discussion_r4436435886 PR Review Comment: https://git.openjdk.org/jdk/pull/33433#discussion_r4436442049
