7176627: CertPath/jep124/PreferCRL_SoftFail test fails (Could not determine revocation status)

Reviewed-by: xuelei

src/share/classes/sun/security/provider/certpath/CertStoreHelper.java

@@ -35,6 +35,7 @@ import java.security.InvalidAlgorithmParameterException;
 import java.security.PrivilegedActionException;
 import java.security.PrivilegedExceptionAction;
 import java.security.cert.CertStore;
+import java.security.cert.CertStoreException;
 import java.security.cert.X509CertSelector;
 import java.security.cert.X509CRLSelector;
 import javax.security.auth.x500.X500Principal;
@@ -96,6 +97,25 @@ public abstract class CertStoreHelper {
         }
     }
 
+    static boolean isCausedByNetworkIssue(String type, CertStoreException cse) {
+        switch (type) {
+            case "LDAP":
+            case "SSLServer":
+                try {
+                    CertStoreHelper csh = CertStoreHelper.getInstance(type);
+                    return csh.isCausedByNetworkIssue(cse);
+                } catch (NoSuchAlgorithmException nsae) {
+                    return false;
+                }
+            case "URI":
+                Throwable t = cse.getCause();
+                return (t != null && t instanceof IOException);
+            default:
+                // we don't know about any other remote CertStore types
+                return false;
+        }
+    }
+
     /**
      * Returns a CertStore using the given URI as parameters.
      */
@@ -119,4 +139,10 @@ public abstract class CertStoreHelper {
                          Collection<X500Principal> certIssuers,
                          String dn)
         throws IOException;
+
+    /**
+     * Returns true if the cause of the CertStoreException is a network
+     * related issue.
+     */
+    public abstract boolean isCausedByNetworkIssue(CertStoreException e);
 }

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

