This is an automated email from the ASF dual-hosted git repository.
rohit pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/cloudstack.git
The following commit(s) were added to refs/heads/master by this push:
new 28a1931 server: Enable revocation checking for uploaded certificates
(#4065)
28a1931 is described below
commit 28a19311f4bb4eda0ce2ccd0dfcaa2ec23f3a029
Author: Artem Smotrakov <[email protected]>
AuthorDate: Thu Jun 4 04:17:05 2020 +0200
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 power [...]
This has been discussed on [email protected]
---
.../org/apache/cloudstack/api/ApiConstants.java | 1 +
.../user/loadbalancer/UploadSslCertCmd.java | 7 +++
.../cloudstack/network/ssl/CertServiceImpl.java | 12 ++--
.../cloudstack/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 2a201db..932f62d 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 309e43f..c206a16 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 c602475..ae34748 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<Certificate> 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<Certificate> chain, final
Certificate cert) {
+ private void validateChain(final List<Certificate> chain, final
Certificate cert, boolean revocationEnabled) {
final List<Certificate> certs = new ArrayList<Certificate>();
final Set<TrustAnchor> anchors = new HashSet<TrustAnchor>();
@@ -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 a8514f9..7008a81 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
