Copilot commented on code in PR #12911:
URL: https://github.com/apache/cloudstack/pull/12911#discussion_r3057558748
##########
scripts/util/keystore-cert-import:
##########
@@ -70,8 +70,8 @@ elif [ ! -f "$CACERT_FILE" ]; then
fi
# Import cacerts into the keystore
-awk '/-----BEGIN CERTIFICATE-----?/{n++}{print > "cloudca." n }' "$CACERT_FILE"
-for caChain in $(ls cloudca.*); do
+awk 'BEGIN{n=0} /-----BEGIN CERTIFICATE-----/{n++} n>0{print > "cloudca." n }'
"$CACERT_FILE"
+for caChain in $(ls cloudca.* 2>/dev/null); do
keytool -delete -noprompt -alias "$caChain" -keystore "$KS_FILE"
-storepass "$KS_PASS" > /dev/null 2>&1 || true
keytool -import -noprompt -storepass "$KS_PASS" -trustcacerts -alias
"$caChain" -file "$caChain" -keystore "$KS_FILE" > /dev/null 2>&1
done
Review Comment:
Same temp-file safety issue as in `patch-sysvms.sh`: the script writes
`cloudca.*` into the CWD and relies on `ls` command substitution for iteration
(breaks on whitespace and behaves poorly when no files exist). Use a temp
directory + safe globs (`for caChain in cloudca.*; do [ -e \"$caChain\" ] ||
continue; ...`) and clean up only files created in that temp dir.
##########
server/src/main/java/org/apache/cloudstack/ca/CAManagerImpl.java:
##########
@@ -400,9 +558,54 @@ public boolean start() {
logger.error("Failed to find valid configured CA provider, please
check!");
return false;
}
+ if (CaInjectDefaultTruststore.value()) {
+ injectCaCertIntoDefaultTruststore();
+ }
return true;
}
+ private void injectCaCertIntoDefaultTruststore() {
+ try {
+ final List<X509Certificate> caCerts =
configuredCaProvider.getCaCertificate();
+ if (caCerts == null || caCerts.isEmpty()) {
+ logger.debug("No CA certificates found from the configured
provider, skipping JVM truststore injection");
+ return;
+ }
+
+ final KeyStore trustStore =
KeyStore.getInstance(KeyStore.getDefaultType());
+ trustStore.load(null, null);
+
+ // Copy existing default trusted certs
+ final TrustManagerFactory defaultTmf =
TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm());
+ defaultTmf.init((KeyStore) null);
+ final X509TrustManager defaultTm = (X509TrustManager)
defaultTmf.getTrustManagers()[0];
+ int aliasIndex = 0;
+ for (final X509Certificate cert : defaultTm.getAcceptedIssuers()) {
+ trustStore.setCertificateEntry("default-ca-" + aliasIndex++,
cert);
+ }
+
+ // Add CA provider's certificates
+ int count = 0;
+ for (final X509Certificate caCert : caCerts) {
+ final String alias = "cloudstack-ca-" + count;
+ trustStore.setCertificateEntry(alias, caCert);
+ count++;
+ logger.info("Injected CA certificate into JVM default
truststore: subject={}, alias={}",
+ caCert.getSubjectX500Principal().getName(), alias);
+ }
+
+ // Reinitialize default SSLContext with the updated truststore
+ final TrustManagerFactory updatedTmf =
TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm());
+ updatedTmf.init(trustStore);
+ final SSLContext sslContext = SSLContext.getInstance("TLS");
+ sslContext.init(null, updatedTmf.getTrustManagers(), new
SecureRandom());
+ SSLContext.setDefault(sslContext);
Review Comment:
This approach rebuilds the JVM default trust configuration from
`X509TrustManager#getAcceptedIssuers()` and then calls
`SSLContext.setDefault(...)`. There are two concrete problems: (1)
`getAcceptedIssuers()` is not guaranteed to enumerate all trust anchors (some
implementations return an empty/partial list), so this can silently *drop*
system CAs and break TLS validation; (2) `defaultTmf.getTrustManagers()[0]` is
not guaranteed to be an `X509TrustManager`, so the cast can fail at runtime.
Prefer constructing a TrustManager that delegates to the platform default
TrustManager and additionally trusts the configured CA chain (or load the
actual default truststore KeyStore and add entries), and avoid globally
overriding the JVM default SSLContext unless strictly necessary—set a dedicated
SSLContext on the specific outbound clients instead.
##########
ui/src/config/section/infra/hosts.js:
##########
@@ -103,7 +103,7 @@ export default {
show: (record) => {
return record.hypervisor === 'KVM' || record.hypervisor ===
store.getters.customHypervisorName
},
- args: ['hostid'],
+ args: ['hostid', 'forced'],
mapping: {
hostid: {
value: (record) => { return record.id }
Review Comment:
`args` now includes `forced`, but there is no corresponding
`mapping.forced`. In this UI config pattern, that typically results in the
parameter being missing/undefined (or an extra prompt not being rendered
correctly), which can break the action invocation or prevent users from setting
the new flag. Add a `forced` mapping with an explicit default (e.g., `false`)
and/or a UI control so the API receives a deterministic boolean.
```suggestion
value: (record) => { return record.id }
},
forced: {
value: false
```
##########
utils/src/main/java/org/apache/cloudstack/utils/security/CertUtils.java:
##########
@@ -98,9 +98,17 @@ public static KeyFactory getKeyFactory() {
return keyFactory;
}
- public static X509Certificate pemToX509Certificate(final String pem)
throws CertificateException, IOException {
+ public static List<X509Certificate> pemToX509Certificates(final String
pem) throws CertificateException, IOException {
+ final List<X509Certificate> certs = new ArrayList<>();
final PEMParser pemParser = new PEMParser(new StringReader(pem));
- return new
JcaX509CertificateConverter().setProvider("BC").getCertificate((X509CertificateHolder)
pemParser.readObject());
+ final JcaX509CertificateConverter certConverter = new
JcaX509CertificateConverter().setProvider("BC");
+ Object parsedObj;
+ while ((parsedObj = pemParser.readObject()) != null) {
+ if (parsedObj instanceof X509CertificateHolder) {
+ certs.add(certConverter.getCertificate((X509CertificateHolder)
parsedObj));
+ }
+ }
+ return certs;
}
Review Comment:
`PEMParser` is never closed. Even though it wraps a `StringReader`, leaving
it unclosed is still a resource-handling bug pattern and can trigger static
analysis failures. Use try-with-resources around the `PEMParser` (and reader if
you keep it separate) so the parser is always closed.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
