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

8153005: Upgrade the default PKCS12 encryption/MAC algorithms #473

Closed
wants to merge 5 commits into from

Conversation

@wangweij
Copy link
Contributor

@wangweij wangweij commented Oct 1, 2020

Default algorithms are bumped to be based on PBES2 with AES-256 and SHA-256. Please also review the CSR at https://bugs.openjdk.java.net/browse/JDK-8228481.


Progress

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

Testing

Linux x32 Linux x64 Windows x64 macOS x64
Build (1/1 running) (5/5 running) (2/2 running) (2/2 running)

Issue

  • JDK-8153005: Upgrade the default PKCS12 encryption/MAC algorithms

Reviewers

Download

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Oct 1, 2020

👋 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 openjdk bot added the rfr label Oct 1, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 1, 2020

@wangweij 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 Oct 1, 2020
@wangweij
Copy link
Contributor Author

@wangweij wangweij commented Oct 1, 2020

TBD: We bumped iteration counts for PBE and HMAC to 50000 and 100000 when we were using weak algorithms. Now that the algorithms are strong, we can consider lower them. Currently, openssl 3.0.0 uses 2048 and Windows Server 2019 uses 2000.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 1, 2020

@@ -48,7 +48,11 @@

public static void main(String[] args) throws Throwable {

SecurityTools.keytool("-genkeypair -storetype pkcs12 -keystore ks"
// Using the old algorithms to make sure the file is recognized
Copy link
Member

@seanjmullan seanjmullan Oct 2, 2020

Choose a reason for hiding this comment

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

Do we also want to have a test that uses the new algorithms?

Copy link
Contributor Author

@wangweij wangweij Oct 2, 2020

Choose a reason for hiding this comment

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

I only know Windows Server 2019 can accept the new algorithms.

Copy link
Member

@seanjmullan seanjmullan Oct 6, 2020

Choose a reason for hiding this comment

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

Ok, but maybe we can split this test in two and use the jtreg @requires tag to run the newer algorithms on Windows Server 2019? It would be a useful test if this is the only test where we test PKCS12 interop with Windows.

Copy link
Contributor Author

@wangweij wangweij Oct 6, 2020

Choose a reason for hiding this comment

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

OK. Or I can see if there is an existing method in test/lib that can detects the version.

@@ -1,5 +1,5 @@
/*
Copy link
Member

@seanjmullan seanjmullan Oct 2, 2020

Choose a reason for hiding this comment

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

Is this test change supposed to be a part of this fix?

Copy link
Contributor Author

@wangweij wangweij Oct 2, 2020

Choose a reason for hiding this comment

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

Yes, the change simplifies checkAlg calls so they don't need to convert KnownOIDs or String to ObjectIdentifier first.

Copy link
Contributor

@haimaychao haimaychao left a comment

Looks good. Only minor comments.

private static final String DEFAULT_KEY_PBE_ALGORITHM
= "PBEWithHmacSHA256AndAES_256";
private static final String DEFAULT_MAC_ALGORITHM = "HmacPBESHA256";
private static final int DEFAULT_PBE_ITERATION_COUNT = 50000;
Copy link
Contributor

@haimaychao haimaychao Oct 7, 2020

Choose a reason for hiding this comment

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

As we have keystore.pkcs12.certPbeIterationCount and keystore.pkcs12.keyPbeIterationCount, I would like to suggest that we can define DEFAULT_CERT_PBE_ITERATION_COUNT and DEFAULT_KEY_PBE_ITERATION_COUNT, specifying each of the values for finer granularity. Same for LEGACY_PBE_ITERATION_COUNT.

@@ -48,7 +48,11 @@

Copy link
Contributor

@haimaychao haimaychao Oct 7, 2020

Choose a reason for hiding this comment

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

Add bug number to @bug.

Copy link
Contributor Author

@wangweij wangweij Oct 7, 2020

Choose a reason for hiding this comment

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

OK.

@haimaychao
Copy link
Contributor

@haimaychao haimaychao commented Oct 7, 2020

CSR looks good. In "Sepcification" section: a typo in 'Thr iteration counts used by'. At the end, it describes the new system property will override the security properties and use the older and weaker algorithms, so suggest we could also add text about setting the iteration counts to the default legacy values.

@wangweij
Copy link
Contributor Author

@wangweij wangweij commented Oct 7, 2020

CSR updated. More description, and iteration counts lowered to 10000. Will update code soon.

@wangweij
Copy link
Contributor Author

@wangweij wangweij commented Oct 8, 2020

New commit updating ic to 10000. I also created separate constants for DEFAULT_CERT_PBE_ITERATION_COUNT and DEFAULT_KEY_PBE_ITERATION_COUNT. I haven't made the change for LEGACY_PBE_ITERATION_COUNT since they will never change.

@seanjmullan
Copy link
Member

@seanjmullan seanjmullan commented Oct 8, 2020

Are you still planning, or is it possible to add a test for Windows 2019? Also, have you considered adding a test that checks if the JDK can read OpenSSL PKCS#12 files and vice versa? Maybe we can do that later as a follow-on issue. Otherwise, I will approve.

@wangweij
Copy link
Contributor Author

@wangweij wangweij commented Oct 9, 2020

I tried but cannot find a way to tell if a system is Windows Server 2016 or 2019. Their os.version is all 10.0. I've filed an enhancement at https://bugs.openjdk.java.net/browse/JDK-8254241 for it. That said, I did try running the test on a Windows Server 2019 using new algorithms and it succeeds.

@wangweij
Copy link
Contributor Author

@wangweij wangweij commented Oct 9, 2020

There are existing tests reading openssl generated pkcs12 files in https://github.com/openjdk/jdk/tree/master/test/jdk/sun/security/pkcs12/params, it already contains files using both weak and strong algorithms.

@wangweij
Copy link
Contributor Author

@wangweij wangweij commented Oct 9, 2020

Update params/README, exclude it from the de-BASE64 list (don't know it succeeded) in the ParamsTest.java test. Also remove a useless call in the test.

Thinking about adding a benchmark, but it will be in another commit.

@wangweij
Copy link
Contributor Author

@wangweij wangweij commented Oct 9, 2020

/csr needed

@openjdk openjdk bot added the csr label Oct 9, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 9, 2020

@wangweij this pull request will not be integrated until the CSR request JDK-8228481 for issue JDK-8153005 has been approved.

@openjdk openjdk bot removed the csr label Oct 14, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 30, 2020

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

8153005: Upgrade the default PKCS12 encryption/MAC algorithms

Reviewed-by: mullan

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

  • 8a065ef: 8255005: Fix indentation levels in classFileParser.cpp
  • e48016b: 8255565: [Vector API] Add missing format strings for extract instructs in x86.ad
  • 2c7fc85: 8254972: Fix pretouch chunk calculations
  • d128191: 8253473: Javadoc clean up in HttpHandler, HttpPrincipal, HttpContext, and HttpsConfigurator
  • 379ba80: 8255595: delay_to_keep_mmu passes wrong arguments to Monitor wait
  • 1a89d68: 8255285: Move JVMFlag origins into a new enum JVMFlagOrigin
  • 56eb5f5: 8255466: C2 crashes at ciObject::get_oop() const+0x0
  • 5782a2a: 8254975: lambda proxy fails to access a protected member inherited from a split package
  • d5138d1: 8255604: java/nio/channels/DatagramChannel/Connect.java fails with java.net.BindException: Cannot assign requested address: connect
  • 2a2fa13: 8255449: Improve the exception message of MethodHandles::permuteArguments
  • ... and 71 more: https://git.openjdk.java.net/jdk/compare/e8b75b13dc3efdfc5da602db400d98871f90d074...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 Oct 30, 2020
@wangweij
Copy link
Contributor Author

@wangweij wangweij commented Oct 30, 2020

/integrate

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

@openjdk openjdk bot commented Oct 30, 2020

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

  • 8a065ef: 8255005: Fix indentation levels in classFileParser.cpp
  • e48016b: 8255565: [Vector API] Add missing format strings for extract instructs in x86.ad
  • 2c7fc85: 8254972: Fix pretouch chunk calculations
  • d128191: 8253473: Javadoc clean up in HttpHandler, HttpPrincipal, HttpContext, and HttpsConfigurator
  • 379ba80: 8255595: delay_to_keep_mmu passes wrong arguments to Monitor wait
  • 1a89d68: 8255285: Move JVMFlag origins into a new enum JVMFlagOrigin
  • 56eb5f5: 8255466: C2 crashes at ciObject::get_oop() const+0x0
  • 5782a2a: 8254975: lambda proxy fails to access a protected member inherited from a split package
  • d5138d1: 8255604: java/nio/channels/DatagramChannel/Connect.java fails with java.net.BindException: Cannot assign requested address: connect
  • 2a2fa13: 8255449: Improve the exception message of MethodHandles::permuteArguments
  • ... and 71 more: https://git.openjdk.java.net/jdk/compare/e8b75b13dc3efdfc5da602db400d98871f90d074...master

Your commit was automatically rebased without conflicts.

Pushed as commit f77a658.

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

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