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

8179503: Java should support GET OCSP calls #847

Closed
wants to merge 92 commits into from

Conversation

apavlyutkin
Copy link
Contributor

@apavlyutkin apavlyutkin commented Mar 2, 2022

Hello! I'd like to backport

8179503: Java should support GET OCSP calls (dependency)

to jdk11u-dev. This one is required as a dependency for

8274471: Add support for RSASSA-PSS in OCSP Response

The following changes were done to apply original patch:

src/java.base/share/classes/sun/security/provider/certpath/OCSP.java

  • resolved baseline conflict taking place due to absent revocation checking code

The rest of the code applied without changes

Verification/regression (20.04/amd64): jdk_security


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

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk11u-dev.git pull/847/head:pull/847
$ git checkout pull/847

Update a local copy of the PR:
$ git checkout pull/847
$ git pull https://git.openjdk.org/jdk11u-dev.git pull/847/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 847

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk11u-dev/pull/847.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 2, 2022

👋 Welcome back apavlyutkin! 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 changed the title Backport f5ee356540d7aa4a7663c0d5d74f5fdb0726b426 8179503: Java should support GET OCSP calls Mar 2, 2022
@openjdk
Copy link

openjdk bot commented Mar 2, 2022

This backport pull request has now been updated with issue from the original commit.

@openjdk openjdk bot added the backport label Mar 2, 2022
@openjdk openjdk bot added the rfr Pull request is ready for review label Mar 2, 2022
@mlbridge
Copy link

mlbridge bot commented Mar 2, 2022

Webrevs

@apavlyutkin
Copy link
Contributor Author

@TheRealMDoerr @alexeybakhtin I splitted #788, this is the 1st part for 8179503

Copy link
Contributor

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

