From d844a37f96b873b01743c925ef39037b869b7800 Mon Sep 17 00:00:00 2001 From: Lucas Saldanha Date: Mon, 16 Aug 2021 20:02:20 +1200 Subject: [PATCH] Refactoring CmsValidator (internal CRL resolution) (#2635) * Refactoring CmsValidator (internal CRL resolution) Signed-off-by: Lucas Saldanha --- .../besu/pki/cms/CmsValidator.java | 42 ++++++++++++------- .../pki/cms/CmsCreationAndValidationTest.java | 39 +++++++++-------- 2 files changed, 49 insertions(+), 32 deletions(-) diff --git a/pki/src/main/java/org/hyperledger/besu/pki/cms/CmsValidator.java b/pki/src/main/java/org/hyperledger/besu/pki/cms/CmsValidator.java index a7001afe34..a260c40478 100644 --- a/pki/src/main/java/org/hyperledger/besu/pki/cms/CmsValidator.java +++ b/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 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( - CRLs -> { - params.addCertStore(CRLs); - PKIXRevocationChecker rc = (PKIXRevocationChecker) cpb.getRevocationChecker(); - rc.setOptions(EnumSet.of(Option.PREFER_CRLS)); - params.addCertPathChecker(rc); - }, - () -> { - LOGGER.warn("No CRL CertStore provided. CRL validation will be disabled."); - params.setRevocationEnabled(false); - }); + createCRLCertStore(truststore) + .ifPresentOrElse( + CRLs -> { + params.addCertStore(CRLs); + PKIXRevocationChecker rc = (PKIXRevocationChecker) cpb.getRevocationChecker(); + rc.setOptions(EnumSet.of(Option.PREFER_CRLS)); + params.addCertPathChecker(rc); + }, + () -> { + 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 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(); + } + } } diff --git a/pki/src/test/java/org/hyperledger/besu/pki/cms/CmsCreationAndValidationTest.java b/pki/src/test/java/org/hyperledger/besu/pki/cms/CmsCreationAndValidationTest.java index 8ea2ec38ba..6a82eb3d87 100644 --- a/pki/src/test/java/org/hyperledger/besu/pki/cms/CmsCreationAndValidationTest.java +++ b/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 KeyStoreWrapper truststoreWrapper; - private static CertStore CRLs; + private static KeyStore keystore; + private static KeyStore truststore; + private static List 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 = - CertStore.getInstance( - "Collection", - new CollectionCertStoreParameters( - Set.of(caCRL, intercaCRL, partnerACACRL, selfsignedCRL))); + CRLs = List.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();