On Sat, 6 Dec 2025 05:27:03 GMT, Bradford Wetmore <[email protected]> wrote:
>> Hai-May Chao has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Updates with Weijun's comments
>> - Comments added for possession creation per handshake
>
> src/java.base/share/classes/sun/security/ssl/DHasKEM.java line 91:
>
>> 89: // The order of shares in the concatenation for group name
>> 90: // X25519MLKEM768 has been reversed. This is due to IETF
>> 91: // historical reasons.
>
> Can we just change this to something like "as per the current draft RFC?"
>
> "historical reasons" is too vague. The draft is the real reason.
Updated comment as suggested (comment is in HybridProvider class now).
> src/java.base/share/classes/sun/security/ssl/DHasKEM.java line 152:
>
>> 150: }
>> 151:
>> 152: static final class Handler
>
> private?
Yes, fixed.
> src/java.base/share/classes/sun/security/ssl/DHasKEM.java line 261:
>
>> 259:
>> 260: X448(56, 56,
>> 261: "XDH", "XDH", NamedParameterSpec.X448),
>
> unneeded comma, move the semicolon below to the end of this "XDH" line
> (currently line 261).
Removed.
> src/java.base/share/classes/sun/security/ssl/DHasKEM.java line 268:
>
>> 266: private final String keyAlgorithm;
>> 267: private final AlgorithmParameterSpec spec;
>> 268:
>
> nit: extra line?
Removed extra line.
> src/java.base/share/classes/sun/security/ssl/DHasKEM.java line 331:
>
>> 329: }
>> 330:
>> 331: private static class HybridService extends Provider.Service {
>
> Reminder: this would also be moved to the new Provider file....
Moved to new HybridProvider file.
> src/java.base/share/classes/sun/security/ssl/Hybrid.java line 61:
>
>> 59: * in a single TLS hybrid named group.
>> 60: * It implements:
>> 61: * - Hybrid KeyPair generation
>
> This seems to be a mix of javadoc and markdown. If anyone ever does generate
> javadocs for these internal classes, this will not render well.
Fixed the comments.
> src/java.base/share/classes/sun/security/ssl/NamedGroup.java line 314:
>
>> 312: try {
>> 313: // Skip AlgorithmParameters for KEMs (not supported)
>> 314: if (namedGroupSpec == NamedGroupSpec.NAMED_GROUP_KEM) {
>
> Can you also put in a comment here as to why you're doing these
> `getInstances`.
>
> It looks like you're checking for the existence of the `KeyFactory`, but the
> exception logger only talks about `AlgorithmParameters`.
Comment added.
> src/java.base/share/classes/sun/security/ssl/ServerHello.java line 572:
>
>> 570: // model, depending on what key exchange group is used.
>> 571: //
>> 572: // In JSSE for KA flow, the server usually generates its key
>
> Minor doc nit here. For KA flows, the server first receives the client's
> share, THEN generates its key share. Then we finally come here. Just
> reverse the order and you should be good.
Updated the comment as suggested.
> src/java.base/share/classes/sun/security/ssl/ServerHello.java line 581:
>
>> 579: // Traditional Key Agreement (KA):
>> 580: // - Both peers generate a key share and exchange it.
>> 581: // - Each peer computes a shared secret upon receiving the
>
> We currently do the share generation in the KeyShare extension, and calculate
> the shared secret here in ServerHello. It's not immediate, but this suggests
> it is. Maybe change:
>
> upon receiving
> ->
> sometime after receiving
Updated the comment as suggested..
> src/java.base/share/classes/sun/security/ssl/ServerHello.java line 585:
>
>> 583: //
>> 584: // Key Encapsulation Mechanism (KEM):
>> 585: // The decapsulator (the client) publishes a public key,
>> and
>
> I would suggest rewording this to be something like:
>
> // Key Encapsulation Mechanism (KEM):
> // The client push a public key via a KeyShareExtension, which
> // the server uses to:
> //
> // - generate the shared secret
> // - encapsulate the message which is sent to the client in another
> // KeyShareExtension
> //
> // The derived shared secret must be stored in a KEMSenderPossession
> // so it can be retrieved for handshake traffic secret derivation later.
Updated the comments as suggested.
> src/java.base/share/classes/sun/security/ssl/ServerHello.java line 635:
>
>> 633:
>> 634: if (handshakeSecret == null) {
>> 635: SSLKeyDerivation handshakeKD =
>> ke.createKeyDerivation(shc);
>
> Since we only end up with one possession, can we move this up into the for
> loop? Each of the various possession types goes through the `for` loop, so
> seems this would be a tiny bit clearer.
>
> for () {
> if (pos instanceof ...) {
> handshakeSecret =...
> } else {
> SSLKeyDerivation handshakeKD = ...
> }
> }
Keep the code as it is. This is because if we move the KA derivation code to
the for loop as the else branch, we make it appear to be associated with a
possession. KA does not store the shared secret in a possession like KEM, so
keep its logic separate.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2595150752
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2595150801
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2595150830
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2595150910
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2595151002
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2595151129
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2595150325
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2595150512
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2595150564
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2595150597
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2595150633
