Conversation
|
@blueorangutan package |
|
@vishesh92 a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #12911 +/- ##
============================================
- Coverage 18.02% 18.01% -0.01%
- Complexity 16460 16477 +17
============================================
Files 5968 5977 +9
Lines 537213 537905 +692
Branches 65975 66061 +86
============================================
+ Hits 96825 96912 +87
- Misses 429469 430058 +589
- Partials 10919 10935 +16
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
server/src/main/java/org/apache/cloudstack/ca/CAManagerImpl.java
Outdated
Show resolved
Hide resolved
plugins/ca/root-ca/src/main/java/org/apache/cloudstack/ca/provider/RootCAProvider.java
Show resolved
Hide resolved
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17282 |
|
[SF] Trillian Build Failed (tid-15758) |
There was a problem hiding this comment.
Pull request overview
This PR extends CloudStack’s CA/certificate provisioning to support forced re-provisioning via SSH (for disconnected KVM hosts and SystemVMs) and improves truststore handling so CloudStack CA roots can be trusted by management and SystemVM JVM processes.
Changes:
- Add a
forcedflag toprovisionCertificate(API + manager) and implement SSH-based provisioning paths for KVM hosts and SystemVMs. - Inject configured CA certificate(s) into the management server JVM default truststore on startup (configurable via a new global setting).
- Update SystemVM/scripts truststore import logic to also populate
realhostip.keystorewith CA certs; improve cert-chain splitting robustness.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| utils/src/main/java/org/apache/cloudstack/utils/security/KeyStoreUtils.java | Removes an unused SystemVM import-script constant. |
| utils/src/main/java/com/cloud/utils/nio/Link.java | Adjusts handshake wrap error handling to fail the handshake on SSL wrap exceptions. |
| systemvm/patch-sysvms.sh | Imports CA cert chain into realhostip.keystore during live patch for CPVM/SSVM. |
| server/src/test/java/org/apache/cloudstack/ca/CAManagerImplTest.java | Updates tests for the new provisionCertificate(..., forced) signature. |
| server/src/test/java/org/apache/cloudstack/ca/CABackgroundTaskTest.java | Updates mocks/verifications for the new method signature. |
| server/src/main/java/org/apache/cloudstack/ca/CAManagerImpl.java | Implements forced provisioning via SSH + JVM truststore injection feature and wires new dependencies. |
| server/src/main/java/com/cloud/hypervisor/kvm/discoverer/LibvirtServerDiscoverer.java | Refactors KVM discovery provisioning to call caManager.provisionCertificateViaSsh(...). |
| scripts/util/keystore-cert-import | Improves cert split loop robustness; imports CA into realhostip.keystore for SystemVMs. |
| plugins/ca/root-ca/src/main/java/org/apache/cloudstack/ca/provider/RootCAProvider.java | Clarifies config docs, improves fallback logging, and fixes startup loading sequence. |
| api/src/main/java/org/apache/cloudstack/ca/CAManager.java | Adds forced support + introduces CaInjectDefaultTruststore config key + adds provisionCertificateViaSsh. |
| api/src/main/java/org/apache/cloudstack/api/command/admin/ca/ProvisionCertificateCmd.java | Adds forced API parameter and passes it through to CA manager. |
Comments suppressed due to low confidence (1)
scripts/util/keystore-cert-import:78
- The certificate-chain splitting uses temp files named
cloudca.*in the current working directory. This can fail or be unsafe if the script runs from an unexpected directory (clobbering existing files or following symlinks). Consider using a dedicated temporary directory viamktemp -dand writing the split certs there, then cleaning up that directory.
awk 'BEGIN{n=0} /-----BEGIN CERTIFICATE-----/{n++}{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
rm -f cloudca.*
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
[SF] Trillian Build Failed (tid-15759) |
|
@blueorangutan package |
|
@vishesh92 a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✖️ debian ✔️ suse15. SL-JID 17290 |
|
@blueorangutan package |
|
@vishesh92 a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17300 |
|
@blueorangutan package |
|
@vishesh92 a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17301 |
|
@blueorangutan package |
|
@vishesh92 a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17330 |
vishesh92
force-pushed
the
update-ca-certs
branch
from
a66f741 to
6adca9a
Compare
|
@blueorangutan package |
|
@vishesh92 a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17352 |
|
@blueorangutan package |
|
@vishesh92 a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17370 |
boring-cyborg
bot
added
component:integration-test
Python
|
@blueorangutan package |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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); |
There was a problem hiding this 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.
| 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; | ||
| } |
There was a problem hiding this 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.
| 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 |
There was a problem hiding this 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.
| args: ['hostid', 'forced'], | ||
| mapping: { | ||
| hostid: { | ||
| value: (record) => { return record.id } |
There was a problem hiding this 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.
| value: (record) => { return record.id } | |
| value: (record) => { return record.id } | |
| }, | |
| forced: { | |
| value: false |
Description
This PR adds ROOT CAs to the trust store in System VMs & the management
This PR also adds another parameter
forcedtoprovisionCertificatecommand which allows provisioning certs via SSH to KVM hosts & system VMs.Doc PR: apache/cloudstack-documentation#641
Details
This pull request introduces several improvements and new features to the CloudStack Certificate Authority (CA) management system, with a focus on enhancing certificate provisioning flexibility, supporting user-provided CA material, and improving truststore management. The most significant changes add support for forced certificate re-provisioning via SSH for disconnected agents, clarify and enforce requirements for custom CA keys, and enable automatic truststore injection for outgoing HTTPS connections.
Certificate Provisioning Enhancements:
forcedboolean parameter to theProvisionCertificateCmdAPI and theCAManager.provisionCertificatemethod, allowing administrators to re-provision agent certificates via SSH when agents are disconnected (e.g., after a CA change). This is supported for KVM hosts and SystemVMs. The implementation includes a newprovisionCertificateViaSshmethod for KVM hosts. [1] [2] [3] [4] [5] [6]CA Provider and Configuration Improvements:
RootCAProvider, including PEM format requirements and the need to set all three keys together. Enhanced error handling and logging when loading user-provided CA keys/certificates fails, with fallback to auto-generation. [1] [2]ca.framework.provider.pluginconfiguration key to clarify its purpose and the requirements for custom CA material.Truststore Management:
ca.framework.inject.default.truststoreto control whether the CA provider's certificate is injected into the JVM default truststore on management server startup, enabling outgoing HTTPS connections from the management server to trust servers signed by the configured CA.keystore-cert-importscript to also import the CA certificate into therealhostip.keystoreused by the SSVM JVM, ensuring trust for CloudStack CA-signed servers.Script and Utility Fixes:
keystore-cert-importscript to handle certificate splitting and file cleanup more robustly, preventing errors if no certificates are present.Code Cleanup:
LibvirtServerDiscovererand related classes as part of the refactoring for SSH-based certificate provisioning. [1] [2] [3] [4] [5] [6] [7]These changes collectively improve CloudStack's certificate management flexibility, security, and ease of integration with external CA infrastructures.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?