Skip to content

<fix>[kvm]: update TLS certs via kvmagent on host reconnect #3706

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
zstack-robot-2 wants to merge 1 commit into
base: 5.5.12
Choose a base branch
from
Open

<fix>[kvm]: update TLS certs via kvmagent on host reconnect #3706

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions plugin/kvm/src/main/java/org/zstack/kvm/KVMAgentCommands.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -455,6 +455,23 @@ public void setFailedInterfaceNames(List<String> failedInterfaceNames) {
public static class HostFactCmd extends AgentCommand {
}

public static class UpdateTlsCertCmd extends AgentCommand {
private String caCert;
@NoLogging
private String caKey;
private String certIps;

public String getCaCert() { return caCert; }
public void setCaCert(String caCert) { this.caCert = caCert; }
public String getCaKey() { return caKey; }
public void setCaKey(String caKey) { this.caKey = caKey; }
public String getCertIps() { return certIps; }
public void setCertIps(String certIps) { this.certIps = certIps; }
}

public static class UpdateTlsCertResponse extends AgentResponse {
}

public static class HostFactResponse extends AgentResponse {
@GrayVersion(value = "5.0.0")
private String osDistribution;
Expand Down
1 change: 1 addition & 0 deletions plugin/kvm/src/main/java/org/zstack/kvm/KVMConstant.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ public interface KVMConstant {
String KVM_DELETE_CONSOLE_FIREWALL_PATH = "/vm/console/deletefirewall";
String KVM_UPDATE_HOST_OS_PATH = "/host/updateos";
String KVM_HOST_UPDATE_DEPENDENCY_PATH = "/host/updatedependency";
String KVM_UPDATE_TLS_CERT_PATH = "/host/updateTlsCert";
String HOST_SHUTDOWN = "/host/shutdown";
String HOST_REBOOT = "/host/reboot";
String HOST_UPDATE_SPICE_CHANNEL_CONFIG_PATH = "/host/updateSpiceChannelConfig";
Expand Down
70 changes: 70 additions & 0 deletions plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import org.zstack.core.db.SQLBatch;
import org.zstack.core.db.SimpleQuery;
import org.zstack.core.db.SimpleQuery.Op;
import org.zstack.core.jsonlabel.JsonLabel;
import org.zstack.core.thread.*;
import org.zstack.core.timeout.ApiTimeoutManager;
import org.zstack.core.timeout.TimeHelper;
Expand Down Expand Up @@ -195,6 +196,7 @@ public class KVMHost extends HostBase implements Host {
private String checkSnapshotPath;
private String mergeSnapshotPath;
private String hostFactPath;
private String updateTlsCertPath;
private String hostCheckFilePath;
private String attachIsoPath;
private String detachIsoPath;
Expand Down Expand Up @@ -328,6 +330,10 @@ public KVMHost(KVMHostVO self, KVMHostContext context) {
ub.path(KVMConstant.KVM_HOST_FACT_PATH);
hostFactPath = ub.build().toString();

ub = UriComponentsBuilder.fromHttpUrl(baseUrl);
ub.path(KVMConstant.KVM_UPDATE_TLS_CERT_PATH);
updateTlsCertPath = ub.build().toString();

ub = UriComponentsBuilder.fromHttpUrl(baseUrl);
ub.path(KVMConstant.KVM_HOST_CHECK_FILE_PATH);
hostCheckFilePath = ub.build().toString();
Expand Down Expand Up @@ -6025,6 +6031,70 @@ public void fail(ErrorCode errorCode) {

flow(createCollectHostFactsFlow(info));

flow(new NoRollbackFlow() {
String __name__ = "update-tls-certs-if-needed";

@Override
public boolean skip(Map data) {
return CoreGlobalProperty.UNIT_TEST_ON
|| !KVMGlobalConfig.LIBVIRT_TLS_ENABLED.value(Boolean.class);
}

@Override
public void run(FlowTrigger trigger, Map data) {
String managementIp = getSelf().getManagementIp();
String extraIps = HostSystemTags.EXTRA_IPS.getTokenByResourceUuid(
self.getUuid(), HostSystemTags.EXTRA_IPS_TOKEN);

List<String> allIps = new ArrayList<>();
allIps.add(managementIp);
if (extraIps != null && !extraIps.isEmpty()) {
for (String ip : extraIps.split(",")) {
String trimmed = ip.trim();
if (!trimmed.isEmpty() && !allIps.contains(trimmed)) {
allIps.add(trimmed);
}
}
}

String certIps = String.join(",", allIps);

String caCert = new JsonLabel().get("libvirtTLSCA", String.class);
String caKey = new JsonLabel().get("libvirtTLSPrivateKey", String.class);
if (caCert == null || caKey == null) {
logger.warn("TLS CA cert/key not found in database, skipping cert update");
Comment on lines +6062 to +6065
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

把空白证书内容也当成缺失处理

这里只判断了 null，但 "" 或全空白字符串会继续下发到 agent，绕过“缺失就跳过”的保护分支。这里先做 trimToNull 更稳妥。

🩹 建议修改 
-                        String caCert = new JsonLabel().get("libvirtTLSCA", String.class);
-                        String caKey = new JsonLabel().get("libvirtTLSPrivateKey", String.class);
+                        String caCert = StringUtils.trimToNull(new JsonLabel().get("libvirtTLSCA", String.class));
+                        String caKey = StringUtils.trimToNull(new JsonLabel().get("libvirtTLSPrivateKey", String.class));
                         if (caCert == null || caKey == null) {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
String caCert = new JsonLabel().get("libvirtTLSCA", String.class);
String caKey = new JsonLabel().get("libvirtTLSPrivateKey", String.class);
if (caCert == null || caKey == null) {
logger.warn("TLS CA cert/key not found in database, skipping cert update");
String caCert = StringUtils.trimToNull(new JsonLabel().get("libvirtTLSCA", String.class));
String caKey = StringUtils.trimToNull(new JsonLabel().get("libvirtTLSPrivateKey", String.class));
if (caCert == null || caKey == null) {
logger.warn("TLS CA cert/key not found in database, skipping cert update");
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java` around lines 6062 -
6065, The current check only treats caCert/caKey as missing when null, allowing
empty or whitespace-only strings to be sent; update the retrieval and check
around JsonLabel.get("libvirtTLSCA", String.class) and
JsonLabel.get("libvirtTLSPrivateKey", String.class) (used in the block that logs
"TLS CA cert/key not found in database, skipping cert update") to treat blank
values as missing by applying a trimToNull-style normalization (or
StringUtils.isBlank) and then conditionally skipping and logging when the
normalized values are null/blank.

trigger.next();
return;
}

UpdateTlsCertCmd cmd = new UpdateTlsCertCmd();
cmd.setCaCert(caCert);
cmd.setCaKey(caKey);
cmd.setCertIps(certIps);

new Http<>(updateTlsCertPath, cmd, UpdateTlsCertResponse.class)
.call(new ReturnValueCompletion<UpdateTlsCertResponse>(trigger) {
@Override
public void success(UpdateTlsCertResponse ret) {
if (!ret.isSuccess()) {
logger.warn(String.format("Failed to update TLS certs on host[uuid:%s]: %s",
self.getUuid(), ret.getError()));
}
// cert update failure should not block reconnect
trigger.next();
}

@Override
public void fail(ErrorCode errorCode) {
logger.warn(String.format("Failed to update TLS certs on host[uuid:%s]: %s",
self.getUuid(), errorCode));
// cert update failure should not block reconnect
trigger.next();
}
});
}
});
Comment on lines +6034 to +6096
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

避免在连接流程里无条件覆盖宿主机现有 TLS 证书

这里在 LIBVIRT_TLS_ENABLED 打开后会直接向 host 下发并更新 libvirt TLS 证书，但没有判断当前证书是否由平台托管，也没有记录原状态或提供回退路径。这样会把宿主机上可能由用户手工维护的证书一起覆盖掉，尤其这段逻辑还会跑在新增/重连流程里。建议至少加上“仅处理 ZStack 托管证书”的标记/指纹校验，或者放到显式开关后面。

As per coding guidelines “平台不应直接覆盖用户可能自定义修改过的东西……应当对原状态做好记录，做好二次确认，做好回退准备”.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java` around lines 6034 -
6096, The flow "update-tls-certs-if-needed" currently unconditionally pushes
libvirt TLS certs; change it to first read the host's existing libvirt cert
metadata and only proceed if the cert is already managed by ZStack (e.g.,
compare a stored marker/fingerprint or host tag), otherwise skip or require an
explicit override flag (add a new config like
KVMGlobalConfig.LIBVIRT_TLS_FORCE_UPDATE). If proceeding, before calling
UpdateTlsCertCmd record the original cert/key/fingerprint (store under JsonLabel
keys such as "libvirtTLSOriginalCert"/"libvirtTLSOriginalKey" or a host tag) so
you can roll back, and modify the success/fail handlers to leave originals
intact and expose a rollback path; ensure checks live inside the run(...) of the
NoRollbackFlow and gate the new Http<>(updateTlsCertPath, ...) call by the
marker/fingerprint/or-force-flag logic.


if (info.isNewAdded()) {
flow(new NoRollbackFlow() {
String __name__ = "check-qemu-libvirt-version";
Expand Down