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

8261502: ECDHKeyAgreement: Allows alternate ECPrivateKey impl and revised exception handling #2659

Closed
wants to merge 4 commits into from

Conversation

@ascarpino
Copy link
Contributor

@ascarpino ascarpino commented Feb 20, 2021

Hi,

I need a code review of this change to ECDH. It is a combination of fixing the implementation to not only accept ECPrivateKeyImpl along with a fix to the exception handling. They started as two fixes, but with the exception handling the underlying code changed significantly that made the ECPrivateKey change in a different place. The new exception handling is a result of no longer having the native library. Many of the checks waited until generateSecret() to send the keys to the native library. Now that native is gone, checks can happen when keys are provided to the methods and proper exceptions can be thrown instead of wrapping everything as a ProviderException

thanks,

Tony


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8261502: ECDHKeyAgreement: Allows alternate ECPrivateKey impl and revised exception handling

Reviewers

  • Jamil Nimeh (@jnimeh - Reviewer) ⚠️ Review applies to aec24cc107bd3bd7ac931d7ab6af116642102de7

Download

To checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/2659/head:pull/2659
$ git checkout pull/2659

To update a local copy of the PR:
$ git checkout pull/2659
$ git pull https://git.openjdk.java.net/jdk pull/2659/head

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Feb 20, 2021

👋 Welcome back ascarpino! 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 openjdk bot commented Feb 20, 2021

@ascarpino 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.

@openjdk openjdk bot added the security label Feb 20, 2021
@ascarpino ascarpino marked this pull request as ready for review Feb 22, 2021
@openjdk openjdk bot added the rfr label Feb 22, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Feb 22, 2021

Webrevs

(nc != null ? nc.toString() : "unknown")));
try {
result = deriveKeyImpl(privateKey, privateKeyOps, publicKey);
} catch (Exception e) {
Copy link
Member

@jnimeh jnimeh Mar 18, 2021

Choose a reason for hiding this comment

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

Why such a broad exception catch here? It looks like deriveKeyImpl is only explicitly throwing IKE. Are there other unchecked exceptions that you're trying to snag here that I'm missing in the deriveKeyImpl below?

Copy link
Contributor Author

@ascarpino ascarpino Mar 19, 2021

Choose a reason for hiding this comment

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

Just being cautious and wrapping anything. Maybe there will be some exceptions in the math methods that could throw exceptions.

Copy link
Member

@jnimeh jnimeh Mar 19, 2021

Choose a reason for hiding this comment

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

Fair enough. I'm OK to leave this as-is.

/*
* @test
* @bug 8238911
* @summary Check that ECPrivateKey's that are not ECPrivateKeyImpl can use
Copy link
Member

@jnimeh jnimeh Mar 18, 2021

Choose a reason for hiding this comment

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

Do the bug and summary tags need to be updated to 8261502 and "ECDHKeyAgreement: Allows alternate ECPrivateKey impl and revised exception handling", respectively?

jnimeh
jnimeh approved these changes Mar 19, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 19, 2021

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

8261502: ECDHKeyAgreement: Allows alternate ECPrivateKey impl and revised exception handling

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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 ready label Mar 19, 2021
@ascarpino
Copy link
Contributor Author

@ascarpino ascarpino commented Mar 25, 2021

/integrate

@openjdk openjdk bot closed this Mar 25, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Mar 25, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 25, 2021

@ascarpino Since your change was applied there have been 53 commits pushed to the master branch:

  • dbc9e4b: 8253795: Implementation of JEP 391: macOS/AArch64 Port
  • b006f22: 4833719: (bf) Views of MappedByteBuffers are not MappedByteBuffers, and cannot be forced
  • 8307aa6: 8264165: jpackage BasicTest fails after JDK-8220266: Check help text contains plaform specific parameters
  • c037e1e: 8263454: com.apple.laf.AquaFileChooserUI ignores the result of String.trim()
  • a1e717f: 8264146: Make Mutex point to rather than embed _name
  • f69afba: 8263300: add HtmlId for the block containing a class's description.
  • d82464f: 8263528: Make static page ids safe from collision with language elements
  • d602ae0: 8263884: Clean up os::is_allocatable() across Posix platforms
  • a9d287a: 8260388: Listing (sub)packages at package level of API documentation
  • 8120064: 8263781: C2: Cannot hoist independent load above arraycopy
  • ... and 43 more: https://git.openjdk.java.net/jdk/compare/d7268fa3a68c2356548651a27b307d7f7158e700...master

Your commit was automatically rebased without conflicts.

Pushed as commit 374272f.

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

@ascarpino ascarpino deleted the ecdh branch Mar 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants