Skip to content

<fix>[crypto]: clean TPM key ref on VM destroy #3679

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-1 wants to merge 3 commits into
base: feature-zsv-5.0.0-vm-support-vtpm-and-secuceboot
Choose a base branch
from
+94 −9
Open

<fix>[crypto]: clean TPM key ref on VM destroy #3679

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1327d10
<fix>[crypto]: keep TPM key ref until VM removed from DB
Apr 3, 2026
ca5b400
Merge remote-tracking branch 'origin/feature-zsv-5.0.0-vm-support-vtp…
Apr 4, 2026
04c3a44
<fix>[crypto]: keep TPM key ref until VM removed from DB
Apr 4, 2026
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
16 changes: 16 additions & 0 deletions compute/src/main/java/org/zstack/compute/vm/VmInstanceBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import org.springframework.dao.DataIntegrityViolationException;
import org.springframework.transaction.annotation.Transactional;
import org.zstack.compute.allocator.HostAllocatorManager;
import org.zstack.compute.vm.devices.VmTpmManager;
import org.zstack.core.Platform;
import org.zstack.core.asyncbatch.While;
import org.zstack.core.cascade.CascadeConstant;
Expand Down Expand Up @@ -142,6 +143,8 @@ public class VmInstanceBase extends AbstractVmInstance {
private VmInstanceResourceMetadataManager vidm;
@Autowired
private NetworkServiceManager nwServiceMgr;
@Autowired
private VmTpmManager vmTpmManager;

protected VmInstanceVO self;
protected VmInstanceVO originalCopy;
Expand Down Expand Up @@ -1272,6 +1275,8 @@ public void handle(Map data) {
CollectionUtils.safeForEach(pluginRgty.getExtensionList(VmAfterExpungeExtensionPoint.class),
arg -> arg.vmAfterExpunge(inv));

final String tpmUuidForEncryptedKeyRef = vmTpmManager.findTpmUuidForVmOrNull(self.getUuid());

callVmJustBeforeDeleteFromDbExtensionPoint();

dbf.reload(self);
Expand All @@ -1283,6 +1288,9 @@ public void handle(Map data) {
if (inv.getRootVolumeUuid() != null) {
dbf.eoCleanup(VolumeVO.class, inv.getRootVolumeUuid());
}
if (tpmUuidForEncryptedKeyRef != null) {
vmTpmManager.deleteResourceKeyRefForTpm(tpmUuidForEncryptedKeyRef);
}
Comment on lines +1291 to +1293
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Apr 4, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

删除后置清理缺少异常隔离，可能导致“VM 已删除但流程返回失败”

Line 1292 与 Line 2599 在 VM 记录已删除/清理后直接调用 deleteResourceKeyRefForTpm。若该调用抛异常，会中断后续成功路径（如 completion.success()、afterDestroyVm），造成对外返回失败但资源已不可恢复。建议将该清理改为 best-effort 并记录告警日志。

🔧 建议修复 
@@
     private void callVmJustAfterDeleteFromDbExtensionPoint(VmInstanceInventory inv, String accountUuid) {
         CollectionUtils.safeForEach(pluginRgty.getExtensionList(VmJustAfterDeleteFromDbExtensionPoint.class), p -> p.vmJustAfterDeleteFromDbExtensionPoint(inv, accountUuid));
     }
+
+    private void safeDeleteResourceKeyRefForTpm(String tpmUuid) {
+        if (tpmUuid == null) {
+            return;
+        }
+        try {
+            vmTpmManager.deleteResourceKeyRefForTpm(tpmUuid);
+        } catch (Exception e) {
+            logger.warn(String.format("failed to delete TPM resource key ref for tpm[uuid:%s], continue VM delete flow", tpmUuid), e);
+        }
+    }
@@
-                if (tpmUuidForEncryptedKeyRef != null) {
-                    vmTpmManager.deleteResourceKeyRefForTpm(tpmUuidForEncryptedKeyRef);
-                }
+                safeDeleteResourceKeyRefForTpm(tpmUuidForEncryptedKeyRef);
@@
-                    if (tpmUuidForEncryptedKeyRef != null) {
-                        vmTpmManager.deleteResourceKeyRefForTpm(tpmUuidForEncryptedKeyRef);
-                    }
+                    safeDeleteResourceKeyRefForTpm(tpmUuidForEncryptedKeyRef);

Also applies to: 2598-2600

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

In `@compute/src/main/java/org/zstack/compute/vm/VmInstanceBase.java` around lines
1291 - 1293, The call to vmTpmManager.deleteResourceKeyRefForTpm in
VmInstanceBase is currently unprotected and can throw, causing the VM deletion
flow (e.g., completion.success() / afterDestroyVm) to fail even though the VM
was removed; change both occurrences (the one after VM record cleanup and the
one around lines 2598-2600) to best-effort cleanup by wrapping
vmTpmManager.deleteResourceKeyRefForTpm(...) in a try-catch that catches
Exception, logs a warning/error including the TPM UUID and exception details,
and does not rethrow so the success path and downstream cleanup
(completion.success(), afterDestroyVm) proceed. Ensure the log message is
descriptive and include the VM id/tpmUuid to aid debugging.

completion.success();
}
}).error(new FlowErrorHandler(completion) {
Expand Down Expand Up @@ -2582,12 +2590,17 @@ public void success() {
if (self.getState() != VmInstanceState.Destroyed) {
changeVmStateInDb(VmInstanceStateEvent.destroyed);
}
final String tpmUuidForEncryptedKeyRef = vmTpmManager.findTpmUuidForVmOrNull(self.getUuid());
callVmJustBeforeDeleteFromDbExtensionPoint();
dbf.removeCollection(self.getVmCdRoms(), VmCdRomVO.class);
dbf.remove(getSelf());
dbf.eoCleanup(VmInstanceVO.class, self.getUuid());
if (tpmUuidForEncryptedKeyRef != null) {
vmTpmManager.deleteResourceKeyRefForTpm(tpmUuidForEncryptedKeyRef);
}
} else if (deletionPolicy == VmInstanceDeletionPolicy.DBOnly || deletionPolicy == VmInstanceDeletionPolicy.KeepVolume) {
String accountUuid = acntMgr.getOwnerAccountUuidOfResource(inv.getUuid());
final String tpmUuidForEncryptedKeyRef = vmTpmManager.findTpmUuidForVmOrNull(self.getUuid());
new SQLBatch() {
@Override
protected void scripts() {
Expand All @@ -2599,6 +2612,9 @@ protected void scripts() {
.hardDelete();
sql(VmCdRomVO.class).eq(VmCdRomVO_.vmInstanceUuid, self.getUuid()).hardDelete();
sql(VmInstanceVO.class).eq(VmInstanceVO_.uuid, self.getUuid()).hardDelete();
if (tpmUuidForEncryptedKeyRef != null) {
vmTpmManager.deleteResourceKeyRefForTpm(tpmUuidForEncryptedKeyRef);
}
}
}.execute();
callVmJustAfterDeleteFromDbExtensionPoint(inv, accountUuid);
Expand Down
5 changes: 5 additions & 0 deletions compute/src/main/java/org/zstack/compute/vm/devices/DummyEncryptedResourceKeyManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,9 @@ public void getOrCreateKey(GetOrCreateResourceKeyContext ctx,
public void rollbackCreatedKey(ResourceKeyResult result, Completion completion) {
completion.success();
}

@Override
public void deleteRef(String resourceType, String resourceUuid) {
// do-nothing
}
}
6 changes: 1 addition & 5 deletions compute/src/main/java/org/zstack/compute/vm/devices/VmTpmExtensions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,11 +70,7 @@ public void afterRollbackPersistVmInstanceVO(VmInstanceVO vo, CreateVmInstanceMs
new SQLBatch() {
@Override
protected void scripts() {
try {
resourceKeyBackend.detachKeyProviderFromTpm(tpmUuid);
} finally {
vmTpmManager.deleteTpmVO(tpmUuid);
}
vmTpmManager.deleteTpmAndResourceKeyRef(tpmUuid);
}
}.execute();
}
Expand Down
31 changes: 31 additions & 0 deletions compute/src/main/java/org/zstack/compute/vm/devices/VmTpmManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import org.zstack.core.db.DatabaseFacade;
import org.zstack.core.db.Q;
import org.zstack.header.image.ImageBootMode;
import org.zstack.header.keyprovider.EncryptedResourceKeyManager;
import org.zstack.header.tpm.entity.TpmVO;
import org.zstack.header.tpm.entity.TpmVO_;
import org.zstack.resourceconfig.ResourceConfig;
Expand All @@ -23,6 +24,8 @@ public class VmTpmManager {
private DatabaseFacade databaseFacade;
@Autowired
private ResourceConfigFacade resourceConfigFacade;
@Autowired
private EncryptedResourceKeyManager resourceKeyManager;

public TpmVO persistTpmVO(String tpmUuid, String vmUuid) {
if (tpmUuid == null) {
Expand All @@ -42,6 +45,34 @@ public void deleteTpmVO(String tpmUuid) {
databaseFacade.removeByPrimaryKey(tpmUuid, TpmVO.class);
}

public String findTpmUuidForVmOrNull(String vmInstanceUuid) {
return Q.New(TpmVO.class)
.eq(TpmVO_.vmInstanceUuid, vmInstanceUuid)
.select(TpmVO_.uuid)
.findValue();
}

public void deleteResourceKeyRefForTpm(String tpmUuid) {
if (tpmUuid == null) {
return;
}
try {
resourceKeyManager.deleteRef(TpmVO.class.getSimpleName(), tpmUuid);
} catch (Throwable t) {
logger.warn(String.format(
"failed to delete encrypted resource key ref for TPM[uuid:%s]; continuing VM cleanup: %s",
tpmUuid, t.getMessage()), t);
}
}

public void deleteTpmAndResourceKeyRef(String tpmUuid) {
try {
deleteResourceKeyRefForTpm(tpmUuid);
} finally {
deleteTpmVO(tpmUuid);
}
}

/**
* @param bootMode boot mode, null is Legacy
*/
Expand Down
15 changes: 15 additions & 0 deletions header/src/main/java/org/zstack/header/keyprovider/EncryptedResourceKeyManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,17 @@ void getOrCreateKey(GetOrCreateResourceKeyContext ctx,
*/
void rollbackCreatedKey(ResourceKeyResult result, Completion completion);

/**
* Delete the resource-to-key-provider relationship for the specified resource.
* This only removes the persisted ref record and does not delete the remote secret.
*/
void deleteRef(String resourceType, String resourceUuid);

class GetOrCreateResourceKeyContext {
private String resourceUuid;
private String resourceType;
private String keyProviderUuid;
private String keyProviderName;
private String purpose;

public String getResourceUuid() {
Expand All @@ -64,6 +71,14 @@ public void setKeyProviderUuid(String keyProviderUuid) {
this.keyProviderUuid = keyProviderUuid;
}

public String getKeyProviderName() {
return keyProviderName;
}

public void setKeyProviderName(String keyProviderName) {
this.keyProviderName = keyProviderName;
}

public String getPurpose() {
return purpose;
}
Expand Down
6 changes: 4 additions & 2 deletions plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmExtensions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ public void fail(ErrorCode errorCode) {
@Override
public boolean skip(Map data) {
boolean shouldSkip = VmGlobalConfig.ALLOWED_TPM_VM_WITHOUT_KMS.value(Boolean.class) &&
(StringUtils.isBlank(context.providerUuid) || StringUtils.isBlank(context.providerName));
(StringUtils.isBlank(context.providerUuid) && StringUtils.isBlank(context.providerName));
if (shouldSkip) {
logger.info("skip create-dek: allowed.tpm.vm.without.kms is enabled and no KMS provider bound");
}
Expand All @@ -182,7 +182,7 @@ public boolean skip(Map data) {

@Override
public void run(FlowTrigger trigger, Map data) {
if (StringUtils.isBlank(context.providerUuid) || StringUtils.isBlank(context.providerName)) {
if (StringUtils.isBlank(context.providerUuid) && StringUtils.isBlank(context.providerName)) {
trigger.fail(operr("missing TPM resource key binding for tpm[uuid:%s], attachKeyProviderToTpm must run before create-dek", context.tpmUuid));
return;
}
Expand All @@ -191,13 +191,15 @@ public void run(FlowTrigger trigger, Map data) {
keyCtx.setResourceUuid(context.tpmUuid);
keyCtx.setResourceType(TpmVO.class.getSimpleName());
keyCtx.setKeyProviderUuid(context.providerUuid);
keyCtx.setKeyProviderName(context.providerName);
keyCtx.setPurpose("vtpm");

resourceKeyManager.getOrCreateKey(keyCtx, new ReturnValueCompletion<ResourceKeyResult>(trigger) {
@Override
public void success(ResourceKeyResult result) {
tpmSpec.setResourceKeyCreatedNew(result.isCreatedNewKey());
tpmSpec.setResourceKeyProviderUuid(result.getKeyProviderUuid());
context.providerUuid = result.getKeyProviderUuid();
context.providerName = result.getKeyProviderName();
context.dekBase64 = result.getDekBase64();
trigger.next();
Expand Down
8 changes: 6 additions & 2 deletions plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,11 @@ private void addTpmToVm(AddTpmToVmContext context, Completion completion) {
})
.rollback(trigger -> {
if (context.tpmCreated && context.createdTpmUuid != null) {
vmTpmManager.deleteTpmVO(context.createdTpmUuid);
if (context.keyProviderAttached) {
vmTpmManager.deleteTpmAndResourceKeyRef(context.createdTpmUuid);
} else {
vmTpmManager.deleteTpmVO(context.createdTpmUuid);
}
}
trigger.rollback();
})
Expand Down Expand Up @@ -392,7 +396,7 @@ public void done(ErrorCodeList errorCodeList) {
.build())
.then(Flow.of("detach-resource-key")
.handle(trigger -> {
tpmKeyBackend.detachKeyProviderFromTpm(context.tpmUuid);
vmTpmManager.deleteResourceKeyRefForTpm(context.tpmUuid);
trigger.next();
})
.build())
Expand Down
16 changes: 16 additions & 0 deletions sdk/src/main/java/org/zstack/sdk/keyprovider/api/RekeyKeyProviderRefsResult.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,14 @@ public int getSkippedCount() {
return this.skippedCount;
}

public int failedCount;
public void setFailedCount(int failedCount) {
this.failedCount = failedCount;
}
public int getFailedCount() {
return this.failedCount;
}

public java.util.List skippedResources;
public void setSkippedResources(java.util.List skippedResources) {
this.skippedResources = skippedResources;
Expand All @@ -35,4 +43,12 @@ public java.util.List getSkippedResources() {
return this.skippedResources;
}

public java.util.List failedResources;
public void setFailedResources(java.util.List failedResources) {
this.failedResources = failedResources;
}
public java.util.List getFailedResources() {
return this.failedResources;
}

}