Skip to content

8179502: Enhance OCSP, CRL and Certificate Fetch Timeouts #2747

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

Closed
wants to merge 3 commits into from

Conversation

alexeybakhtin
Copy link

@alexeybakhtin alexeybakhtin commented Jul 28, 2024

Hello, I'd like to backport JDK-8179502 to JDK17u to improve the timeout adjustment for OCSP GET requests (which was missed in JDK-8179503).

The backport is almost clean except for the following:

  • OCSP.java was merged manually because of JDK-8328638 and JDK-8329213 is already backported into 17u-dev
  • copyright year in GetPropertyAction.java and URICertStore.java files are updated manually
  • CRLReadTimeout.java test is updated manually because of the different notation of internal X509CRLImpl and CRLExtensions classes.

All new and related jtreg tests are passed


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
  • Change requires CSR request JDK-8337407 to be approved
  • JDK-8179502 needs maintainer approval

Issues

  • JDK-8179502: Enhance OCSP, CRL and Certificate Fetch Timeouts (Enhancement - P4 - Approved)
  • JDK-8337407: Enhance OCSP, CRL and Certificate Fetch Timeouts (CSR)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2747

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

Using diff file

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

Using Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Jul 28, 2024

👋 Welcome back abakhtin! 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 bot commented Jul 28, 2024

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

8179502: Enhance OCSP, CRL and Certificate Fetch Timeouts

Reviewed-by: yan

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 466 new commits pushed to 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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot changed the title Backport 2836c34b64e4626e25c86a53e5bef2bf32f95d2e 8179502: Enhance OCSP, CRL and Certificate Fetch Timeouts Jul 28, 2024
@openjdk
Copy link

openjdk bot commented Jul 28, 2024

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

@openjdk openjdk bot added the backport label Jul 28, 2024
@openjdk
Copy link

openjdk bot commented Jul 28, 2024

At least one of the issues associated with this backport has a resolved CSR for a different version. As this means that this backport may also need a CSR, the csr label is being added to this pull request to signal this potential requirement. The command /csr unneeded can be used to remove the label in case a CSR is not needed.

@openjdk openjdk bot added csr Pull request needs approved CSR before integration rfr Pull request is ready for review labels Jul 28, 2024
@mlbridge
Copy link

mlbridge bot commented Jul 28, 2024

Webrevs

@alexeybakhtin
Copy link
Author

alexeybakhtin commented Jul 29, 2024

CSR JDK-8337407 for JDK17 is created

@bridgekeeper
Copy link

bridgekeeper bot commented Aug 27, 2024

@alexeybakhtin This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@HempushpaSahu
Copy link

Hi @alexeybakhtin
Is there any plan to merge this PR anytime soon?

Thanks.

@alexeybakhtin
Copy link
Author

alexeybakhtin commented Sep 4, 2024

Hi @HempushpaSahu. Yes, I will request integration into 17u as soon as it is reviewed here.

@HempushpaSahu
Copy link

Hi @alexeybakhtin,
I noticed from JDK-8337407 issue, jnimeh is the reviewer.
Could you please confirm whether the review is currently in progress?
Also, is it possible to assign an additional reviewer if one is available?

Thanks.

@alexeybakhtin
Copy link
Author

alexeybakhtin commented Sep 17, 2024

Hello everyone,
Could you please review the backport? I want to backport it for parity with Oracle.

@HempushpaSahu
Copy link

Hi,
Since there is multiple follow-ups for this PR and customer is awaiting the fix, could someone please provide an update on the review status?

Thanks.

@HempushpaSahu
Copy link

Hi,
Could someone please review the PR?

Thanks.

@HempushpaSahu
Copy link

HempushpaSahu commented Oct 7, 2024

Hi @jnimeh ,
Could you please assist with reviewing the PR or if you are occupied with other tasks please tag the appropriate person who can help us to review?
The customer is waiting for the fix. Once this backport is merged, they will be able to move forward.

Thanks.

@jnimeh
Copy link
Member

jnimeh commented Oct 7, 2024

Hello @HempushpaSahu. I can review the CSR. I think you need someone who has reviewer status in the jdk-updates project in order to be able to commit this. I only have committer status there. You should be able to find many folks with jdk-updates reviewer status on the OpenJDK census page though.

@jnimeh
Copy link
Member

jnimeh commented Oct 7, 2024

I would also suggest looking at incorporating JDK-8309740 and JDK-8309754 in follow-on integrations as they pertain to the tests in this PR.

@HempushpaSahu
Copy link

Hi @seanjmullan ,
Could you please help us here to review the PR?

@HempushpaSahu
Copy link

Hello @HempushpaSahu. I can review the CSR. I think you need someone who has reviewer status in the jdk-updates project in order to be able to commit this. I only have committer status there. You should be able to find many folks with jdk-updates reviewer status on the OpenJDK census page though.

