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

8235710: Remove the legacy elliptic curves #289

Closed
wants to merge 3 commits into from

Conversation

@ascarpino
Copy link
Contributor

@ascarpino ascarpino commented Sep 21, 2020

This change removes the native elliptic curves library code; as well as, and calls to that code, tests, and files associated with those libraries. The makefiles have been changed to remove from all source builds of the ec code. The SunEC system property is removed and java.security configurations changed to reflect the removed curves.

This will remove the following elliptic curves from SunEC: secp112r1, secp112r2, secp128r1, secp128r2, secp160k1, secp160r1, secp160r2, secp192k1, secp192r1, secp224k1, secp224r1, secp256k1, sect113r1, sect113r2, sect131r1, sect131r2, sect163k1, sect163r1, sect163r2, sect193r1, sect193r2, sect233k1, sect233r1, sect239k1, sect283k1, sect283r1, sect409k1, sect409r1, sect571k1, sect571r1, X9.62 c2tnb191v1, X9.62 c2tnb191v2, X9.62 c2tnb191v3, X9.62 c2tnb239v1, X9.62 c2tnb239v2, X9.62 c2tnb239v3, X9.62 c2tnb359v1, X9.62 c2tnb431r1, X9.62 prime192v2, X9.62 prime192v3, X9.62 prime239v1, X9.62 prime239v2, X9.62 prime239v3, brainpoolP256r1 brainpoolP320r1, brainpoolP384r1, brainpoolP512r1


Progress

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

Issue

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/289/head:pull/289
$ git checkout pull/289

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 21, 2020

👋 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 Sep 21, 2020

@ascarpino The following labels will be automatically applied to this pull request: build security.

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

@ascarpino
Copy link
Contributor Author

@ascarpino ascarpino commented Sep 21, 2020

/csr

@openjdk
Copy link

@openjdk openjdk bot commented Sep 21, 2020

@ascarpino the issue for this pull request, JDK-8235710, already has an approved CSR request: JDK-8251547

@ascarpino ascarpino marked this pull request as ready for review Sep 21, 2020
@openjdk openjdk bot added the rfr label Sep 21, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 21, 2020

Webrevs

Copy link
Member

@XueleiFan XueleiFan left a comment

Looks good to me.

@openjdk
Copy link

@openjdk openjdk bot commented Sep 22, 2020

@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 more details.

After integration, the commit message for the final commit will be:

8235710: Remove the legacy elliptic curves

Reviewed-by: xuelei, erikj

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

  • 8239b67: 8253322: Update the specification in the newly added constructors
  • 24a4248: 8253615: Change to Visual Studio 2019 16.7.2 for building on Windows at Oracle
  • 8b85c3a: 8251261: CDS dumping should not clear states in live classes
  • 9ac162e: 8251999: remove usage of PropertyResolvingWrapper in vmTestbase/nsk/aod
  • 527a309: 8252722: More Swing plaf APIs that rely on default constructors
  • 3495c19: 8250855: Address reliance on default constructors in the Java 2D APIs
  • a9d0440: 8250859: Address reliance on default constructors in the Accessibility APIs
  • 89c5e49: 8253500: [REDO] JDK-8253208 Move CDS related code to a separate class
  • bf442c5: 8253539: Remove unused JavaThread functions for set_last_Java_fp/pc
  • dd2c456: 8253583: java/util/StringJoiner tests failing on 32-bit VMs after JDK-8246697
  • ... and 41 more: https://git.openjdk.java.net/jdk/compare/f800af978c42c88170def1b4996ffa524aaed4bd...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.

@openjdk openjdk bot added the ready label Sep 22, 2020
Copy link
Member

@erikj79 erikj79 left a comment

Build changes look good.

Copy link
Member

@seanjmullan seanjmullan left a comment

        throw new IllegalStateException(
            new InvalidAlgorithmParameterException(
                "Curve not supported:  Private: " +
                    ((privNC != null) ? privNC.toString() : " unknown") +
                    ", PublicKey:" +
                    ((pubNC != null) ? pubNC.toString() : " unknown")));

