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

8307134: Add GTS root CAs #13754

Closed
wants to merge 5 commits into from
Closed

Conversation

jianglizhou
Copy link
Contributor

@jianglizhou jianglizhou commented May 2, 2023

This PR was requested by awarner@google.com. The updates were provided by awarner@google.com.


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

Reviewers

Contributors

  • Andy Warner <awarner@google.com>
  • Rajan Halade <rhalade@openjdk.org>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13754

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

Using diff file

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

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented May 2, 2023

👋 Welcome back jiangli! 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 Pull request is ready for review label May 2, 2023
@openjdk
Copy link

openjdk bot commented May 2, 2023

@jianglizhou 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 security-dev@openjdk.org label May 2, 2023
@mlbridge
Copy link

mlbridge bot commented May 2, 2023

Webrevs

@jianglizhou
Copy link
Contributor Author

/contributor add Andy Warner awarner@google.com

@openjdk
Copy link

openjdk bot commented May 2, 2023

@jianglizhou
Contributor Andy Warner <awarner@google.com> successfully added.

@seanjmullan
Copy link
Member

Ideally, an infra test for testing test certs should also be added. @rhalade may be able to contribute this.

@rhalade
Copy link
Member

rhalade commented May 2, 2023

I have infra tests for interop implemented. @jianglizhou, please check https://github.com/openjdk/jdk/compare/master...rhalade:jdk:googletrust-certify?expand=1

@@ -1,20 +1,19 @@
Owner: CN=GlobalSign, O=GlobalSign, OU=GlobalSign ECC Root CA - R4
Issuer: CN=GlobalSign, O=GlobalSign, OU=GlobalSign ECC Root CA - R4
Serial number: 2a38a41c960a04de42b228a50be8349802
Serial number: 203e57ef53f93fda50921b2a6
Copy link
Member

Choose a reason for hiding this comment

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

Why is this certificate changed?

Copy link
Contributor

@aww-aww aww-aww May 2, 2023

Choose a reason for hiding this comment

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

The original R4 did not have the digitalSignature keyUsage set. This root signs OCSP responses, so it needed to be reissued to comply with section 7.1.2.1 of the CA/B Forum baseline requirements. The only change between the two versions aside from the serial number is the addition of the digitalSignature key usage bit.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explanation. Please file a different issue for this change, since it is outside the scope of this issue, which is to specifically add new roots that have been approved by the Java SE CA Root Program processes. Updated roots, even for small changes such as this, should be handled and approved using an equivalent process.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted src/java.base/share/data/cacerts/globalsigneccrootcar4 in this PR. Looks like the update for "globalsigneccrootcar4 [jdk]" in test/jdk/sun/security/lib/cacerts/VerifyCACerts.java also needs to be reverted, otherwise the test fails with the following error. I'll go ahead and revert that as well.

ERROR: wrong checksum72:03:89:C2:7B:BF:87:87:E1:65:44:6E:43:5C:65:FF:B5:E8:F9:4C:8A:D1:63:6D:D1:91:4C:AD:1C:9A:CB:3B
Expected checksum23:6E:7A:1C:37:AD:82:31:FD:32:E8:31:63:4B:1A:88:BA:1A:4D:F6:D3:91:CD:0F:B4:09:EC:55:9A:B2:01:51
ERROR: globalsigneccrootcar4 [jdk] SHA-256 fingerprint is incorrect
java.lang.RuntimeException: At least one cacert test failed
        at VerifyCACerts.main(VerifyCACerts.java:380)
        at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
        at java.base/java.lang.reflect.Method.invoke(Method.java:578)
        at com.sun.javatest.regtest.agent.MainWrapper$MainTask.run(MainWrapper.java:138)
        at java.base/java.lang.Thread.run(Thread.java:1592)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Also updated 'CHECKSUM' value in test/jdk/sun/security/lib/cacerts/VerifyCACerts.java reflectively.

@jianglizhou
Copy link
Contributor Author

Ideally, an infra test for testing test certs should also be added. @rhalade may be able to contribute this.

Thanks!

@jianglizhou
Copy link
Contributor Author

I have infra tests for interop implemented. @jianglizhou, please check https://github.com/openjdk/jdk/compare/master...rhalade:jdk:googletrust-certify?expand=1

@rhalade, thanks! I have a minor comment below for your test/jdk/security/infra/java/security/cert/CertPathValidator/certification/GoogleCA.java test. I'll defer to @awarner@google.com for detailed review, as I don't have much context.

Please fix the bug id, 8303394:

