Refactoring CmsValidator (internal CRL resolution) (#2635)

* Refactoring CmsValidator (internal CRL resolution)

Signed-off-by: Lucas Saldanha <lucascrsaldanha@gmail.com>

pki/src/main/java/org/hyperledger/besu/pki/cms/CmsValidator.java

@@ -20,6 +20,7 @@ import java.security.Security;
 import java.security.cert.CertPathBuilder;
 import java.security.cert.CertPathBuilderException;
 import java.security.cert.CertStore;
+import java.security.cert.CollectionCertStoreParameters;
 import java.security.cert.PKIXBuilderParameters;
 import java.security.cert.PKIXRevocationChecker;
 import java.security.cert.PKIXRevocationChecker.Option;
@@ -54,11 +55,9 @@ public class CmsValidator {
   private static final Logger LOGGER = LogManager.getLogger();
 
   private final KeyStoreWrapper truststore;
-  private final Optional<CertStore> crlCertStore;
 
-  public CmsValidator(final KeyStoreWrapper truststore, final CertStore crlCertStore) {
+  public CmsValidator(final KeyStoreWrapper truststore) {
     this.truststore = truststore;
-    this.crlCertStore = Optional.ofNullable(crlCertStore);
   }
 
   /**
@@ -146,17 +145,18 @@ public class CmsValidator {
           new PKIXBuilderParameters(truststore.getKeyStore(), targetConstraints);
 
       // Adding CertStore with CRLs (if present, otherwise disabling revocation check)
-      crlCertStore.ifPresentOrElse(
+      createCRLCertStore(truststore)
-          CRLs -> {
+          .ifPresentOrElse(
-            params.addCertStore(CRLs);
+              CRLs -> {
-            PKIXRevocationChecker rc = (PKIXRevocationChecker) cpb.getRevocationChecker();
+                params.addCertStore(CRLs);
-            rc.setOptions(EnumSet.of(Option.PREFER_CRLS));
+                PKIXRevocationChecker rc = (PKIXRevocationChecker) cpb.getRevocationChecker();
-            params.addCertPathChecker(rc);
+                rc.setOptions(EnumSet.of(Option.PREFER_CRLS));
-          },
+                params.addCertPathChecker(rc);
-          () -> {
+              },
-            LOGGER.warn("No CRL CertStore provided. CRL validation will be disabled.");
+              () -> {
-            params.setRevocationEnabled(false);
+                LOGGER.warn("No CRL CertStore provided. CRL validation will be disabled.");
-          });
+                params.setRevocationEnabled(false);
+              });
 
       // Read certificates sent on the CMS message and adding it to the path building algorithm
       final CertStore cmsCertificates =
@@ -178,4 +178,18 @@ public class CmsValidator {
       throw new RuntimeException("Error validating certificate chain", e);
     }
   }
+
+  private Optional<CertStore> createCRLCertStore(final KeyStoreWrapper truststore) {
+    if (truststore.getCRLs() != null) {
+      try {
+        return Optional.of(
+            CertStore.getInstance(
+                "Collection", new CollectionCertStoreParameters(truststore.getCRLs())));
+      } catch (final Exception e) {
+        throw new RuntimeException("Error loading CRLs from Truststore", e);
+      }
+    } else {
+      return Optional.empty();
+    }
+  }
 }

pki/src/test/java/org/hyperledger/besu/pki/cms/CmsCreationAndValidationTest.java

@@ -20,6 +20,8 @@ import static org.hyperledger.besu.pki.util.TestCertificateUtils.createCRL;
 import static org.hyperledger.besu.pki.util.TestCertificateUtils.createKeyPair;
 import static org.hyperledger.besu.pki.util.TestCertificateUtils.createSelfSignedCertificate;
 import static org.hyperledger.besu.pki.util.TestCertificateUtils.issueCertificate;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.when;
 
 import org.hyperledger.besu.pki.keystore.KeyStoreWrapper;
 import org.hyperledger.besu.pki.keystore.SoftwareKeyStoreWrapper;
@@ -27,14 +29,13 @@ import org.hyperledger.besu.pki.keystore.SoftwareKeyStoreWrapper;
 import java.security.KeyPair;
 import java.security.KeyStore;
 import java.security.PrivateKey;
-import java.security.cert.CertStore;
 import java.security.cert.Certificate;
-import java.security.cert.CollectionCertStoreParameters;
 import java.security.cert.X509CRL;
 import java.security.cert.X509Certificate;
 import java.time.Instant;
 import java.time.temporal.ChronoUnit;
 import java.util.Collections;
+import java.util.List;
 import java.util.Set;
 
 import org.apache.tuweni.bytes.Bytes;
@@ -54,10 +55,12 @@ import org.junit.Test;
 
 public class CmsCreationAndValidationTest {
 
-  private static KeyStoreWrapper keystoreWrapper;
+  private static KeyStore keystore;
-  private static KeyStoreWrapper truststoreWrapper;
+  private static KeyStore truststore;
-  private static CertStore CRLs;
+  private static List<X509CRL> CRLs;
 
+  private KeyStoreWrapper keystoreWrapper;
+  private KeyStoreWrapper truststoreWrapper;
   private CmsValidator cmsValidator;
 
   @BeforeClass
@@ -150,19 +153,17 @@ public class CmsCreationAndValidationTest {
     /*
      Create truststore wrapper with 3 trusted certificates: 'ca', 'interca' and 'selfsigned'
     */
-    final KeyStore truststore = KeyStore.getInstance("PKCS12");
+    truststore = KeyStore.getInstance("PKCS12");
     truststore.load(null, null);
 
     truststore.setCertificateEntry("ca", caCertificate);
     truststore.setCertificateEntry("interca", interCACertificate);
     truststore.setCertificateEntry("selfsigned", selfsignedCertificate);
 
-    truststoreWrapper = new SoftwareKeyStoreWrapper(truststore, "");
-
     /*
      Create keystore with certificates used in tests
     */
-    final KeyStore keystore = KeyStore.getInstance("PKCS12");
+    keystore = KeyStore.getInstance("PKCS12");
     keystore.load(null, null);
 
     keystore.setKeyEntry(
@@ -195,7 +196,6 @@ public class CmsCreationAndValidationTest {
         untrustedIntermediateKeyPair.getPrivate(),
         "".toCharArray(),
         new Certificate[] {untrustedIntermediateCertificate, untrustedSelfsignedCertificate});
-    keystoreWrapper = new SoftwareKeyStoreWrapper(keystore, "");
 
     /*
      Create CRLs for all CA certificates (mostly empty, only ca has one revoked certificate)
@@ -207,17 +207,17 @@ public class CmsCreationAndValidationTest {
         createCRL(partnerACACertificate, partnerACAPair, Collections.emptyList());
     final X509CRL selfsignedCRL =
         createCRL(selfsignedCertificate, selfsignedKeyPair, Collections.emptyList());
-
+    CRLs = List.of(caCRL, intercaCRL, partnerACACRL, selfsignedCRL);
-    CRLs =
-        CertStore.getInstance(
-            "Collection",
-            new CollectionCertStoreParameters(
-                Set.of(caCRL, intercaCRL, partnerACACRL, selfsignedCRL)));
   }
 
   @Before
   public void before() {
-    cmsValidator = new CmsValidator(truststoreWrapper, CRLs);
+    keystoreWrapper = new SoftwareKeyStoreWrapper(keystore, "");
+
+    truststoreWrapper = spy(new SoftwareKeyStoreWrapper(truststore, ""));
+    when(truststoreWrapper.getCRLs()).thenReturn(CRLs);
+
+    cmsValidator = new CmsValidator(truststoreWrapper);
   }
 
   @Test
@@ -285,10 +285,13 @@ public class CmsCreationAndValidationTest {
     final CmsCreator cmsCreator = new CmsCreator(keystoreWrapper, "revoked");
     final Bytes data = Bytes.random(32);
 
+    // Removing CRLs
+    when(truststoreWrapper.getCRLs()).thenReturn(null);
+
     final Bytes cms = cmsCreator.create(data);
 
     // Overriding validator with instance without CRL CertStore
-    cmsValidator = new CmsValidator(truststoreWrapper, null);
+    cmsValidator = new CmsValidator(truststoreWrapper);
 
     // Because we don't have a CRL CertStore, revocation is not checked
     assertThat(cmsValidator.validate(cms, data)).isTrue();