On Fri, 30 Oct 2026 13:01:32 GMT, Weijun Wang <[email protected]> wrote:
>> Hai-May Chao has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Remove unused adapter from DHasKEM
>
> src/java.base/share/classes/sun/security/ssl/Hybrid.java line 73:
>
>> 71: // For the negotiated shared secret represented as a SecretKey,
>> getFormat()
>> 72: // must return "RAW", and getEncoded() must return the combined key
>> material
>> 73: // as a byte array.
>
> The last paragraph is no longer true for us. A third-party provider is free
> to decide what their shared secret looks like. If they also need a `null`
> format, they would have to provide their own HKDF implementations that accept
> it.
Fixed the comment.
> src/java.base/share/classes/sun/security/ssl/Hybrid.java line 117:
>
>> 115: // provider selection.
>> 116: return KeyFactory.getInstance(name, provider);
>> 117: }
>
> Why force using ML-KEM in SunJCE? There could be users having their own
> ML-KEM provider but we should not expect their provider having the hybrid
> KEMs.
Fixed.
> src/java.base/share/classes/sun/security/ssl/Hybrid.java line 312:
>
>> 310: @Override
>> 311: public EncapsulatorSpi engineNewEncapsulator(PublicKey
>> publicKey,
>> 312: AlgorithmParameterSpec spec, SecureRandom secureRandom)
>> throws
>
> We should make sure `spec` is null.
Fixed.
> src/java.base/share/classes/sun/security/ssl/Hybrid.java line 327:
>
>> 325: @Override
>> 326: public DecapsulatorSpi engineNewDecapsulator(PrivateKey
>> privateKey,
>> 327: AlgorithmParameterSpec spec)
>
> We should make sure the `spec` argument is null in `engineNewEncapsulator`.
Fixed.
> src/java.base/share/classes/sun/security/ssl/Hybrid.java line 440:
>
>> 438: @Override
>> 439: public String getFormat() {
>> 440: return "RAW";
>
> Should be `null`. The `getFormat()` and `getEncoded()` return values need to
> be consistent.
Fixed.
> src/java.base/share/classes/sun/security/ssl/Hybrid.java line 543:
>
>> 541: }
>> 542:
>> 543: // getEncoded() returns an empty byte array because there is no
>
> Not empty, but null.
Fixed the comment.
> src/java.base/share/classes/sun/security/ssl/NamedGroup.java line 361:
>
>> 359: }
>> 360:
>> 361: Provider getHybridKEMProvider() {
>
> Still don't know why this is required. Why not just call
> `KeyPairGenerator.getInstance(alg)` and `KEM.getInstance(alg)` directly? Are
> you worried they come from different providers? Even so, delayed provider
> selection should take care of it.
Not required, and removed.
> test/jdk/sun/security/provider/all/Deterministic.java line 214:
>
>> 212: g.initialize(new NamedParameterSpec(alg), random);
>> 213: return g.generateKeyPair();
>> 214: }
>
> I see why this is necessary, but I would change the hybrid
> `KeyPairGenerator.initialize(int, rand)` to be identical to
> `KeyPairGenerator.initialize(params, rand)` so calling it can also set the
> random.
>
> Then there is no need to modify this test.
Changed Hybrid as suggested, and removed the change in this test.
> test/jdk/sun/security/provider/all/Deterministic.java line 311:
>
>> 309: static String getKeyAlgFromKEM(String algorithm) {
>> 310: return switch (algorithm) {
>> 311: case "DHKEM", "DH" -> "X25519";
>
> Is this still needed? "KEM.DH" is not public now.
Fixed.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/33433#discussion_r4431502352
PR Review Comment: https://git.openjdk.org/jdk/pull/33433#discussion_r4431505033
PR Review Comment: https://git.openjdk.org/jdk/pull/33433#discussion_r4431502722
PR Review Comment: https://git.openjdk.org/jdk/pull/33433#discussion_r4431503057
PR Review Comment: https://git.openjdk.org/jdk/pull/33433#discussion_r4431500255
PR Review Comment: https://git.openjdk.org/jdk/pull/33433#discussion_r4431502530
PR Review Comment: https://git.openjdk.org/jdk/pull/33433#discussion_r4431501004
PR Review Comment: https://git.openjdk.org/jdk/pull/33433#discussion_r4431501549
PR Review Comment: https://git.openjdk.org/jdk/pull/33433#discussion_r4431501718
