Skip to content

Add certificate validation to check headers #9255

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
DaanHoogland merged 4 commits into from
Aug 28, 2024
Merged

Add certificate validation to check headers #9255

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
aa796e6
Add certificate validation to check headers
harikrishna-patnala Jun 14, 2024
fb1f2a7
log message corrections
harikrishna-patnala Jun 14, 2024
aa13ec4
improved validations
harikrishna-patnala Aug 21, 2024
25be98d
Code refactoring
harikrishna-patnala Aug 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion ...rity/src/main/java/org/apache/cloudstack/framework/security/keystore/KeystoreManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -59,7 +60,7 @@ public String getRootCACert() {
}
}

boolean validateCertificate(String certificate, String key, String domainSuffix);
Pair<Boolean, String> validateCertificate(String certificate, String key, String domainSuffix);

void saveCertificate(String name, String certificate, String key, String domainSuffix);

Expand Down
31 changes: 23 additions & 8 deletions .../src/main/java/org/apache/cloudstack/framework/security/keystore/KeystoreManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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);
Comment thread
DaanHoogland marked this conversation as resolved.
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-----");
}
Copy link
Copy Markdown
Contributor

@DaanHoogland DaanHoogland Jun 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

@harikrishna-patnala harikrishna-patnala Jun 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

@harikrishna-patnala harikrishna-patnala Aug 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated the PR based on your comments @DaanHoogland this now validates the root certificates as mentioned by the ticket reporter.


@Override
Expand Down
7 changes: 5 additions & 2 deletions server/src/main/java/com/cloud/server/ManagementServerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Boolean, String> 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) {
Expand Down