On Sun, 1 Nov 2026 16:21:28 GMT, Weijun Wang <[email protected]> wrote:
>> Hai-May Chao has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Update with Weijun's 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.
Added.
> 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?
Removed.
> 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 `'`.
Fixed.
> 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`.
Fixed.
> 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.
Fixed.
> 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.
Fixed.
> 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`.
Fixed.
> 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`
Fixed.
> 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.
Removed.
> 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.
Done.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/33433#discussion_r4437172536
PR Review Comment: https://git.openjdk.org/jdk/pull/33433#discussion_r4437172206
PR Review Comment: https://git.openjdk.org/jdk/pull/33433#discussion_r4437171279
PR Review Comment: https://git.openjdk.org/jdk/pull/33433#discussion_r4437170720
PR Review Comment: https://git.openjdk.org/jdk/pull/33433#discussion_r4437170405
PR Review Comment: https://git.openjdk.org/jdk/pull/33433#discussion_r4437169129
PR Review Comment: https://git.openjdk.org/jdk/pull/33433#discussion_r4437169307
PR Review Comment: https://git.openjdk.org/jdk/pull/33433#discussion_r4437168108
PR Review Comment: https://git.openjdk.org/jdk/pull/33433#discussion_r4437176790
PR Review Comment: https://git.openjdk.org/jdk/pull/33433#discussion_r4437178597
