From 28a19311f4bb4eda0ce2ccd0dfcaa2ec23f3a029 Mon Sep 17 00:00:00 2001 From: Artem Smotrakov Date: Thu, 4 Jun 2020 04:17:05 +0200 Subject: [PATCH] server: Enable revocation checking for uploaded certificates (#4065) This update turns on certificate revocation checking for uploaded certificates: - Updated `CertServiceImpl` to be able to enable revocation checking. - Introduced a new parameter `ENABLED_REVOCATION_CHECK` for `UploadSslCertCmd`. - Updated `CertServiceTest`. Even if no CLRs are specified via `PKIXParameters`, the certificates themselves may still provide info for revocation checking: - The AIA extension may contains a URL to the OCSP responder. - The CLRDP extension contains a URL to the CLR. Those extensions may need to be explicitly enabled by setting the system properties `com.sun.security.enableAIAcaIssuers` and `com.sun.security.enableCRLDP` to true. See [Java PKI Programmer's Guide](https://docs.oracle.com/en/java/javase/11/security/java-pki-programmers-guide.html). Using a revoked certificate may be dangerous. One of the most common reasons why a certificate authority (CA) revokes a certificate is that the private key has been compromised. For example, the private key might have been stolen by an adversary. If I understand correctly, the `CertServiceImpl` bean is used for operations with certificates on a load balancer. In particular, it validates a certificate chain without revocation checking while uploading a certificate. If a compromised revoked certificate is then used by the load balancer, then it may result to compromising TLS connections. However, the attacker has to be able to implement man-in-the-middle attack to compromise the connections. So the attacker has to be quite powerful. Therefore, such an attack is definitely not easy to implement. On the other hand, the impact may be significant because of loss of confidentiality. This has been discussed on security@cloudstack.apache.org --- .../apache/cloudstack/api/ApiConstants.java | 1 + .../user/loadbalancer/UploadSslCertCmd.java | 7 ++ .../network/ssl/CertServiceImpl.java | 12 ++-- .../network/ssl/CertServiceTest.java | 65 +++++++++++++++++++ 4 files changed, 79 insertions(+), 6 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java b/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java index 2a201dba196..932f62dfa29 100644 --- a/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java +++ b/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java @@ -55,6 +55,7 @@ public class ApiConstants { public static final String CERTIFICATE_CHAIN = "certchain"; public static final String CERTIFICATE_FINGERPRINT = "fingerprint"; public static final String CERTIFICATE_ID = "certid"; + public static final String ENABLED_REVOCATION_CHECK = "enabledrevocationcheck"; public static final String CONTROLLER = "controller"; public static final String CONTROLLER_UNIT = "controllerunit"; public static final String COPY_IMAGE_TAGS = "copyimagetags"; diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/loadbalancer/UploadSslCertCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/loadbalancer/UploadSslCertCmd.java index 309e43fcbbe..c206a165bc6 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/loadbalancer/UploadSslCertCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/loadbalancer/UploadSslCertCmd.java @@ -76,6 +76,9 @@ public class UploadSslCertCmd extends BaseCmd { @Parameter(name = ApiConstants.NAME , type = CommandType.STRING, required = true, description = "Name for the uploaded certificate") private String name; + @Parameter(name = ApiConstants.ENABLED_REVOCATION_CHECK, type = CommandType.BOOLEAN, description = "Enables revocation checking for certificates", since = "4.15") + private Boolean enabledRevocationCheck = Boolean.TRUE; + ///////////////////////////////////////////////////// /////////////////// Accessors /////////////////////// ///////////////////////////////////////////////////// @@ -112,6 +115,10 @@ public class UploadSslCertCmd extends BaseCmd { return name; } + public Boolean getEnabledRevocationCheck() { + return enabledRevocationCheck; + } + ///////////////////////////////////////////////////// /////////////// API Implementation/////////////////// ///////////////////////////////////////////////////// diff --git a/server/src/main/java/org/apache/cloudstack/network/ssl/CertServiceImpl.java b/server/src/main/java/org/apache/cloudstack/network/ssl/CertServiceImpl.java index c602475aac9..ae347488e4d 100644 --- a/server/src/main/java/org/apache/cloudstack/network/ssl/CertServiceImpl.java +++ b/server/src/main/java/org/apache/cloudstack/network/ssl/CertServiceImpl.java @@ -125,7 +125,7 @@ public class CertServiceImpl implements CertService { final String chain = certCmd.getChain(); final String name = certCmd.getName(); - validate(cert, key, password, chain); + validate(cert, key, password, chain, certCmd.getEnabledRevocationCheck()); s_logger.debug("Certificate Validation succeeded"); final String fingerPrint = CertificateHelper.generateFingerPrint(parseCertificate(cert)); @@ -278,7 +278,7 @@ public class CertServiceImpl implements CertService { return certResponseList; } - private void validate(final String certInput, final String keyInput, final String password, final String chainInput) { + private void validate(final String certInput, final String keyInput, final String password, final String chainInput, boolean revocationEnabled) { try { List chain = null; final Certificate cert = parseCertificate(certInput); @@ -292,7 +292,7 @@ public class CertServiceImpl implements CertService { validateKeys(cert.getPublicKey(), key); if (chainInput != null) { - validateChain(chain, cert); + validateChain(chain, cert, revocationEnabled); } } catch (final IOException | CertificateException e) { throw new IllegalStateException("Parsing certificate/key failed: " + e.getMessage(), e); @@ -388,7 +388,7 @@ public class CertServiceImpl implements CertService { } } - private void validateChain(final List chain, final Certificate cert) { + private void validateChain(final List chain, final Certificate cert, boolean revocationEnabled) { final List certs = new ArrayList(); final Set anchors = new HashSet(); @@ -410,7 +410,7 @@ public class CertServiceImpl implements CertService { PKIXBuilderParameters params = null; try { params = new PKIXBuilderParameters(anchors, target); - params.setRevocationEnabled(false); + params.setRevocationEnabled(revocationEnabled); params.addCertStore(CertStore.getInstance("Collection", new CollectionCertStoreParameters(certs))); final CertPathBuilder builder = CertPathBuilder.getInstance("PKIX", "BC"); builder.build(params); @@ -458,4 +458,4 @@ public class CertServiceImpl implements CertService { return certificateFactory.generateCertificate(bais); } -} \ No newline at end of file +} diff --git a/server/src/test/java/org/apache/cloudstack/network/ssl/CertServiceTest.java b/server/src/test/java/org/apache/cloudstack/network/ssl/CertServiceTest.java index a8514f915db..7008a81c0b5 100644 --- a/server/src/test/java/org/apache/cloudstack/network/ssl/CertServiceTest.java +++ b/server/src/test/java/org/apache/cloudstack/network/ssl/CertServiceTest.java @@ -140,9 +140,74 @@ public class CertServiceTest { chainField.setAccessible(true); chainField.set(uploadCmd, chain); + final Field enabledRevocationCheckField = klazz.getDeclaredField("enabledRevocationCheck"); + enabledRevocationCheckField.setAccessible(true); + enabledRevocationCheckField.set(uploadCmd, Boolean.FALSE); + certService.uploadSslCert(uploadCmd); } + @Test + /** + * Given a certificate signed by a CA and a valid CA chain, but without any info for revocation checking, upload should fail. + */ + public void runUploadSslCertWithNoRevocationInfo() throws Exception { + Assume.assumeTrue(isOpenJdk() || isJCEInstalled()); + + TransactionLegacy.open("runUploadSslCertWithCAChain"); + + final String certFile = URLDecoder.decode(getClass().getResource("/certs/rsa_ca_signed.crt").getFile(),Charset.defaultCharset().name()); + final String keyFile = URLDecoder.decode(getClass().getResource("/certs/rsa_ca_signed.key").getFile(),Charset.defaultCharset().name()); + final String chainFile = URLDecoder.decode(getClass().getResource("/certs/root_chain.crt").getFile(),Charset.defaultCharset().name()); + + final String cert = readFileToString(new File(certFile)); + final String key = readFileToString(new File(keyFile)); + final String chain = readFileToString(new File(chainFile)); + + final CertServiceImpl certService = new CertServiceImpl(); + + //setting mock objects + certService._accountMgr = Mockito.mock(AccountManager.class); + final Account account = new AccountVO("testaccount", 1, "networkdomain", (short)0, UUID.randomUUID().toString()); + when(certService._accountMgr.getAccount(anyLong())).thenReturn(account); + + certService._domainDao = Mockito.mock(DomainDao.class); + final DomainVO domain = new DomainVO("networkdomain", 1L, 1L, "networkdomain"); + when(certService._domainDao.findByIdIncludingRemoved(anyLong())).thenReturn(domain); + + certService._sslCertDao = Mockito.mock(SslCertDao.class); + when(certService._sslCertDao.persist(Matchers.any(SslCertVO.class))).thenReturn(new SslCertVO()); + + certService._accountDao = Mockito.mock(AccountDao.class); + when(certService._accountDao.findByIdIncludingRemoved(anyLong())).thenReturn((AccountVO)account); + + //creating the command + final UploadSslCertCmd uploadCmd = new UploadSslCertCmdExtn(); + final Class klazz = uploadCmd.getClass().getSuperclass(); + + final Field certField = klazz.getDeclaredField("cert"); + certField.setAccessible(true); + certField.set(uploadCmd, cert); + + final Field keyField = klazz.getDeclaredField("key"); + keyField.setAccessible(true); + keyField.set(uploadCmd, key); + + final Field chainField = klazz.getDeclaredField("chain"); + chainField.setAccessible(true); + chainField.set(uploadCmd, chain); + + try { + certService.uploadSslCert(uploadCmd); + } catch (Exception e) { + Assert.assertTrue(e.getMessage().contains("Invalid certificate chain")); + Throwable cause = e.getCause(); + Assert.assertTrue(cause.getMessage().contains("Certification path could not be validated")); + cause = cause.getCause(); + Assert.assertTrue(cause.getMessage().contains("No CRLs found")); + } + } + // @Test /** * Given a Self-signed Certificate with encrypted key, upload should succeed