Copilot commented on code in PR #12911:
URL: https://github.com/apache/cloudstack/pull/12911#discussion_r3008235725
##########
systemvm/patch-sysvms.sh:
##########
@@ -126,7 +126,28 @@ patch_systemvm() {
if [ "$TYPE" = "consoleproxy" ] || [ "$TYPE" = "secstorage" ]; then
# Import global cacerts into 'cloud' service's keystore
- keytool -importkeystore -srckeystore /etc/ssl/certs/java/cacerts
-destkeystore /usr/local/cloud/systemvm/certs/realhostip.keystore -srcstorepass
changeit -deststorepass vmops.com -noprompt 2>/dev/null || true
+ REALHOSTIP_KS_FILE="/usr/local/cloud/systemvm/certs/realhostip.keystore"
+ REALHOSTIP_PASS="vmops.com"
+
+ keytool -importkeystore -srckeystore /etc/ssl/certs/java/cacerts \
+ -destkeystore "$REALHOSTIP_KS_FILE" -srcstorepass changeit
-deststorepass \
+ "$REALHOSTIP_PASS" -noprompt 2>/dev/null || true
+
+ # Import CA cert(s) into realhostip.keystore so the SSVM JVM
+ # (which overrides the truststore via -Djavax.net.ssl.trustStore in
_run.sh)
+ # can trust servers signed by the CloudStack CA
+ CACERT_FILE="/usr/local/share/ca-certificates/cloudstack/ca.crt"
+
+ if [ -f "$CACERT_FILE" ] && [ -f "$REALHOSTIP_KS_FILE" ]; then
+ 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
"$REALHOSTIP_KS_FILE" \
+ -storepass "$REALHOSTIP_PASS" > /dev/null 2>&1 || true
+ keytool -import -noprompt -trustcacerts -alias "$caChain" -file
"$caChain" \
+ -keystore "$REALHOSTIP_KS_FILE" -storepass "$REALHOSTIP_PASS"
> /dev/null 2>&1
+ done
+ rm -f cloudca.*
Review Comment:
This adds the same `cloudca.*` temp-file splitting/import logic in the
livepatch script, again using the current working directory. For the same
reasons as `keystore-cert-import`, it would be safer to use a dedicated `mktemp
-d` directory and avoid importing `cloudca.0` when the PEM has any
preamble/whitespace.
```suggestion
TMP_CA_DIR="$(mktemp -d 2>/dev/null)" || TMP_CA_DIR=""
if [ -z "$TMP_CA_DIR" ]; then
echo "Failed to create temporary directory for CA cert import;
skipping CloudStack CA import." >> $logfile 2>&1
else
awk 'BEGIN{n=0} /-----BEGIN CERTIFICATE-----/{n++}{print >
"'"$TMP_CA_DIR"'/cloudca." n }' "$CACERT_FILE"
for caChain in "$TMP_CA_DIR"/cloudca.*; do
# If the glob did not match anything, break out of the loop.
[ -e "$caChain" ] || break
caAlias="$(basename "$caChain")"
# Skip potential preamble/whitespace chunk (cloudca.0).
if [ "$caAlias" = "cloudca.0" ]; then
continue
fi
keytool -delete -noprompt -alias "$caAlias" -keystore
"$REALHOSTIP_KS_FILE" \
-storepass "$REALHOSTIP_PASS" > /dev/null 2>&1 || true
keytool -import -noprompt -trustcacerts -alias "$caAlias"
-file "$caChain" \
-keystore "$REALHOSTIP_KS_FILE" -storepass
"$REALHOSTIP_PASS" > /dev/null 2>&1
done
rm -f "$TMP_CA_DIR"/cloudca.* 2>/dev/null || true
rmdir "$TMP_CA_DIR" 2>/dev/null || true
fi
```
##########
api/src/main/java/org/apache/cloudstack/ca/CAManager.java:
##########
@@ -130,12 +141,25 @@ public interface CAManager extends CAService,
Configurable, PluggableService {
boolean revokeCertificate(final BigInteger certSerial, final String
certCn, final String provider);
/**
- * Provisions certificate for given active and connected agent host
+ * Provisions certificate for given agent host.
+ * When forced=true, uses SSH to re-provision bypassing the NIO agent
connection (for disconnected agents).
* @param host
+ * @param reconnect
* @param provider
+ * @param forced when true, provisions via SSH instead of NIO; supports
KVM hosts and SystemVMs
* @return returns success/failure as boolean
*/
- boolean provisionCertificate(final Host host, final Boolean reconnect,
final String provider);
+ boolean provisionCertificate(final Host host, final Boolean reconnect,
final String provider, final boolean forced);
+
+ /**
+ * Provisions certificate for a KVM host using an existing SSH connection.
+ * Runs keystore-setup to generate a CSR, issues a certificate, then runs
keystore-cert-import.
+ * Used during host discovery and for forced re-provisioning when the NIO
agent is unreachable.
+ * @param sshConnection active SSH connection to the KVM host
+ * @param agentIp IP address of the KVM host agent
+ * @param agentHostname hostname of the KVM host agent
+ */
+ void provisionCertificateViaSsh(Connection sshConnection, String agentIp,
String agentHostname);
Review Comment:
`CAManager.provisionCertificateViaSsh` exposes `com.trilead.ssh2.Connection`
in the public API interface, coupling the API module (and any implementers) to
a specific SSH library. Consider keeping the SSH connection type internal to
the server layer (e.g., accept host/credentials and let the implementation
manage SSH), or introduce a small CloudStack-owned SSH abstraction/interface
instead of leaking Trilead into the API surface.
##########
server/src/main/java/org/apache/cloudstack/ca/CAManagerImpl.java:
##########
@@ -200,6 +228,138 @@ public boolean provisionCertificate(final Host host,
final Boolean reconnect, fi
}
}
+ private boolean provisionCertificateForced(Host host, Boolean reconnect,
String caProvider) {
+ if (host.getType() == Host.Type.Routing && host.getHypervisorType() ==
com.cloud.hypervisor.Hypervisor.HypervisorType.KVM) {
+ return provisionKvmHostViaSsh(host);
+ } else if (host.getType() == Host.Type.ConsoleProxy || host.getType()
== Host.Type.SecondaryStorageVM) {
+ return provisionSystemVmViaSsh(host, reconnect);
+ }
+ throw new CloudRuntimeException("Forced certificate provisioning is
only supported for KVM hosts and SystemVMs.");
+ }
+
+ @Override
+ public void provisionCertificateViaSsh(final Connection sshConnection,
final String agentIp, final String agentHostname) {
+ Integer validityPeriod = CAManager.CertValidityPeriod.value();
+ if (validityPeriod < 1) {
+ validityPeriod = 1;
+ }
+
+ String keystorePassword = PasswordGenerator.generateRandomPassword(16);
+
+ // 1. Setup Keystore and Generate CSR
+ final SSHCmdHelper.SSHCmdResult keystoreSetupResult =
SSHCmdHelper.sshExecuteCmdWithResult(sshConnection,
+ String.format("sudo
/usr/share/cloudstack-common/scripts/util/%s " +
+ "/etc/cloudstack/agent/agent.properties " +
+ "/etc/cloudstack/agent/%s " +
+ "%s %d " +
+ "/etc/cloudstack/agent/%s",
+ KeyStoreUtils.KS_SETUP_SCRIPT,
+ KeyStoreUtils.KS_FILENAME,
+ keystorePassword,
+ validityPeriod,
+ KeyStoreUtils.CSR_FILENAME));
+
+ if (!keystoreSetupResult.isSuccess()) {
+ throw new CloudRuntimeException("Failed to setup keystore and
generate CSR via SSH on host: " + agentIp);
+ }
+
+ // 2. Issue Certificate based on returned CSR
+ final String csr = keystoreSetupResult.getStdOut();
+ final Certificate certificate = issueCertificate(csr,
Arrays.asList(agentHostname, agentIp),
+ Collections.singletonList(agentIp), null, null);
+
Review Comment:
The forced provisioning flow currently ignores the `caProvider` argument:
`provisionCertificateForced` receives it but `provisionCertificateViaSsh`
issues the certificate with `caProvider=null` (default provider). This makes
`provisionCertificate(..., provider, forced=true)` behave differently than the
non-forced path. Consider threading the provider through (e.g., add a provider
parameter to `provisionCertificateViaSsh` or have `provisionCertificateForced`
call `issueCertificate(..., caProvider)`).
##########
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++}{print > "cloudca." n }'
"$CACERT_FILE"
Review Comment:
The `awk` split writes to `cloudca.0` until the first `BEGIN CERTIFICATE`
line is encountered (because `{print > "cloudca." n}` runs even when `n==0`).
That can lead to attempting to import an invalid/empty file. Consider only
printing once `n>0` (or changing the awk script to skip preamble) so only real
PEM blocks become import candidates.
```suggestion
awk 'BEGIN{n=0} /-----BEGIN CERTIFICATE-----/{n++} n>0 {print > "cloudca." n
}' "$CACERT_FILE"
```
##########
scripts/util/keystore-cert-import:
##########
@@ -137,6 +137,22 @@ if [ -f "$SYSTEM_FILE" ]; then
chmod 644 /usr/local/share/ca-certificates/cloudstack/ca.crt
update-ca-certificates > /dev/null 2>&1 || true
+ # Import CA cert(s) into realhostip.keystore so the SSVM JVM
+ # (which overrides the truststore via -Djavax.net.ssl.trustStore in
_run.sh)
+ # can trust servers signed by the CloudStack CA
+ REALHOSTIP_KS_FILE="$(dirname $(dirname
$PROPS_FILE))/certs/realhostip.keystore"
Review Comment:
`REALHOSTIP_KS_FILE` is computed with nested `dirname` calls but
`$PROPS_FILE` isn’t quoted inside the command substitution. While uncommon,
this will break if the path contains spaces or glob characters. Quote
`$PROPS_FILE` within the `dirname` invocations to make this robust.
```suggestion
REALHOSTIP_KS_FILE="$(dirname "$(dirname
"$PROPS_FILE")")/certs/realhostip.keystore"
```
##########
server/src/main/java/org/apache/cloudstack/ca/CAManagerImpl.java:
##########
@@ -400,9 +560,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);
Review Comment:
`injectCaCertIntoDefaultTruststore()` assumes
`configuredCaProvider.getCaCertificate()` contains only non-null entries. Some
providers (e.g., RootCAProvider) can return a list containing `null` if
initialization failed, which would cause `trustStore.setCertificateEntry(...)`
to throw. Consider filtering out null certificates (and logging) before
attempting to inject them.
```suggestion
int count = 0;
int i = 0;
for (final X509Certificate caCert : caCerts) {
if (caCert == null) {
logger.warn("Skipping null CA certificate at index {}
from configured CA provider during JVM truststore injection", i);
i++;
continue;
}
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);
i++;
```
##########
server/src/main/java/org/apache/cloudstack/ca/CAManagerImpl.java:
##########
@@ -200,6 +228,138 @@ public boolean provisionCertificate(final Host host,
final Boolean reconnect, fi
}
}
+ private boolean provisionCertificateForced(Host host, Boolean reconnect,
String caProvider) {
+ if (host.getType() == Host.Type.Routing && host.getHypervisorType() ==
com.cloud.hypervisor.Hypervisor.HypervisorType.KVM) {
+ return provisionKvmHostViaSsh(host);
+ } else if (host.getType() == Host.Type.ConsoleProxy || host.getType()
== Host.Type.SecondaryStorageVM) {
+ return provisionSystemVmViaSsh(host, reconnect);
+ }
+ throw new CloudRuntimeException("Forced certificate provisioning is
only supported for KVM hosts and SystemVMs.");
+ }
+
+ @Override
+ public void provisionCertificateViaSsh(final Connection sshConnection,
final String agentIp, final String agentHostname) {
+ Integer validityPeriod = CAManager.CertValidityPeriod.value();
+ if (validityPeriod < 1) {
+ validityPeriod = 1;
+ }
+
+ String keystorePassword = PasswordGenerator.generateRandomPassword(16);
+
+ // 1. Setup Keystore and Generate CSR
+ final SSHCmdHelper.SSHCmdResult keystoreSetupResult =
SSHCmdHelper.sshExecuteCmdWithResult(sshConnection,
+ String.format("sudo
/usr/share/cloudstack-common/scripts/util/%s " +
+ "/etc/cloudstack/agent/agent.properties " +
+ "/etc/cloudstack/agent/%s " +
+ "%s %d " +
+ "/etc/cloudstack/agent/%s",
+ KeyStoreUtils.KS_SETUP_SCRIPT,
+ KeyStoreUtils.KS_FILENAME,
+ keystorePassword,
+ validityPeriod,
+ KeyStoreUtils.CSR_FILENAME));
+
+ if (!keystoreSetupResult.isSuccess()) {
+ throw new CloudRuntimeException("Failed to setup keystore and
generate CSR via SSH on host: " + agentIp);
+ }
+
+ // 2. Issue Certificate based on returned CSR
+ final String csr = keystoreSetupResult.getStdOut();
+ final Certificate certificate = issueCertificate(csr,
Arrays.asList(agentHostname, agentIp),
+ Collections.singletonList(agentIp), null, null);
+
+ if (certificate == null || certificate.getClientCertificate() == null)
{
+ throw new CloudRuntimeException("Failed to issue certificates for
host: " + agentIp);
+ }
+
+ // 3. Import Certificate into agent keystore
+ final SetupCertificateCommand certificateCommand = new
SetupCertificateCommand(certificate);
+ final SSHCmdHelper.SSHCmdResult setupCertResult =
SSHCmdHelper.sshExecuteCmdWithResult(sshConnection,
+ String.format("sudo
/usr/share/cloudstack-common/scripts/util/%s " +
+ "/etc/cloudstack/agent/agent.properties %s " +
+ "/etc/cloudstack/agent/%s %s " +
+ "/etc/cloudstack/agent/%s \"%s\" " +
+ "/etc/cloudstack/agent/%s \"%s\" " +
+ "/etc/cloudstack/agent/%s \"%s\"",
+ KeyStoreUtils.KS_IMPORT_SCRIPT,
+ keystorePassword,
+ KeyStoreUtils.KS_FILENAME,
+ KeyStoreUtils.SSH_MODE,
+ KeyStoreUtils.CERT_FILENAME,
+ certificateCommand.getEncodedCertificate(),
+ KeyStoreUtils.CACERT_FILENAME,
+ certificateCommand.getEncodedCaCertificates(),
+ KeyStoreUtils.PKEY_FILENAME,
+ certificateCommand.getEncodedPrivateKey()));
+
+ if (!setupCertResult.isSuccess()) {
+ throw new CloudRuntimeException("Failed to import certificates
into agent keystore via SSH on host: " + agentIp);
+ }
+ }
+
+ private boolean provisionKvmHostViaSsh(Host host) {
+ final HostVO hostVO = (HostVO) host;
+ hostDao.loadDetails(hostVO);
+ String username = hostVO.getDetail(ApiConstants.USERNAME);
+ String password = hostVO.getDetail(ApiConstants.PASSWORD);
+ String hostIp = host.getPrivateIpAddress();
+
+ int port =
AgentManager.KVMHostDiscoverySshPort.valueIn(host.getClusterId());
+ if (hostVO.getDetail(Host.HOST_SSH_PORT) != null) {
+ port = NumberUtils.toInt(hostVO.getDetail(Host.HOST_SSH_PORT),
port);
+ }
+
+ Connection sshConnection = null;
+ try {
+ sshConnection = new Connection(hostIp, port);
+ sshConnection.connect(null, 60000, 60000);
+
+ String privateKey = configDao.getValue("ssh.privatekey");
+ if
(!SSHCmdHelper.acquireAuthorizedConnectionWithPublicKey(sshConnection,
username, privateKey)) {
+ if (StringUtils.isEmpty(password) ||
!sshConnection.authenticateWithPassword(username, password)) {
+ throw new CloudRuntimeException("Failed to authenticate to
host via SSH for forced provisioning: " + hostIp);
+ }
+ }
+
+ provisionCertificateViaSsh(sshConnection, hostIp, host.getName());
+
+ SSHCmdHelper.sshExecuteCmd(sshConnection, "sudo service
cloudstack-agent restart");
+ return true;
+ } catch (Exception e) {
+ logger.error("Error during forced SSH provisioning for KVM host "
+ host.getUuid(), e);
+ return false;
+ } finally {
+ if (sshConnection != null) {
+ sshConnection.close();
+ }
+ }
+ }
+
+ private boolean provisionSystemVmViaSsh(Host host, Boolean reconnect) {
+ VMInstanceVO vm = vmInstanceDao.findVMByInstanceName(host.getName());
+ if (vm == null) {
+ throw new CloudRuntimeException("Cannot find underlying VM for
host: " + host.getName());
+ }
+
+ final Map<String, String> sshAccessDetails =
networkOrchestrationService.getSystemVMAccessDetails(vm);
+ final Map<String, String> ipAddressDetails = new
HashMap<>(sshAccessDetails);
+ ipAddressDetails.remove(NetworkElementCommand.ROUTER_NAME);
+
+ try {
+ final Host hypervisorHost = hostDao.findById(vm.getHostId());
+ if (hypervisorHost == null) {
+ throw new CloudRuntimeException("Cannot find hypervisor host
for system VM: " + host.getName());
+ }
+
+ final Certificate certificate = issueCertificate(null,
Arrays.asList(vm.getHostName(), vm.getInstanceName()),
+ new ArrayList<>(ipAddressDetails.values()),
CertValidityPeriod.value(), null);
+ return deployCertificate(hypervisorHost, certificate, reconnect,
sshAccessDetails);
Review Comment:
`provisionSystemVmViaSsh` also hard-codes `caProvider=null` when issuing the
certificate, so the API `provider` parameter is ignored for forced SystemVM
provisioning. This should use the requested provider (or explicitly disallow
`provider` overrides when `forced=true`).
##########
server/src/main/java/org/apache/cloudstack/ca/CAManagerImpl.java:
##########
@@ -177,12 +200,17 @@ public boolean revokeCertificate(final BigInteger
certSerial, final String certC
@Override
@ActionEvent(eventType = EventTypes.EVENT_CA_CERTIFICATE_PROVISION,
eventDescription = "provisioning certificate for host", async = true)
- public boolean provisionCertificate(final Host host, final Boolean
reconnect, final String caProvider) {
+ public boolean provisionCertificate(final Host host, final Boolean
reconnect, final String caProvider, final boolean forced) {
if (host == null) {
throw new CloudRuntimeException("Unable to find valid host to
renew certificate for");
}
CallContext.current().setEventDetails("Host ID: " + host.getUuid());
CallContext.current().putContextParameter(Host.class, host.getUuid());
+
+ if (forced) {
+ return provisionCertificateForced(host, reconnect, caProvider);
+ }
+
String csr = null;
Review Comment:
New behavior paths were added (forced SSH provisioning + JVM default
truststore injection) but there are no unit tests covering these flows. Adding
tests for `forced=true` (KVM and SystemVM) and for `CaInjectDefaultTruststore`
would help prevent regressions in authentication and startup behavior.
--
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]
