8314323: Implement JEP 527: TLS 1.3 Hybrid Key Exchange - #662
Conversation
|
👋 Welcome back rmesde! A progress list of the required criteria for merging this PR into |
|
@rm-gh-8 This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 27 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
openjdk
Bot
changed the title
|
This backport pull request has now been updated with issue from the original commit. |
openjdk
Bot
added
backport
|
@stooke Please take a look. |
|
/reviewers 2 |
stooke
left a comment
stooke
left a comment
There was a problem hiding this comment.
(disclaimer: I am not an OpenJDK reviewer)
I have been working on this patch too and my version, is identical give or take one whitespace line in FipsModeTLS.java.
As such, I have confidence in it so I say it looks good to me.
|
I'd like to review and test this myself too. ETA, a couple of days. |
simonis
left a comment
simonis
left a comment
There was a problem hiding this comment.
Thanks @rm-gh-8 , the backport looks good. Is my understanding correct that you only ran the JMH benchmarks after the downport? How can we compare this to the performance results before the downport to ensure that the downport doesn't introduce any regression?
At a first glance, the downport introduces some new benchmarks so you probably can't easily run the new benchmarks with the old JDK, but maybe you can run the old benchmarks with the new code? It would definitely be nice to have at least some comparison between the two versions (and doing this comparison on Linux/x64/aarch64 would be enough).
gnu-andrew
left a comment
gnu-andrew
left a comment
There was a problem hiding this comment.
The patch itself looks near-clean and I agree that JDK-8372004, while caught in the context of this change, is unrelated.
The patch doesn't touch any API classes and so is safe for backport from trunk in that respect. I verified that the old codepaths are still present as is (if refactored) when not using hybrid exchange.
The main risk to be aware of is X25519MLKEM768 being added to the top of the default named groups where it will now take precedence over x25519 alone. However, there is a user-accessible workaround for this by setting custom named groups with jdk.tls.namedGroups. The other two algorithms have to be explicitly requested.
I think @franferrax should look this one over too.
/reviewers 4
|
|
|
@gnu-andrew |
The benchmark changes add a new case using the new provider, so yes, that case would fail if the new tests were run on the old code. I agree it would be useful to compare the benchmarks from the unpatched code on both patched and unpatched builds to ensure the change has not caused a noticeable slowdown in the old key exchange mechanisms. |
|
@simonis and @gnu-andrew , as requested, here are the findings from running the old (unpatched) benchmarks on both the unpatched and patched codebases (linux-x64).
Unpatched: Patched: Files: linux-x64-old-benchmark-test-before.log
Unpatched: Patched: Files: linux-x64-old-benchmark-2-test-before.log
Patched: Files: linux-x64-old-benchmark-3-test-before.log |
|
Analysis:
|
|
Thanks for the benchmark comparison. I'm a little concerned with the 18% regression in TLS1.3 handshakes. I think we should ask Oracle if they can confirm the regression and if they see a chance to fix it. I've done so on the original issue: https://bugs.openjdk.org/browse/JDK-8314323 |
jerboaa
left a comment
jerboaa
left a comment
There was a problem hiding this comment.
My interop testing of this passed. The patch looks good to me.
simonis
left a comment
simonis
left a comment
There was a problem hiding this comment.
It looks like the increased handshake time is the price we have to pay for additional security (see more details in the discussion on the JBS issue: https://bugs.openjdk.org/browse/JDK-8314323).
@rm-gh-8 , please make sure we mention this backport prominently in our release notes and specifically call out the regression for the TLS handshake and how it can be worked around by changing the order of the supported key exchange schemes programatically or with the help of the jdk.tls.namedGroups system property linking to the appropriate section of JEP 527 and the Customizing the Supported Named Groups for TLS/DTLS Key Exchange section of the "Java Secure Socket Extension (JSSE) Reference Guide".
|
Sounds good Volker. |
|
/approval request for backport of JDK-8314323: Implement JEP 527: TLS 1.3 Hybrid Key Exchange. This PR adds ML-KEM and hybrid post-quantum key exchange support to the JSSE provider. For parity with Oracle JDK. High risk - This is a security critical change that can potentially cause breaking interoperability with other TLS stacks and regressions to existing ECDHE/FFDHE handshakes |
openjdk
Bot
added
the
approval
There is a related release note - https://bugs.openjdk.org/browse/JDK-8377391 - so you should pick that up as a reminder if your tooling tracks those. We do for the upstream project. Regarding the regression, I think the discussion on the ticket resolves most of the worries. |
openjdk
Bot
added
ready
5 participants
Backporting JDK-8314323: Implement JEP 527: TLS 1.3 Hybrid Key Exchange.
This PR adds ML-KEM and hybrid post-quantum key exchange support to the JSSE provider.
This PR is not clean because it skips JDK-8372004 (introduces a large dependency graph), which affects KeyShareExtension.java. However, the conflict only requires minor adjustments (specifically using isOn instead of the isOn() method, which is not available).
For parity with Oracle JDK.
Ran related tests on linux-x64, linux-aarch64, macos-aarch64 and windows-x64:
make test TEST=test/jdk/javax/net/sslmake test TEST=test/jdk/sun/securityResults:
windows-x64-specific-test.log
windows-x64-specific-2-test.log
macos-aarch64-specific-test.log
macos-aarch64-specific-2-test.log
linux-x64-specific-test.log
linux-x64-specific-2-test.log
linux-aarch64-specific-test.log
linux-aarch64-specific-2-test.log
Ran related benchmarks on windows-x64, linux-x64, linux-aarch64, and macos-aarch64:
./build/*/images/jdk/bin/java -jar build/*/images/test/micro/benchmarks.jar org.openjdk.bench.java.security.SSLHandshake./build/*/images/jdk/bin/java -jar build/*/images/test/micro/benchmarks.jar org.openjdk.bench.javax.crypto.full.KEMBench./build/*/images/jdk/bin/java -jar build/*/images/test/micro/benchmarks.jar org.openjdk.bench.javax.crypto.full.KeyPairGeneratorBenchResults:
windows-x64-benchmark-test.log
windows-x64-benchmark-2-test.log
windows-x64-benchmark-3-test.log
macos-aarch64-benchmark-test.log
macos-aarch64-benchmark-2-test.log
macos-aarch64-benchmark-3-test.log
linux-x64-benchmark-test.log
linux-x64-benchmark-2-test.log
linux-x64-benchmark-3-test.log
linux-aarch64-benchmark-test.log
linux-aarch64-benchmark-2-test.log
linux-aarch64-benchmark-3-test.log
Risk Analysis: High - This is a security critical change that can potentially cause breaking interoperability with other TLS stacks and regressions to existing ECDHE/FFDHE handshakes
Progress
Issues
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk25u-dev.git pull/662/head:pull/662$ git checkout pull/662Update a local copy of the PR:
$ git checkout pull/662$ git pull https://git.openjdk.org/jdk25u-dev.git pull/662/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 662View PR using the GUI difftool:
$ git pr show -t 662Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk25u-dev/pull/662.diff
Using Webrev
Link to Webrev Comment