@@ -1,5 +1,5 @@
/*
* Copyright (c) 2009, 2017, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2009, 2022, Oracle and/or its affiliates. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Copyright year update not needed for pure backports when the original change doesn't touch it. May cause merge conflicts. Feel free to revert.

@openjdk
Copy link

openjdk bot commented Mar 2, 2022

@apavlyutkin This change now passes all automated pre-integration checks.

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

8179503: Java should support GET OCSP calls

Reviewed-by: mdoerr

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:

  • 6b4feb5: 8305975: Add TWCA Global Root CA

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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@TheRealMDoerr) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Mar 2, 2022
Copy link
Contributor

@TheRealMDoerr TheRealMDoerr left a comment

Choose a reason for hiding this comment

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

Thumbs up from my side!

@alexeybakhtin
Copy link
Contributor

Looks good.

@GoeLin
Copy link
Member

GoeLin commented Mar 7, 2022

Please configure Pre-submit tests.

@apavlyutkin
Copy link
Contributor Author

@GoeLin I've configured pre-submit tests, but they constantly fail on cross-compilation for different target architectures due to absense of required packages. Is that my fault, should I do something else?

@TheRealMDoerr
Copy link
Contributor

A lot of pre-submit tests have failed for other PRs, too. I don't think it's your fault. This change is not platform dependent, so I think your tests are fine.

@GoeLin
Copy link
Member

GoeLin commented Mar 10, 2022

Thanks for the tests, the failures are unrelated.

@apavlyutkin
Copy link
Contributor Author

jdk11u-fix-no

@apavlyutkin apavlyutkin closed this Apr 6, 2022
@apavlyutkin apavlyutkin reopened this Mar 17, 2023
RealCLanger and others added 8 commits March 17, 2023 11:36
…icationException

Change implementation to use iterator instead of plain LinkedList

Reviewed-by: phh
Backport-of: d948bfd
…ption message

Reviewed-by: mchung, phh
Backport-of: 9f9d678591e02ecaeae7b81eeefb0ba41c7b4dae
Backport-of: e82dc6935b5f575a53fcba9c96767cee1b535cb8
…ust be at least loaded

Set InstanceKlass::loaded before adding classes to the subklass list, which can be read concurrently by the compiler.

Reviewed-by: rrich
Backport-of: cab9667
Backport-of: 3330f9c2ad508fdb9100a2400abdd3366342dde1
naotoj and others added 12 commits May 10, 2023 17:03
Co-authored-by: Justin Lu <jlu@openjdk.org>
Reviewed-by: lancea, iris, rriggs
Backport-of: 0a700c6c3d150ed375c113b31b8e6185cbe57ae6
…or GB 18030-2022 Implementation Level 2

Reviewed-by: lancea, iris, rriggs
Backport-of: 926910977080dda181ae3772293d2bc9bd458f2a
…call in early potential CHECK_NULL return

Backport-of: b51ea4204eaa18687e7712e87cdc92efbddfcb5b
…ll in early potential CHECK_NULL return

Backport-of: a7e308ab6e5dba7df790840d29fc7edbf3af2e24
…rsion

Reviewed-by: lancea, rriggs, naoto, iris
Backport-of: e5ac7a1b7e8df1e56a7e78bba6a2b9ed7fc297f1
Remove old version specific code in reflection.cpp

Reviewed-by: clanger
Backport-of: 145582d
… backport of JDK-8303861

Reviewed-by: clanger
Backport-of: 5296357fe27ac9f68c635cd1ba324c4985934354
Reviewed-by: mdoerr
Backport-of: 5b43804b7988ea4abd6458fba0a042b7bd6d9cb8
@phax
Copy link

phax commented May 16, 2023

This issue causes a regression, if the OCSP server is not returning the Content-Length header which at least happens now and then on a DigiCert server. In that case the contentLength is set to Integer.MAX_VALUE and IOUtils.readExactlyNBytes fails with an EOFException because it can't read Integer.MAX_VALUE bytes

See also phax/phase4#124 for my original analysis on the Java 17 problem

@apavlyutkin
Copy link
Contributor Author

This issue causes a regression, if the OCSP server is not returning the Content-Length header which at least happens now and then on a DigiCert server. In that case the contentLength is set to Integer.MAX_VALUE and IOUtils.readExactlyNBytes fails with an EOFException because it can't read Integer.MAX_VALUE bytes

See also phax/phase4#124 for my original analysis on the Java 17 problem

Thanks a lot, Philip! I will keep it on mind.

@apavlyutkin
Copy link
Contributor Author

@RealCLanger

Hi, Chris! As I see you've marked dependent JDK-8274471 with jdk11u-todo. Does it mean I should keep working on this and create depended PR(s)? Previously this one was rejected with jdk11u-fix-request-no as inappropriate for jdk11

@RealCLanger
Copy link
Contributor

RealCLanger commented May 17, 2023

This issue causes a regression, if the OCSP server is not returning the Content-Length header which at least happens now and then on a DigiCert server. In that case the contentLength is set to Integer.MAX_VALUE and IOUtils.readExactlyNBytes fails with an EOFException because it can't read Integer.MAX_VALUE bytes

See also phax/phase4#124 for my original analysis on the Java 17 problem

This is recorded in JBS now as JDK-8308255.

@RealCLanger
Copy link
Contributor

@RealCLanger

Hi, Chris! As I see you've marked dependent JDK-8274471 with jdk11u-todo. Does it mean I should keep working on this and create depended PR(s)? Previously this one was rejected with jdk11u-fix-request-no as inappropriate for jdk11

Hi @apavlyutkin,
I have removed jdk11u-fix-no from JDK-8179503 now, as I think it was backported for Oracle as well (although the backport is not public) and there is justification for backporting this change as well as JDK-8274471.
You need not care about the jdk11u-todo label. It is just some flagging that helps us keep the JBS filters for open backports lean. You could remove the label once a backport to 11u was merged. But you can also keep the label around for us to clean up.

So, please go ahead with your efforts to backport these two items. As for this PR, I think a merge with master would clean it up and also trigger GHA, hoping for all green results.

@openjdk
Copy link

openjdk bot commented May 19, 2023

⚠️ @apavlyutkin This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

Copy link
Member

@phohensee phohensee left a comment

Choose a reason for hiding this comment

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

@apavlyutkin, there are 494 changed files in the PR vs 3 in the original commit. Looks like a merge issue.

Copy link
Member

@phohensee phohensee left a comment

Choose a reason for hiding this comment

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

Somehow the last merge changed 494 files, please fix.

@apavlyutkin
Copy link
Contributor Author

apavlyutkin commented May 30, 2023

Somehow the last merge changed 494 files, please fix.

The request was initiated Mar 2, 2022. It's Ok

@TheRealMDoerr
Copy link
Contributor

It may be correct. We can see the actual patch here: https://patch-diff.githubusercontent.com/raw/openjdk/jdk11u-dev/pull/847.patch
The GitHub PR view is broken. We can't see what is being changed. Maybe it's possible to fix it by changing what exactly is supposed to get diffed.

@apavlyutkin
Copy link
Contributor Author

Ok, I'm raising new #1917 instead of this one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport ready Pull request is ready to be integrated rfr Pull request is ready for review
Development

Successfully merging this pull request may close these issues.