fix(rest): validate layer_stack on rg-modify + rd inherit (Bug 434)

pkg/rest/bug_434_rg_layer_stack_validation_test.go

@@ -0,0 +1,229 @@
+// SPDX-License-Identifier: Apache-2.0
+
+/*
+Copyright 2026 Cozystack contributors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package rest
+
+import (
+	"bytes"
+	"encoding/json"
+	"net/http"
+	"testing"
+
+	apiv1 "github.com/cozystack/blockstor/pkg/api/v1"
+	"github.com/cozystack/blockstor/pkg/store"
+)
+
+// Bug 434 (P2 correctness+availability): the RD/RG CREATE paths validate
+// the layer stack (validateLayerStack: allowlist {DRBD,LUKS,STORAGE},
+// DRBD first, STORAGE terminal), but two sibling paths bypassed it:
+//
+//  1. `rg modify` (handleRGUpdate) gated only place_count — an invalid
+//     select_filter.layer_stack was merged and persisted unvalidated.
+//  2. handleRDCreate ran validateRDCreateBody (which validates an
+//     EXPLICIT layer_list) BEFORE inheritLayerStackFromRG, so an RD
+//     created against such an RG inherited the invalid stack unchecked.
+//
+// Net: an invalid, unmaterialisable layer stack the DIRECT create path
+// refuses (400) reached a persisted RD spec via the rg-modify → inherit
+// chain. Same asymmetry class as the resize-bounds bypass (create
+// validates; a sibling modify/inherit path bypasses).
+//
+// Fix: (1) validate the layer stack in the rg-update wire gate, and
+// (2) defense-in-depth re-validate the resolved (possibly inherited)
+// stack in handleRDCreate after the RG inherit.
+//
+// These FAIL on the pre-fix tree (the invalid stack is accepted /
+// inherited) and PASS with the two gates.
+
+// bug434InvalidStack is STORAGE-before-DRBD: STORAGE must be terminal and
+// DRBD must be first, so this is refused by the create path.
+var bug434InvalidStack = []string{"STORAGE", "DRBD"} //nolint:gochecknoglobals // shared test fixture
+
+// TestBug434RGUpdateRefusesInvalidLayerStack — FAIL-on-bug (Fix 1).
+//
+// `rg modify` with an invalid select_filter.layer_stack must be rejected
+// with the same 400 the create path returns, and the stored RG must keep
+// its valid stack (the gate runs BEFORE PatchResourceGroup).
+func TestBug434RGUpdateRefusesInvalidLayerStack(t *testing.T) {
+	t.Parallel()
+
+	st := store.NewInMemory()
+	ctx := t.Context()
+
+	if err := st.ResourceGroups().Create(ctx, &apiv1.ResourceGroup{
+		Name:         "rg-b434",
+		SelectFilter: apiv1.AutoSelectFilter{LayerStack: []string{"DRBD", "STORAGE"}},
+	}); err != nil {
+		t.Fatalf("seed RG: %v", err)
+	}
+
+	base, stop := startServerWithStore(t, st)
+	defer stop()
+
+	body, _ := json.Marshal(map[string]any{
+		"select_filter": map[string]any{"layer_stack": bug434InvalidStack},
+	})
+
+	req, _ := http.NewRequestWithContext(ctx, http.MethodPut, base+"/v1/resource-groups/rg-b434", bytes.NewReader(body))
+	req.Header.Set("Content-Type", "application/json")
+
+	resp, err := http.DefaultClient.Do(req)
+	if err != nil {
+		t.Fatalf("PUT: %v", err)
+	}
+	defer func() { _ = resp.Body.Close() }()
+
+	if resp.StatusCode != http.StatusBadRequest {
+		got, _ := readAllBody(resp)
+		t.Fatalf("status: got %d, want 400 (Bug 434: rg modify must refuse an invalid layer_stack). Body: %s",
+			resp.StatusCode, got)
+	}
+
+	// The rejected update must not reach the store: the RG keeps its
+	// valid stack.
+	stored, err := st.ResourceGroups().Get(ctx, "rg-b434")
+	if err != nil {
+		t.Fatalf("re-fetch RG: %v", err)
+	}
+
+	if got := stored.SelectFilter.LayerStack; len(got) != 2 || got[0] != "DRBD" || got[1] != "STORAGE" {
+		t.Errorf("stored LayerStack changed after rejection: got %v, want [DRBD STORAGE]", got)
+	}
+}
+
+// TestBug434RGUpdateValidLayerStackAccepted pins the healthy path: a
+// valid re-order still lands.
+func TestBug434RGUpdateValidLayerStackAccepted(t *testing.T) {
+	t.Parallel()
+
+	st := store.NewInMemory()
+	ctx := t.Context()
+
+	if err := st.ResourceGroups().Create(ctx, &apiv1.ResourceGroup{
+		Name:         "rg-b434-ok",
+		SelectFilter: apiv1.AutoSelectFilter{LayerStack: []string{"DRBD", "STORAGE"}},
+	}); err != nil {
+		t.Fatalf("seed RG: %v", err)
+	}
+
+	base, stop := startServerWithStore(t, st)
+	defer stop()
+
+	body, _ := json.Marshal(map[string]any{
+		"select_filter": map[string]any{"layer_stack": []string{"DRBD", "LUKS", "STORAGE"}},
+	})
+
+	req, _ := http.NewRequestWithContext(ctx, http.MethodPut, base+"/v1/resource-groups/rg-b434-ok", bytes.NewReader(body))
+	req.Header.Set("Content-Type", "application/json")
+
+	resp, err := http.DefaultClient.Do(req)
+	if err != nil {
+		t.Fatalf("PUT: %v", err)
+	}
+	defer func() { _ = resp.Body.Close() }()
+
+	if resp.StatusCode != http.StatusOK {
+		got, _ := readAllBody(resp)
+		t.Fatalf("status: got %d, want 200 (valid layer_stack must be accepted). Body: %s",
+			resp.StatusCode, got)
+	}
+}
+
+// TestBug434RGUpdateAbsentLayerStackUntouched pins "field absent = leave
+// alone": a PUT that doesn't mention layer_stack must NOT fire the gate.
+func TestBug434RGUpdateAbsentLayerStackUntouched(t *testing.T) {
+	t.Parallel()
+
+	st := store.NewInMemory()
+	ctx := t.Context()
+
+	if err := st.ResourceGroups().Create(ctx, &apiv1.ResourceGroup{
+		Name:         "rg-b434-absent",
+		SelectFilter: apiv1.AutoSelectFilter{LayerStack: []string{"DRBD", "STORAGE"}},
+	}); err != nil {
+		t.Fatalf("seed RG: %v", err)
+	}
+
+	base, stop := startServerWithStore(t, st)
+	defer stop()
+
+	body, _ := json.Marshal(map[string]any{"description": "operator note"})
+
+	req, _ := http.NewRequestWithContext(ctx, http.MethodPut, base+"/v1/resource-groups/rg-b434-absent", bytes.NewReader(body))
+	req.Header.Set("Content-Type", "application/json")
+
+	resp, err := http.DefaultClient.Do(req)
+	if err != nil {
+		t.Fatalf("PUT: %v", err)
+	}
+	defer func() { _ = resp.Body.Close() }()
+
+	if resp.StatusCode != http.StatusOK {
+		got, _ := readAllBody(resp)
+		t.Fatalf("status: got %d, want 200 (absent layer_stack must NOT fire the gate). Body: %s",
+			resp.StatusCode, got)
+	}
+}
+
+// TestBug434RDCreateRefusesInheritedInvalidLayerStack — FAIL-on-bug (Fix 2).
+//
+// Even if an RG already carries an invalid layer stack (seeded directly
+// here to model an RG that predates the rg-modify gate, or was persisted
+// by any other path), an RD created against it — inheriting the stack
+// with no explicit layer_list — must be refused, and no RD may persist.
+// handleRDCreate validates the EXPLICIT body before the RG inherit, so
+// without the post-inherit re-validation the invalid stack reaches the RD.
+func TestBug434RDCreateRefusesInheritedInvalidLayerStack(t *testing.T) {
+	t.Parallel()
+
+	st := store.NewInMemory()
+	ctx := t.Context()
+
+	// Seed an RG that ALREADY holds the invalid stack (store Create does
+	// not validate — validation lives in the REST handlers).
+	if err := st.ResourceGroups().Create(ctx, &apiv1.ResourceGroup{
+		Name:         "rg-b434-bad",
+		SelectFilter: apiv1.AutoSelectFilter{LayerStack: bug434InvalidStack},
+	}); err != nil {
+		t.Fatalf("seed bad RG: %v", err)
+	}
+
+	base, stop := startServerWithStore(t, st)
+	defer stop()
+
+	body, _ := json.Marshal(map[string]any{
+		"resource_definition": map[string]any{
+			"name":                "rd-b434-inherit",
+			"resource_group_name": "rg-b434-bad",
+		},
+	})
+
+	resp := httpPost(t, base+"/v1/resource-definitions", body)
+	defer func() { _ = resp.Body.Close() }()
+
+	if resp.StatusCode != http.StatusBadRequest {
+		got, _ := readAllBody(resp)
+		t.Fatalf("status: got %d, want 400 (Bug 434: RD must not inherit an invalid layer stack). Body: %s",
+			resp.StatusCode, got)
+	}
+
+	// No RD may persist with the invalid inherited stack.
+	if _, err := st.ResourceDefinitions().Get(ctx, "rd-b434-inherit"); err == nil {
+		t.Errorf("RD rd-b434-inherit persisted after rejection — must not (invalid inherited layer stack)")
+	}
+}

pkg/rest/resource_definitions.go

@@ -310,6 +310,19 @@ func (s *Server) handleRDCreate(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	// Bug 434 (defense-in-depth): validateRDCreateBody validates an
+	// EXPLICIT layer_list, but it runs BEFORE the RG inherit above — so an
+	// invalid stack inherited from an RG (e.g. one that predates the
+	// rg-modify validation gate) would reach the store unchecked. Re-validate
+	// the resolved (possibly inherited) stack here, mirroring the create
+	// path's 400, so an unmaterialisable layer chain never persists onto an RD.
+	lsErr := validateLayerStack(rd.LayerStack)
+	if lsErr != nil {
+		writeError(w, http.StatusBadRequest, lsErr.Error())
+
+		return
+	}
+
 	// Bug 262 (P2): stand-caught — `linstor rd lp <new-rd> | grep
 	// quorum` reported `DrbdOptions/Resource/quorum off` on freshly-
 	// created RDs because the RG-create surface never seeded the

pkg/rest/resource_groups.go

@@ -215,11 +215,12 @@ func (s *Server) handleRGUpdate(w http.ResponseWriter, r *http.Request) {
 	// PUT's contribution.
 	var rebalanceScheduled bool
 
-	// Bug 367 / 361: refuse negative or absurdly-large place_count
-	// on the patch body BEFORE we hand it to PatchResourceGroup. See
-	// rgUpdatePlaceCountGate — keeps the wire-validation rule out of
-	// the store error path and the handler under the funlen budget.
-	gateErr := rgUpdatePlaceCountGate(raw, &patch)
+	// Bug 367 / 361 + Bug 434: refuse a negative/absurd place_count OR an
+	// invalid select_filter.layer_stack on the patch body BEFORE we hand
+	// it to PatchResourceGroup. See rgUpdateWireGate — keeps the
+	// wire-validation rules out of the store error path and the handler
+	// under the funlen budget.
+	gateErr := rgUpdateWireGate(raw, &patch)
 	if gateErr != nil {
 		writeError(w, http.StatusBadRequest, gateErr.Error())
 
@@ -895,8 +896,9 @@ func rgDeleteRefusedMessage(name string, count int) string {
 // allocates against an absurd target.
 const rgPlaceCountSanityCeiling = 1_000_000
 
-// rgUpdatePlaceCountGate runs the Bug 367 / 361 wire-validation
-// rule on the PUT patch body BEFORE PatchResourceGroup sees it.
+// rgUpdateWireGate runs the PUT patch body's wire-validation rules
+// (Bug 367 / 361 place_count + Bug 434 layer_stack) BEFORE
+// PatchResourceGroup sees it.
 //
 // Extracted from handleRGUpdate to keep the handler under the funlen
 // budget; the split tracks the natural "validate → patch → persist"
@@ -910,7 +912,7 @@ const rgPlaceCountSanityCeiling = 1_000_000
 // validator's actionable message; routing the error through the
 // PatchResourceGroup callback would demote it to a 500 (writeStoreError
 // has no band for "bad request").
-func rgUpdatePlaceCountGate(raw []byte, patch *apiv1.ResourceGroup) error {
+func rgUpdateWireGate(raw []byte, patch *apiv1.ResourceGroup) error {
 	mentioned := rgSelectFilterKeys(raw)
 	if _, ok := mentioned["place_count"]; ok {
 		err := validateRGSelectFilterPlaceCount(patch.SelectFilter.PlaceCount, 0)
@@ -926,6 +928,20 @@ func rgUpdatePlaceCountGate(raw []byte, patch *apiv1.ResourceGroup) error {
 		}
 	}
 
+	// Bug 434: `rg modify` must reject an invalid select_filter.layer_stack
+	// with the same 400 the create path (handleRGCreate) returns, instead
+	// of storing it unvalidated for a child RD to inherit
+	// (handleRDCreate → inheritLayerStackFromRG). Validate exactly the
+	// value mergeRGSelectFilterLists would persist (patch.LayerStack !=
+	// nil); a nil/empty stack is the legitimate "clear to default" and
+	// validateLayerStack accepts it.
+	if patch.SelectFilter.LayerStack != nil {
+		err := validateLayerStack(patch.SelectFilter.LayerStack)
+		if err != nil {
+			return err
+		}
+	}
+
 	return nil
 }
 

tests/e2e/cli-matrix/README.md

@@ -44,6 +44,7 @@ A cell counts as **FAIL** if either leg times out, the `linstor` CLI exits non-z
 | `rd-clone-vd-data-plane.sh` | 020 | `linstor rd clone <src> <dst>` on a VD-bearing source (plain CLI body, no `use_zfs_clone`) AND the raw-REST `use_zfs_clone=true` body linstor-csi sends both materialise a real clone: 2 replicas UpToDate, marker bytes from the source present on EVERY clone replica (promote each in turn), clone status COMPLETE, internal `clone-<dst>` snapshot visible on the source. Pre-fix: 400 on `use_zfs_clone`, 501 on VD-bearing sources (Bug 114 gate). |
 | `encryption-passphrase-luks-rd.sh` | 023 | Secret-only LUKS flow: `linstor encryption create-passphrase` alone (legacy `DrbdOptions/EncryptPassphrase` controller prop asserted ABSENT throughout) unlocks `rd c -l drbd,luks,storage` + autoplace to UpToDate, and the Secret-backed passphrase actually opens the LUKS header on each replica's backing device. Requires the Bug-023 fix (PR #143); pre-fix the rd-create is rejected with "LUKS layer requires DrbdOptions/EncryptPassphrase to be set first". |
 | `vd-modify-preserves-drbd-minor.sh` | 433 | A legal in-bounds VD-scoped modify (`vd set-size` grow / `vd set-property`) must NOT change the per-volume DRBDMinor — the /dev/drbd<N> device identity. Creates rd-a (lowest minor Ma) + rd-b (next minor Mb), frees Ma by deleting rd-a, then modifies rd-b: its DRBDMinor AND device path must stay Mb over a settle window. Pre-fix `wireToCRDVD` dropped the minor on the VD-scoped write-back and the allocator re-stamped the freed lower Ma — a permanent device-identity change on a live resized volume. |
+| `rg-modify-invalid-layer-rejected.sh` | 434 | `rg modify --layer-list storage,drbd` (STORAGE-before-DRBD, the ordering the create path refuses) must be rejected, the RG must keep its valid `[DRBD,STORAGE]` stack, and an RD inheriting from the RG must get the valid stack — never `[STORAGE,DRBD]`. Pre-fix `rg modify` gated only place_count and `handleRDCreate` validated the explicit layer_list BEFORE the RG inherit, so an invalid stack reached a persisted RD via the modify→inherit chain. |
 
 ## Running