Skip to content

Commit 666846f

Browse files
vishesh92vishesh92
committed
Fix issues due to malformed or bad input
1 parent 5d8e797 commit 666846f

2 files changed

Lines changed: 119 additions & 21 deletions

File tree

‎plugins/ca/root-ca/src/main/java/org/apache/cloudstack/ca/provider/RootCAProvider.java‎

Lines changed: 62 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,7 @@
5959
import org.apache.cloudstack.framework.ca.Certificate;
6060
import org.apache.cloudstack.framework.config.ConfigKey;
6161
import org.apache.cloudstack.framework.config.Configurable;
62+
import org.apache.cloudstack.framework.config.ValidatedConfigKey;
6263
import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
6364
import org.apache.cloudstack.utils.security.CertUtils;
6465
import org.apache.cloudstack.utils.security.KeyStoreUtils;
@@ -103,25 +104,25 @@ public final class RootCAProvider extends AdapterBase implements CAProvider, Con
103104
/////////////// Root CA Settings ///////////////////
104105
////////////////////////////////////////////////////
105106

106-
private static ConfigKey<String> rootCAPrivateKey = new ConfigKey<>("Hidden", String.class,
107-
"ca.plugin.root.private.key",
108-
null,
107+
private static ConfigKey<String> rootCAPrivateKey = new ValidatedConfigKey<>("Hidden", String.class,
108+
"ca.plugin.root.private.key", null,
109109
"The ROOT CA private key in PEM format. " +
110110
"When set along with the public key and certificate, CloudStack uses this custom CA instead of auto-generating one. " +
111-
"All three ca.plugin.root.* keys must be set together. Restart management server(s) when changed.", false);
111+
"All three ca.plugin.root.* keys must be set together. Restart management server(s) when changed.",
112+
false, ConfigKey.Scope.Global, null, RootCAProvider::validatePrivateKeyPem);
112113

113-
private static ConfigKey<String> rootCAPublicKey = new ConfigKey<>("Hidden", String.class,
114-
"ca.plugin.root.public.key",
115-
null,
114+
private static ConfigKey<String> rootCAPublicKey = new ValidatedConfigKey<>("Hidden", String.class,
115+
"ca.plugin.root.public.key", null,
116116
"The ROOT CA public key in PEM format (X.509/SPKI: must start with '-----BEGIN PUBLIC KEY-----'). " +
117-
"Required when providing a custom CA. Restart management server(s) when changed.", false);
117+
"Required when providing a custom CA. Restart management server(s) when changed.",
118+
false, ConfigKey.Scope.Global, null, RootCAProvider::validatePublicKeyPem);
118119

119-
private static ConfigKey<String> rootCACertificate = new ConfigKey<>("Hidden", String.class,
120-
"ca.plugin.root.ca.certificate",
121-
null,
120+
private static ConfigKey<String> rootCACertificate = new ValidatedConfigKey<>("Hidden", String.class,
121+
"ca.plugin.root.ca.certificate", null,
122122
"The CA certificate(s) in PEM format (must start with '-----BEGIN CERTIFICATE-----'). " +
123123
"For intermediate CAs, concatenate the signing cert first, followed by intermediate(s) and root. " +
124-
"Required when providing a custom CA. Restart management server(s) when changed.", false);
124+
"Required when providing a custom CA. Restart management server(s) when changed.",
125+
false, ConfigKey.Scope.Global, null, RootCAProvider::validateCACertificatePem);
125126