/*
 * @test
 * @bug 8303394
 * @summary Interoperability tests with Google's GlobalSign R4 and GTS Root certificates
 ...

Could you please also let me know your plan on committing the GoogleCA.java? Do you plan to create a PR?

@rhalade
Copy link
Member

rhalade commented May 2, 2023

I have infra tests for interop implemented. @jianglizhou, please check https://github.com/openjdk/jdk/compare/master...rhalade:jdk:googletrust-certify?expand=1

@rhalade, thanks! I have a minor comment below for your test/jdk/security/infra/java/security/cert/CertPathValidator/certification/GoogleCA.java test. I'll defer to @awarner@google.com for detailed review, as I don't have much context.

Please fix the bug id, 8303394:

/*
 * @test
 * @bug 8303394
 * @summary Interoperability tests with Google's GlobalSign R4 and GTS Root certificates
 ...

Could you please also let me know your plan on committing the GoogleCA.java? Do you plan to create a PR?

You can include this contribution in your PR. Then it will be easier to backport to JDK 20u as one changeset. I updated bug id in the changeset.

@aww-aww
Copy link
Contributor

aww-aww commented May 2, 2023

I have infra tests for interop implemented. @jianglizhou, please check https://github.com/openjdk/jdk/compare/master...rhalade:jdk:googletrust-certify?expand=1

Aside from the bug number @jianglizhou raised, the interop tests look good to me.

@jianglizhou
Copy link
Contributor Author

/contributor add rhalade

@openjdk
Copy link

openjdk bot commented May 2, 2023

@jianglizhou
Contributor Rajan Halade <rhalade@openjdk.org> successfully added.

@jianglizhou
Copy link
Contributor Author

You can include this contribution in your PR. Then it will be easier to backport to JDK 20u as one changeset. I updated bug id in the changeset.

Done. Please double check.

I ran the GoogleCA.java test with a test JDK binary built with this PR changes included. The test passed.

Copy link
Member

@seanjmullan seanjmullan left a comment

Choose a reason for hiding this comment

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

Looks good. Please also wait for approval from @rhalade before integrating.

@openjdk
Copy link

openjdk bot commented May 3, 2023

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

8307134: Add GTS root CAs

Co-authored-by: Andy Warner <awarner@google.com>
Co-authored-by: Rajan Halade <rhalade@openjdk.org>
Reviewed-by: mullan, rhalade

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

  • 3930709: 8068925: Add @OverRide in javax.tools classes
  • fc76687: 8306836: Remove pinned tag for G1 heap regions
  • ccf91f8: 8306933: C2: "assert(false) failed: infinite loop" failure
  • e9807a4: 8306042: C2: failed: Missed optimization opportunity in PhaseCCP (adding LShift->Cast->Add notification)
  • fcb280a: 8305092: Improve Thread.sleep(millis, nanos) for sub-millisecond granularity
  • 891530f: 8307005: Make CardTableBarrierSet::initialize non-virtual
  • e0774be: 8306997: C2: "malformed control flow" assert due to missing safepoint on backedge with a switch
  • 462b1df: 8307106: Allow concurrent GCs to walk CLDG without ClassLoaderDataGraph_lock
  • c8f3756: 8306729: Add nominal descriptors of modules and packages to Constants API
  • 0b5b642: 8307150: RISC-V: Remove remaining StoreLoad barrier with UseCondCardMark for Serial/Parallel GC
  • ... and 7 more: https://git.openjdk.org/jdk/compare/a8bf2acb7db63b508ef169e42a27b9c99178cbb1...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 Pull request is ready to be integrated label May 3, 2023
@jianglizhou
Copy link
Contributor Author

Looks good. Please also wait for approval from @rhalade before integrating.

Thanks @seanjmullan. Will wait for @rhalade's approval as well.

Copy link
Member

@rhalade rhalade left a comment

Choose a reason for hiding this comment

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

Looks good to me. Please don't forget to update the release note and backport to JDK 20u.

@jianglizhou
Copy link
Contributor Author

Looks good to me. Please don't forget to update the release note and backport to JDK 20u.

Thanks @rhalade. Will do.

@jianglizhou
Copy link
Contributor Author

/integrate

@openjdk
Copy link

openjdk bot commented May 3, 2023

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

  • 63cd0a3: 4200096: OffScreenImageSource.removeConsumer NullPointerException
  • db8b3cd: 8305963: Typo in java.security.Security.getProperty
  • dcb2f3f: 8306320: BufferedImage spec needs clarification w.r.t its implementation of the WritableRenderedImage interface
  • 1487477: 8305815: Update Libpng to 1.6.39
  • 705ad7d: 8306014: Update javax.net.ssl TLS tests to use SSLContextTemplate or SSLEngineTemplate
  • 3930709: 8068925: Add @OverRide in javax.tools classes
  • fc76687: 8306836: Remove pinned tag for G1 heap regions
  • ccf91f8: 8306933: C2: "assert(false) failed: infinite loop" failure
  • e9807a4: 8306042: C2: failed: Missed optimization opportunity in PhaseCCP (adding LShift->Cast->Add notification)
  • fcb280a: 8305092: Improve Thread.sleep(millis, nanos) for sub-millisecond granularity
  • ... and 12 more: https://git.openjdk.org/jdk/compare/a8bf2acb7db63b508ef169e42a27b9c99178cbb1...master

Your commit was automatically rebased without conflicts.

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

openjdk bot commented May 3, 2023

@jianglizhou Pushed as commit 03030d4.

💡 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
Labels
integrated Pull request has been integrated security security-dev@openjdk.org
4 participants