Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -80,4 +80,9 @@ public void cleanEncryptedResourceKey(String vmHostBackupFileUuid) {
// do nothing
logger.debug("ignore cleanup encrypted resource key request for VmHostBackupFileVO: " + vmHostBackupFileUuid);
}

@Override
public void cleanTpmKeyBackupEncryptedResourceKey(String tpmKeyBackupUuid) {
logger.debug("ignore cleanup encrypted resource key request for TpmKeyBackupVO: " + tpmKeyBackupUuid);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -97,4 +97,9 @@ static class RestoreEncryptedResourceKeyContext {
void restoreEncryptedResourceKey(RestoreEncryptedResourceKeyContext context);

void cleanEncryptedResourceKey(String vmHostBackupFileUuid);

/**
* Remove encryption key material stored for a {@link org.zstack.header.tpm.entity.TpmKeyBackupVO}.
*/
void cleanTpmKeyBackupEncryptedResourceKey(String tpmKeyBackupUuid);
}
7 changes: 7 additions & 0 deletions conf/db/zsv/V5.1.0__schema.sql
Original file line number Diff line number Diff line change
@@ -1,3 +1,10 @@
CREATE TABLE IF NOT EXISTS `zstack`.`TpmKeyBackupVO` (
`uuid` char(32) NOT NULL UNIQUE,
`lastOpDate` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP,
`createDate` timestamp NOT NULL DEFAULT '1999-12-31 23:59:59',
PRIMARY KEY (`uuid`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8;

DELETE FROM `EncryptedResourceKeyRefVO`
WHERE `resourceUuid` NOT IN (SELECT `uuid` FROM `ResourceVO`);
ALTER TABLE `EncryptedResourceKeyRefVO`
Expand Down
1 change: 1 addition & 0 deletions conf/persistence.xml
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
<class>org.zstack.header.managementnode.ManagementNodeVO</class>
<class>org.zstack.header.managementnode.ManagementNodeContextVO</class>
<class>org.zstack.header.tpm.entity.TpmVO</class>
<class>org.zstack.header.tpm.entity.TpmKeyBackupVO</class>
<class>org.zstack.header.vm.additions.VmHostFileVO</class>
<class>org.zstack.header.vm.additions.VmHostBackupFileVO</class>
<class>org.zstack.header.vm.additions.VmHostFileContentVO</class>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
package org.zstack.header.tpm.entity;

import org.zstack.header.vo.ResourceVO;

import javax.persistence.Column;
import javax.persistence.Entity;
import javax.persistence.Table;
import java.sql.Timestamp;

/**
* Internal holder for a TPM encryption resource key snapshot during snapshot-group revert.
* The key material is stored in {@code EncryptedResourceKeyRefVO} with {@code resourceUuid} = this VO's uuid.
*/
@Entity
@Table
public class TpmKeyBackupVO extends ResourceVO {
@Column
private Timestamp createDate;
@Column
private Timestamp lastOpDate;

public Timestamp getCreateDate() {
return createDate;
}

public void setCreateDate(Timestamp createDate) {
this.createDate = createDate;
}

public Timestamp getLastOpDate() {
return lastOpDate;
}

public void setLastOpDate(Timestamp lastOpDate) {
this.lastOpDate = lastOpDate;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package org.zstack.header.tpm.entity;

import org.zstack.header.vo.ResourceVO_;

import javax.persistence.metamodel.SingularAttribute;
import javax.persistence.metamodel.StaticMetamodel;
import java.sql.Timestamp;

@StaticMetamodel(TpmKeyBackupVO.class)
public class TpmKeyBackupVO_ extends ResourceVO_ {
public static volatile SingularAttribute<TpmKeyBackupVO, Timestamp> createDate;
public static volatile SingularAttribute<TpmKeyBackupVO, Timestamp> lastOpDate;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
package org.zstack.header.tpm.message;

import org.zstack.header.message.NeedReplyMessage;

public class DeleteTpmKeyBackupMsg extends NeedReplyMessage {
private String tpmUuid;
private String tpmKeyBackupUuid;

public String getTpmUuid() {
return tpmUuid;
}

public void setTpmUuid(String tpmUuid) {
this.tpmUuid = tpmUuid;
}

public String getTpmKeyBackupUuid() {
return tpmKeyBackupUuid;
}

public void setTpmKeyBackupUuid(String tpmKeyBackupUuid) {
this.tpmKeyBackupUuid = tpmKeyBackupUuid;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package org.zstack.header.tpm.message;

import org.zstack.header.message.MessageReply;

public class DeleteTpmKeyBackupReply extends MessageReply {
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@
public class RestoreTpmEncryptionKeyMsg extends NeedReplyMessage {
private String srcResourceUuid;
private String dstResourceUuid;
/**
* When true, the current encryption key on {@link #dstResourceUuid} (TPM) is copied to a
* {@link org.zstack.header.tpm.entity.TpmKeyBackupVO} before restoring from {@link #srcResourceUuid}.
*/
private boolean backupCurrentKey = true;

public String getSrcResourceUuid() {
return srcResourceUuid;
Expand All @@ -21,4 +26,12 @@ public String getDstResourceUuid() {
public void setDstResourceUuid(String dstResourceUuid) {
this.dstResourceUuid = dstResourceUuid;
}

public boolean isBackupCurrentKey() {
return backupCurrentKey;
}

public void setBackupCurrentKey(boolean backupCurrentKey) {
this.backupCurrentKey = backupCurrentKey;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,13 @@
import org.zstack.header.message.MessageReply;

public class RestoreTpmEncryptionKeyReply extends MessageReply {
private String tpmKeyBackupUuid;

public String getTpmKeyBackupUuid() {
return tpmKeyBackupUuid;
}

public void setTpmKeyBackupUuid(String tpmKeyBackupUuid) {
this.tpmKeyBackupUuid = tpmKeyBackupUuid;
}
}
59 changes: 54 additions & 5 deletions plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,19 @@
import org.zstack.compute.vm.VmGlobalConfig;
import org.zstack.compute.vm.devices.TpmEncryptedResourceKeyBackend;
import org.zstack.compute.vm.devices.VmTpmManager;
import org.zstack.core.Platform;
import org.zstack.core.asyncbatch.While;
import org.zstack.core.cloudbus.CloudBus;
import org.zstack.core.cloudbus.CloudBusCallBack;
import org.zstack.core.cloudbus.MessageSafe;
import org.zstack.core.db.DatabaseFacade;
import org.zstack.core.db.Q;
import org.zstack.core.db.SQL;
import org.zstack.core.db.SQLBatch;
import org.zstack.core.thread.ChainTask;
import org.zstack.core.thread.SyncTaskChain;
import org.zstack.core.thread.ThreadFacade;
import org.zstack.core.timeout.TimeHelper;
import org.zstack.core.workflow.SimpleFlowChain;
import org.zstack.header.AbstractService;
import org.zstack.header.core.Completion;
Expand All @@ -36,10 +39,13 @@
import org.zstack.header.tpm.api.APIUpdateTpmMsg;
import org.zstack.header.tpm.entity.TpmCapabilityView;
import org.zstack.header.tpm.entity.TpmInventory;
import org.zstack.header.tpm.entity.TpmKeyBackupVO;
import org.zstack.header.tpm.entity.TpmVO;
import org.zstack.header.tpm.entity.TpmVO_;
import org.zstack.header.tpm.message.AddTpmMsg;
import org.zstack.header.tpm.message.AddTpmReply;
import org.zstack.header.tpm.message.DeleteTpmKeyBackupMsg;
import org.zstack.header.tpm.message.DeleteTpmKeyBackupReply;
import org.zstack.header.tpm.message.RestoreTpmEncryptionKeyMsg;
import org.zstack.header.tpm.message.RestoreTpmEncryptionKeyReply;
import org.zstack.header.tpm.message.TpmDeletionMsg;
Expand Down Expand Up @@ -74,6 +80,7 @@
import org.zstack.utils.Utils;
import org.zstack.utils.logging.CLogger;

import java.sql.Timestamp;
import java.util.ArrayList;
import java.util.HashSet;
import java.util.HashMap;
Expand Down Expand Up @@ -113,6 +120,10 @@ public class KvmTpmManager extends AbstractService {
private EncryptedResourceKeyManager resourceKeyManager;
@Autowired
private KvmSecureBootExtensions secureBootExtensions;
@Autowired
private DatabaseFacade databaseFacade;
@Autowired
private TimeHelper timeHelper;

@Override
public boolean start() {
Expand Down Expand Up @@ -153,6 +164,8 @@ private void handleLocalMessage(Message msg) {
handle((BackupTpmEncryptionKeyMsg) msg);
} else if (msg instanceof RestoreTpmEncryptionKeyMsg) {
handle((RestoreTpmEncryptionKeyMsg) msg);
} else if (msg instanceof DeleteTpmKeyBackupMsg) {
handle((DeleteTpmKeyBackupMsg) msg);
} else if (msg instanceof ResetVmTpmMsg) {
handle((ResetVmTpmMsg) msg);
} else {
Expand Down Expand Up @@ -648,11 +661,47 @@ private void handle(BackupTpmEncryptionKeyMsg msg) {
}

private void handle(RestoreTpmEncryptionKeyMsg msg) {
RestoreEncryptedResourceKeyContext content = new RestoreEncryptedResourceKeyContext();
content.srcResourceUuid = msg.getSrcResourceUuid();
content.dstResourceUuid = msg.getDstResourceUuid();
tpmKeyBackend.restoreEncryptedResourceKey(content);
bus.reply(msg, new RestoreTpmEncryptionKeyReply());
RestoreTpmEncryptionKeyReply reply = new RestoreTpmEncryptionKeyReply();
String tpmKeyBackupUuid = null;
try {
if (msg.isBackupCurrentKey()
&& msg.getDstResourceUuid() != null
&& tpmKeyBackend.checkTpmKeyProviderAttached(msg.getDstResourceUuid())) {
tpmKeyBackupUuid = Platform.getUuid();
TpmKeyBackupVO backupVo = new TpmKeyBackupVO();
backupVo.setUuid(tpmKeyBackupUuid);
Timestamp now = new Timestamp(timeHelper.getCurrentTimeMillis());
backupVo.setCreateDate(now);
backupVo.setLastOpDate(now);
databaseFacade.persist(backupVo);
BackupEncryptedResourceKeyContext backupCtx = new BackupEncryptedResourceKeyContext();
backupCtx.srcResourceUuid = msg.getDstResourceUuid();
backupCtx.dstResourceUuid = tpmKeyBackupUuid;
tpmKeyBackend.backupEncryptedResourceKey(backupCtx);
reply.setTpmKeyBackupUuid(tpmKeyBackupUuid);
}

RestoreEncryptedResourceKeyContext content = new RestoreEncryptedResourceKeyContext();
content.srcResourceUuid = msg.getSrcResourceUuid();
content.dstResourceUuid = msg.getDstResourceUuid();
tpmKeyBackend.restoreEncryptedResourceKey(content);
bus.reply(msg, reply);
} catch (Exception t) {
if (tpmKeyBackupUuid != null) {
tpmKeyBackend.cleanTpmKeyBackupEncryptedResourceKey(tpmKeyBackupUuid);
databaseFacade.removeByPrimaryKey(tpmKeyBackupUuid, TpmKeyBackupVO.class);
}
throw t;
}
}

private void handle(DeleteTpmKeyBackupMsg msg) {
DeleteTpmKeyBackupReply reply = new DeleteTpmKeyBackupReply();
if (msg.getTpmKeyBackupUuid() != null) {
tpmKeyBackend.cleanTpmKeyBackupEncryptedResourceKey(msg.getTpmKeyBackupUuid());
databaseFacade.removeByPrimaryKey(msg.getTpmKeyBackupUuid(), TpmKeyBackupVO.class);
}
bus.reply(msg, reply);
}

static class ResetVmTpmContext {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,9 @@
import org.zstack.header.tpm.entity.TpmVO;
import org.zstack.header.tpm.entity.TpmVO_;
import org.zstack.header.tpm.message.AddTpmMsg;
import org.zstack.header.tpm.message.DeleteTpmKeyBackupMsg;
import org.zstack.header.tpm.message.RestoreTpmEncryptionKeyMsg;
import org.zstack.header.tpm.message.RestoreTpmEncryptionKeyReply;
import org.zstack.header.tpm.message.TpmDeletionMsg;
import org.zstack.header.vm.additions.RestoreVmHostFileMsg;
import org.zstack.header.vm.RestoreVmInstanceMsg;
Expand Down Expand Up @@ -389,6 +391,7 @@ class Context {
VolumeSnapshotGroupVO newGroup;
boolean snapshotGroupHasTpm;
String tpmUuid, newCreateTpmUuid;
String tpmKeyBackupUuid;
}
Context context = new Context();
context.snapshotGroupHasTpm = VolumeSnapshotGroupSystemTags.WITH_TPM.hasTag(msg.getUuid());
Expand Down Expand Up @@ -457,11 +460,15 @@ public void run(MessageReply reply) {
RestoreTpmEncryptionKeyMsg restoreMsg = new RestoreTpmEncryptionKeyMsg();
restoreMsg.setSrcResourceUuid(msg.getUuid());
restoreMsg.setDstResourceUuid(context.tpmUuid);
restoreMsg.setBackupCurrentKey(true);
bus.makeTargetServiceIdByResourceUuid(restoreMsg, SERVICE_ID, context.tpmUuid);
bus.send(restoreMsg, new CloudBusCallBack(msg) {
@Override
public void run(MessageReply reply) {
if (reply.isSuccess()) {
if (reply instanceof RestoreTpmEncryptionKeyReply) {
context.tpmKeyBackupUuid = ((RestoreTpmEncryptionKeyReply) reply).getTpmKeyBackupUuid();
}
logger.debug(String.format(
"restore resource key of Tpm[uuid:%s] for VM[uuid:%s] for snapshotGroup[uuid:%s]",
context.tpmUuid, vmUuid, msg.getUuid()));
Expand All @@ -472,7 +479,43 @@ public void run(MessageReply reply) {
}
});
})
// TODO: It should has rollback
.rollback(trigger -> {
if (context.tpmKeyBackupUuid == null) {
trigger.rollback();
return;
}
RestoreTpmEncryptionKeyMsg rollbackMsg = new RestoreTpmEncryptionKeyMsg();
rollbackMsg.setBackupCurrentKey(false);
rollbackMsg.setSrcResourceUuid(context.tpmKeyBackupUuid);
rollbackMsg.setDstResourceUuid(context.tpmUuid);
bus.makeTargetServiceIdByResourceUuid(rollbackMsg, SERVICE_ID, context.tpmUuid);
bus.send(rollbackMsg, new CloudBusCallBack(trigger) {
@Override
public void run(MessageReply reply) {
if (!reply.isSuccess()) {
logger.debug(String.format(
"failed to rollback TPM encryption key from TpmKeyBackupVO[uuid:%s] to Tpm[uuid:%s]",
context.tpmKeyBackupUuid, context.tpmUuid));
}
DeleteTpmKeyBackupMsg delMsg = new DeleteTpmKeyBackupMsg();
delMsg.setTpmUuid(context.tpmUuid);
delMsg.setTpmKeyBackupUuid(context.tpmKeyBackupUuid);
bus.makeTargetServiceIdByResourceUuid(delMsg, SERVICE_ID, context.tpmUuid);
bus.send(delMsg, new CloudBusCallBack(trigger) {
@Override
public void run(MessageReply reply2) {
if (!reply2.isSuccess()) {
logger.debug(String.format(
"failed to delete TpmKeyBackupVO[uuid:%s] after TPM key rollback",
context.tpmKeyBackupUuid));
}
context.tpmKeyBackupUuid = null;
trigger.rollback();
}
});
}
});
})
.build())
.then(Flow.of("remove-tpm-if-needed")
.runIf(data -> !context.snapshotGroupHasTpm && context.tpmUuid != null)
Expand Down Expand Up @@ -549,6 +592,26 @@ public void done(ErrorCodeList errorCodeList) {
});
})
.build())
.then(Flow.of("delete-tpm-key-backup")
.runIf(data -> context.tpmKeyBackupUuid != null)
.handle(trigger -> {
DeleteTpmKeyBackupMsg delMsg = new DeleteTpmKeyBackupMsg();
delMsg.setTpmUuid(context.tpmUuid);
delMsg.setTpmKeyBackupUuid(context.tpmKeyBackupUuid);
bus.makeTargetServiceIdByResourceUuid(delMsg, SERVICE_ID, context.tpmUuid);
bus.send(delMsg, new CloudBusCallBack(trigger) {
@Override
public void run(MessageReply r) {
if (r.isSuccess()) {
context.tpmKeyBackupUuid = null;
trigger.next();
} else {
trigger.fail(r.getError());
}
}
});
})
.build())
Comment on lines +595 to +614
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

备份清理失败不应导致整个 revert 操作失败。

此时主要的 revert 操作(卷恢复、TPM 密钥恢复)已经成功完成。TpmKeyBackupVO 是临时性的辅助记录,其清理失败不应该使整个流程失败。建议改为记录警告日志并继续执行。

🔧 建议的修复方案
             .then(Flow.of("delete-tpm-key-backup")
                 .runIf(data -> context.tpmKeyBackupUuid != null)
                 .handle(trigger -> {
                     DeleteTpmKeyBackupMsg delMsg = new DeleteTpmKeyBackupMsg();
                     delMsg.setTpmUuid(context.tpmUuid);
                     delMsg.setTpmKeyBackupUuid(context.tpmKeyBackupUuid);
                     bus.makeTargetServiceIdByResourceUuid(delMsg, SERVICE_ID, context.tpmUuid);
                     bus.send(delMsg, new CloudBusCallBack(trigger) {
                         `@Override`
                         public void run(MessageReply r) {
-                            if (r.isSuccess()) {
-                                context.tpmKeyBackupUuid = null;
-                                trigger.next();
-                            } else {
-                                trigger.fail(r.getError());
+                            if (!r.isSuccess()) {
+                                logger.warn(String.format(
+                                        "failed to delete TpmKeyBackupVO[uuid:%s] after successful revert, but continuing: %s",
+                                        context.tpmKeyBackupUuid, r.getError().getDetails()));
                             }
+                            context.tpmKeyBackupUuid = null;
+                            trigger.next();
                         }
                     });
                 })
                 .build())
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@storage/src/main/java/org/zstack/storage/snapshot/group/VolumeSnapshotGroupBase.java`
around lines 595 - 614, The delete-tpm-key-backup flow in
VolumeSnapshotGroupBase currently treats a DeleteTpmKeyBackupMsg failure as
fatal (calls trigger.fail), but TPM key backup cleanup is best-effort and should
not abort the overall revert; change the CloudBusCallBack in the
"delete-tpm-key-backup" Flow (the bus.send callback handling
DeleteTpmKeyBackupMsg) so that on r.isSuccess() it clears
context.tpmKeyBackupUuid and calls trigger.next(), and on failure it logs a
warning (including r.getError()) instead of calling trigger.fail, then still
calls trigger.next() so the main revert continues.

.propagateExceptionTo(msg)
.done(() -> bus.reply(msg, reply))
.error(errorCode -> {
Expand Down