Skip to content

Commit 217fdd5

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

2 files changed

Lines changed: 123 additions & 28 deletions

File tree

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

Lines changed: 64 additions & 23 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",
@@ -327,24 +328,26 @@ private boolean saveNewRootCAKeypair() {
327328
return loadRootCAKeyPair();
328329
}
329330

330-
private boolean saveNewRootCACertificate() {
331+
boolean saveNewRootCACertificate() {
331332
if (caKeyPair == null) {
332333
throw new CloudRuntimeException("Cannot issue self-signed root CA certificate as CA keypair is not initialized");
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() {
@@ -360,27 +363,31 @@ private boolean loadRootCAKeyPair() {
360363
return caKeyPair.getPrivate() != null && caKeyPair.getPublic() != null;
361364
}
362365

363-
private boolean loadRootCACertificate() {
366+
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+
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: 59 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@
3939

4040
import org.apache.cloudstack.framework.ca.Certificate;
4141
import org.apache.cloudstack.framework.config.ConfigKey;
42+
import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
4243
import org.apache.cloudstack.utils.security.CertUtils;
4344
import org.apache.cloudstack.utils.security.SSLUtils;
4445
import org.bouncycastle.asn1.x509.GeneralName;
@@ -50,7 +51,6 @@
5051
import org.junit.runner.RunWith;
5152
import org.mockito.Mockito;
5253
import org.mockito.junit.MockitoJUnitRunner;
53-
import org.springframework.test.util.ReflectionTestUtils;
5454

5555

5656
@RunWith(MockitoJUnitRunner.class)
@@ -218,8 +218,8 @@ public void testIsManagementCertificateNoAltNames() {
218218
}
219219

220220
@Test
221-
public void testIsManagementCertificateNoMatch() {
222-
ReflectionTestUtils.setField(provider, "managementCertificateCustomSAN", "cloudstack");
221+
public void testIsManagementCertificateNoMatch() throws Exception {
222+
addField(provider, "managementCertificateCustomSAN", "cloudstack");
223223
try {
224224
X509Certificate certificate = Mockito.mock(X509Certificate.class);
225225
List<List<?>> altNames = new ArrayList<>();
@@ -234,9 +234,9 @@ public void testIsManagementCertificateNoMatch() {
234234
}
235235

236236
@Test
237-
public void testIsManagementCertificateMatch() {
237+
public void testIsManagementCertificateMatch() throws Exception {
238238
String customSAN = "cloudstack";
239-
ReflectionTestUtils.setField(provider, "managementCertificateCustomSAN", customSAN);
239+
addField(provider, "managementCertificateCustomSAN", customSAN);
240240
try {
241241
X509Certificate certificate = Mockito.mock(X509Certificate.class);
242242
List<List<?>> altNames = new ArrayList<>();
@@ -249,4 +249,58 @@ 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+
addField(provider, "rootCACertificate", mockCertKey);
262+
263+
addField(provider, "caCertificate", null);
264+
addField(provider, "caCertificates", null);
265+
266+
Boolean result = provider.loadRootCACertificate();
267+
Assert.assertFalse(result);
268+
Assert.assertNull(provider.getCaCertificate());
269+
}
270+
271+
@Test
272+
public void testSaveNewRootCACertificateWithStaleCache() throws Exception {
273+
ConfigurationDao configDao = Mockito.mock(ConfigurationDao.class);
274+
addField(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+
addField(provider, "rootCACertificate", mockCertKey);
280+
281+
ConfigKey<String> mockIssuerKey = Mockito.mock(ConfigKey.class);
282+
Mockito.when(mockIssuerKey.value()).thenReturn("CN=ca.cloudstack.apache.org");
283+
addField(provider, "rootCAIssuerDN", mockIssuerKey);
284+
285+
addField(provider, "caCertificate", null);
286+
addField(provider, "caCertificates", null);
287+
288+
Mockito.when(configDao.update(Mockito.anyString(), Mockito.anyString(), Mockito.anyString())).thenReturn(true);
289+
290+
Boolean result = 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+
try {
300+
RootCAProvider.validateCACertificatePem(truncatedPem);
301+
Assert.fail("Expected IllegalArgumentException");
302+
} catch (IllegalArgumentException e) {
303+
Assert.assertTrue(e.getMessage().contains("is not a valid PEM certificate"));
304+
}
305+
}
252306
}

0 commit comments

Comments
 (0)