On Wed, 26 Nov 2025 00:19:13 GMT, Bradford Wetmore <[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/com/sun/crypto/provider/DH.java line 75:
>
>> 73: private static final long serialVersionUID = 0L;
>> 74: private ProviderImpl() {
>> 75: super("InternalJCE", PROVIDER_VER, "");
>
> Maybe include an `info` String here?
Added.
> src/java.base/share/classes/com/sun/crypto/provider/DH.java line 75:
>
>> 73: private static final long serialVersionUID = 0L;
>> 74: private ProviderImpl() {
>> 75: super("InternalJCE", PROVIDER_VER, "");
>
> Wondering if we might use another name here for the Provider?
>
> Perhaps `InternalSunJCE`, `InternalJCEKEM`, `InternalJCEHybridJSSE`,
> `InternalJCEHybridJSSEKEM`, or something similar?
>
> This may not be necessary, but would help indicate what this is.
Changed the provider name to` InternalJCEDHasKEM`.
> src/java.base/share/classes/com/sun/crypto/provider/DH.java line 82:
>
>> 80: // The order of shares in the concatenation for group name
>> 81: // X25519MLKEM768 has been reversed. This is due to
>> historical
>> 82: // reasons.
>
> `...due to IETF historical reasons...`, not JSSE reasons.
Fixed to say `due to IETF.` Also added some comments for the Provider.
> src/java.base/share/classes/com/sun/crypto/provider/DH.java line 251:
>
>> 249: "XDH", "XDH", NamedParameterSpec.X448),
>> 250: ;
>> 251: private final int Nsecret;
>
> Minor nits: lower case name starting letters.
>
> Maybe `nSecret`/`secretLen` and `nPK`/`publicKeyLen`
>
> (Is the N prefix a Solaris convention?)
Changed names as suggested. (N prefix is from the DHKEM class)
> src/java.base/share/classes/sun/security/ssl/Hybrid.java line 139:
>
>> (failed to retrieve contents of file, check the PR for context)
> Is this method actually called? (I didn't see it in
> `KEMKeyExchange.KEMReceiverPossession`).
>
> I know the method is needed to fully extend the `KeyPairGeneratorSpi`, but it
> looks like we're doing the actual initialization of both sides in the
> constructor, and that is considered it's "default method of initialization."
>
> This duplicate code could probably be removed, and just be a NO-OP.
Changed it to NO-OP.
> src/java.base/share/classes/sun/security/ssl/Hybrid.java line 239:
>
>> (failed to retrieve contents of file, check the PR for context)
> Maybe add "KeySpec type: " to the Exception message?
Added `KeySpec type:`.
> src/java.base/share/classes/sun/security/ssl/KeyShareExtension.java line 580:
>
>> 578: shc.handshakeKeyExchange = ke;
>> 579: shc.handshakePossessions.add(pos);
>> 580:
>
> Noticed several <=80 lines throughout the code. Will mention just here.
>
> It helps when reviewing side-by-side not having to vertically scroll.
>
> Thanks.
Fixed > 80 chars for the code changes in sun.security.ssl package.
> src/java.base/share/classes/sun/security/ssl/ServerHello.java line 568:
>
>> 566: shc.serverHelloRandom = shm.serverRandom;
>> 567:
>> 568: // Key Encapsulation Mechanism (KEM):
>
> I really like the details you included here, but a little wordsmithing would
> improve readability.
>
> Can you please add an intro sentence like: "For key derivation, we'll either
> use the KA or a KEM model, depending on what group is used."
>
> Then add some of the lines from 579-584, then do the KEM and KA sections, and
> move the final sentence to the KEM section.
>
> I think this would read much cleaner.
Updated the comments. Hope it is more cleaner.
> src/java.base/share/classes/sun/security/util/Hybrid.java line 35:
>
>> 33: import javax.crypto.KEMSpi;
>> 34: import javax.crypto.SecretKey;
>> 35: import java.io.ByteArrayOutputStream;
>
> import no longer needed.
import removed.
> src/java.base/share/classes/sun/security/util/Hybrid.java line 57:
>
>> 55: public class Hybrid {
>> 56:
>> 57: public record SecretKeyImpl(SecretKey k1, SecretKey k2) implements
>> SecretKey {
>
> Minor nit: consider moving`SecretKeyImpl` to keep all the keys
> `PrivateKeyImpl`/`PublicKeyImpl` together?
Moving done.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2584262244
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2584263097
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2584263966
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2584263346
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2584265282
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2584265795
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2584262066
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2584266212
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2584262853
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2584265543
