From c008d33d40fd813ae56692a77127138057ddf089 Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Mon, 21 Sep 2026 11:23:56 +0100 Subject: [PATCH 1/7] test(configapi): probe verifySSL persistence for #2140 Reproduction probe for the reported SSL toggle not sticking. Change-source: pulse-maintainer --- .../api/configapi/zz_verify_ssl_repro_test.go | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) create mode 100644 internal/api/configapi/zz_verify_ssl_repro_test.go diff --git a/internal/api/configapi/zz_verify_ssl_repro_test.go b/internal/api/configapi/zz_verify_ssl_repro_test.go new file mode 100644 index 000000000..ace1f7566 --- /dev/null +++ b/internal/api/configapi/zz_verify_ssl_repro_test.go @@ -0,0 +1,49 @@ +package configapi + +import ( + "bytes" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "github.com/rcourtman/pulse-go-rewrite/internal/config" +) + +// Reproduction probe for issue #2140: a PVE node's "Verify SSL certificate" +// toggle must survive a PUT that disables it, and the GET projection must +// report the disabled value back to the form. +func TestRepro2140VerifySSLPersistsOnUpdate(t *testing.T) { + cfg := &config.Config{ + DataPath: t.TempDir(), + PVEInstances: []config.PVEInstance{{ + Name: "node-a", + Host: "https://pve.local:8006", + TokenName: "pulse-monitor@pve!pulse", + TokenValue: "secret", + Fingerprint: "AA:BB:CC", + VerifySSL: true, + }}, + } + handler := newTestConfigHandlers(t, cfg) + + body, _ := json.Marshal(map[string]any{"verifySSL": false}) + req := httptest.NewRequest(http.MethodPut, "/api/config/nodes/pve-0", bytes.NewBuffer(body)) + rec := httptest.NewRecorder() + handler.HandleUpdateNode(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("update status = %d: %s", rec.Code, rec.Body.String()) + } + if cfg.PVEInstances[0].VerifySSL { + t.Fatalf("stored VerifySSL = true, want false after disabling") + } + + nodes := handler.GetAllNodesForAPI(req.Context()) + if len(nodes) != 1 { + t.Fatalf("nodes = %d, want 1", len(nodes)) + } + if nodes[0].VerifySSL { + t.Fatalf("GET projection VerifySSL = true, want false") + } +} From 529e8155253c4536696986e2993bb3b2f75eb71c Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Mon, 21 Sep 2026 11:26:23 +0100 Subject: [PATCH 2/7] fix(configapi): preserve operator VerifySSL on auto-register Re-registration of an existing node overwrote the stored VerifySSL value with the fingerprint-capture result, silently re-enabling a disabled Verify SSL certificate setting on every agent health-check repair (#2140). Preserve the operator's choice and keep only the legacy heal where strict verification is on with no pin (#1303). Change-source: pulse-maintainer --- ...g_handlers_canonical_auto_register_test.go | 99 +++++++++++++++++++ .../api/configapi/config_setup_handlers.go | 22 ++++- 2 files changed, 119 insertions(+), 2 deletions(-) diff --git a/internal/api/configapi/config_handlers_canonical_auto_register_test.go b/internal/api/configapi/config_handlers_canonical_auto_register_test.go index 5bad3bc29..7c7d4e168 100644 --- a/internal/api/configapi/config_handlers_canonical_auto_register_test.go +++ b/internal/api/configapi/config_handlers_canonical_auto_register_test.go @@ -1557,3 +1557,102 @@ func TestHandleCanonicalAutoRegister_PBSKeepsDistinctSameNameTokenWhenFingerprin t.Fatalf("new site name = %q, want disambiguation from existing %q", registered.Name, existing.Name) } } + +// TestHandleCanonicalAutoRegister_PVEPreservesDisabledVerifySSL covers #2140: +// an operator who disables "Verify SSL certificate" on an existing node must +// not have it silently re-enabled by an agent health-check re-registration. +func TestHandleCanonicalAutoRegister_PVEPreservesDisabledVerifySSL(t *testing.T) { + tempDir := t.TempDir() + t.Setenv("PULSE_DATA_DIR", tempDir) + + server := newIPv4TLSServer(t, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + })) + defer server.Close() + + tokenID := "pulse-monitor@pve!" + buildPulseMonitorTokenName("pulse.example.com") + cfg := &config.Config{ + DataPath: tempDir, + ConfigPath: tempDir, + PVEInstances: []config.PVEInstance{ + { + Name: "pve01", + Host: server.URL, + TokenName: tokenID, + TokenValue: "existing-token", + VerifySSL: false, + }, + }, + } + handler := newTestConfigHandlers(t, cfg) + + reqBody := AutoRegisterRequest{ + Type: "pve", + Host: server.URL, + ServerName: "pve01", + TokenID: tokenID, + TokenValue: "rotated-token", + Source: "agent", + } + req := httptest.NewRequest(http.MethodPost, "/api/auto-register", nil) + rec := httptest.NewRecorder() + + handler.handleCanonicalAutoRegister(rec, req, &reqBody, "127.0.0.1") + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200: %s", rec.Code, rec.Body.String()) + } + instance := handler.defaultConfig.PVEInstances[0] + if instance.VerifySSL { + t.Fatalf("re-registration re-enabled VerifySSL on an existing node that disabled it (#2140)") + } +} + +// TestHandleCanonicalAutoRegister_PBSReservesDisabledVerifySSL is the PBS twin +// of TestHandleCanonicalAutoRegister_PVEPreservesDisabledVerifySSL. +func TestHandleCanonicalAutoRegister_PBSReservesDisabledVerifySSL(t *testing.T) { + tempDir := t.TempDir() + t.Setenv("PULSE_DATA_DIR", tempDir) + + server := newIPv4TLSServer(t, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + })) + defer server.Close() + + tokenID := "pulse-monitor@pbs!" + buildPulseMonitorTokenName("pulse.example.com") + cfg := &config.Config{ + DataPath: tempDir, + ConfigPath: tempDir, + PBSInstances: []config.PBSInstance{ + { + Name: "pbs01", + Host: server.URL, + TokenName: tokenID, + TokenValue: "existing-token", + VerifySSL: false, + }, + }, + } + handler := newTestConfigHandlers(t, cfg) + + reqBody := AutoRegisterRequest{ + Type: "pbs", + Host: server.URL, + ServerName: "pbs01", + TokenID: tokenID, + TokenValue: "rotated-token", + Source: "agent", + } + req := httptest.NewRequest(http.MethodPost, "/api/auto-register", nil) + rec := httptest.NewRecorder() + + handler.handleCanonicalAutoRegister(rec, req, &reqBody, "127.0.0.1") + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200: %s", rec.Code, rec.Body.String()) + } + instance := handler.defaultConfig.PBSInstances[0] + if instance.VerifySSL { + t.Fatalf("re-registration re-enabled VerifySSL on an existing node that disabled it (#2140)") + } +} diff --git a/internal/api/configapi/config_setup_handlers.go b/internal/api/configapi/config_setup_handlers.go index 8eae0f591..c58cfd472 100644 --- a/internal/api/configapi/config_setup_handlers.go +++ b/internal/api/configapi/config_setup_handlers.go @@ -2265,7 +2265,16 @@ func (h *ConfigHandlers) handleCanonicalAutoRegister(w http.ResponseWriter, r *h if pveNode.Fingerprint != "" { instance.Fingerprint = pveNode.Fingerprint } - instance.VerifySSL = pveNode.VerifySSL + // Preserve the operator's TLS choice on an existing connection. + // The incoming value records only whether this registration + // captured a fingerprint; applying it unconditionally re-enabled a + // disabled "Verify SSL certificate" setting on every agent + // health-check re-registration (#2140). Heal only the legacy state + // where strict verification is on with no pin, which can never + // connect to a self-signed endpoint (#1303). + if instance.VerifySSL && instance.Fingerprint == "" { + instance.VerifySSL = false + } log.Info().Str("host", host).Str("type", "pve").Msg(canonicalAutoRegisterMatchMessage("host; updated token in-place")) } else if h.adoptCanonicalAutoRegisterClusterMember(r.Context(), serverName, host, fingerprint, fullTokenID, tokenValue, candidateHosts, registrationSource) { // A non-primary cluster member: the cluster connection already @@ -2319,7 +2328,16 @@ func (h *ConfigHandlers) handleCanonicalAutoRegister(w http.ResponseWriter, r *h if pbsNode.Fingerprint != "" { instance.Fingerprint = pbsNode.Fingerprint } - instance.VerifySSL = pbsNode.VerifySSL + // Preserve the operator's TLS choice on an existing connection. + // The incoming value records only whether this registration + // captured a fingerprint; applying it unconditionally re-enabled a + // disabled "Verify SSL certificate" setting on every agent + // health-check re-registration (#2140). Heal only the legacy state + // where strict verification is on with no pin, which can never + // connect to a self-signed endpoint (#1303). + if instance.VerifySSL && instance.Fingerprint == "" { + instance.VerifySSL = false + } log.Info().Str("host", host).Str("type", "pbs").Msg(canonicalAutoRegisterMatchMessage("host; updated token in-place")) } else { // Agent-token auth is restricted to updating existing nodes only. From a211d2ea17a5c08c50608b24e3a6889c81ffe172 Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Mon, 21 Sep 2026 11:38:14 +0100 Subject: [PATCH 3/7] docs(release-control): complete #2140 TLS-preservation contracts Record the canonical-shape completion for the auto-register VerifySSL preservation in the agent-lifecycle contract and its api-contracts and storage-recovery dependents, so the runtime change is documented in the same candidate. Change-source: pulse-maintainer --- .../v6/internal/subsystems/agent-lifecycle.md | 14 ++++++++++++++ .../v6/internal/subsystems/api-contracts.md | 9 +++++++++ .../v6/internal/subsystems/storage-recovery.md | 10 ++++++++++ 3 files changed, 33 insertions(+) diff --git a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md index 820656a4c..36b377fd7 100644 --- a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md +++ b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md @@ -3395,6 +3395,20 @@ Agent` secondary handoff against the live setup wizard instead of relying ## Current State +### Auto-register preserves the operator's stored TLS choice + +Re-registering an existing Proxmox node through the canonical auto-register +path preserves the stored `VerifySSL` value instead of replacing it with the +fingerprint-capture result. This keeps a disabled "Verify SSL certificate" +setting disabled across an agent health-check re-registration after a +disconnect, while the legacy heal (`VerifySSL` true with no stored fingerprint) +still downgrades to an insecure, parseable-certificate connection. The +fingerprint pin may still refresh from the registration. New nodes continue to +take the captured value. Regression tests +`TestHandleCanonicalAutoRegister_PVEPreservesDisabledVerifySSL` and +`TestHandleCanonicalAutoRegister_PBSReservesDisabledVerifySSL` pin the existing +node branch. + ### Manual update freshness Server update-check freshness is owned by internal/updates and its API adapter. diff --git a/docs/release-control/v6/internal/subsystems/api-contracts.md b/docs/release-control/v6/internal/subsystems/api-contracts.md index 37bc55d86..e5fd6fe22 100644 --- a/docs/release-control/v6/internal/subsystems/api-contracts.md +++ b/docs/release-control/v6/internal/subsystems/api-contracts.md @@ -4541,6 +4541,15 @@ auto-register mutation boundary. ## Current State +### Node auto-register TLS fields are operator-owned on update + +The `/api/auto-register` completion request and response shapes are unchanged. +When a registration matches an existing node, the captured fingerprint may +still refresh the stored pin, but the stored `verifySSL` preference is not +replaced by the registration's capture state. New nodes continue to take the +captured value. This is a behavioural clarification of the existing endpoint, +not a new field or contract delta. + ### Manual update freshness GET /api/updates/check accepts an optional boolean force query alongside the diff --git a/docs/release-control/v6/internal/subsystems/storage-recovery.md b/docs/release-control/v6/internal/subsystems/storage-recovery.md index cb8e03f21..69a99cd6b 100644 --- a/docs/release-control/v6/internal/subsystems/storage-recovery.md +++ b/docs/release-control/v6/internal/subsystems/storage-recovery.md @@ -2611,6 +2611,16 @@ vdev layout is reported` in ## Current State +### TLS verification preference survives node re-registration + +A PVE or PBS node whose operator disabled certificate verification keeps that +preference when the host agent re-registers the node after a disconnect, +including when the stored fingerprint was cleared. This prevents a rotating or +self-signed certificate from silently re-enabling verification and breaking the +monitoring path used for backup and recovery evidence. New nodes still take the +captured registration value. See the agent-lifecycle contract for the runtime +mechanism. + ### Manual update freshness Server update-check freshness changes observation only. A refreshed availability From 10ad85e0cf3cff6e98e5c45dff1e9720567b21d5 Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Mon, 21 Sep 2026 11:43:51 +0100 Subject: [PATCH 4/7] fix(config): keep operator VerifySSL across PVE consolidation ConsolidatePVEInstances promoted VerifySSL false->true when folding a duplicate cluster or overlapping standalone into the canonical instance, silently re-enabling a disabled Verify SSL certificate setting on every save, load and monitor reconciliation (#2140). The canonical instance now owns the preference; a merged fingerprint still pins the peer, so verification is not downgraded. Regression tests cover the standalone and duplicate-cluster merges and the reported PUT-save path through normalizePVEConfigState. Change-source: pulse-maintainer --- .../v6/internal/subsystems/agent-lifecycle.md | 11 +++ .../configapi/config_handlers_update_test.go | 57 ++++++++++++ internal/config/pve_instances.go | 10 ++- internal/config/pve_instances_test.go | 90 ++++++++++++++++++- 4 files changed, 161 insertions(+), 7 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md index 36b377fd7..380137980 100644 --- a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md +++ b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md @@ -3409,6 +3409,17 @@ take the captured value. Regression tests `TestHandleCanonicalAutoRegister_PBSReservesDisabledVerifySSL` pin the existing node branch. +`ConsolidatePVEInstances` keeps the canonical (destination) instance's +`VerifySSL` when it folds a duplicate cluster or an overlapping standalone into +it. A merge no longer promotes `VerifySSL` from `false` to `true` on the +surviving instance, so a disabled setting is not silently re-enabled on every +save, load and monitor reconciliation. Certificate pinning is carried separately +by the merged `Fingerprint`, so a pin still verifies the peer. Regression tests +`TestConsolidatePVEInstancesPreservesDisabledVerifySSLOnStandaloneMerge`, +`TestConsolidatePVEInstancesPreservesDisabledVerifySSLOnDuplicateClusterMerge` +and `TestHandleUpdateNodePreservesDisabledVerifySSLThroughConsolidation` cover +the merge and save paths. + ### Manual update freshness Server update-check freshness is owned by internal/updates and its API adapter. diff --git a/internal/api/configapi/config_handlers_update_test.go b/internal/api/configapi/config_handlers_update_test.go index b0050c46e..789a9bb80 100644 --- a/internal/api/configapi/config_handlers_update_test.go +++ b/internal/api/configapi/config_handlers_update_test.go @@ -541,3 +541,60 @@ func TestHandleUpdateNode_UserOnlyEditDoesNotClearProxmoxTokenAuth(t *testing.T) t.Fatalf("PMG monitor mail stats should preserve false when omitted") } } + +// TestHandleUpdateNodePreservesDisabledVerifySSLThroughConsolidation covers +// #2140 end to end through the reported save path: an operator disables +// "Verify SSL certificate" on a cluster that also has an overlapping +// auto-registered standalone entry, saves, and reopens Manage. The save runs +// normalizePVEConfigState, which folds the standalone into the cluster; the +// disabled setting must survive that consolidation. +func TestHandleUpdateNodePreservesDisabledVerifySSLThroughConsolidation(t *testing.T) { + tempDir := t.TempDir() + cfg := &config.Config{ + DataPath: tempDir, + PVEInstances: []config.PVEInstance{ + { + Name: "homelab", + ClusterName: "cluster-A", + IsCluster: true, + VerifySSL: true, + ClusterEndpoints: []config.ClusterEndpoint{ + {NodeName: "minipc", Host: "https://10.0.0.5:8006"}, + }, + }, + { + Name: "minipc-standalone", + Host: "10.0.0.5", + Fingerprint: "fp-standalone", + TokenName: "pulse@pve!token", + TokenValue: "secret", + VerifySSL: true, + Source: "agent", + }, + }, + } + handler := newTestConfigHandlers(t, cfg) + + body, _ := json.Marshal(map[string]any{"verifySSL": false}) + req := httptest.NewRequest(http.MethodPut, "/api/config/nodes/pve-0", bytes.NewBuffer(body)) + rec := httptest.NewRecorder() + handler.HandleUpdateNode(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("update status = %d: %s", rec.Code, rec.Body.String()) + } + if len(cfg.PVEInstances) != 1 { + t.Fatalf("instances = %d, want 1 after consolidation", len(cfg.PVEInstances)) + } + if cfg.PVEInstances[0].VerifySSL { + t.Fatalf("save re-enabled the disabled VerifySSL through consolidation (#2140)") + } + + nodes := handler.GetAllNodesForAPI(req.Context()) + if len(nodes) != 1 { + t.Fatalf("nodes = %d, want 1", len(nodes)) + } + if nodes[0].VerifySSL { + t.Fatalf("GET projection VerifySSL = true, want false after disabling (#2140)") + } +} diff --git a/internal/config/pve_instances.go b/internal/config/pve_instances.go index 44fcc2fee..807e4bd52 100644 --- a/internal/config/pve_instances.go +++ b/internal/config/pve_instances.go @@ -318,9 +318,13 @@ func mergePVEInstanceData(dst *PVEInstance, src PVEInstance) { if dst.Source == "" && strings.TrimSpace(src.Source) != "" { dst.Source = strings.TrimSpace(src.Source) } - if !dst.VerifySSL && src.VerifySSL { - dst.VerifySSL = true - } + // The canonical (destination) instance owns the TLS verification + // preference. Merging a duplicate or standalone must not flip a disabled + // "Verify SSL certificate" setting back on: doing so silently re-enabled + // the option on every save, load and monitor reconciliation, and broke + // endpoints whose operator had deliberately opted out (#2140). Certificate + // pinning is carried separately by the Fingerprint copy above, so a merged + // pin still verifies the peer even when VerifySSL stays false. if dst.TemperatureMonitoringEnabled == nil && src.TemperatureMonitoringEnabled != nil { enabled := *src.TemperatureMonitoringEnabled dst.TemperatureMonitoringEnabled = &enabled diff --git a/internal/config/pve_instances_test.go b/internal/config/pve_instances_test.go index ee6853936..4faf76459 100644 --- a/internal/config/pve_instances_test.go +++ b/internal/config/pve_instances_test.go @@ -88,8 +88,8 @@ func TestConsolidatePVEInstancesRemovesStandaloneCoveredByClusterEndpoint(t *tes if got := instances[0].TokenValue; got != "secret" { t.Fatalf("TokenValue = %q, want secret", got) } - if !instances[0].VerifySSL { - t.Fatalf("expected VerifySSL to be promoted") + if instances[0].VerifySSL { + t.Fatalf("expected the canonical cluster's disabled VerifySSL to survive the standalone merge (#2140)") } if got := instances[0].Source; got != "agent" { t.Fatalf("Source = %q, want agent", got) @@ -196,8 +196,8 @@ func TestConsolidatePVEInstancesMergesDuplicateClusterAuthIntoPrimary(t *testing if got := instances[0].Fingerprint; got != "fp-1" { t.Fatalf("Fingerprint = %q, want fp-1", got) } - if !instances[0].VerifySSL { - t.Fatalf("expected VerifySSL to be promoted") + if instances[0].VerifySSL { + t.Fatalf("expected the primary cluster's disabled VerifySSL to survive the duplicate merge (#2140)") } if len(instances[0].ClusterEndpoints) != 2 { t.Fatalf("expected 2 endpoints after consolidation, got %d", len(instances[0].ClusterEndpoints)) @@ -286,3 +286,85 @@ func TestConsolidatePVEInstancesKeepsStandaloneWithContradictingFingerprint(t *t t.Fatalf("expected both instances to remain, got %d", len(instances)) } } + +// TestConsolidatePVEInstancesPreservesDisabledVerifySSLOnStandaloneMerge covers +// #2140: a standalone/auto-registered entry that captured a fingerprint is +// created with VerifySSL true. Folding it into a cluster whose operator +// disabled "Verify SSL certificate" must not re-enable the setting. The +// captured pin still moves onto the cluster endpoint, so verification is not +// silently downgraded. +func TestConsolidatePVEInstancesPreservesDisabledVerifySSLOnStandaloneMerge(t *testing.T) { + instances, changed := ConsolidatePVEInstances([]PVEInstance{ + { + Name: "homelab", + ClusterName: "cluster-A", + IsCluster: true, + VerifySSL: false, + ClusterEndpoints: []ClusterEndpoint{ + {NodeName: "minipc", Host: "https://10.0.0.5:8006"}, + }, + }, + { + Name: "minipc-standalone", + Host: "10.0.0.5", + Fingerprint: "fp-standalone", + TokenName: "pulse@pve!token", + TokenValue: "secret", + VerifySSL: true, + Source: "agent", + }, + }) + + if !changed { + t.Fatalf("expected consolidation change") + } + if len(instances) != 1 { + t.Fatalf("expected 1 instance after consolidation, got %d", len(instances)) + } + if instances[0].VerifySSL { + t.Fatalf("standalone merge re-enabled a disabled VerifySSL (#2140)") + } + if got := instances[0].ClusterEndpoints[0].Fingerprint; got != "fp-standalone" { + t.Fatalf("ClusterEndpoint Fingerprint = %q, want fp-standalone (pin must survive)", got) + } +} + +// TestConsolidatePVEInstancesPreservesDisabledVerifySSLOnDuplicateClusterMerge +// is the duplicate-cluster twin of the #2140 regression above. +func TestConsolidatePVEInstancesPreservesDisabledVerifySSLOnDuplicateClusterMerge(t *testing.T) { + instances, changed := ConsolidatePVEInstances([]PVEInstance{ + { + Name: "c1", + ClusterName: "cluster-A", + IsCluster: true, + VerifySSL: false, + ClusterEndpoints: []ClusterEndpoint{ + {NodeName: "n1", Host: "https://10.0.0.5:8006"}, + }, + }, + { + Name: "c2", + ClusterName: "cluster-A", + IsCluster: true, + VerifySSL: true, + Fingerprint: "fp-1", + ClusterEndpoints: []ClusterEndpoint{ + {NodeName: "n1", Host: "https://10.0.0.5:8006"}, + {NodeName: "n2", Host: "https://10.0.0.6:8006"}, + }, + }, + }) + + if !changed { + t.Fatalf("expected consolidation change") + } + if len(instances) != 1 { + t.Fatalf("expected 1 instance after consolidation, got %d", len(instances)) + } + if instances[0].VerifySSL { + t.Fatalf("duplicate-cluster merge re-enabled a disabled VerifySSL (#2140)") + } + if got := instances[0].Fingerprint; got != "fp-1" { + t.Fatalf("Fingerprint = %q, want fp-1 (pin must survive)", got) + } +} From 919009d7677318c071f888eb28c252d2d79d2728 Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Mon, 21 Sep 2026 11:47:26 +0100 Subject: [PATCH 5/7] fix(config): honour explicit operator VerifySSL across PVE merge ConsolidatePVEInstances promoted VerifySSL false->true whenever a merged duplicate or standalone had it enabled, silently re-enabling a disabled Verify SSL certificate setting on every save, load and monitor reconciliation (#2140). The add and update handlers now record an explicit operator choice in VerifySSLExplicit; a merge never overrides it, while the historical promotion is kept when neither side recorded a choice so an existing secure connection is not downgraded. A merged fingerprint still pins the peer. Regression tests cover the standalone and duplicate-cluster merges and the reported PUT-save path through normalizePVEConfigState. Change-source: pulse-maintainer --- .../v6/internal/subsystems/agent-lifecycle.md | 15 ++++++----- .../api/configapi/config_node_handlers.go | 2 ++ .../api/configapi/config_setup_handlers.go | 7 ++--- internal/config/config.go | 1 + internal/config/pve_instances.go | 23 +++++++++++----- internal/config/pve_instances_test.go | 26 ++++++++++--------- 6 files changed, 46 insertions(+), 28 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md index 380137980..d7d9f033f 100644 --- a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md +++ b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md @@ -3409,12 +3409,15 @@ take the captured value. Regression tests `TestHandleCanonicalAutoRegister_PBSReservesDisabledVerifySSL` pin the existing node branch. -`ConsolidatePVEInstances` keeps the canonical (destination) instance's -`VerifySSL` when it folds a duplicate cluster or an overlapping standalone into -it. A merge no longer promotes `VerifySSL` from `false` to `true` on the -surviving instance, so a disabled setting is not silently re-enabled on every -save, load and monitor reconciliation. Certificate pinning is carried separately -by the merged `Fingerprint`, so a pin still verifies the peer. Regression tests +`ConsolidatePVEInstances` respects an operator's explicit `VerifySSL` choice +when it folds a duplicate cluster or an overlapping standalone into the +canonical instance. An explicit choice is recorded as `VerifySSLExplicit` when +the update or add handler applies a caller-supplied `verifySSL`, and a merge +never overrides it, so a disabled setting is not silently re-enabled on every +save, load and monitor reconciliation. When neither side recorded an explicit +choice the historical promotion is kept, so an existing secure connection is +not silently downgraded. Certificate pinning is carried separately by the +merged `Fingerprint`, so a pin still verifies the peer. Regression tests `TestConsolidatePVEInstancesPreservesDisabledVerifySSLOnStandaloneMerge`, `TestConsolidatePVEInstancesPreservesDisabledVerifySSLOnDuplicateClusterMerge` and `TestHandleUpdateNodePreservesDisabledVerifySSLThroughConsolidation` cover diff --git a/internal/api/configapi/config_node_handlers.go b/internal/api/configapi/config_node_handlers.go index 46f074c20..01aaaf198 100644 --- a/internal/api/configapi/config_node_handlers.go +++ b/internal/api/configapi/config_node_handlers.go @@ -516,6 +516,7 @@ func (h *ConfigHandlers) handleAddNode(w http.ResponseWriter, r *http.Request) { TokenValue: req.TokenValue, Fingerprint: req.Fingerprint, VerifySSL: verifySSL, + VerifySSLExplicit: req.VerifySSL != nil, MonitorVMs: monitorVMs, MonitorContainers: monitorContainers, MonitorStorage: monitorStorage, @@ -1493,6 +1494,7 @@ func (h *ConfigHandlers) handleUpdateNode(w http.ResponseWriter, r *http.Request } if req.VerifySSL != nil { updated.VerifySSL = *req.VerifySSL + updated.VerifySSLExplicit = true } if req.MonitorVMs != nil { updated.MonitorVMs = *req.MonitorVMs diff --git a/internal/api/configapi/config_setup_handlers.go b/internal/api/configapi/config_setup_handlers.go index c58cfd472..c66066907 100644 --- a/internal/api/configapi/config_setup_handlers.go +++ b/internal/api/configapi/config_setup_handlers.go @@ -2270,9 +2270,10 @@ func (h *ConfigHandlers) handleCanonicalAutoRegister(w http.ResponseWriter, r *h // captured a fingerprint; applying it unconditionally re-enabled a // disabled "Verify SSL certificate" setting on every agent // health-check re-registration (#2140). Heal only the legacy state - // where strict verification is on with no pin, which can never - // connect to a self-signed endpoint (#1303). - if instance.VerifySSL && instance.Fingerprint == "" { + // where strict verification is on with no pin and no recorded + // operator choice, which can never connect to a self-signed + // endpoint (#1303). + if !instance.VerifySSLExplicit && instance.VerifySSL && instance.Fingerprint == "" { instance.VerifySSL = false } log.Info().Str("host", host).Str("type", "pve").Msg(canonicalAutoRegisterMatchMessage("host; updated token in-place")) diff --git a/internal/config/config.go b/internal/config/config.go index 9f1599874..a5eb3a80c 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -570,6 +570,7 @@ type PVEInstance struct { TokenValue string Fingerprint string VerifySSL bool + VerifySSLExplicit bool `json:"verifySSLExplicit,omitempty"` // operator-chosen VerifySSL; survives automatic consolidation (#2140) MonitorVMs bool MonitorContainers bool MonitorStorage bool diff --git a/internal/config/pve_instances.go b/internal/config/pve_instances.go index 807e4bd52..437aac838 100644 --- a/internal/config/pve_instances.go +++ b/internal/config/pve_instances.go @@ -318,13 +318,22 @@ func mergePVEInstanceData(dst *PVEInstance, src PVEInstance) { if dst.Source == "" && strings.TrimSpace(src.Source) != "" { dst.Source = strings.TrimSpace(src.Source) } - // The canonical (destination) instance owns the TLS verification - // preference. Merging a duplicate or standalone must not flip a disabled - // "Verify SSL certificate" setting back on: doing so silently re-enabled - // the option on every save, load and monitor reconciliation, and broke - // endpoints whose operator had deliberately opted out (#2140). Certificate - // pinning is carried separately by the Fingerprint copy above, so a merged - // pin still verifies the peer even when VerifySSL stays false. + // TLS verification preference: an explicit operator choice wins over an + // inferred one. When neither side recorded a choice, keep the historical + // promotion of a strict source so an existing secure connection is not + // silently downgraded. A disabled setting that the operator saved through + // the API is explicit and is never re-enabled by a merge (#2140). + // Certificate pinning is carried separately by the Fingerprint copy above, + // so a merged pin still verifies the peer even when VerifySSL stays false. + switch { + case dst.VerifySSLExplicit: + // The canonical instance's explicit choice is authoritative. + case src.VerifySSLExplicit: + dst.VerifySSL = src.VerifySSL + dst.VerifySSLExplicit = true + case !dst.VerifySSL && src.VerifySSL: + dst.VerifySSL = true + } if dst.TemperatureMonitoringEnabled == nil && src.TemperatureMonitoringEnabled != nil { enabled := *src.TemperatureMonitoringEnabled dst.TemperatureMonitoringEnabled = &enabled diff --git a/internal/config/pve_instances_test.go b/internal/config/pve_instances_test.go index 4faf76459..5a54f9ceb 100644 --- a/internal/config/pve_instances_test.go +++ b/internal/config/pve_instances_test.go @@ -88,8 +88,8 @@ func TestConsolidatePVEInstancesRemovesStandaloneCoveredByClusterEndpoint(t *tes if got := instances[0].TokenValue; got != "secret" { t.Fatalf("TokenValue = %q, want secret", got) } - if instances[0].VerifySSL { - t.Fatalf("expected the canonical cluster's disabled VerifySSL to survive the standalone merge (#2140)") + if !instances[0].VerifySSL { + t.Fatalf("expected VerifySSL to be promoted") } if got := instances[0].Source; got != "agent" { t.Fatalf("Source = %q, want agent", got) @@ -196,8 +196,8 @@ func TestConsolidatePVEInstancesMergesDuplicateClusterAuthIntoPrimary(t *testing if got := instances[0].Fingerprint; got != "fp-1" { t.Fatalf("Fingerprint = %q, want fp-1", got) } - if instances[0].VerifySSL { - t.Fatalf("expected the primary cluster's disabled VerifySSL to survive the duplicate merge (#2140)") + if !instances[0].VerifySSL { + t.Fatalf("expected VerifySSL to be promoted") } if len(instances[0].ClusterEndpoints) != 2 { t.Fatalf("expected 2 endpoints after consolidation, got %d", len(instances[0].ClusterEndpoints)) @@ -296,10 +296,11 @@ func TestConsolidatePVEInstancesKeepsStandaloneWithContradictingFingerprint(t *t func TestConsolidatePVEInstancesPreservesDisabledVerifySSLOnStandaloneMerge(t *testing.T) { instances, changed := ConsolidatePVEInstances([]PVEInstance{ { - Name: "homelab", - ClusterName: "cluster-A", - IsCluster: true, - VerifySSL: false, + Name: "homelab", + ClusterName: "cluster-A", + IsCluster: true, + VerifySSL: false, + VerifySSLExplicit: true, ClusterEndpoints: []ClusterEndpoint{ {NodeName: "minipc", Host: "https://10.0.0.5:8006"}, }, @@ -334,10 +335,11 @@ func TestConsolidatePVEInstancesPreservesDisabledVerifySSLOnStandaloneMerge(t *t func TestConsolidatePVEInstancesPreservesDisabledVerifySSLOnDuplicateClusterMerge(t *testing.T) { instances, changed := ConsolidatePVEInstances([]PVEInstance{ { - Name: "c1", - ClusterName: "cluster-A", - IsCluster: true, - VerifySSL: false, + Name: "c1", + ClusterName: "cluster-A", + IsCluster: true, + VerifySSL: false, + VerifySSLExplicit: true, ClusterEndpoints: []ClusterEndpoint{ {NodeName: "n1", Host: "https://10.0.0.5:8006"}, }, From 9fa81dee8fba760b980d821d6e10560589c110f2 Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Mon, 21 Sep 2026 11:54:39 +0100 Subject: [PATCH 6/7] docs(release-control): complete #2140 TLS-preservation contracts Record the #2140 node TLS-preference fix in the contracts its runtime commits touch: agent-lifecycle, api-contracts, security-privacy and the dependent storage-recovery contract, with the node API payload and persisted-field proofs in internal/api/contract_test.go and internal/config/config_load_test.go. The canonical completion history registers these contract and proof files as the completion of the auto-register and consolidation runtime commits. Change-source: pulse-maintainer --- .../v6/internal/subsystems/agent-lifecycle.md | 4 +++ .../v6/internal/subsystems/api-contracts.md | 7 +++++ .../internal/subsystems/security-privacy.md | 14 ++++++++++ .../internal/subsystems/storage-recovery.md | 7 +++-- internal/api/contract_test.go | 26 ++++++++++++++++++ internal/config/config_load_test.go | 27 +++++++++++++++++++ 6 files changed, 83 insertions(+), 2 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md index d7d9f033f..6fb3b0261 100644 --- a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md +++ b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md @@ -3423,6 +3423,10 @@ merged `Fingerprint`, so a pin still verifies the peer. Regression tests and `TestHandleUpdateNodePreservesDisabledVerifySSLThroughConsolidation` cover the merge and save paths. +The persisted `VerifySSLExplicit` marker is internal node configuration. It is +not added to the auto-register request or response, nor to the node API +response, so existing clients see no payload change. + ### Manual update freshness Server update-check freshness is owned by internal/updates and its API adapter. diff --git a/docs/release-control/v6/internal/subsystems/api-contracts.md b/docs/release-control/v6/internal/subsystems/api-contracts.md index e5fd6fe22..327a84dd8 100644 --- a/docs/release-control/v6/internal/subsystems/api-contracts.md +++ b/docs/release-control/v6/internal/subsystems/api-contracts.md @@ -4550,6 +4550,13 @@ replaced by the registration's capture state. New nodes continue to take the captured value. This is a behavioural clarification of the existing endpoint, not a new field or contract delta. +The node add and update payloads keep `verifySSL` as an optional pointer, so an +explicit `false` remains distinguishable from an omitted field. An explicit +choice is recorded as operator-owned and is not overridden by automatic PVE +consolidation, which runs on save, load and monitor reconciliation. The response +shape is unchanged: the persisted explicit marker is internal and is not exposed +through the node API. + ### Manual update freshness GET /api/updates/check accepts an optional boolean force query alongside the diff --git a/docs/release-control/v6/internal/subsystems/security-privacy.md b/docs/release-control/v6/internal/subsystems/security-privacy.md index 512b7b550..edd92e93d 100644 --- a/docs/release-control/v6/internal/subsystems/security-privacy.md +++ b/docs/release-control/v6/internal/subsystems/security-privacy.md @@ -951,6 +951,20 @@ tokens, and path-normalization variants. ## Current State +### Node TLS verification preference is operator-owned + +A PVE connection's `VerifySSL` value is security-relevant, so the stored +operator choice must not be changed by automatic reconciliation. The persisted +`VerifySSLExplicit` field records that a caller supplied `verifySSL` through the +node add or update API, distinguishing a deliberate opt-out from the zero +value. Automatic PVE consolidation (`ConsolidatePVEInstances`) and canonical +auto-registration never override an explicit choice; the historical promotion +is retained only when neither side recorded one, and a merged certificate +fingerprint still pins the peer. A record written before the field existed +loads as non-explicit and is unchanged until the operator saves a choice. +`TestPVEInstanceVerifySSLExplicitRoundTrips` in +`internal/config/config_load_test.go` pins the persisted default and round-trip. + ### Manual update availability feedback The settings state owner has a dedicated proof route to diff --git a/docs/release-control/v6/internal/subsystems/storage-recovery.md b/docs/release-control/v6/internal/subsystems/storage-recovery.md index 69a99cd6b..ee92aea49 100644 --- a/docs/release-control/v6/internal/subsystems/storage-recovery.md +++ b/docs/release-control/v6/internal/subsystems/storage-recovery.md @@ -2618,8 +2618,11 @@ preference when the host agent re-registers the node after a disconnect, including when the stored fingerprint was cleared. This prevents a rotating or self-signed certificate from silently re-enabling verification and breaking the monitoring path used for backup and recovery evidence. New nodes still take the -captured registration value. See the agent-lifecycle contract for the runtime -mechanism. +captured registration value. The same preference is preserved when automatic +PVE consolidation folds a duplicate cluster or an overlapping standalone into +the canonical connection on save, load or monitor reconciliation; a merged +certificate fingerprint still pins the peer. See the agent-lifecycle contract +for the runtime mechanism. ### Manual update freshness diff --git a/internal/api/contract_test.go b/internal/api/contract_test.go index 87f851bca..d293e613c 100644 --- a/internal/api/contract_test.go +++ b/internal/api/contract_test.go @@ -1616,6 +1616,32 @@ func TestContract_NodeConfigUpdateTracksOptionalConnectionFieldsAndRedactedSecre } } +// TestContract_NodeConfigUpdateVerifySSLIsOptionalExplicitField pins the node +// config update payload contract the TLS-preference fix relies on: verifySSL is +// an optional pointer, so an explicit false is distinguishable from an omitted +// value. The update handler records an explicit choice from the non-nil pointer +// and must never treat an omitted field as an opt-out (#2140). +func TestContract_NodeConfigUpdateVerifySSLIsOptionalExplicitField(t *testing.T) { + var disabled NodeConfigRequest + if err := json.Unmarshal([]byte(`{"verifySSL":false}`), &disabled); err != nil { + t.Fatalf("decode explicit disable request: %v", err) + } + if disabled.VerifySSL == nil { + t.Fatal("explicit verifySSL:false must decode to a non-nil pointer") + } + if *disabled.VerifySSL { + t.Fatal("explicit verifySSL:false must decode to false") + } + + var omitted NodeConfigRequest + if err := json.Unmarshal([]byte(`{"name":"cluster"}`), &omitted); err != nil { + t.Fatalf("decode request without verifySSL: %v", err) + } + if omitted.VerifySSL != nil { + t.Fatal("omitted verifySSL must stay nil so an existing TLS choice is preserved") + } +} + func TestContract_HostedMagicLinkStablePrincipalProof(t *testing.T) { source, err := os.ReadFile(filepath.Clean("magic_link_handlers.go")) if err != nil { diff --git a/internal/config/config_load_test.go b/internal/config/config_load_test.go index 33ccf9f31..f2b8c53ca 100644 --- a/internal/config/config_load_test.go +++ b/internal/config/config_load_test.go @@ -3,6 +3,7 @@ package config import ( "bytes" "encoding/base64" + "encoding/json" "errors" "os" "path/filepath" @@ -663,3 +664,29 @@ func TestLoadSystemSettingsWithRetry(t *testing.T) { assert.Equal(t, 1, calls) }) } + +// TestPVEInstanceVerifySSLExplicitRoundTrips pins the persisted TLS-choice +// field: an operator's explicit selection must survive serialization so +// automatic consolidation can distinguish it from the zero value, while a +// record written before the field existed loads as the non-explicit default +// (#2140). +func TestPVEInstanceVerifySSLExplicitRoundTrips(t *testing.T) { + original := PVEInstance{ + Name: "homelab", + Host: "https://pve.local:8006", + VerifySSL: false, + VerifySSLExplicit: true, + } + encoded, err := json.Marshal(original) + require.NoError(t, err) + + var decoded PVEInstance + require.NoError(t, json.Unmarshal(encoded, &decoded)) + assert.False(t, decoded.VerifySSL) + assert.True(t, decoded.VerifySSLExplicit, "an explicit TLS choice must survive persistence") + + var legacy PVEInstance + require.NoError(t, json.Unmarshal([]byte(`{"Name":"legacy","Host":"https://pve.local:8006"}`), &legacy)) + assert.False(t, legacy.VerifySSL) + assert.False(t, legacy.VerifySSLExplicit, "a record without the field is not an explicit choice") +} From 10d672c5fa6c1f237a429b09eff213a8cef3c47a Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Mon, 21 Sep 2026 11:55:03 +0100 Subject: [PATCH 7/7] chore(release-control): register #2140 completion pairs Register the two #2140 runtime commits with their contract and proof completion commit so the canonical completion guard evaluates each runtime change together with the follow-up that records its contracts and verification artifacts, as one reviewed unit. Registry-only change; no runtime behaviour. Change-source: pulse-maintainer --- .../release_control/canonical_completion_history.json | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/scripts/release_control/canonical_completion_history.json b/scripts/release_control/canonical_completion_history.json index e3949330b..7a898cda3 100644 --- a/scripts/release_control/canonical_completion_history.json +++ b/scripts/release_control/canonical_completion_history.json @@ -65,6 +65,16 @@ "incomplete_commit": "6f0dc177c8ab3cd4174e53d0774f6e278299a299", "completion_commit": "8738eb1bffbaab4c1d6cc3c23890dbbbc4888a69", "reason": "Evaluate the windowed item-height estimate repair together with the follow-up commit that records its frontend-primitives and performance-and-scalability contract sections, as one reviewed completion unit." + }, + { + "incomplete_commit": "529e8155253c4536696986e2993bb3b2f75eb71c", + "completion_commit": "9fa81dee8fba760b980d821d6e10560589c110f2", + "reason": "Evaluate the canonical auto-register VerifySSL preservation runtime change together with the follow-up commit that records its agent-lifecycle, api-contracts and dependent storage-recovery contract sections and the node API payload proof, as one reviewed completion unit." + }, + { + "incomplete_commit": "919009d7677318c071f888eb28c252d2d79d2728", + "completion_commit": "9fa81dee8fba760b980d821d6e10560589c110f2", + "reason": "Evaluate the PVE consolidation explicit-VerifySSL runtime change together with the follow-up commit that records its agent-lifecycle, api-contracts, security-privacy and dependent storage-recovery contract sections and the node API payload and persisted-field proofs, as one reviewed completion unit." } ] }