<fix>[crypto]: fill in secret lifecyle operations#3684
<fix>[crypto]: fill in secret lifecyle operations#3684zstack-robot-1 wants to merge 1 commit intofeature-zsv-5.0.0-vm-support-vtpm-and-secucebootfrom
Conversation
Walkthrough引入密钥版本(keyVersion)以取代 providerName,用于 TPM 加密资源密钥的查找与生命周期;增加主机端密钥查询消息/命令流水并更新 KVM 代理交互、TPM 扩展流程和测试模拟器的缓存键逻辑。 Changes
Sequence Diagram(s)sequenceDiagram
participant TPMExt as TPM 扩展
participant KeyMgr as 密钥管理器
participant KVMHost as KVM 主机
participant Agent as KVM 代理
participant Cache as 缓存/数据库
TPMExt->>KeyMgr: getOrCreateKey(...)
KeyMgr-->>TPMExt: ResourceKeyResult (含 keyVersion)
TPMExt->>KVMHost: SecretHostGetMsg(vmUuid, purpose, keyVersion)
KVMHost->>Agent: SecretHostGetCmd(vmUuid, purpose, keyVersion)
Agent->>Cache: 查询复合缓存键(hostUuid,vmUuid,purpose,keyVersion)
Cache-->>Agent: secretUuid 或 未找到 错误
alt secret 存在
Agent-->>KVMHost: SecretHostGetResponse(secretUuid)
KVMHost-->>TPMExt: SecretHostGetReply(secretUuid)
else 未找到
Agent-->>KVMHost: AgentResponse(error: KEY_AGENT_SECRET_NOT_FOUND)
KVMHost->>Agent: SecretHostDefineCmd(keyVersion, dekEnvelope...)
Agent->>Cache: 创建/更新 secret(返回 secretUuid)
Agent-->>KVMHost: define response(secretUuid)
KVMHost-->>TPMExt: SecretHostGetReply(secretUuid)
end
sequenceDiagram
participant Client as 调用方
participant KVMHost as KVM 主机
participant Agent as KVM 代理
Client->>KVMHost: SecretHostGetMsg(hostUuid, vmUuid, purpose, keyVersion)
KVMHost->>KVMHost: 验证 vmUuid/purpose/keyVersion 非空
alt 验证通过
KVMHost->>Agent: SecretHostGetCmd(vmUuid,purpose,keyVersion)
Agent->>Agent: 查询缓存(复合键)
alt 找到
Agent-->>KVMHost: SecretHostGetResponse(secretUuid)
KVMHost-->>Client: SecretHostGetReply(secretUuid)
else 未找到
Agent-->>KVMHost: AgentResponse(error: KEY_AGENT_SECRET_NOT_FOUND)
KVMHost-->>Client: SecretHostGetReply(error)
end
else 验证失败
KVMHost-->>Client: SecretHostGetReply(error)
end
Estimated code review effort🎯 4 (复杂) | ⏱️ ~50 分钟 Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.42.0)plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.javaComment |
a66d6bf to
c47d1a4
Compare
Resolves: ZSV-11630 Change-Id: I616a776e78666179637976616c776162616c6577
c47d1a4 to
a0354b4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmExtensions.java (1)
177-183:⚠️ Potential issue | 🔴 Critical同步跳过无 KMS 场景下的 DEK 加载。
前一个 flow 已经把
allowed.tpm.vm.without.kms && providerUuid为空视为合法放行,但这里仍会继续调用getOrCreateKey()并传入空的keyProviderUuid。这样会把本来允许的无 KMS 路径重新变成失败路径。💡 建议修改
`@Override` public boolean skip(Map data) { - return StringUtils.isNotBlank(spec.getDevicesSpec().getTpm().getSecretUuid()) || context.dekBase64 != null; + boolean skipWithoutKms = VmGlobalConfig.ALLOWED_TPM_VM_WITHOUT_KMS.value(Boolean.class) + && StringUtils.isBlank(context.providerUuid); + return skipWithoutKms + || StringUtils.isNotBlank(spec.getDevicesSpec().getTpm().getSecretUuid()) + || context.dekBase64 != null; }Also applies to: 265-275
🤖 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/tpm/KvmTpmExtensions.java` around lines 177 - 183, 当前在 KvmTpmExtensions 中即使在 allowed.tpm.vm.without.kms 且 providerUuid 为空时会设置 shouldSkip=true,后续仍会调用 getOrCreateKey() 导致本应允许的无 KMS 路径变成失败。修复方法:在涉及创建/加载 DEK 的流程里(查找使用 shouldSkip 变量的函数),在任何调用 getOrCreateKey(keyProviderUuid, ...) 之前立即判断 shouldSkip 并直接返回/跳过创建流程;同样在文件中另一个相同位置(原注释提到的 265-275 区块)也做相同早退处理,确保 getOrCreateKey 不会在 providerUuid 为空且允许无 KMS 的情况下被调用。
🧹 Nitpick comments (2)
plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java (2)
5319-5323:vmUuid/purpose/keyVersion的校验逻辑可抽成私有方法以减少重复。两处参数校验结构一致,抽取后可降低后续字段演进时的漏改风险。
Also applies to: 5361-5365
🤖 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 5319 - 5323, Extract the repeated vmUuid/purpose/keyVersion validation into a private helper (e.g., a method on KVMHost like validateSecretRequest or checkSecretRequestParams) that accepts the request message (the object with getVmUuid(), getPurpose(), getKeyVersion()) and either returns an Optional<ErrorCode>/boolean or throws/returns a prepared ErrorCode; replace both locations (the blocks using msg.getVmUuid(), msg.getPurpose(), msg.getKeyVersion() and bus.reply(msg, reply)) to call the helper and, on failure, set reply.setError(...) and bus.reply(msg, reply) as before. Ensure the helper name and signature make clear it is for secret-get requests and keep existing reply behavior intact.
5468-5476: 统一错误构造方式,与代码库中的既有模式保持一致。当前
buildSecretAgentError()方法直接创建ErrorCode并设置动态错误文本为 code,这与代码库中同文件内其他地方的处理模式不一致。建议采用operr(defaultMessage).withOpaque("response.error", rsp.getError())的模式,该模式已在此文件多处使用(如 NQN/主机名/配额/虚拟机信息的更新处理中),可保证错误码语义的稳定性。同时建议用StringUtils.isNotBlank()替代空值检查。♻️ 建议修改
private ErrorCode buildSecretAgentError(KVMAgentCommands.AgentResponse rsp, String defaultMessage) { - if (rsp != null && rsp.getError() != null) { - ErrorCode err = new ErrorCode(); - err.setCode(rsp.getError()); - err.setDetails(rsp.getError()); - return err; - } - return operr(defaultMessage); + if (rsp != null && StringUtils.isNotBlank(rsp.getError())) { + return operr(defaultMessage).withOpaque("response.error", rsp.getError()); + } + return operr(defaultMessage); }🤖 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 5468 - 5476, The buildSecretAgentError(KVMAgentCommands.AgentResponse rsp, String defaultMessage) currently constructs ErrorCode directly and uses rsp.getError() as the code; change it to follow the project pattern by returning operr(defaultMessage).withOpaque("response.error", rsp.getError()) when rsp.getError() is present, and use StringUtils.isNotBlank(rsp.getError()) to check for non-empty error text; keep the fallback to operr(defaultMessage) when no error is present so callers of buildSecretAgentError behave consistently with other error builders in this file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmExtensions.java`:
- Around line 249-257: The SECRET_NOT_FOUND branch currently calls
trigger.next() but leaves the prior secretUuid in the flow context, causing
later flows (load-dek-for-host, define-secret-on-host) to skip rebuilds; modify
the branch that checks SecretHostGetReply.ERROR_CODE_SECRET_NOT_FOUND (in
KvmTpmExtensions.java) to clear the stored secretUuid from the context/state
(set it to null or remove the key where secretUuid is kept) before calling
trigger.next(), so subsequent flows will treat secretUuid as empty and properly
recreate the secret on the new host.
---
Outside diff comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmExtensions.java`:
- Around line 177-183: 当前在 KvmTpmExtensions 中即使在 allowed.tpm.vm.without.kms 且
providerUuid 为空时会设置 shouldSkip=true,后续仍会调用 getOrCreateKey() 导致本应允许的无 KMS
路径变成失败。修复方法:在涉及创建/加载 DEK 的流程里(查找使用 shouldSkip 变量的函数),在任何调用
getOrCreateKey(keyProviderUuid, ...) 之前立即判断 shouldSkip
并直接返回/跳过创建流程;同样在文件中另一个相同位置(原注释提到的 265-275 区块)也做相同早退处理,确保 getOrCreateKey 不会在
providerUuid 为空且允许无 KMS 的情况下被调用。
---
Nitpick comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java`:
- Around line 5319-5323: Extract the repeated vmUuid/purpose/keyVersion
validation into a private helper (e.g., a method on KVMHost like
validateSecretRequest or checkSecretRequestParams) that accepts the request
message (the object with getVmUuid(), getPurpose(), getKeyVersion()) and either
returns an Optional<ErrorCode>/boolean or throws/returns a prepared ErrorCode;
replace both locations (the blocks using msg.getVmUuid(), msg.getPurpose(),
msg.getKeyVersion() and bus.reply(msg, reply)) to call the helper and, on
failure, set reply.setError(...) and bus.reply(msg, reply) as before. Ensure the
helper name and signature make clear it is for secret-get requests and keep
existing reply behavior intact.
- Around line 5468-5476: The
buildSecretAgentError(KVMAgentCommands.AgentResponse rsp, String defaultMessage)
currently constructs ErrorCode directly and uses rsp.getError() as the code;
change it to follow the project pattern by returning
operr(defaultMessage).withOpaque("response.error", rsp.getError()) when
rsp.getError() is present, and use StringUtils.isNotBlank(rsp.getError()) to
check for non-empty error text; keep the fallback to operr(defaultMessage) when
no error is present so callers of buildSecretAgentError behave consistently with
other error builders in this file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bf83a882-9937-4cbb-82e3-af586d4a0920
📒 Files selected for processing (11)
compute/src/main/java/org/zstack/compute/vm/devices/DummyTpmEncryptedResourceKeyBackend.javacompute/src/main/java/org/zstack/compute/vm/devices/TpmEncryptedResourceKeyBackend.javaheader/src/main/java/org/zstack/header/keyprovider/EncryptedResourceKeyManager.javaheader/src/main/java/org/zstack/header/secret/SecretHostDefineMsg.javaheader/src/main/java/org/zstack/header/secret/SecretHostGetMsg.javaheader/src/main/java/org/zstack/header/secret/SecretHostGetReply.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMAgentCommands.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMConstant.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMHost.javaplugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmExtensions.javatestlib/src/main/java/org/zstack/testlib/KVMSimulator.groovy
| ErrorCode errorCode = reply.getError(); | ||
| if (errorCode != null | ||
| && (SecretHostGetReply.ERROR_CODE_SECRET_NOT_FOUND.equals(errorCode.getCode()) | ||
| || SecretHostGetReply.ERROR_CODE_SECRET_NOT_FOUND.equals(errorCode.getDetails()))) { | ||
| trigger.next(); | ||
| return; | ||
| } | ||
|
|
||
| trigger.fail(errorCode != null ? errorCode : operr("get secret on host failed")); |
There was a problem hiding this comment.
SECRET_NOT_FOUND 分支要清掉旧的 secretUuid。
这里直接 next() 会保留之前 host 上的 secretUuid。后面的 load-dek-for-host 和 define-secret-on-host 都把 secretUuid 非空当成 skip 条件,跨 host 重试时会跳过重建并把旧 UUID 带到新 host。
💡 建议修改
if (errorCode != null
&& (SecretHostGetReply.ERROR_CODE_SECRET_NOT_FOUND.equals(errorCode.getCode())
|| SecretHostGetReply.ERROR_CODE_SECRET_NOT_FOUND.equals(errorCode.getDetails()))) {
+ spec.getDevicesSpec().getTpm().setSecretUuid(null);
trigger.next();
return;
}🤖 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/tpm/KvmTpmExtensions.java` around
lines 249 - 257, The SECRET_NOT_FOUND branch currently calls trigger.next() but
leaves the prior secretUuid in the flow context, causing later flows
(load-dek-for-host, define-secret-on-host) to skip rebuilds; modify the branch
that checks SecretHostGetReply.ERROR_CODE_SECRET_NOT_FOUND (in
KvmTpmExtensions.java) to clear the stored secretUuid from the context/state
(set it to null or remove the key where secretUuid is kept) before calling
trigger.next(), so subsequent flows will treat secretUuid as empty and properly
recreate the secret on the new host.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
testlib/src/main/java/org/zstack/testlib/KVMSimulator.groovy (1)
369-369: 建议避免错误码魔法字符串这里直接写
"KEY_AGENT_SECRET_NOT_FOUND",后续重命名或拼写漂移时不易统一维护。建议提取为常量并复用。♻️ 建议修改
class KVMSimulator implements Simulator { + private static final String KEY_AGENT_SECRET_NOT_FOUND = "KEY_AGENT_SECRET_NOT_FOUND" @@ - rsp.setError("KEY_AGENT_SECRET_NOT_FOUND") + rsp.setError(KEY_AGENT_SECRET_NOT_FOUND)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testlib/src/main/java/org/zstack/testlib/KVMSimulator.groovy` at line 369, Replace the literal error-code string passed to rsp.setError("KEY_AGENT_SECRET_NOT_FOUND") with a single shared constant; add a public static final String (e.g., KEY_AGENT_SECRET_NOT_FOUND) in the KVMSimulator class (or a central ErrorCodes class if one exists) and use that constant in the rsp.setError call to avoid magic-string duplication and make future renames safe.plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java (1)
5338-5344: 把空secretUuid也视为协议错误。当前成功分支只判断了
rsp.isSuccess()。如果 agent 返回success=true但secretUuid为空,这里会回一个“成功但没有结果”的 reply,下游很难区分和正常命中。建议把secretUuid非空也纳入成功条件。🤖 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 5338 - 5344, In the success callback of the success(KVMAgentCommands.SecretHostGetResponse rsp) handler, expand the success condition to require a non-empty rsp.getSecretUuid() (in addition to rsp.isSuccess()); if secretUuid is null/empty treat it as protocol error and call reply.setError(buildSecretAgentError(rsp, "get secret failed")) before bus.reply(msg, reply) so that reply.setSecretUuid is only invoked for valid non-empty values and empty results return an error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java`:
- Around line 5325-5351: The async secret GET/ENSURE in KVMHost currently calls
restf.asyncJsonPost(...) directly (e.g., SecretHostGetCmd/SecretHostGetResponse
block), which bypasses the host-aware HTTP wrapper that injects
Constants.AGENT_HTTP_HEADER_RESOURCE_UUID and triggers
KVMBeforeAsyncJsonPostExtensionPoint; update these secret calls to use the same
host-aware call path (reuse Http.call(...) or the existing host-aware helper
used elsewhere in KVMHost) so the hostUuid is injected and the extensions run,
ensuring the simulator cache key (hostUuid, vmUuid, purpose, keyVersion) is
correct for both GET and ENSURE flows (also apply the same change to the
SecretHostEnsure code path referenced around lines 5435-5442).
---
Nitpick comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java`:
- Around line 5338-5344: In the success callback of the
success(KVMAgentCommands.SecretHostGetResponse rsp) handler, expand the success
condition to require a non-empty rsp.getSecretUuid() (in addition to
rsp.isSuccess()); if secretUuid is null/empty treat it as protocol error and
call reply.setError(buildSecretAgentError(rsp, "get secret failed")) before
bus.reply(msg, reply) so that reply.setSecretUuid is only invoked for valid
non-empty values and empty results return an error.
In `@testlib/src/main/java/org/zstack/testlib/KVMSimulator.groovy`:
- Line 369: Replace the literal error-code string passed to
rsp.setError("KEY_AGENT_SECRET_NOT_FOUND") with a single shared constant; add a
public static final String (e.g., KEY_AGENT_SECRET_NOT_FOUND) in the
KVMSimulator class (or a central ErrorCodes class if one exists) and use that
constant in the rsp.setError call to avoid magic-string duplication and make
future renames safe.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 21e72dfc-baf8-4400-ac03-eefcf6e25d16
📒 Files selected for processing (11)
compute/src/main/java/org/zstack/compute/vm/devices/DummyTpmEncryptedResourceKeyBackend.javacompute/src/main/java/org/zstack/compute/vm/devices/TpmEncryptedResourceKeyBackend.javaheader/src/main/java/org/zstack/header/keyprovider/EncryptedResourceKeyManager.javaheader/src/main/java/org/zstack/header/secret/SecretHostDefineMsg.javaheader/src/main/java/org/zstack/header/secret/SecretHostGetMsg.javaheader/src/main/java/org/zstack/header/secret/SecretHostGetReply.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMAgentCommands.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMConstant.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMHost.javaplugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmExtensions.javatestlib/src/main/java/org/zstack/testlib/KVMSimulator.groovy
✅ Files skipped from review due to trivial changes (3)
- plugin/kvm/src/main/java/org/zstack/kvm/KVMConstant.java
- header/src/main/java/org/zstack/header/secret/SecretHostGetReply.java
- header/src/main/java/org/zstack/header/secret/SecretHostGetMsg.java
🚧 Files skipped from review as they are similar to previous changes (5)
- header/src/main/java/org/zstack/header/keyprovider/EncryptedResourceKeyManager.java
- compute/src/main/java/org/zstack/compute/vm/devices/DummyTpmEncryptedResourceKeyBackend.java
- compute/src/main/java/org/zstack/compute/vm/devices/TpmEncryptedResourceKeyBackend.java
- plugin/kvm/src/main/java/org/zstack/kvm/KVMAgentCommands.java
- plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmExtensions.java
| String url = buildUrl(KVMConstant.KVM_GET_SECRET_PATH); | ||
| KVMAgentCommands.SecretHostGetCmd cmd = new KVMAgentCommands.SecretHostGetCmd(); | ||
| cmd.setVmUuid(msg.getVmUuid()); | ||
| cmd.setPurpose(msg.getPurpose()); | ||
| cmd.setKeyVersion(msg.getKeyVersion()); | ||
| restf.asyncJsonPost(url, cmd, new JsonAsyncRESTCallback<KVMAgentCommands.SecretHostGetResponse>(msg, reply) { | ||
| @Override | ||
| public void fail(ErrorCode err) { | ||
| reply.setError(err != null ? err : operr("get secret on agent failed")); | ||
| bus.reply(msg, reply); | ||
| } | ||
|
|
||
| @Override | ||
| public void success(KVMAgentCommands.SecretHostGetResponse rsp) { | ||
| if (rsp != null && rsp.isSuccess()) { | ||
| reply.setSecretUuid(rsp.getSecretUuid()); | ||
| } else { | ||
| reply.setError(buildSecretAgentError(rsp, "get secret failed")); | ||
| } | ||
| bus.reply(msg, reply); | ||
| } | ||
|
|
||
| @Override | ||
| public Class<KVMAgentCommands.SecretHostGetResponse> getReturnClass() { | ||
| return KVMAgentCommands.SecretHostGetResponse.class; | ||
| } | ||
| }, TimeUnit.SECONDS, KVMConstant.ENVELOPE_KEY_HTTP_TIMEOUT_SEC); |
There was a problem hiding this comment.
Secret 的 GET/ENSURE 调用不要绕过主机侧 HTTP 封装。
这里直接走 restf.asyncJsonPost(...),跳过了 Http.call(...) 里统一注入的 Constants.AGENT_HTTP_HEADER_RESOURCE_UUID 和 KVMBeforeAsyncJsonPostExtensionPoint。testlib/src/main/java/org/zstack/testlib/KVMSimulator.groovy 现在已经按 (hostUuid, vmUuid, purpose, keyVersion) 建 cache key;按当前实现,hostUuid 会一直是 null,跨主机场景下 secret 作用域会串,GET/ENSURE 也拿不到统一的 host addon 处理。建议两条 secret 路径都复用同一套 host-aware 调用封装,或在这里显式补齐相同的 header / extension 处理。
Also applies to: 5435-5442
🤖 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 5325 -
5351, The async secret GET/ENSURE in KVMHost currently calls
restf.asyncJsonPost(...) directly (e.g., SecretHostGetCmd/SecretHostGetResponse
block), which bypasses the host-aware HTTP wrapper that injects
Constants.AGENT_HTTP_HEADER_RESOURCE_UUID and triggers
KVMBeforeAsyncJsonPostExtensionPoint; update these secret calls to use the same
host-aware call path (reuse Http.call(...) or the existing host-aware helper
used elsewhere in KVMHost) so the hostUuid is injected and the extensions run,
ensuring the simulator cache key (hostUuid, vmUuid, purpose, keyVersion) is
correct for both GET and ENSURE flows (also apply the same change to the
SecretHostEnsure code path referenced around lines 5435-5442).
Resolves: ZSV-11489
Change-Id: I616a776e78666179637976616c776162616c6577
sync from gitlab !9546