diff --git a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md index 7a25ae4733..5d5e1e7226 100644 --- a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md +++ b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md @@ -743,6 +743,7 @@ installer download and the agent's subsequent Pulse TLS connection. - `internal/agentexec/guest_exec_safety.go` - `internal/hostagent/guest_exec_guard.go` +- `internal/hostagent/guest_exec_completion.go` - `internal/hostagent/guest_exec_config_linux.go` - `internal/hostagent/guest_exec_config_other.go` @@ -3550,8 +3551,12 @@ admission serialises concurrent commands, respects cancellation and bounds state to 4,096 entries. Unknown handed-off completion or postflight lock/owner loss imposes a one-minute uncertainty pause; no command is replayed. The next explicit request must pass fresh config admission. A local process exit or kill -is not proof that guest work stopped; postflight failure retains the actual -exit/output but disqualifies successful live evidence. +is not proof that guest work stopped: a zero CLI exit with only a guest PID, +nonterminal, malformed or ambiguous JSON stays uncertain. Only one complete +canonical `exited` true/1 dictionary establishes observed guest termination; +this does not alter the existing CLI exit/output payload semantics or prove +thaw. Postflight failure retains that actual exit/output but disqualifies +successful live evidence. The guard is not shared with the HTTP poller, separate agent processes or PVE backup workers, and config checks do not reserve QGA. Agent restart also does diff --git a/internal/hostagent/command_client_test.go b/internal/hostagent/command_client_test.go index 13f3edf9d8..7c9f5ee1fe 100644 --- a/internal/hostagent/command_client_test.go +++ b/internal/hostagent/command_client_test.go @@ -522,3 +522,7 @@ func TestCommandClientGuestExecutionAdmissionContract(t *testing.T) { func TestGuestExecRealServerAgentDiscoveryAdmission(t *testing.T) { testGuestExecRealServerAgentDiscoveryAdmission(t) } + +func TestCommandClientGuestCompletionContract(t *testing.T) { + TestGuestExecZeroCLIExitDoesNotProveGuestCompletion(t) +} diff --git a/internal/hostagent/commands.go b/internal/hostagent/commands.go index cb7bd06a49..e40dca1b51 100644 --- a/internal/hostagent/commands.go +++ b/internal/hostagent/commands.go @@ -1668,7 +1668,7 @@ func (c *CommandClient) executeCommand(ctx context.Context, payload executeComma if guard != nil { // Killing the local qm process does not prove a handed-off guest // command stopped. Never retry it or immediately start the next one. - if err != nil || cmdCtx.Err() != nil || outputIncomplete { + if err != nil || cmdCtx.Err() != nil || outputIncomplete || !guestExecCompleted(result.Stdout) { uncertain = true result.Success = false result.Error = agentexec.GuestExecDeferred(agentexec.GuestExecCompletionUnknown).Error() diff --git a/internal/hostagent/guest_exec_completion.go b/internal/hostagent/guest_exec_completion.go new file mode 100644 index 0000000000..8e07755c79 --- /dev/null +++ b/internal/hostagent/guest_exec_completion.go @@ -0,0 +1,52 @@ +package hostagent + +import ( + "bytes" + "encoding/json" + "io" + "strings" +) + +// qm can exit zero after handing off a guest PID without observing completion +// (for example when synchronous waiting expires). Only its complete terminal +// dictionary, not a local CLI exit, permits another QGA command. Keep the raw +// output in the ordinary command result; this decoder never logs guest data. +func guestExecCompleted(stdout string) bool { + if len(stdout) == 0 || len(stdout) > maxCommandOutputSize { + return false + } + d := json.NewDecoder(strings.NewReader(stdout)) + start, err := d.Token() + if err != nil || start != json.Delim('{') { + return false + } + fields := make(map[string]json.RawMessage) + for d.More() { + token, err := d.Token() + name, ok := token.(string) + if err != nil || !ok { + return false + } + if _, duplicate := fields[name]; duplicate { + return false + } + if name != "exited" && strings.EqualFold(name, "exited") { + return false + } + var value json.RawMessage + if d.Decode(&value) != nil { + return false + } + fields[name] = value + } + end, err := d.Token() + if err != nil || end != json.Delim('}') { + return false + } + if _, err := d.Token(); err != io.EOF { + return false + } + // PVE/QGA's numeric true is 1; accept JSON true from compatible codecs. + exited := bytes.TrimSpace(fields["exited"]) + return bytes.Equal(exited, []byte("1")) || bytes.Equal(exited, []byte("true")) +} diff --git a/internal/hostagent/guest_exec_completion_regression_test.go b/internal/hostagent/guest_exec_completion_regression_test.go new file mode 100644 index 0000000000..b3f6203a6d --- /dev/null +++ b/internal/hostagent/guest_exec_completion_regression_test.go @@ -0,0 +1,41 @@ +package hostagent + +import ( + "context" + "os/exec" + "strings" + "sync/atomic" + "testing" + + "github.com/rcourtman/pulse-go-rewrite/internal/agentexec" +) + +// Uses only the previously existing execute/guard boundary, so the unchanged +// parent can demonstrate that zero local CLI exit is not guest completion. +func TestGuestExecZeroCLIExitDoesNotProveGuestCompletion(t *testing.T) { + for _, response := range []string{ + `{"pid":731}`, `{"exited":0,"pid":731}`, `{"exited":null}`, + `{"exited":1,"exited":0}`, `{"exited":1,"EXITED":0}`, + `{"exited":1} {"exited":0}`, `{"exited":1`, `not-json`, + } { + t.Run(response, func(t *testing.T) { + var calls atomic.Int32 + g := newGuestExecGuard(func(context.Context, string) ([]byte, error) { return []byte("name: vm\n"), nil }) + c := &CommandClient{guestExecAdmission: g} + oldExec := execCommandContext + execCommandContext = func(ctx context.Context, _ string, _ ...string) *exec.Cmd { + calls.Add(1) + return exec.CommandContext(ctx, "sh", "-c", "printf '%s' "+shellQuote(response)) + } + t.Cleanup(func() { execCommandContext = oldExec }) + payload := testApprovedCommandPayload(t, c, executeCommandPayload{Command: "echo original", TargetType: "vm", TargetID: "105", Trusted: true}) + got := c.executeCommand(context.Background(), payload) + if got.Success || !agentexec.IsGuestExecDeferred(got.Error) || !strings.Contains(got.Error, "completion") || got.Stdout != response || got.ExitCode != 0 { + t.Fatalf("CLI receipt became guest success: %#v", got) + } + if next := c.executeCommand(context.Background(), payload); next.Success || calls.Load() != 1 || !strings.Contains(next.Error, "uncertain") { + t.Fatalf("unverified completion permitted another handoff: %#v calls=%d", next, calls.Load()) + } + }) + } +} diff --git a/internal/hostagent/guest_exec_completion_test.go b/internal/hostagent/guest_exec_completion_test.go new file mode 100644 index 0000000000..b56fa758f0 --- /dev/null +++ b/internal/hostagent/guest_exec_completion_test.go @@ -0,0 +1,20 @@ +package hostagent + +import "testing" + +func TestGuestExecTerminalDictionary(t *testing.T) { + for _, response := range []string{ + `{"exited":1,"exitcode":0,"out-data":"observation"}`, + `{"exited":true,"exitcode":1,"err-data":"command failed"}`, + `{"exited":1,"signal":15}`, // terminal signal is not an unknown handoff + } { + if !guestExecCompleted(response) { + t.Fatalf("terminal dictionary not retained: %s", response) + } + } + for _, response := range []string{`null`, `[]`, `{"exited":"true"}`, `{"exited":1.0}`, `{"exited":2}`, `{"exited":false}`, `{"exited":1,"out-data":"a","out-data":"b"}`} { + if guestExecCompleted(response) { + t.Fatalf("ambiguous terminal dictionary admitted: %s", response) + } + } +} diff --git a/internal/hostagent/guest_exec_discovery_integration_test.go b/internal/hostagent/guest_exec_discovery_integration_test.go index cb87ac0067..d5fd6ac705 100644 --- a/internal/hostagent/guest_exec_discovery_integration_test.go +++ b/internal/hostagent/guest_exec_discovery_integration_test.go @@ -69,7 +69,7 @@ func testGuestExecRealServerAgentDiscoveryAdmission(t *testing.T) { locked.Store(true) } } - return exec.CommandContext(ctx, "sh", "-c", "printf safe-observation") + return exec.CommandContext(ctx, "sh", "-c", `printf '%s' '{"exited":1,"exitcode":0,"out-data":"safe-observation"}'`) } t.Cleanup(func() { execCommandContext = oldExec }) ctx, cancel := context.WithCancel(context.Background()) diff --git a/internal/hostagent/guest_exec_guard_test.go b/internal/hostagent/guest_exec_guard_test.go index d99cc36da4..170e487d2a 100644 --- a/internal/hostagent/guest_exec_guard_test.go +++ b/internal/hostagent/guest_exec_guard_test.go @@ -86,7 +86,7 @@ func TestGuestExecCommandPreflightAndPostflight(t *testing.T) { if name != localGuestExecQM || !reflect.DeepEqual(args, []string{"guest", "exec", "105", "--", "sh", "-c", "echo original"}) { t.Errorf("changed command/identity: %s %#v", name, args) } - return exec.CommandContext(ctx, "sh", "-c", "printf 'original-result'") + return exec.CommandContext(ctx, "sh", "-c", `printf '%s' '{"exited":1,"exitcode":0,"out-data":"original-result"}'`) } t.Cleanup(func() { execCommandContext = oldExec }) payload := testApprovedCommandPayload(t, c, executeCommandPayload{RequestID: "scan", Command: "echo original", TargetType: " VM ", TargetID: "000105", Trusted: true}) @@ -100,7 +100,7 @@ func TestGuestExecCommandPreflightAndPostflight(t *testing.T) { if tc.wantCalls == 0 && (got.Stdout != "" || got.Stderr != "") { t.Fatal("preflight published guest output") } - if tc.wantCalls == 1 && (got.Stdout != "original-result" || got.ExitCode != 0) { + if tc.wantCalls == 1 && (got.Stdout != `{"exited":1,"exitcode":0,"out-data":"original-result"}` || got.ExitCode != 0) { t.Fatalf("lost actual process completion: %#v", got) } if strings.Contains(got.Error, "private") { @@ -198,10 +198,10 @@ func TestGuestExecCanceledHandoffNeverRetriesAndRechecksOnExplicitResumption(t * g.mu.Unlock() execCommandContext = func(ctx context.Context, _ string, _ ...string) *exec.Cmd { calls.Add(1) - return exec.CommandContext(ctx, "sh", "-c", "printf resumed") + return exec.CommandContext(ctx, "sh", "-c", `printf '%s' '{"exited":1,"exitcode":0,"out-data":"resumed"}'`) } got = c.executeCommand(context.Background(), payload) - if !got.Success || got.Stdout != "resumed" || reads.Load() != 3 || calls.Load() != 2 { + if !got.Success || got.Stdout != `{"exited":1,"exitcode":0,"out-data":"resumed"}` || reads.Load() != 3 || calls.Load() != 2 { t.Fatalf("explicit resumption failed: %#v", got) } }