fix supportedcurves in SunEC
@@ -642,8 +633,7 @@ jdk.disabled.namedCurves = secp112r1, secp112r2, secp128r1, secp128r2, \
#
#
jdk.certpath.disabledAlgorithms=MD2, MD5, SHA1 jdkCA & usage TLSServer, \
RSA keySize < 1024, DSA keySize < 1024, EC keySize < 224, \
include jdk.disabled.namedCurves
RSA keySize < 1024, DSA keySize < 1024, EC keySize < 224
Copy link
Contributor

@wangweij wangweij Sep 24, 2020

jdk.disabled.namedCurves still exists. If someone decides to add a curve there, shouldn't it be also disabled here?

Copy link
Contributor Author

@ascarpino ascarpino Sep 24, 2020

jdk.disabled.namedCurves is commented out and I don't think it's good for every operation disabled algorithms call to check an empty property. The description for the disabledAlgorithm properties say you have to use "include", so I don't think it is necessary to we keep it active..

Copy link
Contributor

@wangweij wangweij Sep 24, 2020

I just think this is an unnecessary behavior change. After all, the purpose of jdk.disabled.namedCurves is to be included in other disabledAlgorithms properties.

No strong opinion on this. Please decide yourself.

Copy link
Contributor Author

@ascarpino ascarpino Sep 24, 2020

I understand what you are saying. The property only existed because there were so many curves that would have overwhelmed the disabledAlgorithms. I also don't like making this a permanent addition to the disabledAlgorithm properties. It's possible we may remove the property in the future as it's likely unnecessary going forward.

@ascarpino
Copy link
Contributor Author

@ascarpino ascarpino commented Sep 25, 2020

/integrate

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

@openjdk openjdk bot commented Sep 25, 2020

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

  • 8239b67: 8253322: Update the specification in the newly added constructors
  • 24a4248: 8253615: Change to Visual Studio 2019 16.7.2 for building on Windows at Oracle
  • 8b85c3a: 8251261: CDS dumping should not clear states in live classes
  • 9ac162e: 8251999: remove usage of PropertyResolvingWrapper in vmTestbase/nsk/aod
  • 527a309: 8252722: More Swing plaf APIs that rely on default constructors
  • 3495c19: 8250855: Address reliance on default constructors in the Java 2D APIs
  • a9d0440: 8250859: Address reliance on default constructors in the Accessibility APIs
  • 89c5e49: 8253500: [REDO] JDK-8253208 Move CDS related code to a separate class
  • bf442c5: 8253539: Remove unused JavaThread functions for set_last_Java_fp/pc
  • dd2c456: 8253583: java/util/StringJoiner tests failing on 32-bit VMs after JDK-8246697
  • ... and 41 more: https://git.openjdk.java.net/jdk/compare/f800af978c42c88170def1b4996ffa524aaed4bd...master

Your commit was automatically rebased without conflicts.

Pushed as commit 0b83fc0.

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

@ascarpino ascarpino deleted the ecremove branch Sep 29, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 26, 2020

Mailing list message from Magnus Ihse Bursie on security-dev:

Sorry for being late on this one (I'm working through a huge backlog),
but it does not seem like the removal was complete.

ENABLE_INTREE_EC is still present in spec.gmk. And it is still checked
in modules/jdk.crypto.ec/Lib.gmk. In fact, this entire file should be
removed.

Anthony, can you please open a new JBS issue to fix the remaining cleanup?

/Magnus

On 2020-09-22 15:23, Erik Joelsson wrote:

@magicus
Copy link
Member

@magicus magicus commented Oct 28, 2020

I opened https://bugs.openjdk.java.net/browse/JDK-8255530 for the remaining cleanup.

@ascarpino
Copy link
Contributor Author

@ascarpino ascarpino commented Oct 28, 2020

I have the change in a workspace, just hadn't created the bug yet.. thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment