DaanHoogland commented on code in PR #9255:
URL: https://github.com/apache/cloudstack/pull/9255#discussion_r1639740153
##########
framework/security/src/main/java/org/apache/cloudstack/framework/security/keystore/KeystoreManagerImpl.java:
##########
@@ -47,24 +48,38 @@ public class KeystoreManagerImpl extends ManagerBase
implements KeystoreManager
private KeystoreDao _ksDao;
@Override
- public boolean validateCertificate(String certificate, String key, String
domainSuffix) {
+ public Pair<Boolean, String> 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 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);
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: %s", 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-----");
}
Review Comment:
I think we could re-use the code from `CertificateHelper#buildCertificate()`
here to do more validation then just the header/footer. I.E. make sure it is a
valid X.509 certificate.
That said as is it is already an improvement.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
