From aa796e6c6780375697a997cbf38e303b9c970cd8 Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Fri, 14 Jun 2024 15:16:14 +0530 Subject: [PATCH 1/4] Add certificate validation to check headers --- .../security/keystore/KeystoreManager.java | 3 +- .../keystore/KeystoreManagerImpl.java | 31 ++++++++++++++----- .../cloud/server/ManagementServerImpl.java | 7 +++-- 3 files changed, 30 insertions(+), 11 deletions(-) diff --git a/framework/security/src/main/java/org/apache/cloudstack/framework/security/keystore/KeystoreManager.java b/framework/security/src/main/java/org/apache/cloudstack/framework/security/keystore/KeystoreManager.java index c44347c85efe..18b840e7a8ca 100644 --- a/framework/security/src/main/java/org/apache/cloudstack/framework/security/keystore/KeystoreManager.java +++ b/framework/security/src/main/java/org/apache/cloudstack/framework/security/keystore/KeystoreManager.java @@ -18,6 +18,7 @@ import com.cloud.agent.api.LogLevel; import com.cloud.agent.api.LogLevel.Log4jLevel; +import com.cloud.utils.Pair; import com.cloud.utils.component.Manager; public interface KeystoreManager extends Manager { @@ -59,7 +60,7 @@ public String getRootCACert() { } } - boolean validateCertificate(String certificate, String key, String domainSuffix); + Pair validateCertificate(String certificate, String key, String domainSuffix); void saveCertificate(String name, String certificate, String key, String domainSuffix); diff --git a/framework/security/src/main/java/org/apache/cloudstack/framework/security/keystore/KeystoreManagerImpl.java b/framework/security/src/main/java/org/apache/cloudstack/framework/security/keystore/KeystoreManagerImpl.java index 03a91fecc8ef..21277b556ff4 100644 --- a/framework/security/src/main/java/org/apache/cloudstack/framework/security/keystore/KeystoreManagerImpl.java +++ b/framework/security/src/main/java/org/apache/cloudstack/framework/security/keystore/KeystoreManagerImpl.java @@ -30,6 +30,7 @@ import javax.inject.Inject; +import com.cloud.utils.Pair; import org.apache.commons.lang3.StringUtils; import org.apache.log4j.Logger; import org.springframework.stereotype.Component; @@ -47,10 +48,18 @@ public class KeystoreManagerImpl extends ManagerBase implements KeystoreManager private KeystoreDao _ksDao; @Override - public boolean validateCertificate(String certificate, String key, String domainSuffix) { + public Pair validateCertificate(String certificate, String key, String domainSuffix) { + String errMsg = null; if (StringUtils.isAnyEmpty(certificate, key, domainSuffix)) { - s_logger.error("Invalid parameter found in (certificate, key, domainSuffix) tuple for domain: " + domainSuffix); - return false; + errMsg = String.format("Invalid parameter found in (certificate, key, domainSuffix) tuple for domain: %s", domainSuffix); + s_logger.error(errMsg); + return new Pair<>(false, errMsg); + } + + if (!verifyCertificateHeaders(certificate)) { + errMsg = "Certificate headers verification failed: Invalid PEM format."; + s_logger.error(errMsg); + return new Pair<>(false, errMsg); } try { @@ -58,13 +67,19 @@ public boolean validateCertificate(String certificate, String key, String domain byte[] ksBits = CertificateHelper.buildAndSaveKeystore(domainSuffix, certificate, getKeyContent(key), ksPassword); KeyStore ks = CertificateHelper.loadKeystore(ksBits, ksPassword); if (ks != null) - return true; - - s_logger.error("Unabled to construct keystore for domain: " + domainSuffix); + return new Pair<>(true, errMsg); + errMsg = String.format("Unable to construct keystore for domain: %s", domainSuffix); + s_logger.error(errMsg); } catch (Exception e) { - s_logger.error("Certificate validation failed due to exception for domain: " + domainSuffix, e); + errMsg = String.format("Certificate validation failed due to exception for domain: %d", domainSuffix); + s_logger.error(errMsg, e); } - return false; + return new Pair<>(false, errMsg); + } + + private boolean verifyCertificateHeaders(String certificate) { + return certificate.startsWith("-----BEGIN CERTIFICATE-----") && + certificate.endsWith("-----END CERTIFICATE-----"); } @Override diff --git a/server/src/main/java/com/cloud/server/ManagementServerImpl.java b/server/src/main/java/com/cloud/server/ManagementServerImpl.java index 9b635cea5f34..015357ac0019 100644 --- a/server/src/main/java/com/cloud/server/ManagementServerImpl.java +++ b/server/src/main/java/com/cloud/server/ManagementServerImpl.java @@ -4484,8 +4484,11 @@ public String uploadCertificate(final UploadCustomCertificateCmd cmd) { final String certificate = cmd.getCertificate(); final String key = cmd.getPrivateKey(); - if (cmd.getPrivateKey() != null && !_ksMgr.validateCertificate(certificate, key, cmd.getDomainSuffix())) { - throw new InvalidParameterValueException("Failed to pass certificate validation check"); + if (key != null) { + Pair result = _ksMgr.validateCertificate(certificate, key, cmd.getDomainSuffix()); + if (!result.first()) { + throw new InvalidParameterValueException(String.format("Failed to pass certificate validation check with error: %s", result.second())); + } } if (cmd.getPrivateKey() != null) { From fb1f2a768f05de8dbdb9f1b2f116b50fac51575e Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Fri, 14 Jun 2024 17:08:48 +0530 Subject: [PATCH 2/4] log message corrections --- .../framework/security/keystore/KeystoreManagerImpl.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/framework/security/src/main/java/org/apache/cloudstack/framework/security/keystore/KeystoreManagerImpl.java b/framework/security/src/main/java/org/apache/cloudstack/framework/security/keystore/KeystoreManagerImpl.java index 21277b556ff4..a4c766a3c3d7 100644 --- a/framework/security/src/main/java/org/apache/cloudstack/framework/security/keystore/KeystoreManagerImpl.java +++ b/framework/security/src/main/java/org/apache/cloudstack/framework/security/keystore/KeystoreManagerImpl.java @@ -57,7 +57,7 @@ public Pair validateCertificate(String certificate, String key, } if (!verifyCertificateHeaders(certificate)) { - errMsg = "Certificate headers verification failed: Invalid PEM format."; + errMsg = "Certificate delimiters verification failed: Invalid PEM format."; s_logger.error(errMsg); return new Pair<>(false, errMsg); } @@ -71,7 +71,7 @@ public Pair validateCertificate(String certificate, String key, errMsg = String.format("Unable to construct keystore for domain: %s", domainSuffix); s_logger.error(errMsg); } catch (Exception e) { - errMsg = String.format("Certificate validation failed due to exception for domain: %d", domainSuffix); + errMsg = String.format("Certificate validation failed due to exception for domain: %s", domainSuffix); s_logger.error(errMsg, e); } return new Pair<>(false, errMsg); From aa13ec4810c17367d3045cae058a48a10a324786 Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Wed, 21 Aug 2024 18:08:18 +0530 Subject: [PATCH 3/4] improved validations --- .../security/keystore/KeystoreManagerImpl.java | 11 ----------- .../java/com/cloud/server/ManagementServerImpl.java | 11 +++++++++++ 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/framework/security/src/main/java/org/apache/cloudstack/framework/security/keystore/KeystoreManagerImpl.java b/framework/security/src/main/java/org/apache/cloudstack/framework/security/keystore/KeystoreManagerImpl.java index a4c766a3c3d7..68166b055928 100644 --- a/framework/security/src/main/java/org/apache/cloudstack/framework/security/keystore/KeystoreManagerImpl.java +++ b/framework/security/src/main/java/org/apache/cloudstack/framework/security/keystore/KeystoreManagerImpl.java @@ -56,12 +56,6 @@ public Pair validateCertificate(String certificate, String key, return new Pair<>(false, errMsg); } - if (!verifyCertificateHeaders(certificate)) { - errMsg = "Certificate delimiters verification failed: Invalid PEM format."; - s_logger.error(errMsg); - return new Pair<>(false, errMsg); - } - try { String ksPassword = "passwordForValidation"; byte[] ksBits = CertificateHelper.buildAndSaveKeystore(domainSuffix, certificate, getKeyContent(key), ksPassword); @@ -77,11 +71,6 @@ public Pair validateCertificate(String certificate, String key, return new Pair<>(false, errMsg); } - private boolean verifyCertificateHeaders(String certificate) { - return certificate.startsWith("-----BEGIN CERTIFICATE-----") && - certificate.endsWith("-----END CERTIFICATE-----"); - } - @Override public void saveCertificate(String name, String certificate, String key, String domainSuffix) { if (name == null || name.isEmpty() || certificate == null || certificate.isEmpty() || key == null || key.isEmpty() || domainSuffix == null || diff --git a/server/src/main/java/com/cloud/server/ManagementServerImpl.java b/server/src/main/java/com/cloud/server/ManagementServerImpl.java index 015357ac0019..3dbf15eb1b50 100644 --- a/server/src/main/java/com/cloud/server/ManagementServerImpl.java +++ b/server/src/main/java/com/cloud/server/ManagementServerImpl.java @@ -17,6 +17,7 @@ package com.cloud.server; import java.lang.reflect.Field; +import java.security.cert.CertificateException; import java.util.ArrayList; import java.util.Arrays; import java.util.Calendar; @@ -43,6 +44,7 @@ import javax.inject.Inject; import javax.naming.ConfigurationException; +import com.cloud.utils.security.CertificateHelper; import org.apache.cloudstack.acl.ControlledEntity; import org.apache.cloudstack.acl.SecurityChecker; import org.apache.cloudstack.affinity.AffinityGroupProcessor; @@ -4489,6 +4491,15 @@ public String uploadCertificate(final UploadCustomCertificateCmd cmd) { if (!result.first()) { throw new InvalidParameterValueException(String.format("Failed to pass certificate validation check with error: %s", result.second())); } + } else { + try { + s_logger.debug(String.format("Trying to validate the root certificate format")); + CertificateHelper.buildCertificate(certificate); + } catch (CertificateException e) { + String errorMsg = String.format("Failed to pass certificate validation check with error: Certificate validation failed due to exception: %s", e.getMessage()); + s_logger.error(errorMsg); + throw new InvalidParameterValueException(errorMsg); + } } if (cmd.getPrivateKey() != null) { From 25be98de82cf264a21ae7a01910a1b0c071334a6 Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Fri, 23 Aug 2024 15:40:58 +0530 Subject: [PATCH 4/4] Code refactoring --- .../keystore/KeystoreManagerImpl.java | 3 +- .../cloud/server/ManagementServerImpl.java | 37 +++++++++++-------- 2 files changed, 23 insertions(+), 17 deletions(-) diff --git a/framework/security/src/main/java/org/apache/cloudstack/framework/security/keystore/KeystoreManagerImpl.java b/framework/security/src/main/java/org/apache/cloudstack/framework/security/keystore/KeystoreManagerImpl.java index 68166b055928..f89b6eebf9b5 100644 --- a/framework/security/src/main/java/org/apache/cloudstack/framework/security/keystore/KeystoreManagerImpl.java +++ b/framework/security/src/main/java/org/apache/cloudstack/framework/security/keystore/KeystoreManagerImpl.java @@ -60,8 +60,9 @@ public Pair validateCertificate(String certificate, String key, String ksPassword = "passwordForValidation"; byte[] ksBits = CertificateHelper.buildAndSaveKeystore(domainSuffix, certificate, getKeyContent(key), ksPassword); KeyStore ks = CertificateHelper.loadKeystore(ksBits, ksPassword); - if (ks != null) + if (ks != null) { return new Pair<>(true, errMsg); + } errMsg = String.format("Unable to construct keystore for domain: %s", domainSuffix); s_logger.error(errMsg); } catch (Exception e) { diff --git a/server/src/main/java/com/cloud/server/ManagementServerImpl.java b/server/src/main/java/com/cloud/server/ManagementServerImpl.java index 3dbf15eb1b50..d91cd4effbb9 100644 --- a/server/src/main/java/com/cloud/server/ManagementServerImpl.java +++ b/server/src/main/java/com/cloud/server/ManagementServerImpl.java @@ -4485,25 +4485,12 @@ public String uploadCertificate(final UploadCustomCertificateCmd cmd) { final String certificate = cmd.getCertificate(); final String key = cmd.getPrivateKey(); + String domainSuffix = cmd.getDomainSuffix(); - if (key != null) { - Pair result = _ksMgr.validateCertificate(certificate, key, cmd.getDomainSuffix()); - if (!result.first()) { - throw new InvalidParameterValueException(String.format("Failed to pass certificate validation check with error: %s", result.second())); - } - } else { - try { - s_logger.debug(String.format("Trying to validate the root certificate format")); - CertificateHelper.buildCertificate(certificate); - } catch (CertificateException e) { - String errorMsg = String.format("Failed to pass certificate validation check with error: Certificate validation failed due to exception: %s", e.getMessage()); - s_logger.error(errorMsg); - throw new InvalidParameterValueException(errorMsg); - } - } + validateCertificate(certificate, key, domainSuffix); if (cmd.getPrivateKey() != null) { - _ksMgr.saveCertificate(ConsoleProxyManager.CERTIFICATE_NAME, certificate, key, cmd.getDomainSuffix()); + _ksMgr.saveCertificate(ConsoleProxyManager.CERTIFICATE_NAME, certificate, key, domainSuffix); // Reboot ssvm here since private key is present - meaning server cert being passed final List alreadyRunning = _secStorageVmDao.getSecStorageVmListInStates(null, State.Running, State.Migrating, State.Starting); @@ -4520,6 +4507,24 @@ public String uploadCertificate(final UploadCustomCertificateCmd cmd) { + "please give a few minutes for console access and storage services service to be up and working again"; } + private void validateCertificate(String certificate, String key, String domainSuffix) { + if (key != null) { + Pair result = _ksMgr.validateCertificate(certificate, key, domainSuffix); + if (!result.first()) { + throw new InvalidParameterValueException(String.format("Failed to pass certificate validation check with error: %s", result.second())); + } + } else { + try { + s_logger.debug(String.format("Trying to validate the root certificate format")); + CertificateHelper.buildCertificate(certificate); + } catch (CertificateException e) { + String errorMsg = String.format("Failed to pass certificate validation check with error: Certificate validation failed due to exception: %s", e.getMessage()); + s_logger.error(errorMsg); + throw new InvalidParameterValueException(errorMsg); + } + } + } + @Override public List getHypervisors(final Long zoneId) { final List result = new ArrayList();