On Mon, 1 Dec 2025 21:42:12 GMT, Bradford Wetmore <[email protected]> wrote:
>> Also, if it is only used by JSSE, I think it should be in the >> `sun.security.ssl` package. > > We did include the internal `Tls*` implementations Sun in the `SunJCE` > provider, but those were actually exposed/available as `KeyGenerator`s. I > was never a fan, but it is what it is. > > `sun.security.ssl` is a better fit for this and `Hybrid.java`, especially > since these are strictly internal implementations for now. > The purpose of this class from the opening javadoc was confusing on my first > several reads. I expected that this class just created a `KEM` layer for DH, > but surprise(!), it also crammed in a Hybrid provider implementation too! > > My preference is to break the `DH` and `Provider`+`HybridService` code into > separate files for cleaner abstractions, but if not, then maybe use something > like this for the description? > > ``` > /** > * The DH class presents a KEM abstraction layer over traditional > * DH-based key exchange, which can be used for either straight > * DH/ECDH/XDH or TLS hybrid key exchanges. > * > * This class can be alongside standard full post-quantum KEMs > * when hybrid implementations are required. > */ > ``` Added description as suggested. > Could this class could be renamed to something more meaningful? e.g. > `DHasKEM`, `DHasaKEM` or something similar. A class name of `DH` by itself > hints this will be a DH implementation. I would expect a `KeyAgreement` impl, > not as a wrapper. Renamed `DH.java` to `DHasKEM.java`. > One other nit, currently the `Params` class doesn't actually handle `DH`, > just `ECDH`/`XDH`. Should you remove `DH` from the `DH/ECDH/XDH` javadoc? Removed `DH`. > Also, if it is only used by JSSE, I think it should be in the > `sun.security.ssl` package. Moved it to `sun.security.ssl` package. > We did include the internal `Tls*` implementations Sun in the `SunJCE` > provider, but those were actually exposed/available as `KeyGenerator`s. I was > never a fan, but it is what it is. > > `sun.security.ssl` is a better fit for this and `Hybrid.java`, especially > since these are strictly internal implementations for now. Move done. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2584262600
