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"}, },