8200566: DistributionPointFetcher fails to fetch CRLs if the DistributionPoints field contains more than one DistributionPoint and the first one fails #18656

seanjmullan · 2024-04-05T13:17:21Z

Please review this change which fixes an issue in revocation checking of CRLs. A certificate's CRL Distribution Points extension can contain multiple Distribution Points (DPs), and each DP can contain one or more references to a CRL. These CRL references are typically specified as URLs.

If there is an issue fetching one of the CRLs (ex: a network error), the JDK implementation saves the exception, but continues to check for other CRLs, and if no other CRLs can be fetched, it throws the exception. This was working for the case in which multiple CRL references were in the same DP, but not if they were in separate DPs - in that case the exception was thrown immediately and no further CRLs were checked.

This also caused inconsistent behavior when the CRL cache was still fresh, as subsequent attempts would skip the CRL with the network issue (while the cache was fresh) and find the other CRLs, until the cache became stale again (30 seconds). The cache is working correctly though. The problem is that the code should continue to check for more CRLs.

A new test has been added which exercises both cases above.

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

JDK-8200566: DistributionPointFetcher fails to fetch CRLs if the DistributionPoints field contains more than one DistributionPoint and the first one fails (Bug - P4)

Reviewers

Weijun Wang (@wangweij - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18656

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-04-05T13:17:44Z

👋 Welcome back mullan! 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 · 2024-04-05T13:18:08Z

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

8200566: DistributionPointFetcher fails to fetch CRLs if the DistributionPoints field contains more than one DistributionPoint and the first one fails

Reviewed-by: weijun

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

3ebf8c9: 8329663: hs_err file event log entry for thread adding/removing should print current thread
be45de1: 8328627: JShell documentation should be clearer about "remote runtime system"
8648890: 8329749: Obsolete the unused UseNeon flag
fc18201: 8327111: Replace remaining usage of create_bool_from_template_assertion_predicate() which requires additional OpaqueLoop*Nodes transformation strategies
7c66465: 8325088: Overloads that differ in type parameters may be lost
6f087cb: 8328698: oopDesc::klass_raw() decodes without a null check
d1aad71: 8321204: C2: assert(false) failed: node should be in igvn hash table
51b0abc: 8329340: Remove unused libawt code
3a3b77d: 8329641: RISC-V: Enable some tests related to SHA-2 instrinsic
d771ec6: 8329733: Update the documentation in java.net.SocketOptions to direct to java.net.StandardSocketOptions
... and 32 more: https://git.openjdk.org/jdk/compare/b9da14012da5f1f72d4f6e690c18a43e87523173...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 · 2024-04-05T13:18:33Z

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

mlbridge · 2024-04-05T13:21:19Z

Webrevs

wangweij · 2024-04-05T13:59:41Z

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

+                        certStores, trustAnchors, validity, variant, anchor);
+                    results.addAll(crls);
+                } catch (CertStoreException cse) {
+                    savedCSE = cse;


Are you going to addSuppressed the exception if savedCSE is already not null here? Also, better to print out a debug info.

Yes, that would be a good idea to call addSupressed in case there is more than one exception. For debug, there is already a debug statement further down the call stack in either the URICertStore or LDAPCertStore code where the exception cause is captured so I didn't want to duplicate that info.

wangweij · 2024-04-05T14:16:47Z

test/jdk/java/security/cert/CertPathValidator/crlDP/CheckAllCRLs.java

+        X509Certificate eeCert2 = createCert(cb, "CN=End Entity",
+            rootKeyPair, eeKeyPair, rootCert, "SHA384withRSA", false, false);
+
+        // Create a CRL with no revoked certificates and store it in a file


I think the test is based on a fact that if both paths (HTTP and file) fail then validation would fail because there is no way to check for revocation. However, I have a slightest concern that what if it does not fail and everything goes on and validation succeeds. So, if the CRL is not empty and the test detects the cert is revoked it will be more reliable.

Yes, this is a good suggestion, as something could go undetected later on. I will update the CRL so that the certificate is revoked and then the test should always expect a failure with the proper reason.

wangweij

Thanks for addressing my feedback. The latest update seems good.

seanjmullan · 2024-04-08T20:07:17Z

/integrate

openjdk · 2024-04-08T20:08:59Z

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

7e5ef79: 8323116: [REDO] Computational test more than 2x slower when AVX instructions are used
9467720: 8329875: Serial: Move preservedMarks.inline.hpp to serialFullGC.cpp
a4dd2e9: 8329766: Serial: Refactor SerialBlockOffsetTable API
212a253: 8329623: NegativeArraySizeException encoding large String to UTF-8
dd930c5: 8329787: Fix typo in CLDRConverter
115f419: 8329659: Serial: Extract allowed_dead_ratio from ContiguousSpace
9ac3b77: 8329775: Serial: Remove unused declarations in serialFullGC.hpp
7475824: 8329510: Update ProblemList for JFileChooser/8194044/FileSystemRootTest.java
6439375: 8329533: TestCDSVMCrash fails on libgraal
3ebf8c9: 8329663: hs_err file event log entry for thread adding/removing should print current thread
... and 41 more: https://git.openjdk.org/jdk/compare/b9da14012da5f1f72d4f6e690c18a43e87523173...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-04-08T20:09:05Z

@seanjmullan Pushed as commit e702646.

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

seanjmullan added 3 commits April 4, 2024 08:44

Initial fix with test.

280f5dc

Merge

a30a5e7

Rename test.

b543830

openjdk bot added the rfr Pull request is ready for review label Apr 5, 2024

openjdk bot added the security security-dev@openjdk.org label Apr 5, 2024

Remove unnecessary module java.base/sun.security.provider.certpath.

7183875

wangweij reviewed Apr 5, 2024

View reviewed changes

seanjmullan added 2 commits April 8, 2024 08:34

Change test to check if cert is revoked.

e4fe67c

Add previous CertStoreExceptions to suppressed exceptions.

3ab9589

wangweij approved these changes Apr 8, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 8, 2024

openjdk bot added the integrated Pull request has been integrated label Apr 8, 2024

openjdk bot closed this Apr 8, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8200566: DistributionPointFetcher fails to fetch CRLs if the DistributionPoints field contains more than one DistributionPoint and the first one fails #18656

8200566: DistributionPointFetcher fails to fetch CRLs if the DistributionPoints field contains more than one DistributionPoint and the first one fails #18656

seanjmullan commented Apr 5, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Apr 5, 2024

openjdk bot commented Apr 5, 2024 •

edited

Loading

openjdk bot commented Apr 5, 2024

mlbridge bot commented Apr 5, 2024 •

edited

Loading

wangweij Apr 5, 2024

seanjmullan Apr 5, 2024

wangweij Apr 5, 2024

seanjmullan Apr 5, 2024

wangweij left a comment

seanjmullan commented Apr 8, 2024

openjdk bot commented Apr 8, 2024

openjdk bot commented Apr 8, 2024

8200566: DistributionPointFetcher fails to fetch CRLs if the DistributionPoints field contains more than one DistributionPoint and the first one fails #18656

8200566: DistributionPointFetcher fails to fetch CRLs if the DistributionPoints field contains more than one DistributionPoint and the first one fails #18656

Conversation

seanjmullan commented Apr 5, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Apr 5, 2024

openjdk bot commented Apr 5, 2024 • edited Loading

openjdk bot commented Apr 5, 2024

mlbridge bot commented Apr 5, 2024 • edited Loading

Webrevs

wangweij Apr 5, 2024

Choose a reason for hiding this comment

seanjmullan Apr 5, 2024

Choose a reason for hiding this comment

wangweij Apr 5, 2024

Choose a reason for hiding this comment

seanjmullan Apr 5, 2024

Choose a reason for hiding this comment

wangweij left a comment

Choose a reason for hiding this comment

seanjmullan commented Apr 8, 2024

openjdk bot commented Apr 8, 2024

openjdk bot commented Apr 8, 2024

seanjmullan commented Apr 5, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Apr 5, 2024 •

edited

Loading

mlbridge bot commented Apr 5, 2024 •

edited

Loading