Thanks @jnimeh for your inputs.

@HempushpaSahu
Copy link

Hi @GoeLin
Could you please help to review this PR or tag the appropriate person who can help us to review?
Thanks.

@HempushpaSahu
Copy link

Hi @GoeLin
Could you please help to review this PR?
Thanks.

@HempushpaSahu
Copy link

Hi @alexeybakhtin,
JDK-8337407 issue has some activity in last week.
Could you please confirm whether the review is currently in progress?

Thanks.

@openjdk openjdk bot removed the csr Pull request needs approved CSR before integration label Oct 18, 2024
@alexeybakhtin
Copy link
Author

alexeybakhtin commented Oct 18, 2024

CSR is approved.
Please review the PR.

@openjdk openjdk bot added the csr Pull request needs approved CSR before integration label Nov 13, 2024
@HempushpaSahu
Copy link

Hi @alexeybakhtin , I have tested the four backports mentioned above together, and they have passed successfully. Should we include these tests as part of the PR?
Thanks.

@alexeybakhtin
Copy link
Author

alexeybakhtin commented Nov 18, 2024

@franferrax , @HempushpaSahu, Thank you!
I do not think it is possible to add other bug fixes to this PR, so I submitted dependent backport PRs for the mentioned test fixes:

Backports are clean, so no review is required

@franferrax
Copy link
Contributor

Hi @alexeybakhtin, thank you for the additional time and effort put into this! My intention is to help move things faster, because I've been made aware about customers waiting for this.

However, as @GoeLin explained, if Oracle doesn't include this backport in 17.0.14, the documentation won't be updated, so we'll need to wait for them to proceed. Apparently, Oracle's reason for not doing the backport is its low priority. Customers are now trying to get it prioritized through Oracle support, but it looks like we won't make it for the 17.0.14 rampdown date (December 3).

NOTE: FYI, AFAIK, you can add multiple backports to a pull requiest with the /issue add <id>[,<id>,...] command.

@alexeybakhtin
Copy link
Author

alexeybakhtin commented Nov 19, 2024

Hi @franferrax,
Thank you for your support.
We also have customers who are waiting for this enhancement.

About /issue command - I do not like this approach much. It will bring a much more difference between the backport and the original fix. Right now, all test fixes are applied cleanly. But, if it would help to integrate this enhancement, I can combine all follow-up backports into this one.

@bridgekeeper bridgekeeper bot added the oca Needs verification of OCA signatory status label Nov 20, 2024
@openjdk openjdk bot removed the rfr Pull request is ready for review label Nov 20, 2024
@bridgekeeper bridgekeeper bot removed the oca Needs verification of OCA signatory status label Nov 20, 2024
@openjdk openjdk bot added the rfr Pull request is ready for review label Nov 20, 2024
@bridgekeeper
Copy link

bridgekeeper bot commented Dec 18, 2024

@alexeybakhtin This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@alexeybakhtin
Copy link
Author

/csr 8337407

@openjdk
Copy link

openjdk bot commented Dec 24, 2024

@alexeybakhtin usage: /csr [needed|unneeded], requires that the issue the pull request refers to links to an approved CSR request.

@openjdk openjdk bot removed the csr Pull request needs approved CSR before integration label Jan 8, 2025
@franferrax
Copy link
Contributor

/csr 8337407

@alexeybakhtin: looks like linking the right CSR (JDK-8337407) from the Oracle backport (JDK-8338808) did the trick.

Also, I think "parity with Oracle" is now a valid reason for maintainer approval, since jdk17u-dev is in the development phase of 17.0.15, the same version for which Oracle did the backport.

@GoeLin
Copy link
Member

GoeLin commented Jan 10, 2025

This change looks good to me now. Thanks for improving the compatibility with 17!
Unfortunately Oracle did not integrate this yet. I'm not sure whether we should go ahead with this already.

@jerboaa
Copy link
Contributor

jerboaa commented Jan 10, 2025

Unfortunately Oracle did not integrate this yet. I'm not sure whether we should go ahead with this already.

+1 on bringing this to JDK 17 in April. There is ample time until that releases.

@alexeybakhtin
Copy link
Author

@openjdk openjdk bot added ready Pull request is ready to be integrated and removed approval labels Jan 17, 2025
@alexeybakhtin
Copy link
Author

/integrate

@openjdk
Copy link

openjdk bot commented Jan 17, 2025

Going to push as commit 25c7a7b.
Since your change was applied there have been 466 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

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

openjdk bot commented Jan 17, 2025

@alexeybakhtin Pushed as commit 25c7a7b.

💡 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
backport integrated Pull request has been integrated
Development

Successfully merging this pull request may close these issues.

7 participants