Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8297878: KEM: Implementation #1

Closed
wants to merge 5 commits into from
Closed

Conversation

wangweij
Copy link
Contributor

@wangweij wangweij commented Apr 18, 2024

This is the KEM API backport to Java SE 17 MR 1.

The src files are identical to those in the current jdk repo except for the change made to KEM.java at openjdk/jdk@59c2aff#diff-7bee547996d0de5692181a509bdf509276c7eb9351722580fd6aee7975745e67.

Update: There are javadoc changes to all the src files in following commits.

The RSA_KEM code is modified because DerOutputStream lacks several methods in JDK 17.

Proc is updated like in JDK 21 to support for some internal interop testing. The test files for Proc are also backported.

This change does not contain the DHKEM implementation in the original JDK 21 change.

This change also covers JDK-8322971 which fixed a follow-on P3 bug of the initial KEM work.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • JDK-8322971 needs maintainer approval
  • Commit message must refer to an issue
  • JDK-8297878 needs maintainer approval
  • Change requires CSR request JDK-8330545 to be approved

Issues

  • JDK-8297878: KEM: Implementation (Enhancement - P3 - Approved) ⚠️ Issue is already resolved. Consider making this a "backport pull request" by setting the PR title to Backport <hash> with the hash of the original commit. See Backports.
  • JDK-8322971: KEM.getInstance() should check if a 3rd-party security provider is signed (Bug - P3 - Approved) ⚠️ Issue is already resolved. Consider making this a "backport pull request" by setting the PR title to Backport <hash> with the hash of the original commit. See Backports.
  • JDK-8330545: KEM: Implementation (CSR)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk17u-ri.git pull/1/head:pull/1
$ git checkout pull/1

Update a local copy of the PR:
$ git checkout pull/1
$ git pull https://git.openjdk.org/jdk17u-ri.git pull/1/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk17u-ri/pull/1.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 18, 2024

👋 Welcome back weijun! 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
Copy link

openjdk bot commented Apr 18, 2024

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

8297878: KEM: Implementation
8322971: KEM.getInstance() should check if a 3rd-party security provider is signed

Reviewed-by: mullan, iris, andrew

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 1 new commit pushed to the master branch:

  • af9a602: 8331151: Update .jcheck/conf and version-numbers for 17.0.0.1

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

@openjdk openjdk bot added the rfr Pull request is ready for review label Apr 18, 2024
@mlbridge
Copy link

mlbridge bot commented Apr 18, 2024

Webrevs

@wangweij
Copy link
Contributor Author

/issue add JDK-8322971

@openjdk
Copy link

openjdk bot commented Apr 18, 2024

@wangweij
Adding additional issue to issue list: 8322971: KEM.getInstance() should check if a 3rd-party security provider is signed.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Apr 19, 2024
Copy link
Member

@irisclark irisclark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Spec changes appear to be identical to those in JDK 21, with the exception of the expected @since 17 and @apiNote which are used to identify these as additions via MR.

Associated CSR (JDK-8330545) also Reviewed.

@openjdk openjdk bot added approval and removed ready Pull request is ready to be integrated labels Apr 25, 2024
Copy link
Member

@gnu-andrew gnu-andrew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be easier to review if JDK-8322971 and JDK-8305846: "Support compilation in Proc test utility" were backported separately. Also, 8322971 seems to be missing from 21u as well.

The changes look correct as far as I can tell. I'm not sure why different copyright years are being used from the original patch, particularly for Provider.java where this is the only difference from 8297878.

The other differences (21->17, removal of the implementation, SSL & ECC changes) make sense. I presume the test differences with getKemImpl are due to the implementation removal making KEM.getInstance unusable?

So Skara correctly recognises this as a backport, the title should be "Backport 6b90b0519e89429300838fa598b2ea9ffda984a2"

@openjdk openjdk bot added ready Pull request is ready to be integrated and removed approval labels Apr 26, 2024
@GoeLin
Copy link
Member

GoeLin commented Apr 29, 2024

Hi @wangweij, you might want to enable the GHA tests?

@wangweij
Copy link
Contributor Author

@gnu-andrew I backported the 3 issues in a single PR because JDK-8305846 was originally created to support the RSA-KEM interop test. I understand the change for it looks a little strange without any usage of it here. I hope grouping them together will make further backport - if exists - simpler.

For the year of Provider.java, I just updated it to now.

Yes, getKemImpl exists because after JDK-8322971 one cannot load a KEM impl from an unsigned security provider in Oracle JDK. This is a little different from OpenJDK.

@wangweij
Copy link
Contributor Author

@GoeLin Maybe you can ask the maintainer of this repo to decide whether GHA tests can be enabled. Some security-related tests would fail because hardcoded certificates have expired.

@wangweij
Copy link
Contributor Author

/integrate

@openjdk
Copy link

openjdk bot commented Apr 29, 2024

Going to push as commit e9d2641.
Since your change was applied there has been 1 commit pushed to the master branch:

  • af9a602: 8331151: Update .jcheck/conf and version-numbers for 17.0.0.1

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Apr 29, 2024
@openjdk openjdk bot closed this Apr 29, 2024
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 29, 2024
@openjdk
Copy link

openjdk bot commented Apr 29, 2024

@wangweij Pushed as commit e9d2641.

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

@gnu-andrew
Copy link
Member

8322971

@gnu-andrew I backported the 3 issues in a single PR because JDK-8305846 was originally created to support the RSA-KEM interop test. I understand the change for it looks a little strange without any usage of it here. I hope grouping them together will make further backport - if exists - simpler.

We usually try and avoid it for more complex patches. It makes it easier for the person backporting, but harder for reviewers who have to pick it apart again.

For the year of Provider.java, I just updated it to now.

Ok, we usually avoid this as it creates differences from the original commit which may then break further backports.

Yes, getKemImpl exists because after JDK-8322971 one cannot load a KEM impl from an unsigned security provider in Oracle JDK. This is a little different from OpenJDK.

Ah, ok, so it's necessary after 8322971.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated
5 participants