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

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

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 {

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)

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
 }

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
+}

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())
+			}
+		})
+	}
+}

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
 		}