FM-603: Proxmox template, clone, and snapshot operations behind the destructive review gate - #106
Merged
Merged
Conversation
…estructive review gate
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 38 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/fleet-application/src/operation.rs">
<violation number="1" location="crates/fleet-application/src/operation.rs:551">
P1: Any caller of the public `Operations::create` API can set `reviewed: true` and bypass the destructive review gate because this service does not verify a token or capability. Make the reviewed path an unforgeable/privileged API or pass verified review material into this layer and validate it here.</violation>
</file>
<file name="crates/providers/fleet-provider-proxmox/src/lib.rs">
<violation number="1" location="crates/providers/fleet-provider-proxmox/src/lib.rs:1557">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**
`list_qemu_resources` performs the version request through `version_and_resources`, then discards the result with `let _ = &version;`. Clone and template idempotency checks therefore pay for an unrelated network call and can fail on its response; extract a resource-only helper and keep the version-bearing helper only for `discover`.</violation>
</file>
<file name="crates/fleetctl/src/lib.rs">
<violation number="1" location="crates/fleetctl/src/lib.rs:1244">
P3: The new verbs are absent from `usage()`, so unknown-flag and top-level parse errors never advertise the destructive workflow. Add the six `proxmox` forms to the usage text.</violation>
</file>
<file name="crates/fleet-controller/src/proxmox_exec.rs">
<violation number="1" location="crates/fleet-controller/src/proxmox_exec.rs:852">
P2: `timeout_seconds` does not bound the mutating request: `call(...).await` completes before this timer starts, so a slow operation can exceed the reviewed deadline without being counted. Start the timer before invoking `call` and enforce the remaining deadline around that await.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // dedicated reviewed endpoint, which binds the operation to a | ||
| // confirmed review token. A generic-surface create is a | ||
| // route-around attempt, refused as malformed. | ||
| if !new.reviewed |
There was a problem hiding this comment.
P1: Any caller of the public Operations::create API can set reviewed: true and bypass the destructive review gate because this service does not verify a token or capability. Make the reviewed path an unforgeable/privileged API or pass verified review material into this layer and validate it here.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/fleet-application/src/operation.rs, line 551:
<comment>Any caller of the public `Operations::create` API can set `reviewed: true` and bypass the destructive review gate because this service does not verify a token or capability. Make the reviewed path an unforgeable/privileged API or pass verified review material into this layer and validate it here.</comment>
<file context>
@@ -525,6 +543,25 @@ impl Operations {
+ // dedicated reviewed endpoint, which binds the operation to a
+ // confirmed review token. A generic-surface create is a
+ // route-around attempt, refused as malformed.
+ if !new.reviewed
+ && (new.kind.starts_with("proxmox.guest.snapshot")
+ || matches!(
</file context>
| let mut resources = Vec::new(); | ||
| for entry in entries { | ||
| if let Ok(Some(resource)) = normalize_resource(&entry) { | ||
| let _ = &version; |
There was a problem hiding this comment.
P2: Custom agent: Flag AI Slop and Fabricated Changes
list_qemu_resources performs the version request through version_and_resources, then discards the result with let _ = &version;. Clone and template idempotency checks therefore pay for an unrelated network call and can fail on its response; extract a resource-only helper and keep the version-bearing helper only for discover.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/providers/fleet-provider-proxmox/src/lib.rs, line 1557:
<comment>`list_qemu_resources` performs the version request through `version_and_resources`, then discards the result with `let _ = &version;`. Clone and template idempotency checks therefore pay for an unrelated network call and can fail on its response; extract a resource-only helper and keep the version-bearing helper only for `discover`.</comment>
<file context>
@@ -1177,6 +1540,30 @@ impl ProxmoxClient {
+ let mut resources = Vec::new();
+ for entry in entries {
+ if let Ok(Some(resource)) = normalize_resource(&entry) {
+ let _ = &version;
+ resources.push(resource);
+ }
</file context>
| } | ||
| _ => Err(CliError { message: usage() }), | ||
| }, | ||
| "snapshot" | "snapshot-revert" | "snapshot-delete" | "clone" | "template" |
There was a problem hiding this comment.
P3: The new verbs are absent from usage(), so unknown-flag and top-level parse errors never advertise the destructive workflow. Add the six proxmox forms to the usage text.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/fleetctl/src/lib.rs, line 1244:
<comment>The new verbs are absent from `usage()`, so unknown-flag and top-level parse errors never advertise the destructive workflow. Add the six `proxmox` forms to the usage text.</comment>
<file context>
@@ -1223,6 +1241,81 @@ fn parse_proxmox_command(verb: &str, rest: &[&str]) -> Result<Command, CliError>
}
_ => Err(CliError { message: usage() }),
},
+ "snapshot" | "snapshot-revert" | "snapshot-delete" | "clone" | "template"
+ | "task-cancel" => {
+ let mut account_id = None;
</file context>
Member
Author
|
All 19 findings addressed in 56ac300. The most important change: the review gate moved into the application layer and is no longer forgeable. P1s
P2s
P3s
Live re-verified against the integration host after the fixes: snapshot create through the review flow → |
There was a problem hiding this comment.
2 issues found across 33 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/fleet-api/src/proxmox.rs">
<violation number="1" location="crates/fleet-api/src/proxmox.rs:1277">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**
The review and run handlers duplicate the security-critical `kind` and payload construction, so a future edit to either site can silently make the reviewed token cover different bytes than the operation executes. Extract this construction into one shared helper and use it in both handlers.</violation>
</file>
<file name="crates/fleet-controller/src/proxmox_exec.rs">
<violation number="1" location="crates/fleet-controller/src/proxmox_exec.rs:601">
P2: When `template` targets an LXC guest, this lookup now accepts the LXC resource and sends a QEMU template-conversion request instead of rejecting the non-QEMU guest. Filter the selected resource to `qemu` or `qemu-template` before calling `guest_convert_template`.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| } | ||
| // The token binds the exact payload the run will carry, so the | ||
| // reviewed bytes and the executed bytes are the same by construction. | ||
| let kind = if action == "task-cancel" { |
There was a problem hiding this comment.
P2: Custom agent: Flag AI Slop and Fabricated Changes
The review and run handlers duplicate the security-critical kind and payload construction, so a future edit to either site can silently make the reviewed token cover different bytes than the operation executes. Extract this construction into one shared helper and use it in both handlers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/fleet-api/src/proxmox.rs, line 1277:
<comment>The review and run handlers duplicate the security-critical `kind` and payload construction, so a future edit to either site can silently make the reviewed token cover different bytes than the operation executes. Extract this construction into one shared helper and use it in both handlers.</comment>
<file context>
@@ -1229,8 +1272,23 @@ pub async fn review_proxmox_operation(
}
+ // The token binds the exact payload the run will carry, so the
+ // reviewed bytes and the executed bytes are the same by construction.
+ let kind = if action == "task-cancel" {
+ "proxmox.task-cancel".to_owned()
+ } else {
</file context>
| // resources are the truth. | ||
| let resources = self | ||
| .client | ||
| .list_guest_resources(request.clone()) |
There was a problem hiding this comment.
P2: When template targets an LXC guest, this lookup now accepts the LXC resource and sends a QEMU template-conversion request instead of rejecting the non-QEMU guest. Filter the selected resource to qemu or qemu-template before calling guest_convert_template.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/fleet-controller/src/proxmox_exec.rs, line 601:
<comment>When `template` targets an LXC guest, this lookup now accepts the LXC resource and sends a QEMU template-conversion request instead of rejecting the non-QEMU guest. Filter the selected resource to `qemu` or `qemu-template` before calling `guest_convert_template`.</comment>
<file context>
@@ -595,7 +598,7 @@ impl OperationExecutor for ProxmoxDestructiveExecutor {
let resources = self
.client
- .list_qemu_resources(request.clone())
+ .list_guest_resources(request.clone())
.await
.map_err(|error| format!("the resource listing failed: {error}"))?;
</file context>
Member
Author
|
Round-2 re-review: the only new finding was the stdin regression — |
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #105 (FM-603).
Implementation approach (from the issue)
FM-602's lifecycle executor and UPID polling are the foundation; this PR extends both with the destructive-adjacent surface.
What landed
fleet-provider-proxmox):guest_snapshot(create — PVE'svmstateflag carries the include-RAM intent),guest_snapshot_rollback,guest_snapshot_delete(synchronous),guest_clone,guest_convert_template,stop_task, andguest_snapshots(listing,currentexcluded). The transport now carriesDeleteand bodies (execute_with_body); the response mapping is shared (status_to_result).proxmox.guest.snapshot|snapshot-revert|snapshot-delete|clone|template,proxmox.task-cancel): six newCREATABLE_KINDSentries (35 → 41) behind the newproxmox.destructivepermission (catalog 39 → 40), catalog-level like the lifecycle kinds.POST .../guests/{vmid}/{action}/reviewrenders exactly what will run and returns a token computed as SHA-256 of the canonical request (account, vmid, action, node, params). The create call (.../run) must present that token — recomputed from its own payload, so any parameter change invalidates it. The generic/operationssurface refuses these kinds outright (NewOperation.reviewedis set only by the dedicated reviewed endpoint); there is no route-around.conflictfailure when the description differs; clone refuses when the target VMID already exists (the cluster's truth, never a duplicate); template conversion is a no-op when already a template.proxmox.task-cancelrunsstop_taskas an authorized, audited operation and reads the outcome back honestly (confirmed/unknown).ProxmoxDestructiveExecutorre-applies the trust gate before any network call and polls with the same Fleet-owned fixed-interval loop.fleetctl proxmox snapshot|snapshot-revert|snapshot-delete|clone|template|task-cancel --account --node --vmid— a two-step flow (review → run with the returned token); the action's parameters arrive as JSON on stdin, never argv;--waitworks.Tests
cargo xtask verifypasses.fleet-test-01created through the review flow (taskState: ok), the idempotent re-run answering the no-op, and the snapshot deleted again through the reviewed flow — the PVE host's snapshot list confirmed both transitions. (One live finding fixed in-PR: PVE's qemu snapshot schema names the RAM flagvmstate, notinclude_ram.)Non-goals (respected)
A backup UI; automatic cleanup policy (M7 Lab leases); guest OS provisioning.
Summary by cubic
Adds Proxmox snapshot, clone, template conversion, and task-cancellation operations behind a mandatory review step, so destructive actions run exactly what was reviewed.
Review gate
POST .../guests/{vmid}/{action}/reviewreturns a token: a SHA-256 over the canonical request (account, vmid, action, node, params), computed infleet-applicationand persisted on the operation./operationssurface refuses these kinds outright.proxmox.destructivepermission.Operations and CLI
fleetctl proxmox snapshot|snapshot-revert|snapshot-delete|clone|template|task-cancelruns the two-step review flow; params arrive as JSON on stdin (read to EOF) while secret inputs stay single-line, and--waitis supported.execute_with_bodyandDelete; failed operations record what ran (UPID, node) without deleting anything.Written for commit 07bae1e. Summary will update on new commits.