@@ -116,12 +116,17 @@ class DistributionPointFetcher {
     /**
      * Download CRLs from the given distribution point, verify and return them.
      * See the top of the class for current limitations.
+     *
+     * @throws CertStoreException if there is an error retrieving the CRLs
+     *         from one of the GeneralNames and no other CRLs are retrieved from
+     *         the other GeneralNames. If more than one GeneralName throws an
+     *         exception then the one from the last GeneralName is thrown.
      */
     private static Collection<X509CRL> getCRLs(X509CRLSelector selector,
         X509CertImpl certImpl, DistributionPoint point, boolean[] reasonsMask,
         boolean signFlag, PublicKey prevKey, String provider,
         List<CertStore> certStores, Set<TrustAnchor> trustAnchors,
-        Date validity) {
+        Date validity) throws CertStoreException {
 
         // check for full name
         GeneralNames fullName = point.getFullName();
@@ -149,24 +154,33 @@ class DistributionPointFetcher {
                 return Collections.emptySet();
             }
         }
-        Collection<X509CRL> possibleCRLs = new ArrayList<X509CRL>();
-        Collection<X509CRL> crls = new ArrayList<X509CRL>(2);
+        Collection<X509CRL> possibleCRLs = new ArrayList<>();
+        CertStoreException savedCSE = null;
         for (Iterator<GeneralName> t = fullName.iterator(); t.hasNext(); ) {
-            GeneralName name = t.next();
-            if (name.getType() == GeneralNameInterface.NAME_DIRECTORY) {
-                X500Name x500Name = (X500Name) name.getName();
-                possibleCRLs.addAll(
-                    getCRLs(x500Name, certImpl.getIssuerX500Principal(),
-                            certStores));
-            } else if (name.getType() == GeneralNameInterface.NAME_URI) {
-                URIName uriName = (URIName)name.getName();
-                X509CRL crl = getCRL(uriName);
-                if (crl != null) {
-                    possibleCRLs.add(crl);
+            try {
+                GeneralName name = t.next();
+                if (name.getType() == GeneralNameInterface.NAME_DIRECTORY) {
+                    X500Name x500Name = (X500Name) name.getName();
+                    possibleCRLs.addAll(
+                        getCRLs(x500Name, certImpl.getIssuerX500Principal(),
+                                certStores));
+                } else if (name.getType() == GeneralNameInterface.NAME_URI) {
+                    URIName uriName = (URIName)name.getName();
+                    X509CRL crl = getCRL(uriName);
+                    if (crl != null) {
+                        possibleCRLs.add(crl);
+                    }
                 }
+            } catch (CertStoreException cse) {
+                savedCSE = cse;
             }
         }
+        // only throw CertStoreException if no CRLs are retrieved
+        if (possibleCRLs.isEmpty() && savedCSE != null) {
+            throw savedCSE;
+        }
 
+        Collection<X509CRL> crls = new ArrayList<>(2);
         for (X509CRL crl : possibleCRLs) {
             try {
                 // make sure issuer is not set
@@ -191,34 +205,43 @@ class DistributionPointFetcher {
     /**
      * Download CRL from given URI.
      */
-    private static X509CRL getCRL(URIName name) {
+    private static X509CRL getCRL(URIName name) throws CertStoreException {
         URI uri = name.getURI();
         if (debug != null) {
             debug.println("Trying to fetch CRL from DP " + uri);
         }
+        CertStore ucs = null;
         try {
-            CertStore ucs = URICertStore.getInstance
+            ucs = URICertStore.getInstance
                 (new URICertStore.URICertStoreParameters(uri));
-            Collection<? extends CRL> crls = ucs.getCRLs(null);
-            if (crls.isEmpty()) {
-                return null;
-            } else {
-                return (X509CRL) crls.iterator().next();
-            }
-        } catch (Exception e) {
+        } catch (InvalidAlgorithmParameterException |
+                 NoSuchAlgorithmException e) {
             if (debug != null) {
-                debug.println("Exception getting CRL from CertStore: " + e);
-                e.printStackTrace();
+                debug.println("Can't create URICertStore: " + e.getMessage());
             }
+            return null;
+        }
+
+        Collection<? extends CRL> crls = ucs.getCRLs(null);
+        if (crls.isEmpty()) {
+            return null;
+        } else {
+            return (X509CRL) crls.iterator().next();
         }
-        return null;
     }
 
     /**
      * Fetch CRLs from certStores.
+     *
+     * @throws CertStoreException if there is an error retrieving the CRLs from
+     *         one of the CertStores and no other CRLs are retrieved from
+     *         the other CertStores. If more than one CertStore throws an
+     *         exception then the one from the last CertStore is thrown.
      */
     private static Collection<X509CRL> getCRLs(X500Name name,
-        X500Principal certIssuer, List<CertStore> certStores)
+                                               X500Principal certIssuer,
+                                               List<CertStore> certStores)
+        throws CertStoreException
     {
         if (debug != null) {
             debug.println("Trying to fetch CRL from DP " + name);
@@ -227,21 +250,27 @@ class DistributionPointFetcher {
         xcs.addIssuer(name.asX500Principal());
         xcs.addIssuer(certIssuer);
         Collection<X509CRL> crls = new ArrayList<>();
+        CertStoreException savedCSE = null;
         for (CertStore store : certStores) {
             try {
                 for (CRL crl : store.getCRLs(xcs)) {
                     crls.add((X509CRL)crl);
                 }
             } catch (CertStoreException cse) {
-                // don't add the CRL
                 if (debug != null) {
-                    debug.println("Non-fatal exception while retrieving " +
+                    debug.println("Exception while retrieving " +
                         "CRLs: " + cse);
                     cse.printStackTrace();
                 }
+                savedCSE = new PKIX.CertStoreTypeException(store.getType(),cse);
             }
         }
-        return crls;
+        // only throw CertStoreException if no CRLs are retrieved
+        if (crls.isEmpty() && savedCSE != null) {
+            throw savedCSE;
+        } else {
+            return crls;
+        }
     }
 
     /**

src/share/classes/sun/security/provider/certpath/ForwardBuilder.java

@@ -369,20 +369,21 @@ class ForwardBuilder extends Builder {
         boolean add = false;
         for (AccessDescription ad : adList) {
             CertStore cs = URICertStore.getInstance(ad);
-            try {
-                if (certs.addAll((Collection<X509Certificate>)
-                    cs.getCertificates(caSelector))) {
-                    add = true;
-                    if (!searchAllCertStores) {
-                        return true;
+            if (cs != null) {
+                try {
+                    if (certs.addAll((Collection<X509Certificate>)
+                        cs.getCertificates(caSelector))) {
+                        add = true;
+                        if (!searchAllCertStores) {
+                            return true;
+                        }
+                    }
+                } catch (CertStoreException cse) {
+                    if (debug != null) {
+                        debug.println("exception getting certs from CertStore:");
+                        cse.printStackTrace();
                     }
                 }
-            } catch (CertStoreException cse) {
-                if (debug != null) {
-                    debug.println("exception getting certs from CertStore:");
-                    cse.printStackTrace();
-                }
-                continue;
             }
         }
         return add;

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

@@ -335,8 +335,8 @@ public final class OCSP {
     static class NetworkFailureException extends CertPathValidatorException {
         private static final long serialVersionUID = 0l;
 
-        private NetworkFailureException(IOException ioe) {
-            super(ioe);
+        NetworkFailureException(Throwable t) {
+            super(t);
         }
 
         @Override

src/share/classes/sun/security/provider/certpath/PKIX.java

@@ -270,6 +270,24 @@ class PKIX {
         }
     }
 
+    /**
+     * A CertStoreException with additional information about the type of
+     * CertStore that generated the exception.
+     */
+    static class CertStoreTypeException extends CertStoreException {
+        private static final long serialVersionUID = 7463352639238322556L;
+
+        private final String type;
+
+        CertStoreTypeException(String type, CertStoreException cse) {
+            super(cse.getMessage(), cse.getCause());
+            this.type = type;
+        }
+        String getType() {
+            return type;
+        }
+    }
+
     /**
      * Comparator that orders CertStores so that local CertStores come before
      * remote CertStores.

src/share/classes/sun/security/provider/certpath/RevocationChecker.java

@@ -50,7 +50,7 @@ import java.util.Set;
 import javax.security.auth.x500.X500Principal;
 
 import static sun.security.provider.certpath.OCSP.*;
-import sun.security.provider.certpath.PKIX.ValidatorParams;
+import static sun.security.provider.certpath.PKIX.*;
 import sun.security.action.GetPropertyAction;
 import sun.security.x509.*;
 import static sun.security.x509.PKIXExtensions.*;
@@ -458,14 +458,23 @@ class RevocationChecker extends PKIXRevocationChecker {
         sel.setCertificateChecking(cert);
         CertPathHelper.setDateAndTime(sel, params.date(), MAX_CLOCK_SKEW);
 
-        // First, check cached CRLs
+        // First, check user-specified CertStores
+        NetworkFailureException nfe = null;
         for (CertStore store : certStores) {
             try {
                 for (CRL crl : store.getCRLs(sel)) {
                     possibleCRLs.add((X509CRL)crl);
                 }
             } catch (CertStoreException e) {
-                // XXX ignore?
+                if (debug != null) {
+                    debug.println("RevocationChecker.checkCRLs() " +
+                                  "CertStoreException: " + e.getMessage());
+                }
+                if (softFail && nfe == null &&
+                    CertStoreHelper.isCausedByNetworkIssue(store.getType(),e)) {
+                    // save this exception, we may need to throw it later
+                    nfe = new NetworkFailureException(e);
+                }
             }
         }
 
@@ -504,9 +513,12 @@ class RevocationChecker extends PKIXRevocationChecker {
                                         reasonsMask, anchors, params.date()));
                 }
             } catch (CertStoreException e) {
-                if (debug != null) {
-                    debug.println("RevocationChecker.checkCRLs() " +
-                                  "unexpected exception: " + e.getMessage());
+                if (softFail && e instanceof CertStoreTypeException) {
+                    CertStoreTypeException cste = (CertStoreTypeException)e;
+                    if (CertStoreHelper.isCausedByNetworkIssue(cste.getType(),
+                                                               e)) {
+                        throw new NetworkFailureException(e);
+                    }
                 }
                 throw new CertPathValidatorException(e);
             }
@@ -516,10 +528,28 @@ class RevocationChecker extends PKIXRevocationChecker {
                 checkApprovedCRLs(cert, approvedCRLs);
             } else {
                 if (allowSeparateKey) {
-                    verifyWithSeparateSigningKey(cert, prevKey, signFlag,
-                                                 stackedCerts);
-                    return;
+                    try {
+                        verifyWithSeparateSigningKey(cert, prevKey, signFlag,
+                                                     stackedCerts);
+                        return;
+                    } catch (CertPathValidatorException cpve) {
+                        if (nfe != null) {
+                            // if a network issue previously prevented us from
+                            // retrieving a CRL from one of the user-specified
+                            // CertStores and SOFT_FAIL is enabled, throw it now
+                            // so it can be handled appropriately
+                            throw nfe;
+                        }
+                        throw cpve;
+                    }
                 } else {
+                    if (nfe != null) {
+                        // if a network issue previously prevented us from
+                        // retrieving a CRL from one of the user-specified
+                        // CertStores and SOFT_FAIL is enabled, throw it now
+                        // so it can be handled appropriately
+                        throw nfe;
+                    }
                     throw new CertPathValidatorException
                     ("Could not determine revocation status", null, null, -1,
                      BasicReason.UNDETERMINED_REVOCATION_STATUS);

src/share/classes/sun/security/provider/certpath/URICertStore.java

@@ -340,7 +340,11 @@ class URICertStore extends CertStoreSpi {
             // Fetch the CRLs via LDAP. LDAPCertStore has its own
             // caching mechanism, see the class description for more info.
             // Safe cast since xsel is an X509 certificate selector.
-            return (Collection<X509CRL>) ldapCertStore.getCRLs(xsel);
+            try {
+                return (Collection<X509CRL>) ldapCertStore.getCRLs(xsel);
+            } catch (CertStoreException cse) {
+                throw new PKIX.CertStoreTypeException("LDAP", cse);
+            }
         }
 
         // Return the CRLs for this entry. It returns the cached value
@@ -391,11 +395,12 @@ class URICertStore extends CertStoreSpi {
                 debug.println("Exception fetching CRL:");
                 e.printStackTrace();
             }
+            // exception, forget previous values
+            lastModified = 0;
+            crl = null;
+            throw new PKIX.CertStoreTypeException("URI",
+                                                  new CertStoreException(e));
         }
-        // exception, forget previous values
-        lastModified = 0;
-        crl = null;
-        return Collections.emptyList();
     }
 
     /**

src/share/classes/sun/security/provider/certpath/ldap/LDAPCertStoreHelper.java

 /*
- * Copyright (c) 2009, 2011, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2009, 2012, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -25,15 +25,18 @@
 
 package sun.security.provider.certpath.ldap;
 
+import java.io.IOException;
 import java.net.URI;
 import java.util.Collection;
 import java.security.NoSuchAlgorithmException;
 import java.security.InvalidAlgorithmParameterException;
 import java.security.cert.CertStore;
+import java.security.cert.CertStoreException;
 import java.security.cert.X509CertSelector;
 import java.security.cert.X509CRLSelector;
+import javax.naming.CommunicationException;
+import javax.naming.ServiceUnavailableException;
 import javax.security.auth.x500.X500Principal;
-import java.io.IOException;
 
 import sun.security.provider.certpath.CertStoreHelper;
 
@@ -68,4 +71,11 @@ public final class LDAPCertStoreHelper
     {
         return new LDAPCertStore.LDAPCRLSelector(selector, certIssuers, ldapDN);
     }
+
+    @Override
+    public boolean isCausedByNetworkIssue(CertStoreException e) {
+        Throwable t = e.getCause();
+        return (t != null && (t instanceof ServiceUnavailableException ||
+                              t instanceof CommunicationException));
+    }
 }

src/share/classes/sun/security/provider/certpath/ssl/SSLServerCertStoreHelper.java

 /*
- * Copyright (c) 2011, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2011, 2012, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -25,15 +25,16 @@
 
 package sun.security.provider.certpath.ssl;
 
+import java.io.IOException;
 import java.net.URI;
-import java.util.Collection;
 import java.security.NoSuchAlgorithmException;
 import java.security.InvalidAlgorithmParameterException;
 import java.security.cert.CertStore;
+import java.security.cert.CertStoreException;
 import java.security.cert.X509CertSelector;
 import java.security.cert.X509CRLSelector;
+import java.util.Collection;
 import javax.security.auth.x500.X500Principal;
-import java.io.IOException;
 
 import sun.security.provider.certpath.CertStoreHelper;
 
@@ -66,4 +67,10 @@ public final class SSLServerCertStoreHelper extends CertStoreHelper {
     {
         throw new UnsupportedOperationException();
     }
+
+    @Override
+    public boolean isCausedByNetworkIssue(CertStoreException e) {
+        Throwable t = e.getCause();
+        return (t != null && t instanceof IOException);
+    }
 }