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