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

[Weijun Wang]

On Fri, 5 Apr 2024 13:48:24 GMT, Sean Mullan <mullan at openjdk.org> wrote:

>> 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.
>
> Sean Mullan has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Remove unnecessary module java.base/sun.security.provider.certpath.

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

> 114:                     results.addAll(crls);
> 115:                 } catch (CertStoreException cse) {
> 116:                     savedCSE = cse;

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

test/jdk/java/security/cert/CertPathValidator/crlDP/CheckAllCRLs.java line 95:

> 93:             rootKeyPair, eeKeyPair, rootCert, "SHA384withRSA", false, false);
> 94: 
> 95:         // 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.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/18656#discussion_r1553711914
PR Review Comment: https://git.openjdk.org/jdk/pull/18656#discussion_r1553739632