8313229: DHEKeySizing.java should be modified to use TLS versions TLSv1, TLSv1.1, TLSv1.2

[seanjmullan]

Please review this change to ensure this test is tested on different TLS protocols (1.0, 1.1, 1.2)

I added a protocol parameter to the test arguments so that different protocols are tested. I also removed the boolean exportable argument as it wasn't doing anything.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8313229: DHEKeySizing.java should be modified to use TLS versions TLSv1, TLSv1.1, TLSv1.2 (Bug - P3)

Reviewers

• Jamil Nimeh (@jnimeh - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/15846/head:pull/15846
$ git checkout pull/15846

Update a local copy of the PR:
$ git checkout pull/15846
$ git pull https://git.openjdk.org/jdk.git pull/15846/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15846

View PR using the GUI difftool:
$ git pr show -t 15846

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/15846.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back mullan! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@seanjmullan The following label will be automatically applied to this pull request:

• security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 01: Full - Incremental (8a36dbf5)
• 00: Full (8068ea61)

[jnimeh]

Just curious why the server key exchange length went up in size by a couple bytes. Was 1643 incorrect before this change?

[seanjmullan]

Good question. Part of this is a cut-and-paste error. The only change to 1645 bytes should be for line 64. The previous version of this test used TLS 1.0 for all the tests. When testing this on different protocols, I noticed the server hello for this cipher suite takes 2 extra bytes on TLSv1.2, and this was enough to cause the test to fail even with the 6 extra bytes for KEY_LEN_BIAS. - I don't know the exact reason why it takes a few extra bytes though.

I fixed this in the latest commit - only line 64 should be different now for the server hello length.

[jnimeh]

An extra two bytes for a server hello could be due to the assertion of a SH extension that was not asserted in earlier versions of the protocol or something along those lines. Since that 1645 bytes relates to "Server Hello Series" (I assume that means the entire SH flight of messages) there could be a two-byte variance in a number of places. The fix looks good to me.

[jnimeh]

LGTM

[openjdk]

@seanjmullan 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:

8313229: DHEKeySizing.java should be modified to use TLS versions TLSv1, TLSv1.1, TLSv1.2

Reviewed-by: jnimeh

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 366 new commits pushed to the master branch:

• 015f6f5: 8315771: [JVMCI] Resolution of bootstrap methods with int[] static arguments
• 83b01cf: 8311922: [macOS] right-Option key fails to generate release event
• 542b300: 8315954: getArgumentValues002.java fails on Graal
• 0637900: 8316433: net.dll should delay load winhttp.dll
• 90bcdbd: 8316581: Improve performance of Symbol::print_value_on()
• 8350268: 8316453: [JVMCI] Using Xcomp on jargraal must eagerly initialize JVMCI
• 3809d69: 8316240: Open source several add/remove MenuBar manual tests
• 38bf119: 8310228: Improve error reporting for uncaught native exceptions on Windows
• 349723c: 8315739: Missing null check in os::vm_min_address
• 8cbe42b: 8316421: libjava should load shell32.dll eagerly
• ... and 356 more: https://git.openjdk.org/jdk/compare/dcd6e756718b656d43f4575558f41ce0c28d0eca...master

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 master branch, type /integrate in a new comment.

[seanjmullan]

/integrate

[openjdk]

Going to push as commit c698b45.
Since your change was applied there have been 366 commits pushed to the master branch:

• 015f6f5: 8315771: [JVMCI] Resolution of bootstrap methods with int[] static arguments
• 83b01cf: 8311922: [macOS] right-Option key fails to generate release event
• 542b300: 8315954: getArgumentValues002.java fails on Graal
• 0637900: 8316433: net.dll should delay load winhttp.dll
• 90bcdbd: 8316581: Improve performance of Symbol::print_value_on()
• 8350268: 8316453: [JVMCI] Using Xcomp on jargraal must eagerly initialize JVMCI
• 3809d69: 8316240: Open source several add/remove MenuBar manual tests
• 38bf119: 8310228: Improve error reporting for uncaught native exceptions on Windows
• 349723c: 8315739: Missing null check in os::vm_min_address
• 8cbe42b: 8316421: libjava should load shell32.dll eagerly
• ... and 356 more: https://git.openjdk.org/jdk/compare/dcd6e756718b656d43f4575558f41ce0c28d0eca...master

Your commit was automatically rebased without conflicts.

[openjdk]

@seanjmullan Pushed as commit c698b45.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.