126127
private static ConfigKey<String> rootCAIssuerDN = new ConfigKey<>("Advanced", String.class,
127128
"ca.plugin.root.issuer.dn",
@@ -333,18 +334,20 @@ private boolean saveNewRootCACertificate() {
333334
}
334335
try {
335336
logger.debug("Generating root CA certificate");
336-
final X509Certificate rootCaCertificate = CertUtils.generateV3Certificate(
337+
final X509Certificate generatedCACert = CertUtils.generateV3Certificate(
337338
null, caKeyPair, caKeyPair.getPublic(),
338339
rootCAIssuerDN.value(), CAManager.CertSignatureAlgorithm.value(),
339340
getCaValidityDays(), null, null);
340-
if (!configDao.update(rootCACertificate.key(), rootCACertificate.category(), CertUtils.x509CertificateToPem(rootCaCertificate))) {
341+
if (!configDao.update(rootCACertificate.key(), rootCACertificate.category(), CertUtils.x509CertificateToPem(generatedCACert))) {
341342
logger.error("Failed to update RootCA public/x509 certificate");
342343
}
344+
caCertificates = new ArrayList<>(java.util.Collections.singletonList(generatedCACert));
345+
caCertificate = generatedCACert;
343346
} catch (final CertificateException | NoSuchAlgorithmException | NoSuchProviderException | SignatureException | InvalidKeyException | OperatorCreationException | IOException e) {
344347
logger.error("Failed to generate RootCA certificate from private/public keys due to exception:", e);
345348
return false;
346349
}
347-
return loadRootCACertificate();
350+
return caCertificate != null;
348351
}
349352

350353
private boolean loadRootCAKeyPair() {
@@ -361,26 +364,30 @@ private boolean loadRootCAKeyPair() {
361364
}
362365

363366
private boolean loadRootCACertificate() {
367+
caCertificate = null;
368+
caCertificates = null;
364369
if (StringUtils.isEmpty(rootCACertificate.value())) {
365370
return false;
366371
}
367372
try {
368-
caCertificates = CertUtils.pemToX509Certificates(rootCACertificate.value());
369-
if (CollectionUtils.isEmpty(caCertificates)) {
373+
final List<X509Certificate> loadedCerts = CertUtils.pemToX509Certificates(rootCACertificate.value());
374+
if (CollectionUtils.isEmpty(loadedCerts)) {
370375
logger.error("No certificates found in ca.plugin.root.ca.certificate");
371376
return false;
372377
}
373-
caCertificate = caCertificates.get(0);
378+
final X509Certificate loadedCACert = loadedCerts.get(0);
374379

375380
// Verify key ownership without enforcing self-signature
376-
if (!caCertificate.getPublicKey().equals(caKeyPair.getPublic())) {
381+
if (!loadedCACert.getPublicKey().equals(caKeyPair.getPublic())) {
377382
logger.error("The public key in the CA certificate does not match the configured CA public key");
378383
return false;
379384
}
380385

381-
if (caCertificates.size() > 1) {
382-
logger.info("Loaded CA certificate chain with {} certificate(s)", caCertificates.size());
386+
if (loadedCerts.size() > 1) {
387+
logger.info("Loaded CA certificate chain with {} certificate(s)", loadedCerts.size());
383388
}
389+
caCertificates = loadedCerts;
390+
caCertificate = loadedCACert;
384391
} catch (final IOException | CertificateException e) {
385392
logger.error("Failed to load saved RootCA certificate due to exception:", e);
386393
return false;
@@ -446,6 +453,40 @@ protected void addConfiguredManagementIp(List<String> ipList) {
446453
}
447454

448455

456+
private static void validatePrivateKeyPem(String value) {
457+
if (StringUtils.isEmpty(value)) return;
458+
try {
459+
CertUtils.pemToPrivateKey(value);
460+
} catch (InvalidKeySpecException | IOException e) {
461+
throw new IllegalArgumentException(
462+
"ca.plugin.root.private.key is not a valid PEM private key: " + e.getMessage());
463+
}
464+
}
465+
466+
private static void validatePublicKeyPem(String value) {
467+
if (StringUtils.isEmpty(value)) return;
468+
try {
469+
CertUtils.pemToPublicKey(value);
470+
} catch (InvalidKeySpecException | IOException e) {
471+
throw new IllegalArgumentException(
472+
"ca.plugin.root.public.key is not a valid PEM public key: " + e.getMessage());
473+
}
474+
}
475+
476+
private static void validateCACertificatePem(String value) {
477+
if (StringUtils.isEmpty(value)) return;
478+
try {
479+
final List<X509Certificate> certs = CertUtils.pemToX509Certificates(value);
480+
if (CollectionUtils.isEmpty(certs)) {
481+
throw new IllegalArgumentException(
482+
"ca.plugin.root.ca.certificate contains no certificates");
483+
}
484+
} catch (IOException | CertificateException e) {
485+
throw new IllegalArgumentException(
486+
"ca.plugin.root.ca.certificate is not a valid PEM certificate: " + e.getMessage());
487+
}
488+
}
489+
449490
private boolean setupCA() {
450491
if (!loadRootCAKeyPair()) {
451492
if (hasUserProvidedCAKeys()) {

‎plugins/ca/root-ca/src/test/java/org/apache/cloudstack/ca/provider/RootCAProviderTest.java‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -249,4 +249,61 @@ public void testIsManagementCertificateMatch() {
249249
Assert.fail(String.format("Exception occurred: %s", e.getMessage()));
250250
}
251251
}
252+
253+
@Test
254+
public void testLoadRootCACertificateWithMismatchedCert() throws Exception {
255+
KeyPair otherKeyPair = CertUtils.generateRandomKeyPair(1024);
256+
X509Certificate mismatchedCert = CertUtils.generateV3Certificate(null, otherKeyPair, otherKeyPair.getPublic(), "CN=other", "SHA256withRSA", 365, null, null);
257+
String mismatchedPem = CertUtils.x509CertificateToPem(mismatchedCert);
258+
259+
ConfigKey<String> mockCertKey = Mockito.mock(ConfigKey.class);
260+
Mockito.when(mockCertKey.value()).thenReturn(mismatchedPem);
261+
ReflectionTestUtils.setField(RootCAProvider.class, "rootCACertificate", mockCertKey);
262+
263+
ReflectionTestUtils.setField(RootCAProvider.class, "caCertificate", null);
264+
ReflectionTestUtils.setField(RootCAProvider.class, "caCertificates", null);
265+
266+
Boolean result = ReflectionTestUtils.invokeMethod(provider, "loadRootCACertificate");
267+
Assert.assertFalse(result);
268+
Assert.assertNull(provider.getCaCertificate());
269+
}
270+
271+
@Test
272+
public void testSaveNewRootCACertificateWithStaleCache() throws Exception {
273+
org.apache.cloudstack.framework.config.dao.ConfigurationDao configDao = Mockito.mock(org.apache.cloudstack.framework.config.dao.ConfigurationDao.class);
274+
ReflectionTestUtils.setField(provider, "configDao", configDao);
275+
276+
ConfigKey<String> mockCertKey = Mockito.mock(ConfigKey.class);
277+
Mockito.when(mockCertKey.key()).thenReturn("ca.plugin.root.ca.certificate");
278+
Mockito.when(mockCertKey.category()).thenReturn("Hidden");
279+
ReflectionTestUtils.setField(RootCAProvider.class, "rootCACertificate", mockCertKey);
280+
281+
ConfigKey<String> mockIssuerKey = Mockito.mock(ConfigKey.class);
282+
Mockito.when(mockIssuerKey.value()).thenReturn("CN=ca.cloudstack.apache.org");
283+
ReflectionTestUtils.setField(RootCAProvider.class, "rootCAIssuerDN", mockIssuerKey);
284+
285+
ReflectionTestUtils.setField(RootCAProvider.class, "caCertificate", null);
286+
ReflectionTestUtils.setField(RootCAProvider.class, "caCertificates", null);
287+
288+
Mockito.when(configDao.update(Mockito.anyString(), Mockito.anyString(), Mockito.anyString())).thenReturn(true);
289+
290+
Boolean result = ReflectionTestUtils.invokeMethod(provider, "saveNewRootCACertificate");
291+
Assert.assertTrue(result);
292+
Assert.assertNotNull(provider.getCaCertificate());
293+
Assert.assertEquals(1, provider.getCaCertificate().size());
294+
}
295+
296+
@Test
297+
public void testValidateCACertificatePem() throws Exception {
298+
String truncatedPem = "-----BEGIN CERTIFICATE-----\nMIICxTCCAa0CAQAw\n";
299+
java.lang.reflect.Method method = RootCAProvider.class.getDeclaredMethod("validateCACertificatePem", String.class);
300+
method.setAccessible(true);
301+
try {
302+
method.invoke(null, truncatedPem);
303+
Assert.fail("Expected java.lang.IllegalArgumentException");
304+
} catch (java.lang.reflect.InvocationTargetException e) {
305+
Assert.assertTrue(e.getCause() instanceof IllegalArgumentException);
306+
Assert.assertTrue(e.getCause().getMessage().contains("is not a valid PEM certificate"));
307+
}
308+
}
252309
}

0 commit comments

Comments
 (0)