From b77e6908e62c2ff4160a5e54b813c7c3ee115894 Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Sun, 4 Oct 2026 15:01:23 +0100 Subject: [PATCH] Require same-target unambiguous locks before guest-agent reads Guest lock verification used ordinary redirect-following config reads and last-key-wins JSON decoding. Refuse redirected or ambiguous lock evidence before dispatch and after a complete guest response, without changing ordinary config redirects, pooled TLS policy or session recovery. Cover all six readers and connected cache/History continuity and fresh resumption; this source hardening does not establish native backup thaw. Change-source: pulse-maintainer --- .../v6/internal/subsystems/monitoring.md | 23 ++- .../monitoring/canonical_guardrails_test.go | 14 ++ .../guest_agent_backup_integration_test.go | 28 ++- pkg/proxmox/guest_agent_guard.go | 33 +++- pkg/proxmox/guest_agent_lock.go | 34 ++++ pkg/proxmox/guest_agent_lock_evidence_test.go | 182 ++++++++++++++++++ pkg/proxmox/guest_agent_response.go | 9 +- 7 files changed, 301 insertions(+), 22 deletions(-) create mode 100644 pkg/proxmox/guest_agent_lock.go create mode 100644 pkg/proxmox/guest_agent_lock_evidence_test.go diff --git a/docs/release-control/v6/internal/subsystems/monitoring.md b/docs/release-control/v6/internal/subsystems/monitoring.md index 5be3457267..46490d7506 100644 --- a/docs/release-control/v6/internal/subsystems/monitoring.md +++ b/docs/release-control/v6/internal/subsystems/monitoring.md @@ -105,6 +105,26 @@ holds per-guest admission until body completion; its failure discards the payload. Complete responses at the limit, including trailing whitespace, remain valid. Ordinary PVE CPU/I/O observations remain independent of this deferral. +Lock verification also requires a direct HTTP 200 from the configured command +endpoint and one unambiguous `data` configuration object. It never follows a +redirect, including a same-origin redirect to another configuration path. +Duplicate fields (including escaped duplicates), differently cased `data` or +`lock` fields, competing error/failure envelopes and non-string operation locks +yield `lock-unverified`; a later unlocked value cannot overwrite earlier backup +evidence. Ordinary `GetVMConfig` remains compatible with its existing decoding +and redirects. The lock check shares the configured pooled transport, TLS, +proxy and timeout policy and retains config-read password-session recovery; +the guest command itself is still single-attempt. This narrows the authority +of lock evidence, not the ordinary API read contract. Failed post-command lock +verification discards the guest payload without inventing a transport cooldown; +subsequent same-target unlocked verification permits normal resumption. +`TestGuestAgentLockEvidenceRejectsAmbiguity` and +`TestGuestAgentLockEvidenceRejectsRedirects` exercise all six readers before and +after dispatch, all five redirect statuses, same/cross endpoint redirects, +ordinary config compatibility and fresh-client resumption. The connected +`TestGuestAgentBackupLockEvidenceContract` covers warm-cache continuity, live +CPU, unrenewed observations and History, then normal fresh poll resumption. + Admission is held through the complete response, not just headers. A transport failure, timeout, wrong-command-ID response or incomplete body defers every command for that guest for a minute; callers neither immediately retry nor @@ -140,7 +160,8 @@ transport or custom TLS dialer defers without dispatch rather than substituting a different trust policy. A fully received redirect discards its body and starts the same per-VM uncertainty cooldown; its destination is never contacted. Ordinary config/resource reads retain pooling, HTTP/2, redirects and session -recovery. This deliberately trades one new connection per guest command for +recovery, except that authoritative guest lock checks refuse redirects as above. +This deliberately trades one new connection per guest command for single-attempt safety; it does not reserve the QGA channel or establish thaw. Wire controls drop a reply only after receiving a command (including when a backup lock then appears), and exercise every read and redirect code. TLS diff --git a/internal/monitoring/canonical_guardrails_test.go b/internal/monitoring/canonical_guardrails_test.go index ae99fe9e63..38865b21a9 100644 --- a/internal/monitoring/canonical_guardrails_test.go +++ b/internal/monitoring/canonical_guardrails_test.go @@ -2976,6 +2976,20 @@ func TestGuestAgentBackupMonitoringContract(t *testing.T) { testGuestAgentBackupMonitoringLifecycle(t) } +// Same-target, unambiguous lock evidence is required even with warm caches. +func TestGuestAgentBackupLockEvidenceContract(t *testing.T) { + for _, test := range []struct{ name, config string }{ + {"redirected config", "redirect"}, + {"overwritten backup lock", `{"data":{"lock":"backup","lock":""}}`}, + {"overwritten config envelope", `{"data":{"lock":"backup"},"data":{}}`}, + {"case-conflicting envelope", `{"data":{"lock":"backup"},"DATA":{}}`}, + } { + t.Run(test.name, func(t *testing.T) { + testGuestAgentBackupMonitoringLifecycleWithUnverifiedConfig(t, test.config) + }) + } +} + func TestDeferredVMGuestMemoryRequiresOriginalEvidence(t *testing.T) { now := time.Now() tests := []struct { diff --git a/internal/monitoring/guest_agent_backup_integration_test.go b/internal/monitoring/guest_agent_backup_integration_test.go index f13b76eca4..d9e7dcf9bc 100644 --- a/internal/monitoring/guest_agent_backup_integration_test.go +++ b/internal/monitoring/guest_agent_backup_integration_test.go @@ -20,6 +20,10 @@ import ( ) func testGuestAgentBackupMonitoringLifecycle(t *testing.T) { + testGuestAgentBackupMonitoringLifecycleWithUnverifiedConfig(t, "") +} + +func testGuestAgentBackupMonitoringLifecycleWithUnverifiedConfig(t *testing.T, unverifiedConfig string) { const mib = uint64(1024 * 1024) var phase, calls atomic.Int32 server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -28,13 +32,23 @@ func testGuestAgentBackupMonitoringLifecycle(t *testing.T) { switch { case strings.HasSuffix(r.URL.Path, "/config"): if p == 4 { - w.Header().Set("Content-Length", "1000") - fmt.Fprint(w, `{"data":{}}`) + switch unverifiedConfig { + case "redirect": + http.Redirect(w, r, "/unrelated-unlocked-config", http.StatusTemporaryRedirect) + case "": + w.Header().Set("Content-Length", "1000") + fmt.Fprint(w, `{"data":{}}`) + default: + fmt.Fprint(w, unverifiedConfig) + } } else if p == 1 || p == 2 { fmt.Fprint(w, `{"data":{"lock":"backup"}}`) } else { fmt.Fprint(w, `{"data":{}}`) } + case r.URL.Path == "/unrelated-unlocked-config": + // Never count this unrelated unlocked response as the guest's lock. + fmt.Fprint(w, `{"data":{}}`) case strings.HasSuffix(r.URL.Path, "/status/current"): if p == 2 { http.Error(w, "unavailable", 503) @@ -111,20 +125,20 @@ func testGuestAgentBackupMonitoringLifecycle(t *testing.T) { memKey := guestMemoryCacheKey("fixture", "node", 105) originalMetadata := m.guestMetadataCache[metadataKey] originalMemory := m.vmAgentMemCache[memKey] - // A syntactically unlocked prefix is not a completed config observation. + // An incomplete, redirected or ambiguous config is not lock clearance. // Warm guest caches must stay labelled last-known rather than becoming new - // disk/memory History samples when the lock response is incomplete. + // disk/memory History samples when the lock response cannot be trusted. phase.Store(4) unverified, _ := build() if unverified.ID != initial.ID || unverified.GuestAgentStatus != "deferred" || unverified.DiskStatusReason != "prev-lock-unverified" || unverified.Disk.Used != initial.Disk.Used || unverified.Memory.Used != initial.Memory.Used { - t.Fatalf("incomplete lock response lost truthful continuity: %#v", unverified) + t.Fatalf("unverified lock response lost truthful continuity: %#v", unverified) } if calls.Load() != before || !reflect.DeepEqual(m.guestMetadataCache[metadataKey], originalMetadata) || !reflect.DeepEqual(m.vmAgentMemCache[memKey], originalMemory) { - t.Fatal("incomplete lock response sent a guest command or renewed caches") + t.Fatal("unverified lock response sent a guest command or renewed caches") } for metric, want := range map[string]int{"cpu": 2, "memory": 1, "memoryused": 1, "disk": 1} { if got := len(m.metricsHistory.GetGuestMetrics(identity, metric, time.Hour)); got != want { - t.Errorf("incomplete lock %s history points = %d, want %d", metric, got, want) + t.Errorf("unverified lock %s history points = %d, want %d", metric, got, want) } } phase.Store(1) diff --git a/pkg/proxmox/guest_agent_guard.go b/pkg/proxmox/guest_agent_guard.go index d5dd85f8c8..59bcff1a3d 100644 --- a/pkg/proxmox/guest_agent_guard.go +++ b/pkg/proxmox/guest_agent_guard.go @@ -142,18 +142,31 @@ func guestAgentPath(path string) (string, int, bool) { func (c *Client) verifyGuestAgentUnlocked(ctx context.Context, node string, vmid int) error { // The config endpoint contains the authoritative PVE operation lock even // when a PVE version omits it from status/current or cluster/resources. - config, err := c.GetVMConfig(ctx, node, vmid) - if err != nil || config == nil { + // Unlike an ordinary config read, redirected or ambiguous evidence cannot + // establish the lock on the endpoint about to receive this guest command. + lockClient := *c.httpClient + lockClient.CheckRedirect = func(*http.Request, []*http.Request) error { + return http.ErrUseLastResponse + } + resp, err := c.requestWithRetryUsingClient(ctx, http.MethodGet, fmt.Sprintf("/nodes/%s/qemu/%d/config", node, vmid), nil, false, &lockClient) + if err != nil { return &guestAgentDeferredError{reason: "lock-unverified", cause: err} } - if value, exists := config["lock"]; exists { - lock, ok := value.(string) - if !ok { - return &guestAgentDeferredError{reason: "lock-unverified"} - } - if strings.TrimSpace(lock) != "" { - return &guestAgentDeferredError{reason: "vm-locked"} - } + if resp.StatusCode != http.StatusOK { + resp.Body.Close() + return &guestAgentDeferredError{reason: "lock-unverified"} + } + body, readErr := readResponseBodyLimited(resp.Body) + closeErr := resp.Body.Close() + if readErr != nil || closeErr != nil { + return &guestAgentDeferredError{reason: "lock-unverified", cause: errors.Join(readErr, closeErr)} + } + lock, valid := guestAgentConfigLock(body) + if !valid { + return &guestAgentDeferredError{reason: "lock-unverified"} + } + if strings.TrimSpace(lock) != "" { + return &guestAgentDeferredError{reason: "vm-locked"} } return nil } diff --git a/pkg/proxmox/guest_agent_lock.go b/pkg/proxmox/guest_agent_lock.go new file mode 100644 index 0000000000..f0e11e8c68 --- /dev/null +++ b/pkg/proxmox/guest_agent_lock.go @@ -0,0 +1,34 @@ +package proxmox + +import ( + "encoding/json" + "strings" +) + +// guestAgentConfigLock accepts only one complete, unambiguous config envelope. +// In particular, encoding/json's last-key-wins and case-insensitive struct +// matching must not erase a backup lock. Ordinary config decoding is unchanged. +func guestAgentConfigLock(body []byte) (string, bool) { + envelope, ok := guestAgentResponseFields(body, "data") + if !ok { + return "", false + } + config, ok := guestAgentResponseFields(envelope["data"]) + if !ok { + return "", false + } + for name := range config { + if name != "lock" && strings.EqualFold(name, "lock") { + return "", false + } + } + value, exists := config["lock"] + if !exists { + return "", true // PVE omits this field when no operation holds the lock. + } + var lock *string + if json.Unmarshal(value, &lock) != nil || lock == nil { + return "", false + } + return *lock, true +} diff --git a/pkg/proxmox/guest_agent_lock_evidence_test.go b/pkg/proxmox/guest_agent_lock_evidence_test.go new file mode 100644 index 0000000000..9231ea6cf6 --- /dev/null +++ b/pkg/proxmox/guest_agent_lock_evidence_test.go @@ -0,0 +1,182 @@ +package proxmox + +import ( + "context" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "sync/atomic" + "testing" + "time" +) + +func TestGuestAgentLockEvidenceRejectsAmbiguity(t *testing.T) { + cases := []struct{ name, body string }{ + {"duplicate-lock", `{"data":{"lock":"backup","lock":""}}`}, + {"duplicate-unlocked", `{"data":{"lock":"","lock":""}}`}, + {"escaped-lock", `{"data":{"lock":"backup","\u006cock":""}}`}, + {"duplicate-data", `{"data":{"lock":"backup"},"data":{}}`}, + {"escaped-data", `{"data":{"lock":"backup"},"\u0064ata":{}}`}, + {"case-data", `{"data":{"lock":"backup"},"DATA":{}}`}, + {"case-only-data", `{"Data":{}}`}, + {"case-only-lock", `{"data":{"Lock":"backup"}}`}, + {"case-lock", `{"data":{"lock":"","LOCK":"backup"}}`}, + {"competing-error", `{"data":{},"errors":{"message":"lock unavailable"}}`}, + {"competing-failure", `{"data":{},"success":false}`}, + {"duplicate-config-field", `{"data":{"name":"first","name":"second"}}`}, + {"null-lock", `{"data":{"lock": null }}`}, + {"number-lock", `{"data":{"lock":0}}`}, + {"object-lock", `{"data":{"lock":{}}}`}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + for name, read := range backupAgentReads() { + for _, postCommand := range []bool{false, true} { + t.Run(fmt.Sprintf("%s/post=%t", name, postCommand), func(t *testing.T) { + var configs, commands atomic.Int32 + var healthy atomic.Bool + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if strings.HasSuffix(r.URL.Path, "/config") { + if configs.Add(1) == 1 && postCommand || healthy.Load() { + fmt.Fprint(w, `{"data":{"name":"fixture","agent":1}}`) + } else { + fmt.Fprint(w, tc.body) + } + return + } + commands.Add(1) + backupAgentPayload(w, r) + })) + defer server.Close() + c := backupTestClient(t, server.URL) + if err := read(context.Background(), c, 105); GuestAgentDeferredReason(err) != "lock-unverified" { + t.Errorf("ambiguous lock admitted/published a command: %v", err) + } + want := int32(0) + if postCommand { + want = 1 + } + if got := commands.Load(); got != want { + t.Errorf("wire commands = %d, want %d", got, want) + } + healthy.Store(true) + // A completed command's failed lock check is not transport + // uncertainty. A new diagnostic must reverify and resume. + diagnostic := backupTestClient(t, server.URL) + if err := read(context.Background(), diagnostic, 105); err != nil { + t.Fatalf("complete unlocked config did not resume: %v", err) + } + if got := commands.Load(); got != want+1 { + t.Errorf("resumed wire commands = %d, want %d", got, want+1) + } + }) + } + } + }) + } +} + +func TestGuestAgentLockEvidenceRejectsRedirects(t *testing.T) { + for _, status := range []int{301, 302, 303, 307, 308} { + for _, crossEndpoint := range []bool{false, true} { + for name, read := range backupAgentReads() { + for _, postCommand := range []bool{false, true} { + t.Run(fmt.Sprintf("%d/cross=%t/%s/post=%t", status, crossEndpoint, name, postCommand), func(t *testing.T) { + var configs, commands, redirected atomic.Int32 + var healthy atomic.Bool + other := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + redirected.Add(1) + fmt.Fprint(w, `{"data":{}}`) + })) + defer other.Close() + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case strings.HasSuffix(r.URL.Path, "/config"): + if configs.Add(1) == 1 && postCommand || healthy.Load() { + fmt.Fprint(w, `{"data":{}}`) + } else { + destination := "/unrelated-unlocked-config" + if crossEndpoint { + destination = other.URL + destination + } + http.Redirect(w, r, destination, status) + } + case r.URL.Path == "/unrelated-unlocked-config": + redirected.Add(1) + fmt.Fprint(w, `{"data":{}}`) + default: + commands.Add(1) + backupAgentPayload(w, r) + } + })) + defer server.Close() + c := backupTestClient(t, server.URL) + if err := read(context.Background(), c, 105); GuestAgentDeferredReason(err) != "lock-unverified" { + t.Errorf("redirected lock admitted/published a command: %v", err) + } + want := int32(0) + if postCommand { + want = 1 + } + if commands.Load() != want || redirected.Load() != 0 { + t.Errorf("wire commands/redirected = %d/%d, want %d/0", commands.Load(), redirected.Load(), want) + } + // Ordinary config reads retain their existing redirect + // policy; do not globally mutate c.httpClient. + if _, err := c.GetVMConfig(context.Background(), "node", 105); err != nil || redirected.Load() != 1 { + t.Errorf("ordinary config redirect policy changed: redirects=%d err=%v", redirected.Load(), err) + } + healthy.Store(true) + if err := read(context.Background(), backupTestClient(t, server.URL), 105); err != nil { + t.Fatalf("valid same-target config did not resume: %v", err) + } + if commands.Load() != want+1 || redirected.Load() != 1 { + t.Errorf("resumed commands/redirected = %d/%d, want %d/1", commands.Load(), redirected.Load(), want+1) + } + }) + } + } + } + } +} + +func TestGuestAgentLockEvidenceHealthyConfigAndSessionRecovery(t *testing.T) { + for _, body := range []string{ + `{"data":{}}`, + `{"data":{"lock":"","agent":1,"digest":"fixture","name":"fixture"}}`, + `{"data":{"lock":" ","description":"backup; lock","scsi0":"fixture:disk","tags":"backup;ha"}}`, + `{"data":{"\u006cock":"","nested":{"data":"fixture","lock":"backup"}}}`, // Only the actual operation lock is authoritative. + } { + t.Run(fmt.Sprintf("config-%d", len(body)), func(t *testing.T) { + var configs, commands, authentications atomic.Int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case strings.HasSuffix(r.URL.Path, "/access/ticket"): + authentications.Add(1) + fmt.Fprint(w, `{"data":{"ticket":"fixture-ticket","CSRFPreventionToken":"fixture-csrf"}}`) + case strings.HasSuffix(r.URL.Path, "/config"): + if configs.Add(1) == 1 { + http.Error(w, "expired config session", http.StatusUnauthorized) + return + } + fmt.Fprint(w, body) + default: + commands.Add(1) + backupAgentPayload(w, r) + } + })) + defer server.Close() + c, err := NewClient(ClientConfig{Host: server.URL, User: "fixture@pam", Password: "fixture-password", Timeout: time.Second}) + if err != nil { + t.Fatal(err) + } + if err := backupAgentReads()["filesystem"](context.Background(), c, 105); err != nil { + t.Fatal(err) + } + if configs.Load() != 3 || authentications.Load() != 2 || commands.Load() != 1 { + t.Errorf("config recovery/command identity lost: configs=%d auth=%d commands=%d", configs.Load(), authentications.Load(), commands.Load()) + } + }) + } +} diff --git a/pkg/proxmox/guest_agent_response.go b/pkg/proxmox/guest_agent_response.go index 5b74e9effd..29aea7c3fb 100644 --- a/pkg/proxmox/guest_agent_response.go +++ b/pkg/proxmox/guest_agent_response.go @@ -61,7 +61,7 @@ func guestAgentErrorMessage(body []byte) (string, bool) { if body[0] != '{' { return string(body), true // Still requires an exact, command-bound match. } - fields, ok := guestAgentErrorFields(body, "data", "message", "errors") + fields, ok := guestAgentResponseFields(body, "data", "message", "errors") if !ok { return "", false } @@ -73,7 +73,7 @@ func guestAgentErrorMessage(body []byte) (string, bool) { if hasMessage { return "", false // Conflicting or duplicate failure evidence. } - errors, ok := guestAgentErrorFields(nested, "message") + errors, ok := guestAgentResponseFields(nested, "message") if !ok { return "", false } @@ -88,7 +88,8 @@ func guestAgentErrorMessage(body []byte) (string, bool) { // Reject ambiguous envelopes (including duplicate keys and trailing values) // instead of letting a favourable final field override other error evidence. -func guestAgentErrorFields(body []byte, allowed ...string) (map[string]json.RawMessage, bool) { +// Without an allowlist, arbitrary unique configuration fields are permitted. +func guestAgentResponseFields(body []byte, allowed ...string) (map[string]json.RawMessage, bool) { decoder := json.NewDecoder(bytes.NewReader(body)) start, err := decoder.Token() if err != nil || start != json.Delim('{') { @@ -104,7 +105,7 @@ func guestAgentErrorFields(body []byte, allowed ...string) (map[string]json.RawM if !ok { return nil, false } - permitted := false + permitted := len(allowed) == 0 for _, candidate := range allowed { permitted = permitted || name == candidate }