Re: [PR] Add certificate validation to check headers [cloudstack]

[via GitHub]

harikrishna-patnala commented on code in PR #9255:
URL: https://github.com/apache/cloudstack/pull/9255#discussion_r1643897075


##########
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:
   You are right @DaanHoogland, buildCertificate() actually checking the 
certificate.
   
   Actually we do have that method call down the line in the same method, even 
without my changes certificate validation is happening, let me check with the 
issue owner about this.
   
   This error is without the PR changes
   
![image](https://github.com/apache/cloudstack/assets/3348673/db82cf57-5431-4ac8-826c-66dd6b1dd5c8)
   



-- 
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