From 92c89831d4b3777482684ee707b2ecb79e47e7a1 Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Mon, 26 Oct 2026 09:00:07 +0100 Subject: [PATCH 01/24] fix(luks)!: read the master passphrase from the Secret only The passphrase could also live in a property: DrbdOptions/ EncryptPassphrase on the controller, the DrbdOptions/Encryption/ passphrase alias on a definition, or LuksPassphrase on a replica. A property is stored in plaintext on its CR, and when one was set it won over the Secret, so any dump of the CRs for a backup carried the key to every LUKS volume while the docs said the key lived in the Secret. The Secret written by encryption create-passphrase is now the only source. The satellite drops every passphrase property before it adds the Secret's value, the dispatcher reads that value alone and drops a wire key that came with the replica, and the LUKS prerequisite checks in REST and the CLI look at the Secret only. The REST API refuses any write that would store such a property, on every door; deleting one stays allowed. BREAKING CHANGE: a passphrase set only in a property no longer unlocks LUKS volumes. Move it into the passphrase Secret. Assisted-by: LLM Signed-off-by: Andrei Kvapil --- internal/cli/encryption.go | 15 --- pkg/dispatcher/dispatcher.go | 81 +++--------- pkg/passphrase/passphrase.go | 43 ++++++- pkg/rest/passphrase_props_guard.go | 124 +++++++++++++++++++ pkg/rest/properties_info.go | 5 - pkg/rest/resource_definitions.go | 54 +------- pkg/rest/server.go | 4 +- pkg/satellite/controllers/luks_passphrase.go | 17 ++- 8 files changed, 196 insertions(+), 147 deletions(-) create mode 100644 pkg/rest/passphrase_props_guard.go diff --git a/internal/cli/encryption.go b/internal/cli/encryption.go index 4d2d5ffb..7fd0d8a5 100644 --- a/internal/cli/encryption.go +++ b/internal/cli/encryption.go @@ -317,20 +317,5 @@ func checkLUKSPrerequisite(ctx context.Context, run *runContext, layers []string return nil } - // The Secret is the primary mechanism, but a cluster provisioned - // before it existed carries the passphrase in the legacy controller - // prop, and the satellite still unlocks with it. REST accepts both; - // checking only the Secret makes the CLI refuse a stack that the - // REST door on the same cluster creates without complaint. - props, err := run.Store.ControllerProps().Get(ctx) - if err != nil { - return fmt.Errorf("read controller properties to check the %s prerequisite: %w", - apiv1.LayerKindLUKS, err) - } - - if props[passphrase.PropKeyCanonical] != "" { - return nil - } - return fmt.Errorf("layer list includes %s: %w", apiv1.LayerKindLUKS, errLUKSWithoutPassphrase) } diff --git a/pkg/dispatcher/dispatcher.go b/pkg/dispatcher/dispatcher.go index 64f7a235..6f7fe41f 100644 --- a/pkg/dispatcher/dispatcher.go +++ b/pkg/dispatcher/dispatcher.go @@ -35,6 +35,7 @@ import ( blockstoriov1alpha1 "github.com/cozystack/blockstor/api/v1alpha1" apiv1 "github.com/cozystack/blockstor/pkg/api/v1" "github.com/cozystack/blockstor/pkg/drbd" + "github.com/cozystack/blockstor/pkg/passphrase" intent "github.com/cozystack/blockstor/pkg/satellite/intent" "github.com/cozystack/blockstor/pkg/store/k8s" ) @@ -219,44 +220,25 @@ func assembleDesired(target *blockstoriov1alpha1.Resource, peers []blockstoriov1 wireProps := mergeEffectiveProps(target.Spec.Props, effectiveProps, drbdOpts) - // LUKS passphrase: lift the operator-set master passphrase onto - // the stable `LuksPassphrase` prop the satellite's LUKS layer - // reads. Two source keys are accepted, in order: - // - // 1. `DrbdOptions/EncryptPassphrase` — upstream LINSTOR's - // canonical cluster-scope master-key name, stamped via - // `linstor controller set-property DrbdOptions/ - // EncryptPassphrase `. Bug 95 enforces its presence - // as a hard prerequisite for any LUKS-layered RD; this is - // the key the REST layer surfaces through - // effectiveprops.Resolve (it copies ControllerConfig. - // ExtraProps onto the output map verbatim). - // - // 2. `DrbdOptions/Encryption/passphrase` — legacy per-RD shape - // the early-Phase-9 dispatcher introduced (Bug 265 root cause: - // this was the ONLY key the dispatcher read, while the - // controller wrote the cluster master under the upstream - // name in #1 → the operator-set master key never reached - // the satellite and every LUKS RD looped on `Props. - // LuksPassphrase empty`). Kept as a backwards-compat alias. - // - // The first non-empty wins; both keys are dropped from drbdOpts - // after lifting so splitDRBDOptions on the satellite side - // doesn't render either as a `passphrase` line in the .res file's - // options block (drbdadm create-md rejects unknown options with - // `expected: cpu-mask | ... but got 'passphrase'`). The - // passphrase reaches the LUKS layer via wireProps, not the .res - // file. + // LUKS passphrase: the satellite puts the Secret's value under the + // upstream master-key name in effectiveProps, after dropping any + // property that carried one (passphrase.DropPlaintextProps). It is + // lifted onto the wire key the satellite's LUKS layer reads and + // dropped from drbdOpts, so it never renders as a `passphrase` line + // in the .res options. A wire key that came in with the replica's + // own properties is dropped first, so nothing but the Secret reaches + // the LUKS layer. + passphrase.DropPlaintextProps(wireProps) + if pass := pickLUKSPassphrase(drbdOpts); pass != "" { if wireProps == nil { wireProps = map[string]string{} } - wireProps["LuksPassphrase"] = pass + wireProps[passphrase.WirePropKey] = pass } - delete(drbdOpts, drbdEncryptionPassphraseKey) - delete(drbdOpts, drbdEncryptPassphraseKey) + passphrase.DropPlaintextProps(drbdOpts) var layerStack []string if rd != nil { @@ -343,39 +325,12 @@ func buildDesiredPeers(dropped []string, peers []blockstoriov1alpha1.Resource) [ return out } -// drbdEncryptionPassphraseKey is the legacy per-RD shape the -// early-Phase-9 dispatcher introduced. Kept as a backwards-compat -// alias source key — operators on older clusters who set the -// passphrase via the inner-namespace form should keep working -// without re-stamping. Bug 265 root cause: the dispatcher used to -// read ONLY this key, never the upstream master-key one below. -// -//nolint:gosec // not a credential value, the string is a prop key name -const drbdEncryptionPassphraseKey = "DrbdOptions/Encryption/passphrase" - -// drbdEncryptPassphraseKey is upstream LINSTOR's canonical cluster- -// scope master-key name. Operators set it via `linstor controller -// set-property DrbdOptions/EncryptPassphrase `; Bug 95 enforces -// its presence as a hard prerequisite for any LUKS-layered RD. The -// REST/effectiveprops layer surfaces it under this exact key, so the -// dispatcher MUST read it to receive the operator-set master key. -// -//nolint:gosec // not a credential value, the string is a prop key name -const drbdEncryptPassphraseKey = "DrbdOptions/EncryptPassphrase" - -// pickLUKSPassphrase returns the LUKS passphrase from the resolved -// options bag, preferring the upstream-canonical master-key shape -// over the legacy per-RD alias. Empty when neither is set; the -// caller suppresses the wire-prop stamp in that case so the -// satellite surfaces the missing-passphrase failure at the LUKS -// layer (the apply-time failure point that produces actionable -// error messages on the resource Status.Conditions). +// pickLUKSPassphrase returns the passphrase the satellite put in the +// resolved options from the Secret, empty when there is none; the +// caller then leaves the wire key unset and the LUKS layer fails the +// apply with an actionable message. func pickLUKSPassphrase(opts map[string]string) string { - if v := opts[drbdEncryptPassphraseKey]; v != "" { - return v - } - - return opts[drbdEncryptionPassphraseKey] + return opts[passphrase.PropKeyCanonical] } // mergeEffectiveProps splits the resolver's output into: diff --git a/pkg/passphrase/passphrase.go b/pkg/passphrase/passphrase.go index 53c207ab..1d4cd65e 100644 --- a/pkg/passphrase/passphrase.go +++ b/pkg/passphrase/passphrase.go @@ -35,6 +35,7 @@ package passphrase import ( "context" + "strings" "github.com/cockroachdb/errors" corev1 "k8s.io/api/core/v1" @@ -56,16 +57,46 @@ const SecretKey = "passphrase" // PropKeyCanonical is upstream LINSTOR's cluster-scope master-key // property name (`linstor controller set-property -// DrbdOptions/EncryptPassphrase `). Bug 023: this LEGACY path -// stores the passphrase in PLAINTEXT on the ControllerConfig CRD — -// kept working for backward compatibility, but the Secret written -// by `encryption create-passphrase` is the primary mechanism. +// DrbdOptions/EncryptPassphrase `). blockstor never reads the +// passphrase from a property: a property is stored in plaintext on its +// CR, and any dump of the CRs for a backup would carry the key to every +// LUKS volume. The REST API refuses to store it, and the satellite drops +// it from a definition's properties before it adds the Secret's value +// under this key for the dispatcher. const PropKeyCanonical = "DrbdOptions/EncryptPassphrase" -// PropKeyLegacy is the early-Phase-9 per-RD alias the dispatcher -// still alias-reads (Bug 265 history, pkg/dispatcher). Deprecated. +// PropKeyLegacy is the early per-RD alias of PropKeyCanonical. It is +// refused and dropped the same way. const PropKeyLegacy = "DrbdOptions/Encryption/passphrase" +// WirePropKey is the property the satellite's LUKS layer reads the +// passphrase from. Only the dispatcher sets it, from the Secret; a +// property of that name an operator set is refused and dropped too. +const WirePropKey = "LuksPassphrase" + +// IsPlaintextPropKey reports whether key would carry the master +// passphrase in a property. Matched without regard to case, so no +// spelling slips through. +func IsPlaintextPropKey(key string) bool { + for _, k := range []string{PropKeyCanonical, PropKeyLegacy, WirePropKey} { + if strings.EqualFold(key, k) { + return true + } + } + + return false +} + +// DropPlaintextProps removes every property that would carry the +// master passphrase from props. +func DropPlaintextProps(props map[string]string) { + for key := range props { + if IsPlaintextPropKey(key) { + delete(props, key) + } + } +} + // SecretName resolves the passphrase Secret's name from the // singleton ControllerConfig, falling back to DefaultSecretName when // the ControllerConfig is absent or doesn't pin a reference. The diff --git a/pkg/rest/passphrase_props_guard.go b/pkg/rest/passphrase_props_guard.go new file mode 100644 index 00000000..4f6f948d --- /dev/null +++ b/pkg/rest/passphrase_props_guard.go @@ -0,0 +1,124 @@ +// 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" + "fmt" + "io" + "net/http" + + "github.com/cozystack/blockstor/pkg/passphrase" +) + +// withPlaintextPassphraseRefusal refuses a write whose body would store the +// LUKS master passphrase in a property, on any door: controller, group, +// definition, replica, volume, node or pool properties, on create or on +// modify. A property is kept in plaintext on its CR, so any dump of the CRs +// for a backup would carry the key to every LUKS volume; the passphrase +// lives in the Secret `encryption create-passphrase` writes, and blockstor +// reads it from nowhere else. Deleting such a property stays allowed: a key +// named in a delete list is a string, not an object key. +func withPlaintextPassphraseRefusal(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Body == nil || !isWriteMethod(r.Method) { + next.ServeHTTP(w, r) + + return + } + + body, err := io.ReadAll(r.Body) + r.Body = replayedBody{Reader: io.MultiReader(bytes.NewReader(body), failingReader{err: err}), closer: r.Body} + + if err == nil { + if key, found := plaintextPassphraseKeyIn(body); found { + writeError(w, http.StatusBadRequest, fmt.Sprintf( + "refusing to store the LUKS master passphrase in property %q: properties are kept in plaintext "+ + "on their resource; run `linstor encryption create-passphrase`, which keeps it in a Secret", key)) + + return + } + } + + next.ServeHTTP(w, r) + }) +} + +func isWriteMethod(method string) bool { + return method == http.MethodPost || method == http.MethodPut || method == http.MethodPatch +} + +// plaintextPassphraseKeyIn finds an object key, at any depth of a JSON body, +// that would carry the master passphrase. A body that is not JSON has none; +// the handler answers it as before. +func plaintextPassphraseKeyIn(body []byte) (string, bool) { + var doc any + + if json.Unmarshal(body, &doc) != nil { + return "", false + } + + return findPlaintextPassphraseKey(doc) +} + +func findPlaintextPassphraseKey(node any) (string, bool) { + switch typed := node.(type) { + case map[string]any: + for key, value := range typed { + if passphrase.IsPlaintextPropKey(key) { + return key, true + } + + if found, ok := findPlaintextPassphraseKey(value); ok { + return found, true + } + } + case []any: + for _, value := range typed { + if found, ok := findPlaintextPassphraseKey(value); ok { + return found, true + } + } + } + + return "", false +} + +// replayedBody hands the handler the bytes the guard read, then the error +// the read ended with, and closes the original body. +type replayedBody struct { + io.Reader + + closer io.Closer +} + +func (b replayedBody) Close() error { + return b.closer.Close() //nolint:wrapcheck // the request body's own error +} + +type failingReader struct{ err error } + +func (f failingReader) Read([]byte) (int, error) { + if f.err != nil { + return 0, f.err + } + + return 0, io.EOF +} diff --git a/pkg/rest/properties_info.go b/pkg/rest/properties_info.go index 6a1bee59..5be073e2 100644 --- a/pkg/rest/properties_info.go +++ b/pkg/rest/properties_info.go @@ -145,11 +145,6 @@ func commonPropsInfoEntries() []propsInfoEntry { Name: drbdNetCramHMACAlgKey, Info: "HMAC algorithm for DRBD per-pair authentication.", }, - { - Name: drbdEncryptPassphraseKey, - Info: "Cluster-scope DRBD encryption passphrase. Redacted on " + - "read; set via `linstor encryption set-passphrase`.", - }, } } diff --git a/pkg/rest/resource_definitions.go b/pkg/rest/resource_definitions.go index cc4533a4..1c0b772f 100644 --- a/pkg/rest/resource_definitions.go +++ b/pkg/rest/resource_definitions.go @@ -27,7 +27,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/log" apiv1 "github.com/cozystack/blockstor/pkg/api/v1" - "github.com/cozystack/blockstor/pkg/passphrase" "github.com/cozystack/blockstor/pkg/store" ) @@ -893,26 +892,9 @@ func layerStackFromLayerData(data []apiv1.ResourceLayer) []string { return out } -// drbdEncryptPassphraseKey is the upstream LINSTOR controller-scope -// property the operator sets via `linstor controller set-property -// DrbdOptions/EncryptPassphrase ` to seed the cluster's -// LUKS master key material. Bug 95 requires SOME master passphrase -// as a hard prerequisite for accepting an RD whose LayerStack -// carries LUKS — without it, the satellite's `cryptsetup luksFormat` -// has no key to format the volume with and the layer chain silently -// fell back to plaintext-on-DRBD before this gate landed. -// -// Bug 023: this prop is the LEGACY mechanism — it stores the -// passphrase in PLAINTEXT on the ControllerConfig CRD. The primary -// (upstream-parity) mechanism is the encryption Secret stamped by -// `linstor encryption create-passphrase`; the prop stays accepted -// for clusters that provisioned before the Secret path existed. -const drbdEncryptPassphraseKey = passphrase.PropKeyCanonical - // ErrLUKSRequiresPassphrase is the static sentinel surfaced as a 400 // when an RD-create body asks for a LUKS layer but the cluster has -// no master passphrase — neither the encryption Secret (primary) -// nor the legacy `DrbdOptions/EncryptPassphrase` controller prop. +// no master passphrase in the encryption Secret. // Sentinel-shaped to match the layer-validation style; the handler // wraps it with the human-readable remediation hint. var ErrLUKSRequiresPassphrase = errors.New( @@ -926,20 +908,9 @@ var ErrLUKSRequiresPassphrase = errors.New( // prerequisite immediately instead of waiting for replicas to come // up plaintext. // -// Two accepted sources, checked in order (Bug 023): -// -// 1. The encryption Secret written by `linstor encryption -// create-passphrase` (POST /v1/encryption/passphrase) — the -// upstream-standard flow and the PRIMARY mechanism. Pre-fix the -// gate ignored it entirely, so the documented create-passphrase -// → LUKS-RD-create sequence 400'd and the hint told operators -// to stamp a PLAINTEXT passphrase into a controller prop. -// 2. The legacy `DrbdOptions/EncryptPassphrase` controller prop — -// deprecated (plaintext on the CRD) but kept for backward -// compatibility with clusters provisioned before the Secret -// path existed. The satellite-facing dispatch keeps honouring -// it with unchanged precedence (props win over the Secret) so -// existing LUKS volumes keep unlocking with the same key. +// The only source is the encryption Secret written by `linstor +// encryption create-passphrase` (POST /v1/encryption/passphrase). A +// passphrase in a property is never read (pkg/passphrase). // // Falls through (nil) when the controller client is unconfigured — // the test-only code paths that build a Server without a Client @@ -964,21 +935,8 @@ func (s *Server) refuseLUKSWithoutPassphrase(ctx context.Context, layers []strin return nil } - ctrl, err := controllerScopeProps(ctx, s.Client) - if err != nil { - return errors.Wrap(err, "read controller props for LUKS prereq check") - } - - if ctrl[drbdEncryptPassphraseKey] != "" { - return nil - } - - return errors.Wrapf(ErrLUKSRequiresPassphrase, - "run `linstor encryption create-passphrase` "+ - "before creating a LUKS-layered RD (legacy alternative: "+ - "`linstor controller set-property %s `, "+ - "deprecated — stores the passphrase in plaintext)", - drbdEncryptPassphraseKey) + return errors.Wrap(ErrLUKSRequiresPassphrase, + "run `linstor encryption create-passphrase` before creating a LUKS-layered RD") } // resourceDefinitionModifyBody is the shape upstream golinstor sends diff --git a/pkg/rest/server.go b/pkg/rest/server.go index 1e01c075..d6eef190 100644 --- a/pkg/rest/server.go +++ b/pkg/rest/server.go @@ -337,10 +337,12 @@ func (s *Server) startTLS(ctx context.Context, handler http.Handler, errCh chan< // oversized POST trips a 413 envelope at // the wire edge instead of leaking the // etcd/k8s rejection string in a 500. +// withPlaintextPassphraseRefusal — refuses a body that would store +// the LUKS master passphrase in a property. func (s *Server) handler() http.Handler { mux := s.buildMux() - wrapped := withLogging(instrumentRequests(mux, withHEADContentLength(with404Envelope(withContentTypeJSON(mux, withBodyLimit(maxRequestBodyBytes, mux)))))) + wrapped := withLogging(instrumentRequests(mux, withHEADContentLength(with404Envelope(withContentTypeJSON(mux, withBodyLimit(maxRequestBodyBytes, withPlaintextPassphraseRefusal(mux))))))) // SSE bypass: the event-stream endpoints push frames over a long- // lived connection, which is fundamentally incompatible with the diff --git a/pkg/satellite/controllers/luks_passphrase.go b/pkg/satellite/controllers/luks_passphrase.go index dfc7023a..f9552be9 100644 --- a/pkg/satellite/controllers/luks_passphrase.go +++ b/pkg/satellite/controllers/luks_passphrase.go @@ -49,9 +49,9 @@ import ( // `DrbdOptions/EncryptPassphrase` key in effectiveProps, so // BuildDesired's pickLUKSPassphrase lifts it onto `LuksPassphrase` // and strips it from the rendered .res options exactly as before. -// Precedence is unchanged — an operator-set controller prop (either -// spelling) wins over the Secret, so existing LUKS volumes keep -// unlocking with the key they were formatted with. +// The Secret is the only source. A passphrase an operator put in a +// property, under the upstream key, the early per-RD alias or the wire +// key itself, is dropped first: it sits in plaintext on its CR. // // Failure posture: a Secret read error is logged and swallowed — // the apply chain then surfaces the established, actionable @@ -61,16 +61,15 @@ import ( // No-ops when: // - the RD has no LUKS layer (the overwhelmingly common path — // zero Secret round-trips); -// - either passphrase prop key is already present (legacy path); // - Config.Namespace is empty (unit-test rigs without a // controller namespace). func (r *ResourceReconciler) injectLUKSMasterPassphrase(ctx context.Context, rd *blockstoriov1alpha1.ResourceDefinition, effectiveProps map[string]string) { - if rd == nil || effectiveProps == nil || !layerStackHasLUKS(rd.Spec.LayerStack) { - return - } + // A passphrase in a property never counts: whatever an operator put + // there under any of its keys goes, so only the Secret's value can + // reach the LUKS layer. + passphrase.DropPlaintextProps(effectiveProps) - if effectiveProps[passphrase.PropKeyCanonical] != "" || - effectiveProps[passphrase.PropKeyLegacy] != "" { + if rd == nil || effectiveProps == nil || !layerStackHasLUKS(rd.Spec.LayerStack) { return } From 72a1480a5891102c465ff2929eacd29a8d591b17 Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Mon, 26 Oct 2026 09:09:40 +0100 Subject: [PATCH 02/24] test(luks): pin the Secret-only passphrase contract in existing tests The tests that asserted a passphrase property still unlocks LUKS now assert the opposite: the controller prop does not satisfy the REST or CLI prerequisite, the early per-RD alias never reaches the wire, and the Secret replaces every passphrase property on the satellite. Tests that only used the controller prop to satisfy the LUKS gate now seed the passphrase Secret through a shared helper, and the refusal checks expect a pointer at encryption create-passphrase with no property named. Assisted-by: LLM Signed-off-by: Andrei Kvapil --- internal/cli/luks_writers_test.go | 35 ++++-- .../dispatcher_luks_master_passphrase_test.go | 109 +++++++++--------- pkg/rest/helpers_test.go | 42 +++++-- pkg/rest/luks_gate_bug023_test.go | 41 +++---- pkg/rest/rd_clone_validation_test.go | 10 +- pkg/rest/resource_definitions_bug116_test.go | 61 ++-------- pkg/rest/resource_definitions_bug95_test.go | 53 +++------ .../luks_passphrase_internal_test.go | 27 ++--- 8 files changed, 169 insertions(+), 209 deletions(-) diff --git a/internal/cli/luks_writers_test.go b/internal/cli/luks_writers_test.go index 36ae2c01..a227cfbb 100644 --- a/internal/cli/luks_writers_test.go +++ b/internal/cli/luks_writers_test.go @@ -5,6 +5,7 @@ package cli_test import ( "bytes" "context" + "strings" "testing" corev1 "k8s.io/api/core/v1" @@ -90,29 +91,41 @@ func TestLUKSPrerequisiteHoldsOnEveryLayerListWriter(t *testing.T) { } } -// The Secret is the primary mechanism, but a cluster provisioned before it -// existed carries the passphrase in the legacy controller prop and the -// satellite still unlocks with it. REST accepts both sources; a CLI that -// reads only the Secret refuses on such a cluster what the REST door on the -// same cluster accepts. -func TestLUKSPrerequisiteAcceptsTheLegacyControllerProp(t *testing.T) { +// The passphrase lives only in the Secret `encryption create-passphrase` +// writes; nothing reads it from a property. A controller prop that carries +// one (put there with kubectl, since REST refuses to store it) must not +// satisfy the prerequisite, and the refusal must point at the Secret flow, +// not at the property. +func TestLUKSPrerequisiteIgnoresTheControllerProp(t *testing.T) { t.Parallel() app, backend, errBuf := newKubeAppWithStore(t) err := backend.ControllerProps().PatchProps(t.Context(), func(props map[string]string) error { - props[passphrase.PropKeyCanonical] = "from-a-pre-Secret-cluster" + props[passphrase.PropKeyCanonical] = "plaintext-in-a-prop" return nil }) if err != nil { - t.Fatalf("seed the legacy prop: %v", err) + t.Fatalf("seed the controller prop: %v", err) } stack := apiv1.LayerKindDRBD + "," + apiv1.LayerKindLUKS + "," + apiv1.LayerKindStorage - if got := app.Run(t.Context(), []string{"rd", "c", "pvc-x", "--layer-list", stack}); got != 0 { - t.Errorf("exit = %d: the cluster has a passphrase in the legacy prop, which REST "+ - "accepts (stderr: %s)", got, errBuf.String()) + if got := app.Run(t.Context(), []string{"rd", "c", "pvc-x", "--layer-list", stack}); got == 0 { + t.Fatalf("exit 0: a passphrase in a controller prop satisfied the LUKS prerequisite "+ + "(stderr: %s)", errBuf.String()) + } + + if !strings.Contains(errBuf.String(), "encryption create-passphrase") { + t.Errorf("the refusal does not point at `encryption create-passphrase`: %s", errBuf.String()) + } + + if strings.Contains(errBuf.String(), passphrase.PropKeyCanonical) { + t.Errorf("the refusal names the property, which is never read: %s", errBuf.String()) + } + + if _, err := backend.ResourceDefinitions().Get(t.Context(), "pvc-x"); err == nil { + t.Error("the refused create wrote the definition") } } diff --git a/pkg/dispatcher/dispatcher_luks_master_passphrase_test.go b/pkg/dispatcher/dispatcher_luks_master_passphrase_test.go index b8474cf5..5b8d617f 100644 --- a/pkg/dispatcher/dispatcher_luks_master_passphrase_test.go +++ b/pkg/dispatcher/dispatcher_luks_master_passphrase_test.go @@ -25,37 +25,21 @@ import ( blockstoriov1alpha1 "github.com/cozystack/blockstor/api/v1alpha1" "github.com/cozystack/blockstor/pkg/dispatcher" + "github.com/cozystack/blockstor/pkg/passphrase" + intent "github.com/cozystack/blockstor/pkg/satellite/intent" ) // Bug 265 (HIGH): the LUKS passphrase key namespace mismatched -// between the controller and the dispatcher. +// between the controller and the dispatcher: the dispatcher looked for +// `DrbdOptions/Encryption/passphrase` while the master key arrived +// under upstream LINSTOR's `DrbdOptions/EncryptPassphrase`, so it +// never reached the satellite and every LUKS RD looped on +// `Props.LuksPassphrase empty`. // -// The operator sets the cluster-scope master passphrase via -// `linstor controller set-property DrbdOptions/EncryptPassphrase -// ` — same shape upstream LINSTOR uses (Bug 95 enforces -// its presence as a hard prerequisite for any LUKS-layered RD). -// The REST/effectiveprops layer surfaces it under that EXACT key: -// `DrbdOptions/EncryptPassphrase`. -// -// The dispatcher, however, only looked for -// `DrbdOptions/Encryption/passphrase` (note the inner namespace + -// lower-case `passphrase`) and lifted just that key onto the -// satellite-facing `LuksPassphrase` wire prop. Result: the cluster -// master key set by the operator NEVER reached the satellite, the -// satellite reconciler tripped on `LUKS in layer stack but -// Props.LuksPassphrase empty`, and every LUKS RD looped forever in -// "Props.LuksPassphrase empty" failures. -// -// Fix: dispatcher must read the SAME key the controller writes — -// `DrbdOptions/EncryptPassphrase`. The legacy -// `DrbdOptions/Encryption/passphrase` shape stays accepted for -// backwards-compat (alias-read) but the canonical key is the upstream -// one. -// -// This test pins the contract: cluster-scope `DrbdOptions/ -// EncryptPassphrase` in effectiveProps MUST propagate to the wire -// as `LuksPassphrase`. Without the fix the dispatcher drops the -// master key on the floor. +// Today the satellite puts the passphrase Secret's value into +// effectiveProps under `DrbdOptions/EncryptPassphrase`, after dropping +// any property that carried a passphrase (pkg/passphrase). This test +// pins that the injected value reaches the wire as `LuksPassphrase`. func TestBug265LUKSMasterPassphraseReachesSatellite(t *testing.T) { rd := &blockstoriov1alpha1.ResourceDefinition{ ObjectMeta: metav1.ObjectMeta{Name: "pvc-bug265-luks"}, @@ -75,12 +59,8 @@ func TestBug265LUKSMasterPassphraseReachesSatellite(t *testing.T) { }, } - // Effective props as the resolver would produce after merging the - // ControllerConfig.Spec.ExtraProps where the operator stamped - // `DrbdOptions/EncryptPassphrase` via `linstor controller set- - // property`. This is the EXACT shape pkg/effectiveprops.Resolve - // returns (the resolver copies ControllerConfig.ExtraProps onto - // the output via maps.Copy). + // Effective props as the satellite hands them over once it has + // injected the Secret's value (luks_passphrase.go). effectiveProps := map[string]string{ "DrbdOptions/EncryptPassphrase": "topsecret", } @@ -92,11 +72,10 @@ func TestBug265LUKSMasterPassphraseReachesSatellite(t *testing.T) { pass, ok := got.Props["LuksPassphrase"] if !ok || pass == "" { - t.Errorf("LuksPassphrase not propagated to wire after operator set "+ + t.Errorf("LuksPassphrase not propagated to wire from the injected "+ "DrbdOptions/EncryptPassphrase; got props=%v. "+ - "The dispatcher must alias-read the cluster-scope master key "+ - "under upstream-LINSTOR's canonical name, not just the inner "+ - "DrbdOptions/Encryption/passphrase namespace. (Bug 265)", + "The dispatcher must lift the injected master key from "+ + "upstream-LINSTOR's canonical name. (Bug 265)", got.Props) } @@ -105,14 +84,11 @@ func TestBug265LUKSMasterPassphraseReachesSatellite(t *testing.T) { } } -// TestBug265LUKSLegacyEncryptionPassphraseStillWorks pins the -// backwards-compat half: any caller that still emits the legacy -// `DrbdOptions/Encryption/passphrase` shape (the pre-fix dispatcher -// surface) MUST keep working, so a half-upgraded cluster where one -// of the prop tiers writes the legacy form doesn't break. -func TestBug265LUKSLegacyEncryptionPassphraseStillWorks(t *testing.T) { - rd := &blockstoriov1alpha1.ResourceDefinition{ - ObjectMeta: metav1.ObjectMeta{Name: "pvc-bug265-legacy"}, +// luksTestRD and luksTestTarget are a LUKS-layered definition and one +// diskful replica of it, for the passphrase tests below. +func luksTestRD(name string) *blockstoriov1alpha1.ResourceDefinition { + return &blockstoriov1alpha1.ResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{Name: name}, Spec: blockstoriov1alpha1.ResourceDefinitionSpec{ LayerStack: []string{"DRBD", "LUKS", "STORAGE"}, VolumeDefinitions: []blockstoriov1alpha1.ResourceDefinitionVolume{ @@ -120,17 +96,47 @@ func TestBug265LUKSLegacyEncryptionPassphraseStillWorks(t *testing.T) { }, }, } +} - target := &blockstoriov1alpha1.Resource{ +func luksTestTarget(name string, props map[string]string) *blockstoriov1alpha1.Resource { + return &blockstoriov1alpha1.Resource{ Spec: blockstoriov1alpha1.ResourceSpec{ - ResourceDefinitionName: "pvc-bug265-legacy", + ResourceDefinitionName: name, NodeName: "n1", StoragePool: "data-hdd", + Props: props, }, } +} + +// assertNoPassphraseOnWire fails when any key that would carry the +// master passphrase made it into the wire props or the DRBD options. +func assertNoPassphraseOnWire(t *testing.T, got *intent.DesiredResource) { + t.Helper() + + for key, value := range got.Props { + if passphrase.IsPlaintextPropKey(key) { + t.Errorf("wire props carry %s=%q; only the Secret may reach the LUKS layer", key, value) + } + } + + for key, value := range got.DrbdOptions { + if passphrase.IsPlaintextPropKey(key) { + t.Errorf("DRBD options carry %s=%q", key, value) + } + } +} + +// The early per-RD alias `DrbdOptions/Encryption/passphrase` is a +// property, kept in plaintext on its CR, and is never read: when it is +// all there is, no passphrase reaches the satellite and the LUKS layer +// fails the apply with its actionable message. +func TestBug265LUKSLegacyEncryptionPassphraseIsIgnored(t *testing.T) { + rd := luksTestRD("pvc-bug265-legacy") + target := luksTestTarget("pvc-bug265-legacy", nil) effectiveProps := map[string]string{ - "DrbdOptions/Encryption/passphrase": "legacy-shape", + passphrase.PropKeyLegacy: "legacy-shape", } got := dispatcher.BuildDesired(target, nil, nil, nil, rd, effectiveProps) @@ -138,10 +144,5 @@ func TestBug265LUKSLegacyEncryptionPassphraseStillWorks(t *testing.T) { t.Fatalf("BuildDesired returned nil") } - pass, ok := got.Props["LuksPassphrase"] - if !ok || pass != "legacy-shape" { - t.Errorf("legacy DrbdOptions/Encryption/passphrase no longer propagates; "+ - "got props=%v. Backwards-compat alias must keep working.", - got.Props) - } + assertNoPassphraseOnWire(t, got) } diff --git a/pkg/rest/helpers_test.go b/pkg/rest/helpers_test.go index c1033676..0fece4b9 100644 --- a/pkg/rest/helpers_test.go +++ b/pkg/rest/helpers_test.go @@ -20,6 +20,7 @@ package rest import ( "context" + "strings" "testing" corev1 "k8s.io/api/core/v1" @@ -61,6 +62,20 @@ func newFakeRESTClient(t *testing.T) client.Client { return fake.NewClientBuilder().WithScheme(scheme).Build() } +// testPassphraseSecret is the Secret `linstor encryption create-passphrase` +// writes, holding value, in the namespace the REST-test helpers pin the +// server to. It is the only place the LUKS master passphrase is read from; +// tests that need the cluster master key seed this, never a property. +func testPassphraseSecret(value string) *corev1.Secret { + return &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: testRESTNamespace, + Name: passphrase.DefaultSecretName, + }, + Data: map[string][]byte{passphrase.SecretKey: []byte(value)}, + } +} + // startServerWithPassphrase is startServerWithStore with the cluster master // key already set, which is the prerequisite for creating anything carrying a // LUKS layer. Without it the handlers answer 400 — correctly, since a LUKS @@ -70,15 +85,7 @@ func startServerWithPassphrase(t *testing.T, st store.Store) (string, func()) { client := newFakeRESTClient(t) - secret := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: testRESTNamespace, - Name: passphrase.DefaultSecretName, - }, - Data: map[string][]byte{passphrase.SecretKey: []byte("test-cluster-key")}, - } - - if err := client.Create(context.Background(), secret); err != nil { + if err := client.Create(context.Background(), testPassphraseSecret("test-cluster-key")); err != nil { t.Fatalf("seed the passphrase Secret: %v", err) } @@ -89,3 +96,20 @@ func startServerWithPassphrase(t *testing.T, st store.Store) (string, func()) { Namespace: testRESTNamespace, }) } + +// assertLUKSRefusalHint checks the body of a LUKS prerequisite refusal: it +// must send the operator to `encryption create-passphrase` and must not name +// a passphrase property, which blockstor never reads. +func assertLUKSRefusalHint(t *testing.T, body []byte) { + t.Helper() + + if !strings.Contains(string(body), "encryption create-passphrase") { + t.Errorf("refusal must hint at `linstor encryption create-passphrase`; body: %s", body) + } + + for _, key := range []string{passphrase.PropKeyCanonical, passphrase.PropKeyLegacy, passphrase.WirePropKey} { + if strings.Contains(string(body), key) { + t.Errorf("refusal names the property %q, which is never read; body: %s", key, body) + } + } +} diff --git a/pkg/rest/luks_gate_bug023_test.go b/pkg/rest/luks_gate_bug023_test.go index 2b4569d7..b534b346 100644 --- a/pkg/rest/luks_gate_bug023_test.go +++ b/pkg/rest/luks_gate_bug023_test.go @@ -21,10 +21,8 @@ package rest import ( "encoding/json" "net/http" - "strings" "testing" - corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" @@ -44,9 +42,9 @@ import ( // // failed with 400, and the error hint told operators to put a // PLAINTEXT passphrase into a controller prop. These tests pin the -// fixed gate: the Secret (primary) unlocks LUKS RD creation, the -// legacy prop keeps working, and the no-passphrase refusal hints at -// `encryption create-passphrase` first. +// fixed gate: the Secret is the only thing that unlocks LUKS RD +// creation, a passphrase property never does, and the refusal sends +// the operator to `encryption create-passphrase` alone. // startLUKSGateServer boots a server over an in-memory store + fake // controller-runtime client seeded with objs. @@ -134,15 +132,7 @@ func TestBug023CreatePassphraseThenLUKSRDCreate(t *testing.T) { func TestBug023SecretSeededDirectlyUnlocksGate(t *testing.T) { t.Parallel() - sec := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: passphrase.DefaultSecretName, - Namespace: testRESTNamespace, - }, - Data: map[string][]byte{passphrase.SecretKey: []byte("gitops-passphrase")}, - } - - base, _, stop := startLUKSGateServer(t, sec) + base, _, stop := startLUKSGateServer(t, testPassphraseSecret("gitops-passphrase")) defer stop() resp := postLUKSRD(t, base, "wf-luks-gitops") @@ -155,11 +145,12 @@ func TestBug023SecretSeededDirectlyUnlocksGate(t *testing.T) { } } -// TestBug023LegacyControllerPropStillUnlocksGate: backward compat — -// clusters provisioned via `linstor controller set-property -// DrbdOptions/EncryptPassphrase ...` (plaintext on ControllerConfig. -// ExtraProps, deprecated) must keep creating LUKS RDs. -func TestBug023LegacyControllerPropStillUnlocksGate(t *testing.T) { +// TestBug023ControllerPropDoesNotUnlockGate: a passphrase that sits in +// a controller property (plaintext on ControllerConfig.ExtraProps, put +// there with kubectl since REST refuses to store it) is never read, so +// it does not satisfy the gate; the refusal names the Secret flow, not +// the property. +func TestBug023ControllerPropDoesNotUnlockGate(t *testing.T) { t.Parallel() cfg := &blockstoriov1alpha1.ControllerConfig{ @@ -177,11 +168,13 @@ func TestBug023LegacyControllerPropStillUnlocksGate(t *testing.T) { resp := postLUKSRD(t, base, "wf-luks-legacy") defer func() { _ = resp.Body.Close() }() - if resp.StatusCode != http.StatusCreated { - bodyBytes, _ := readAllBody(resp) - t.Fatalf("LUKS RD create with legacy prop: status got %d, want 201. Body: %s", + bodyBytes, _ := readAllBody(resp) + if resp.StatusCode != http.StatusBadRequest { + t.Fatalf("LUKS RD create with only a controller prop: status got %d, want 400. Body: %s", resp.StatusCode, bodyBytes) } + + assertLUKSRefusalHint(t, bodyBytes) } // TestBug023RefusalHintsAtCreatePassphrase: with NO passphrase @@ -203,9 +196,7 @@ func TestBug023RefusalHintsAtCreatePassphrase(t *testing.T) { } bodyBytes, _ := readAllBody(resp) - if !strings.Contains(string(bodyBytes), "encryption create-passphrase") { - t.Errorf("refusal must hint at `linstor encryption create-passphrase`; body: %s", bodyBytes) - } + assertLUKSRefusalHint(t, bodyBytes) // The RD must not be persisted on the refusal path. if _, err := st.ResourceDefinitions().Get(t.Context(), "wf-luks-refused"); err == nil { diff --git a/pkg/rest/rd_clone_validation_test.go b/pkg/rest/rd_clone_validation_test.go index 460d4bcb..5cd2d346 100644 --- a/pkg/rest/rd_clone_validation_test.go +++ b/pkg/rest/rd_clone_validation_test.go @@ -9,9 +9,6 @@ import ( "strings" "testing" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - - blockstoriov1alpha1 "github.com/cozystack/blockstor/api/v1alpha1" apiv1 "github.com/cozystack/blockstor/pkg/api/v1" "github.com/cozystack/blockstor/pkg/store" ) @@ -227,12 +224,7 @@ func TestRDCloneReplayOfAFinishedLUKSCloneDoesNotNeedThePassphrase(t *testing.T) body := map[string]any{"layer_list": []string{"DRBD", "LUKS", "STORAGE"}} withPass := newFakeRESTClient(t) - if err := withPass.Create(t.Context(), &blockstoriov1alpha1.ControllerConfig{ - ObjectMeta: metav1.ObjectMeta{Name: blockstoriov1alpha1.ControllerConfigName}, - Spec: blockstoriov1alpha1.ControllerConfigSpec{ - ExtraProps: map[string]string{"DrbdOptions/EncryptPassphrase": "blockstorpass"}, - }, - }); err != nil { + if err := withPass.Create(t.Context(), testPassphraseSecret("blockstorpass")); err != nil { t.Fatalf("seed the passphrase: %v", err) } diff --git a/pkg/rest/resource_definitions_bug116_test.go b/pkg/rest/resource_definitions_bug116_test.go index 7425f59e..a900cfb6 100644 --- a/pkg/rest/resource_definitions_bug116_test.go +++ b/pkg/rest/resource_definitions_bug116_test.go @@ -23,9 +23,6 @@ import ( "strings" "testing" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - - blockstoriov1alpha1 "github.com/cozystack/blockstor/api/v1alpha1" "github.com/cozystack/blockstor/pkg/store" ) @@ -52,8 +49,7 @@ func TestBug116RawLayerStackLUKSRefusedWithoutPassphrase(t *testing.T) { // Raw POST body: layer_stack carried inside resource_definition // (matches the OpenAPI ResourceDefinition.layer_stack field - // blockstor's apiv1 surface accepts). NO passphrase on the - // controller scope. + // blockstor's apiv1 surface accepts). NO passphrase Secret. body := []byte(`{"resource_definition":{"name":"bug116-stack",` + `"layer_stack":["DRBD","LUKS","STORAGE"]}}`) @@ -67,9 +63,7 @@ func TestBug116RawLayerStackLUKSRefusedWithoutPassphrase(t *testing.T) { } bodyBytes, _ := readAllBody(resp) - if !strings.Contains(string(bodyBytes), "DrbdOptions/EncryptPassphrase") { - t.Errorf("response body missing prop name: %s", bodyBytes) - } + assertLUKSRefusalHint(t, bodyBytes) _, err := st.ResourceDefinitions().Get(t.Context(), "bug116-stack") if err == nil { @@ -115,9 +109,7 @@ func TestBug116RawTopLevelLayerListLUKSRefusedWithoutPassphrase(t *testing.T) { } bodyBytes, _ := readAllBody(resp) - if !strings.Contains(string(bodyBytes), "DrbdOptions/EncryptPassphrase") { - t.Errorf("response body missing prop name: %s", bodyBytes) - } + assertLUKSRefusalHint(t, bodyBytes) _, err := st.ResourceDefinitions().Get(t.Context(), "bug116-toplevel") if err == nil { @@ -138,15 +130,8 @@ func TestBug116RawTopLevelLayerListPersistsOnCRD(t *testing.T) { cli := newFakeRESTClient(t) ctx := t.Context() - if err := cli.Create(ctx, &blockstoriov1alpha1.ControllerConfig{ - ObjectMeta: metav1.ObjectMeta{Name: blockstoriov1alpha1.ControllerConfigName}, - Spec: blockstoriov1alpha1.ControllerConfigSpec{ - ExtraProps: map[string]string{ - "DrbdOptions/EncryptPassphrase": "bug116pass", - }, - }, - }); err != nil { - t.Fatalf("seed ControllerConfig: %v", err) + if err := cli.Create(ctx, testPassphraseSecret("bug116pass")); err != nil { + t.Fatalf("seed the passphrase Secret: %v", err) } base, stop := startServerCustom(t, &Server{ @@ -226,8 +211,7 @@ func TestBug116RawLayerDataLUKSRefusedWithoutPassphrase(t *testing.T) { } // TestBug116RawLayerStackLUKSAcceptedWithPassphrase is the happy- -// path counterpart: with `DrbdOptions/EncryptPassphrase` set on -// the controller scope, the raw `layer_stack` shape must land 201 +// path counterpart: with the passphrase Secret seeded, the raw `layer_stack` shape must land 201 // + LUKS in the persisted CRD spec. func TestBug116RawLayerStackLUKSAcceptedWithPassphrase(t *testing.T) { t.Parallel() @@ -236,15 +220,8 @@ func TestBug116RawLayerStackLUKSAcceptedWithPassphrase(t *testing.T) { cli := newFakeRESTClient(t) ctx := t.Context() - if err := cli.Create(ctx, &blockstoriov1alpha1.ControllerConfig{ - ObjectMeta: metav1.ObjectMeta{Name: blockstoriov1alpha1.ControllerConfigName}, - Spec: blockstoriov1alpha1.ControllerConfigSpec{ - ExtraProps: map[string]string{ - "DrbdOptions/EncryptPassphrase": "bug116pass", - }, - }, - }); err != nil { - t.Fatalf("seed ControllerConfig: %v", err) + if err := cli.Create(ctx, testPassphraseSecret("bug116pass")); err != nil { + t.Fatalf("seed the passphrase Secret: %v", err) } base, stop := startServerCustom(t, &Server{ @@ -299,15 +276,8 @@ func TestBug116BothFieldsConsistent(t *testing.T) { cli := newFakeRESTClient(t) ctx := t.Context() - if err := cli.Create(ctx, &blockstoriov1alpha1.ControllerConfig{ - ObjectMeta: metav1.ObjectMeta{Name: blockstoriov1alpha1.ControllerConfigName}, - Spec: blockstoriov1alpha1.ControllerConfigSpec{ - ExtraProps: map[string]string{ - "DrbdOptions/EncryptPassphrase": "bug116pass", - }, - }, - }); err != nil { - t.Fatalf("seed ControllerConfig: %v", err) + if err := cli.Create(ctx, testPassphraseSecret("bug116pass")); err != nil { + t.Fatalf("seed the passphrase Secret: %v", err) } base, stop := startServerCustom(t, &Server{ @@ -358,15 +328,8 @@ func TestBug116BothFieldsAgreeing(t *testing.T) { cli := newFakeRESTClient(t) ctx := t.Context() - if err := cli.Create(ctx, &blockstoriov1alpha1.ControllerConfig{ - ObjectMeta: metav1.ObjectMeta{Name: blockstoriov1alpha1.ControllerConfigName}, - Spec: blockstoriov1alpha1.ControllerConfigSpec{ - ExtraProps: map[string]string{ - "DrbdOptions/EncryptPassphrase": "bug116pass", - }, - }, - }); err != nil { - t.Fatalf("seed ControllerConfig: %v", err) + if err := cli.Create(ctx, testPassphraseSecret("bug116pass")); err != nil { + t.Fatalf("seed the passphrase Secret: %v", err) } base, stop := startServerCustom(t, &Server{ diff --git a/pkg/rest/resource_definitions_bug95_test.go b/pkg/rest/resource_definitions_bug95_test.go index 90b20eb8..16088606 100644 --- a/pkg/rest/resource_definitions_bug95_test.go +++ b/pkg/rest/resource_definitions_bug95_test.go @@ -25,9 +25,6 @@ import ( "strings" "testing" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - - blockstoriov1alpha1 "github.com/cozystack/blockstor/api/v1alpha1" apiv1 "github.com/cozystack/blockstor/pkg/api/v1" "github.com/cozystack/blockstor/pkg/store" ) @@ -195,22 +192,19 @@ func describeLayerChain(layer *apiv1.ResourceLayer) string { // TestBug95LUKSCreateRefusedWithoutPassphrase is the write-side // guard: posting an RD-create body with LUKS in the layer stack -// MUST be rejected with 400 when the controller-scope props don't -// carry `DrbdOptions/EncryptPassphrase`. Without this gate the RD -// was accepted and the encryption silently dropped (Bug 95). +// MUST be rejected with 400 when the cluster has no passphrase +// Secret. Without this gate the RD was accepted and the encryption +// silently dropped (Bug 95). // -// The 400 envelope must name the missing prop so the operator -// knows exactly which `controller set-property` invocation to make -// before retrying the RD-create. +// The 400 envelope must send the operator to `encryption +// create-passphrase`, the only place the passphrase is read from. func TestBug95LUKSCreateRefusedWithoutPassphrase(t *testing.T) { t.Parallel() st := store.NewInMemory() cli := newFakeRESTClient(t) - // No ControllerConfig seed — `DrbdOptions/EncryptPassphrase` is - // absent. (The fake client treats a missing ControllerConfig - // the same way as `controllerScopeProps` does: empty map.) + // No passphrase Secret seeded. base, stop := startServerCustom(t, &Server{ Addr: pickFreeAddr(t), Store: st, @@ -238,9 +232,7 @@ func TestBug95LUKSCreateRefusedWithoutPassphrase(t *testing.T) { } bodyBytes, _ := readAllBody(resp) - if !strings.Contains(string(bodyBytes), "DrbdOptions/EncryptPassphrase") { - t.Errorf("response body missing prop name: %s", bodyBytes) - } + assertLUKSRefusalHint(t, bodyBytes) // Confirm the RD was NOT persisted — partial commit on a 400 is // the silent-failure mode this gate is preventing. @@ -251,8 +243,7 @@ func TestBug95LUKSCreateRefusedWithoutPassphrase(t *testing.T) { } // TestBug95LUKSCreateAcceptedWithPassphrase is the happy-path -// counterpart: once `DrbdOptions/EncryptPassphrase` is set on the -// controller scope, `rd c --layer-list DRBD,LUKS,STORAGE` is +// counterpart: once the passphrase Secret exists, `rd c --layer-list DRBD,LUKS,STORAGE` is // accepted (201) and the resulting RD persists the requested // LayerStack so the dispatcher's read of `rd.Spec.LayerStack` // (pkg/dispatcher/dispatcher.go) carries LUKS through to the @@ -264,15 +255,8 @@ func TestBug95LUKSCreateAcceptedWithPassphrase(t *testing.T) { cli := newFakeRESTClient(t) ctx := t.Context() - if err := cli.Create(ctx, &blockstoriov1alpha1.ControllerConfig{ - ObjectMeta: metav1.ObjectMeta{Name: blockstoriov1alpha1.ControllerConfigName}, - Spec: blockstoriov1alpha1.ControllerConfigSpec{ - ExtraProps: map[string]string{ - "DrbdOptions/EncryptPassphrase": "blockstorpass", - }, - }, - }); err != nil { - t.Fatalf("seed ControllerConfig: %v", err) + if err := cli.Create(ctx, testPassphraseSecret("blockstorpass")); err != nil { + t.Fatalf("seed the passphrase Secret: %v", err) } base, stop := startServerCustom(t, &Server{ @@ -363,9 +347,7 @@ func TestBug95LUKSRefusedWhenCLIUsesLayerData(t *testing.T) { } bodyBytes, _ := readAllBody(resp) - if !strings.Contains(string(bodyBytes), "DrbdOptions/EncryptPassphrase") { - t.Errorf("response body missing prop name: %s", bodyBytes) - } + assertLUKSRefusalHint(t, bodyBytes) _, err := st.ResourceDefinitions().Get(t.Context(), "luktest-cli") if err == nil { @@ -375,7 +357,7 @@ func TestBug95LUKSRefusedWhenCLIUsesLayerData(t *testing.T) { // TestBug95LUKSAcceptedWhenCLIUsesLayerData is the happy-path // counterpart of TestBug95LUKSRefusedWhenCLIUsesLayerData: with the -// controller-scope passphrase set, the CLI's `layer_data: [...]` +// passphrase Secret seeded, the CLI's `layer_data: [...]` // wire shape must round-trip cleanly to `rd.LayerStack` on the // persisted CRD so the dispatcher's `rd.Spec.LayerStack` read // surfaces the LUKS layer to the satellite. @@ -386,15 +368,8 @@ func TestBug95LUKSAcceptedWhenCLIUsesLayerData(t *testing.T) { cli := newFakeRESTClient(t) ctx := t.Context() - if err := cli.Create(ctx, &blockstoriov1alpha1.ControllerConfig{ - ObjectMeta: metav1.ObjectMeta{Name: blockstoriov1alpha1.ControllerConfigName}, - Spec: blockstoriov1alpha1.ControllerConfigSpec{ - ExtraProps: map[string]string{ - "DrbdOptions/EncryptPassphrase": "blockstorpass", - }, - }, - }); err != nil { - t.Fatalf("seed ControllerConfig: %v", err) + if err := cli.Create(ctx, testPassphraseSecret("blockstorpass")); err != nil { + t.Fatalf("seed the passphrase Secret: %v", err) } base, stop := startServerCustom(t, &Server{ diff --git a/pkg/satellite/controllers/luks_passphrase_internal_test.go b/pkg/satellite/controllers/luks_passphrase_internal_test.go index 6d11fe63..a2a130f7 100644 --- a/pkg/satellite/controllers/luks_passphrase_internal_test.go +++ b/pkg/satellite/controllers/luks_passphrase_internal_test.go @@ -41,8 +41,8 @@ import ( // Props.LuksPassphrase empty`. These tests pin // injectLUKSMasterPassphrase: the Secret value must enter the // effective-props bag under the canonical key (so BuildDesired's -// existing lift to the `LuksPassphrase` wire prop fires), with the -// legacy prop paths keeping precedence. +// existing lift to the `LuksPassphrase` wire prop fires), and no +// passphrase property may ever stand in for it. const testControllerNS = "blockstor-system" @@ -137,27 +137,28 @@ func TestBug023SecretPassphraseReachesWireProps(t *testing.T) { } } -// TestBug023LegacyPropKeepsPrecedence: an operator-set controller -// prop must win over the Secret so existing LUKS volumes keep -// unlocking with the key they were formatted with. -func TestBug023LegacyPropKeepsPrecedence(t *testing.T) { +// TestBug023SecretReplacesEveryPassphraseProp: a passphrase in a +// property, under any of its keys, never wins over the Secret. The +// property sits in plaintext on its CR; the Secret is the only source, +// so the injected canonical value is the Secret's and every property +// that carried one is gone. +func TestBug023SecretReplacesEveryPassphraseProp(t *testing.T) { t.Parallel() r := newPassphraseTestReconciler(t, passphraseSecret("secret-value")) - for _, key := range []string{passphrase.PropKeyCanonical, passphrase.PropKeyLegacy} { + for _, key := range []string{passphrase.PropKeyCanonical, passphrase.PropKeyLegacy, passphrase.WirePropKey} { effectiveProps := map[string]string{key: "prop-value"} r.injectLUKSMasterPassphrase(t.Context(), luksRD(), effectiveProps) - if got := effectiveProps[key]; got != "prop-value" { - t.Errorf("legacy prop %q overwritten: got %q, want prop-value", key, got) + if got := effectiveProps[passphrase.PropKeyCanonical]; got != "secret-value" { + t.Errorf("with prop %q set: canonical key got %q, want the Secret value", key, got) } - if key == passphrase.PropKeyLegacy { - if _, added := effectiveProps[passphrase.PropKeyCanonical]; added { - t.Errorf("canonical key injected despite legacy prop present — " + - "pickLUKSPassphrase would switch keys under existing volumes") + for k, v := range effectiveProps { + if v == "prop-value" { + t.Errorf("with prop %q set: %q still carries the property's value", key, k) } } } From 850a57a0b7a2b863d16d3e550620ff2846738062 Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Mon, 26 Oct 2026 09:14:54 +0100 Subject: [PATCH 03/24] test(luks): cover every guard that keeps the passphrase out of props The REST guard is pinned on the controller set-property, a definition create with the early alias, a definition modify with the wire key and a case variant: each answers 400 and stores nothing. A delete list naming the key still passes, and a body that is not JSON reaches the handler unchanged. The satellite is pinned to yield no passphrase from a property alone, and the dispatcher to never put a replica's wire key on the wire or let it override the Secret's value. Assisted-by: LLM Signed-off-by: Andrei Kvapil --- .../dispatcher_luks_wire_prop_test.go | 63 ++++ pkg/rest/passphrase_props_guard_test.go | 268 ++++++++++++++++++ .../luks_passphrase_internal_test.go | 46 +++ 3 files changed, 377 insertions(+) create mode 100644 pkg/dispatcher/dispatcher_luks_wire_prop_test.go create mode 100644 pkg/rest/passphrase_props_guard_test.go diff --git a/pkg/dispatcher/dispatcher_luks_wire_prop_test.go b/pkg/dispatcher/dispatcher_luks_wire_prop_test.go new file mode 100644 index 00000000..03cf6ff2 --- /dev/null +++ b/pkg/dispatcher/dispatcher_luks_wire_prop_test.go @@ -0,0 +1,63 @@ +// 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 dispatcher_test + +import ( + "testing" + + "github.com/cozystack/blockstor/pkg/dispatcher" + "github.com/cozystack/blockstor/pkg/passphrase" +) + +// A `LuksPassphrase` property set on the replica is the satellite's +// wire key itself; it must never reach the wire, whether it arrives +// through the resolved props or straight from the replica (no resolved +// props at all), and it must not override the Secret's value. +func TestLUKSReplicaWirePropNeverReachesTheWire(t *testing.T) { + const name = "pvc-luks-replica-prop" + + replicaProps := map[string]string{passphrase.WirePropKey: "from-a-replica-prop"} + + t.Run("resolved props", func(t *testing.T) { + got := dispatcher.BuildDesired(luksTestTarget(name, replicaProps), nil, nil, nil, + luksTestRD(name), map[string]string{passphrase.WirePropKey: "from-a-replica-prop"}) + + assertNoPassphraseOnWire(t, got) + }) + + t.Run("no resolved props", func(t *testing.T) { + got := dispatcher.BuildDesired(luksTestTarget(name, map[string]string{ + passphrase.WirePropKey: "from-a-replica-prop", + }), nil, nil, nil, luksTestRD(name), nil) + + assertNoPassphraseOnWire(t, got) + }) + + t.Run("the Secret wins", func(t *testing.T) { + got := dispatcher.BuildDesired(luksTestTarget(name, replicaProps), nil, nil, nil, + luksTestRD(name), map[string]string{ + passphrase.WirePropKey: "from-a-replica-prop", + passphrase.PropKeyCanonical: "from-the-secret", + }) + + if pass := got.Props[passphrase.WirePropKey]; pass != "from-the-secret" { + t.Errorf("LuksPassphrase = %q, want the Secret's value", pass) + } + }) +} diff --git a/pkg/rest/passphrase_props_guard_test.go b/pkg/rest/passphrase_props_guard_test.go new file mode 100644 index 00000000..a2d71fe5 --- /dev/null +++ b/pkg/rest/passphrase_props_guard_test.go @@ -0,0 +1,268 @@ +// 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" + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + + blockstoriov1alpha1 "github.com/cozystack/blockstor/api/v1alpha1" + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/passphrase" + "github.com/cozystack/blockstor/pkg/store" +) + +// guardCanary is the passphrase every refused write in this file tries to +// store; finding it anywhere afterwards means the guard leaked. +const guardCanary = "plaintext-passphrase-canary" + +// startGuardServer boots a server over an in-memory store with one plain +// definition and a fake client, and returns both so a test can check that a +// refused write stored nothing. +func startGuardServer(t *testing.T, objs ...client.Object) (string, store.Store, client.Client, func()) { + t.Helper() + + st := store.NewInMemory() + if err := st.ResourceDefinitions().Create(t.Context(), &apiv1.ResourceDefinition{ + Name: "rd-guard", + Props: map[string]string{"Aux/keep": "yes"}, + }); err != nil { + t.Fatalf("seed RD: %v", err) + } + + cli := newFakeRESTClient(t) + for _, obj := range objs { + if err := cli.Create(t.Context(), obj); err != nil { + t.Fatalf("seed %T: %v", obj, err) + } + } + + base, stop := startServerCustom(t, &Server{ + Addr: pickFreeAddr(t), + Store: st, + Client: cli, + Namespace: testRESTNamespace, + }) + + return base, st, cli, stop +} + +// controllerExtraProps reads the controller properties the way they sit on +// the CR, empty when the ControllerConfig does not exist. +func controllerExtraProps(t *testing.T, cli client.Client) map[string]string { + t.Helper() + + var cfg blockstoriov1alpha1.ControllerConfig + + err := cli.Get(t.Context(), client.ObjectKey{Name: blockstoriov1alpha1.ControllerConfigName}, &cfg) + if err != nil { + return map[string]string{} + } + + return cfg.Spec.ExtraProps +} + +// Every door that would store the master passphrase in a property answers +// 400 and stores nothing: the controller set-property, a definition create +// carrying the early per-RD alias, a definition modify carrying the wire key, +// and a spelling that differs only in case. +func TestPlaintextPassphrasePropIsRefusedOnEveryDoor(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name, method, path, body string + stored func(t *testing.T, st store.Store, cli client.Client) bool + }{ + { + name: "controller set-property", method: http.MethodPost, path: "/v1/controller/properties", + body: `{"override_props":{"DrbdOptions/EncryptPassphrase":"` + guardCanary + `"}}`, + stored: func(t *testing.T, _ store.Store, cli client.Client) bool { + t.Helper() + + return controllerExtraProps(t, cli)[passphrase.PropKeyCanonical] != "" + }, + }, + { + name: "rd create with the alias", method: http.MethodPost, path: "/v1/resource-definitions", + body: `{"resource_definition":{"name":"rd-alias","props":{"DrbdOptions/Encryption/passphrase":"` + + guardCanary + `"}}}`, + stored: func(t *testing.T, st store.Store, _ client.Client) bool { + t.Helper() + + _, err := st.ResourceDefinitions().Get(t.Context(), "rd-alias") + + return err == nil + }, + }, + { + name: "rd modify with the wire key", method: http.MethodPut, path: "/v1/resource-definitions/rd-guard", + body: `{"override_props":{"LuksPassphrase":"` + guardCanary + `"}}`, + stored: func(t *testing.T, st store.Store, _ client.Client) bool { + t.Helper() + + rd, err := st.ResourceDefinitions().Get(t.Context(), "rd-guard") + if err != nil { + t.Fatalf("get RD: %v", err) + } + + return rd.Props[passphrase.WirePropKey] != "" + }, + }, + { + name: "case variant", method: http.MethodPost, path: "/v1/controller/properties", + body: `{"override_props":{"drbdoptions/ENCRYPTPASSPHRASE":"` + guardCanary + `"}}`, + stored: func(t *testing.T, _ store.Store, cli client.Client) bool { + t.Helper() + + for key := range controllerExtraProps(t, cli) { + if passphrase.IsPlaintextPropKey(key) { + return true + } + } + + return false + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + base, st, cli, stop := startGuardServer(t) + defer stop() + + resp := sendGuardRequest(t, tc.method, base+tc.path, tc.body) + body, _ := readAllBody(resp) + _ = resp.Body.Close() + + if resp.StatusCode != http.StatusBadRequest { + t.Fatalf("status = %d, want 400; body: %s", resp.StatusCode, body) + } + + if !strings.Contains(string(body), "encryption create-passphrase") { + t.Errorf("the refusal does not point at `encryption create-passphrase`: %s", body) + } + + if strings.Contains(string(body), guardCanary) { + t.Errorf("the refusal echoes the passphrase: %s", body) + } + + if tc.stored(t, st, cli) { + t.Error("the refused write stored the passphrase property") + } + }) + } +} + +// Deleting a passphrase property stays allowed: the key named in a delete +// list is a string, not an object key, and removing one put there with +// kubectl is exactly what an operator moving to the Secret has to do. +func TestPlaintextPassphrasePropDeleteIsAllowed(t *testing.T) { + t.Parallel() + + base, _, cli, stop := startGuardServer(t, &blockstoriov1alpha1.ControllerConfig{ + ObjectMeta: metav1.ObjectMeta{Name: blockstoriov1alpha1.ControllerConfigName}, + Spec: blockstoriov1alpha1.ControllerConfigSpec{ + ExtraProps: map[string]string{ + passphrase.PropKeyCanonical: guardCanary, + "Aux/keep": "yes", + }, + }, + }) + defer stop() + + resp := sendGuardRequest(t, http.MethodPost, base+"/v1/controller/properties", + `{"delete_props":["DrbdOptions/EncryptPassphrase"]}`) + body, _ := readAllBody(resp) + _ = resp.Body.Close() + + if resp.StatusCode != http.StatusCreated { + t.Fatalf("delete_props naming the passphrase key: status = %d, want 201; body: %s", + resp.StatusCode, body) + } + + props := controllerExtraProps(t, cli) + if _, still := props[passphrase.PropKeyCanonical]; still { + t.Errorf("the property survived its delete: %v", props) + } + + if props["Aux/keep"] != "yes" { + t.Errorf("the delete took a sibling with it: %v", props) + } +} + +// A body that is not JSON carries no property to refuse; it reaches the +// handler byte for byte, so the handler answers it exactly as before. +func TestPlaintextPassphraseGuardPassesANonJSONBody(t *testing.T) { + t.Parallel() + + const raw = `not json, though it names "DrbdOptions/EncryptPassphrase"` + + var ( + reached bool + got []byte + ) + + guarded := withPlaintextPassphraseRefusal(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + reached = true + got, _ = io.ReadAll(r.Body) + + w.WriteHeader(http.StatusTeapot) + })) + + rec := httptest.NewRecorder() + guarded.ServeHTTP(rec, httptest.NewRequestWithContext(t.Context(), http.MethodPost, + "/v1/controller/properties", strings.NewReader(raw))) + + if !reached { + t.Fatalf("a non-JSON body did not reach the handler; status %d, body: %s", rec.Code, rec.Body) + } + + if string(got) != raw { + t.Errorf("the handler read %q, want the body unchanged %q", got, raw) + } + + if rec.Code != http.StatusTeapot { + t.Errorf("status = %d, want the handler's own 418", rec.Code) + } +} + +func sendGuardRequest(t *testing.T, method, url, body string) *http.Response { + t.Helper() + + req, err := http.NewRequestWithContext(t.Context(), method, url, bytes.NewBufferString(body)) + if err != nil { + t.Fatalf("build request: %v", err) + } + + req.Header.Set("Content-Type", "application/json") + + resp, err := http.DefaultClient.Do(req) + if err != nil { + t.Fatalf("%s %s: %v", method, url, err) + } + + return resp +} diff --git a/pkg/satellite/controllers/luks_passphrase_internal_test.go b/pkg/satellite/controllers/luks_passphrase_internal_test.go index a2a130f7..5349df22 100644 --- a/pkg/satellite/controllers/luks_passphrase_internal_test.go +++ b/pkg/satellite/controllers/luks_passphrase_internal_test.go @@ -164,6 +164,52 @@ func TestBug023SecretReplacesEveryPassphraseProp(t *testing.T) { } } +// TestBug023PropAloneYieldsNoPassphrase: with no Secret, a passphrase +// in a property, under any key or spelling, does not become one. It +// is dropped from the bag, so BuildDesired stamps no wire key and the +// LUKS layer fails the apply with its actionable missing-key message +// instead of formatting or opening a volume with a plaintext key. A +// definition without LUKS gets the property dropped too, so the value +// never rides along in the wire props. +func TestBug023PropAloneYieldsNoPassphrase(t *testing.T) { + t.Parallel() + + r := newPassphraseTestReconciler(t) + + plainRD := luksRD() + plainRD.Spec.LayerStack = []string{"DRBD", "STORAGE"} + + for _, rd := range []*blockstoriov1alpha1.ResourceDefinition{luksRD(), plainRD} { + for _, key := range []string{ + passphrase.PropKeyCanonical, passphrase.PropKeyLegacy, passphrase.WirePropKey, + "drbdoptions/ENCRYPTPASSPHRASE", + } { + effectiveProps := map[string]string{key: "prop-value", "Aux/keep": "yes"} + + r.injectLUKSMasterPassphrase(t.Context(), rd, effectiveProps) + + if len(effectiveProps) != 1 || effectiveProps["Aux/keep"] != "yes" { + t.Errorf("stack %v, prop %q: props after injection = %v, want only Aux/keep", + rd.Spec.LayerStack, key, effectiveProps) + } + + target := &blockstoriov1alpha1.Resource{ + Spec: blockstoriov1alpha1.ResourceSpec{ + ResourceDefinitionName: rd.Name, + NodeName: "node-a", + StoragePool: "pool-a", + }, + } + + desired := dispatcher.BuildDesired(target, nil, nil, nil, rd, effectiveProps) + if pass, ok := desired.Props[passphrase.WirePropKey]; ok { + t.Errorf("stack %v, prop %q: wire key stamped with %q from a property", + rd.Spec.LayerStack, key, pass) + } + } + } +} + // TestBug023InjectionSkipsNonLUKS: non-LUKS RDs must not trigger a // Secret round-trip nor carry the passphrase. func TestBug023InjectionSkipsNonLUKS(t *testing.T) { From 39a55074f5de79bd7d1561b31689668b18d1428e Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Mon, 26 Oct 2026 09:14:54 +0100 Subject: [PATCH 04/24] fix(cli): refuse to store the LUKS passphrase through set-property The CLI writes the CRs itself, so the REST guard never sees its set-property. It stored the master passphrase in a plaintext property that blockstor no longer reads, leaving the operator with a key in every CR dump and a LUKS stack that still refuses to come up. set-property now refuses any of the passphrase keys, in any case, on every noun, and points at encryption create-passphrase. Setting one to an empty value still deletes it. Assisted-by: LLM Signed-off-by: Andrei Kvapil --- internal/cli/passphrase_props_test.go | 92 +++++++++++++++++++++++++++ internal/cli/write.go | 12 ++++ 2 files changed, 104 insertions(+) create mode 100644 internal/cli/passphrase_props_test.go diff --git a/internal/cli/passphrase_props_test.go b/internal/cli/passphrase_props_test.go new file mode 100644 index 00000000..74dd4062 --- /dev/null +++ b/internal/cli/passphrase_props_test.go @@ -0,0 +1,92 @@ +// SPDX-License-Identifier: Apache-2.0 + +package cli_test + +import ( + "strings" + "testing" + + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/passphrase" +) + +// The CLI writes the CRs itself, so it is a door REST's guard does not cover. +// It refuses the same write: the master passphrase never goes into a +// property, under any of its keys or spellings, on any noun. +func TestSetPropertyRefusesThePlaintextPassphrase(t *testing.T) { + t.Parallel() + + for name, argv := range map[string][]string{ + "controller canonical": {"c", "sp", passphrase.PropKeyCanonical, "prop-value"}, + "controller case": {"c", "sp", "drbdoptions/ENCRYPTPASSPHRASE", "prop-value"}, + "definition alias": {"rd", "sp", "pvc-x", passphrase.PropKeyLegacy, "prop-value"}, + "definition wire key": {"rd", "sp", "pvc-x", passphrase.WirePropKey, "prop-value"}, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + + app, backend, errBuf := newKubeAppWithStore(t) + + err := backend.ResourceDefinitions().Create(t.Context(), &apiv1.ResourceDefinition{Name: "pvc-x"}) + if err != nil { + t.Fatalf("seed: %v", err) + } + + if got := app.Run(t.Context(), argv); got == 0 { + t.Fatalf("exit 0: %v stored the passphrase in a property", argv) + } + + if !strings.Contains(errBuf.String(), "encryption create-passphrase") { + t.Errorf("the refusal does not point at `encryption create-passphrase`: %s", errBuf.String()) + } + + ctrl, err := backend.ControllerProps().Get(t.Context()) + if err != nil { + t.Fatalf("read controller props: %v", err) + } + + rd, err := backend.ResourceDefinitions().Get(t.Context(), "pvc-x") + if err != nil { + t.Fatalf("read the definition: %v", err) + } + + for _, props := range []map[string]string{ctrl, rd.Props} { + for key := range props { + if passphrase.IsPlaintextPropKey(key) { + t.Errorf("the refused write stored %q", key) + } + } + } + }) + } +} + +// Deleting a passphrase property, put there with kubectl, stays allowed: +// that is how an operator clears one on the way to the Secret. +func TestSetPropertyDeletesThePlaintextPassphrase(t *testing.T) { + t.Parallel() + + app, backend, errBuf := newKubeAppWithStore(t) + + err := backend.ControllerProps().PatchProps(t.Context(), func(props map[string]string) error { + props[passphrase.PropKeyCanonical] = "prop-value" + + return nil + }) + if err != nil { + t.Fatalf("seed: %v", err) + } + + if got := app.Run(t.Context(), []string{"c", "sp", passphrase.PropKeyCanonical, ""}); got != 0 { + t.Fatalf("delete through an empty value: exit %d (stderr: %s)", got, errBuf.String()) + } + + ctrl, err := backend.ControllerProps().Get(t.Context()) + if err != nil { + t.Fatalf("read controller props: %v", err) + } + + if _, still := ctrl[passphrase.PropKeyCanonical]; still { + t.Errorf("the property survived its delete: %v", ctrl) + } +} diff --git a/internal/cli/write.go b/internal/cli/write.go index b5ce2077..bd7eccbe 100644 --- a/internal/cli/write.go +++ b/internal/cli/write.go @@ -30,6 +30,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/passphrase" "github.com/cozystack/blockstor/pkg/store" "github.com/cozystack/blockstor/internal/cli/command" @@ -92,6 +93,10 @@ func setProperty(accessor propertyAccessor) handler { value = run.Flags.Positionals[want] } + if value != "" && passphrase.IsPlaintextPropKey(key) { + return fmt.Errorf("%w: %s", errPlaintextPassphraseProp, key) + } + if accessor.namedKey != nil { named := accessor.namedKey(key, value == "") if named != nil { @@ -252,6 +257,13 @@ func patchDefinitionProps( // definition itself. var errServerOwnedProp = errors.New("blockstor sets this property itself") +// errPlaintextPassphraseProp refuses a set-property that would store the +// LUKS master passphrase in a property. A property is kept in plaintext on +// its CR and blockstor never reads the passphrase from one; REST refuses the +// same write. Deleting such a property stays allowed. +var errPlaintextPassphraseProp = errors.New("refusing to store the LUKS master passphrase in a property, " + + "which is kept in plaintext on its resource; run `encryption create-passphrase`, which keeps it in a Secret") + // refuseServerOwnedDefinitionProp is the named-key guard for rdProps. A // delete of the rollback mark goes through; see // store.ServerOwnedPropEditByOperator. From a05644c25afa2408985762b27cabad586a078605 Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Mon, 26 Oct 2026 09:16:47 +0100 Subject: [PATCH 05/24] fix(rest): let an empty override delete a passphrase property The guard refused any body that named a passphrase key as an object key, so `set-property ""`, the documented delete every props door honours, answered 400. That blocked clearing a passphrase property put there with kubectl through anything but an explicit delete list. An override to the empty string stores nothing and now passes. Assisted-by: LLM Signed-off-by: Andrei Kvapil --- pkg/rest/passphrase_props_guard.go | 6 ++-- pkg/rest/passphrase_props_guard_test.go | 46 ++++++++++++++++--------- 2 files changed, 33 insertions(+), 19 deletions(-) diff --git a/pkg/rest/passphrase_props_guard.go b/pkg/rest/passphrase_props_guard.go index 4f6f948d..2e93b52a 100644 --- a/pkg/rest/passphrase_props_guard.go +++ b/pkg/rest/passphrase_props_guard.go @@ -35,7 +35,9 @@ import ( // for a backup would carry the key to every LUKS volume; the passphrase // lives in the Secret `encryption create-passphrase` writes, and blockstor // reads it from nowhere else. Deleting such a property stays allowed: a key -// named in a delete list is a string, not an object key. +// named in a delete list is a string, not an object key, and an override +// that maps the key to an empty string is the documented set-property +// delete (applyPropsModify), which stores nothing. func withPlaintextPassphraseRefusal(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.Body == nil || !isWriteMethod(r.Method) { @@ -82,7 +84,7 @@ func findPlaintextPassphraseKey(node any) (string, bool) { switch typed := node.(type) { case map[string]any: for key, value := range typed { - if passphrase.IsPlaintextPropKey(key) { + if passphrase.IsPlaintextPropKey(key) && value != "" { return key, true } diff --git a/pkg/rest/passphrase_props_guard_test.go b/pkg/rest/passphrase_props_guard_test.go index a2d71fe5..2e9348d4 100644 --- a/pkg/rest/passphrase_props_guard_test.go +++ b/pkg/rest/passphrase_props_guard_test.go @@ -176,9 +176,10 @@ func TestPlaintextPassphrasePropIsRefusedOnEveryDoor(t *testing.T) { } } -// Deleting a passphrase property stays allowed: the key named in a delete -// list is a string, not an object key, and removing one put there with -// kubectl is exactly what an operator moving to the Secret has to do. +// Deleting a passphrase property stays allowed, through a delete list or +// through an override to the empty string (the set-property delete): +// removing one put there with kubectl is exactly what an operator moving to +// the Secret has to do. func TestPlaintextPassphrasePropDeleteIsAllowed(t *testing.T) { t.Parallel() @@ -187,29 +188,40 @@ func TestPlaintextPassphrasePropDeleteIsAllowed(t *testing.T) { Spec: blockstoriov1alpha1.ControllerConfigSpec{ ExtraProps: map[string]string{ passphrase.PropKeyCanonical: guardCanary, + passphrase.PropKeyLegacy: guardCanary, "Aux/keep": "yes", }, }, }) defer stop() - resp := sendGuardRequest(t, http.MethodPost, base+"/v1/controller/properties", - `{"delete_props":["DrbdOptions/EncryptPassphrase"]}`) - body, _ := readAllBody(resp) - _ = resp.Body.Close() + for _, tc := range []struct{ name, key, body string }{ + { + name: "delete list", key: passphrase.PropKeyCanonical, + body: `{"delete_props":["DrbdOptions/EncryptPassphrase"]}`, + }, + { + name: "empty override", key: passphrase.PropKeyLegacy, + body: `{"override_props":{"DrbdOptions/Encryption/passphrase":""}}`, + }, + } { + resp := sendGuardRequest(t, http.MethodPost, base+"/v1/controller/properties", tc.body) + body, _ := readAllBody(resp) + _ = resp.Body.Close() - if resp.StatusCode != http.StatusCreated { - t.Fatalf("delete_props naming the passphrase key: status = %d, want 201; body: %s", - resp.StatusCode, body) - } + if resp.StatusCode != http.StatusCreated { + t.Fatalf("%s naming the passphrase key: status = %d, want 201; body: %s", + tc.name, resp.StatusCode, body) + } - props := controllerExtraProps(t, cli) - if _, still := props[passphrase.PropKeyCanonical]; still { - t.Errorf("the property survived its delete: %v", props) - } + props := controllerExtraProps(t, cli) + if _, still := props[tc.key]; still { + t.Errorf("%s: the property survived its delete: %v", tc.name, props) + } - if props["Aux/keep"] != "yes" { - t.Errorf("the delete took a sibling with it: %v", props) + if props["Aux/keep"] != "yes" { + t.Errorf("%s: the delete took a sibling with it: %v", tc.name, props) + } } } From 3eaa926ca1d67daaef11f7b3b5181dfb80bef09c Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Mon, 26 Oct 2026 09:21:00 +0100 Subject: [PATCH 06/24] test(e2e): create the LUKS passphrase through the Secret only The stand scenarios set the passphrase through the controller property or put it in a definition's props, neither of which blockstor reads any more, and REST now refuses the first. The cli-matrix cells rely on encryption create-passphrase alone, and the kubectl-driven scenarios create the passphrase Secret through a new lib helper that leaves an existing one alone. The Bug 023 cell and both LUKS replay workflows now assert that setting the controller property is refused and stores nothing. The cleanup helper reads the pinned Secret name from the right field. Assisted-by: LLM Signed-off-by: Andrei Kvapil --- tests/e2e/cli-matrix/README.md | 2 +- .../encryption-passphrase-luks-rd.sh | 31 +++++++++++--- tests/e2e/cli-matrix/lib.sh | 10 +++-- .../cli-matrix/luks-autoplace-encrypted.sh | 4 -- tests/e2e/cli-matrix/luks-clone-encrypted.sh | 4 -- .../cli-matrix/luks-rd-create-encrypted.sh | 7 ---- tests/e2e/cli-matrix/luks-resize-encrypted.sh | 4 -- .../luks-snapshot-restore-encrypted.sh | 4 -- tests/e2e/drbd-luks-stack.sh | 8 ++-- tests/e2e/lib.sh | 20 +++++++++ tests/e2e/luks-layer.sh | 8 ++-- tests/e2e/luks-layered-toggle-delete-leak.sh | 19 +++------ tests/e2e/resize-luks.sh | 4 +- .../replay/encryption-passphrase-luks-rd.yaml | 26 +++++++++--- .../replay/luks-encrypted-rd.yaml | 41 ++++++++++--------- 15 files changed, 110 insertions(+), 82 deletions(-) diff --git a/tests/e2e/cli-matrix/README.md b/tests/e2e/cli-matrix/README.md index 966d1888..ee4116cd 100644 --- a/tests/e2e/cli-matrix/README.md +++ b/tests/e2e/cli-matrix/README.md @@ -44,7 +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 ` 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-` snapshot visible on the source. Pre-fix: 400 on `use_zfs_clone`, 501 on VD-bearing sources (Bug 114 gate). | | `rd-clone-retry-semantics.sh` | parity | The clone endpoint's retry contract. `linstor rd clone ` run twice answers the same way twice, and still does after `vd set-size` grows the source: a finished clone is a copy of a point-in-time and owes the source nothing, and judging the replay against the live source would make an ordinary resize refuse every later retry permanently (the snapshot outlives the clone by design, so deleting it is no way out). Also pins a clone body carrying `delete_namespaces`, driven over raw REST because the verb has no flag for it — it is a wire field from golinstor's `GenericPropsModify`. | | `snap-restore-retry-semantics.sh` | parity | The resource restore's retry contract. LINSTOR's documented restore sequence, `rd c`, `s vd restore`, then `s rsc restore`, is the one linstor-csi issues, and the resource restore takes the definition the first two steps made as its own target. Run again with the same argv, the retry CSI issues when a response is lost, it answers success and places nothing more. | -| `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". | +| `encryption-passphrase-luks-rd.sh` | 023 | Secret-only LUKS flow: `linstor encryption create-passphrase` alone (legacy `DrbdOptions/EncryptPassphrase` controller prop asserted ABSENT throughout, and `controller set-property DrbdOptions/EncryptPassphrase` asserted REFUSED) 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 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. | diff --git a/tests/e2e/cli-matrix/encryption-passphrase-luks-rd.sh b/tests/e2e/cli-matrix/encryption-passphrase-luks-rd.sh index 284d310a..6bcf0bf5 100755 --- a/tests/e2e/cli-matrix/encryption-passphrase-luks-rd.sh +++ b/tests/e2e/cli-matrix/encryption-passphrase-luks-rd.sh @@ -21,11 +21,11 @@ # at apply time. # # Post-fix contract (pinned here): the Secret set by `encryption -# create-passphrase` is the PRIMARY, upstream-parity key source — the -# whole LUKS lifecycle must work WITHOUT the legacy controller prop -# ever being set. The sibling cells (luks-rd-create-encrypted.sh, -# luks-clone-encrypted.sh, replay/luks-encrypted-rd.yaml) still set -# the legacy prop and keep covering the deprecated path. +# create-passphrase` is the ONLY key source. blockstor never reads the +# passphrase from a property (a property sits in plaintext on its CR), +# and the REST API refuses to store one: `controller set-property +# DrbdOptions/EncryptPassphrase ` answers 400. The whole LUKS +# lifecycle must work without that prop, and setting it must fail. # # Flow + assertions: # 1. cleanup_encryption_state → known-clean baseline (no Secret, no @@ -33,7 +33,9 @@ # 2. linstor encryption create-passphrase --passphrase → exit 0. # 3. legacy prop ABSENT on `controller list-properties` (and stays # absent through the whole cell — provisioning must not depend on -# anything writing it behind our back). +# anything writing it behind our back); `controller set-property +# DrbdOptions/EncryptPassphrase ` is refused (non-zero exit) +# and stores nothing. # 4. rd c -l drbd,luks,storage → exit 0 (pre-fix: rejected). # 5. vd c + r c --auto-place=2 → both diskful replicas UpToDate. # 6. kernel-level proof on EACH replica: backing LV/zvol carries a @@ -42,6 +44,7 @@ # satellite channel to luksFormat, not just past the REST gate. # # Unit pins: pkg/rest/luks_gate_bug023_test.go, +# pkg/rest/passphrase_props_guard_test.go, # pkg/satellite/controllers/luks_passphrase_internal_test.go. This # cell is the stand-side companion: real python-linstor → apiserver → # satellite → cryptsetup. @@ -115,6 +118,22 @@ rm -f "$err_file" echo ">> [Bug 023] legacy DrbdOptions/EncryptPassphrase prop is ABSENT" assert_legacy_prop_absent "after create-passphrase" +echo ">> [Bug 023] controller set-property DrbdOptions/EncryptPassphrase is REFUSED" +# A decoy, not the real passphrase: if the refusal ever regressed, the +# value it would leave in plaintext on the ControllerConfig is +# worthless, and assert_legacy_prop_absent below catches it. +err_file=$(mktemp) +if "${LCTL[@]}" controller set-property DrbdOptions/EncryptPassphrase \ + 'cli-matrix-023-decoy' 2>"$err_file" >/dev/null; then + echo "FAIL (Bug 023): controller set-property DrbdOptions/EncryptPassphrase was accepted;" >&2 + echo " the passphrase must never be stored in a plaintext property" >&2 + cat "$err_file" >&2 + rm -f "$err_file" + exit 1 +fi +rm -f "$err_file" +assert_legacy_prop_absent "after the refused set-property" + echo ">> [Bug 023] linstor rd c $RD -l drbd,luks,storage (no legacy prop set)" err_file=$(mktemp) if ! "${LCTL[@]}" resource-definition create "$RD" \ diff --git a/tests/e2e/cli-matrix/lib.sh b/tests/e2e/cli-matrix/lib.sh index 53977aa9..e11d4a6c 100755 --- a/tests/e2e/cli-matrix/lib.sh +++ b/tests/e2e/cli-matrix/lib.sh @@ -534,12 +534,14 @@ assert_luks_passphrase_opens() { cleanup_encryption_state() { # Resolve the passphrase Secret name (custom ref or default). local secret - secret=$(kubectl -n "$NS" get controllerconfigs.blockstor.cozystack.io \ - -o jsonpath='{.items[0].spec.passphraseSecretRef}' 2>/dev/null || true) + secret=$(kubectl get controllerconfigs.blockstor.cozystack.io \ + -o jsonpath='{.items[0].spec.passphraseSecretRef.name}' 2>/dev/null || true) [[ -n "$secret" ]] || secret=blockstor-cluster-passphrase kubectl -n "$NS" delete secret "$secret" --ignore-not-found >/dev/null 2>&1 || true - # Clear the LUKS-layer controller property too, so a leftover key - # from a prior cell doesn't leak into the next one's RD create. + # Clear a passphrase controller property a stand may still carry + # from a build that read one. Nothing reads it now and REST refuses + # to set it, but it is a plaintext key on the ControllerConfig; an + # empty value is the set-property delete, which REST allows. if [[ ${#LCTL[@]} -gt 0 ]]; then "${LCTL[@]}" controller set-property DrbdOptions/EncryptPassphrase "" \ >/dev/null 2>&1 || true diff --git a/tests/e2e/cli-matrix/luks-autoplace-encrypted.sh b/tests/e2e/cli-matrix/luks-autoplace-encrypted.sh index dffbfeb5..4f2d09a7 100755 --- a/tests/e2e/cli-matrix/luks-autoplace-encrypted.sh +++ b/tests/e2e/cli-matrix/luks-autoplace-encrypted.sh @@ -62,10 +62,6 @@ fi echo ">> [Bug 333] set cluster passphrase" "${LCTL[@]}" encryption create-passphrase --passphrase "$PASSPHRASE" >/dev/null 2>&1 || true -# LUKS-layered RDs additionally require the controller property -# DrbdOptions/EncryptPassphrase, else rd-create is rejected with -# "LUKS layer requires DrbdOptions/EncryptPassphrase to be set first". -"${LCTL[@]}" controller set-property DrbdOptions/EncryptPassphrase "$PASSPHRASE" >/dev/null 2>&1 || true echo ">> [Bug 333] linstor rd c $RD -l drbd,luks,storage + vd c 64M" "${LCTL[@]}" resource-definition create "$RD" --layer-list drbd,luks,storage >/dev/null diff --git a/tests/e2e/cli-matrix/luks-clone-encrypted.sh b/tests/e2e/cli-matrix/luks-clone-encrypted.sh index b80d42f2..8dba84c2 100755 --- a/tests/e2e/cli-matrix/luks-clone-encrypted.sh +++ b/tests/e2e/cli-matrix/luks-clone-encrypted.sh @@ -64,10 +64,6 @@ fi echo ">> [Bug 333] set cluster passphrase + create source encrypted RD" "${LCTL[@]}" encryption create-passphrase --passphrase "$PASSPHRASE" >/dev/null 2>&1 || true -# LUKS-layered RDs additionally require the controller property -# DrbdOptions/EncryptPassphrase, else rd-create is rejected with -# "LUKS layer requires DrbdOptions/EncryptPassphrase to be set first". -"${LCTL[@]}" controller set-property DrbdOptions/EncryptPassphrase "$PASSPHRASE" >/dev/null 2>&1 || true "${LCTL[@]}" resource-definition create "$RD_SRC" --layer-list drbd,luks,storage >/dev/null "${LCTL[@]}" volume-definition create "$RD_SRC" 64M >/dev/null "${LCTL[@]}" resource create --auto-place=2 --storage-pool="$POOL" "$RD_SRC" >/dev/null diff --git a/tests/e2e/cli-matrix/luks-rd-create-encrypted.sh b/tests/e2e/cli-matrix/luks-rd-create-encrypted.sh index c19f5ca9..56dd96b7 100755 --- a/tests/e2e/cli-matrix/luks-rd-create-encrypted.sh +++ b/tests/e2e/cli-matrix/luks-rd-create-encrypted.sh @@ -71,13 +71,6 @@ if ! "${LCTL[@]}" encryption create-passphrase --passphrase "$PASSPHRASE" >/dev/ exit 1 fi -# A LUKS-layered RD additionally requires the controller property -# DrbdOptions/EncryptPassphrase (the LUKS layer reads the per-volume -# key material from it); the cluster passphrase alone is not enough — -# rd-create is rejected with "LUKS layer requires -# DrbdOptions/EncryptPassphrase to be set first". -"${LCTL[@]}" controller set-property DrbdOptions/EncryptPassphrase "$PASSPHRASE" >/dev/null 2>&1 || true - echo ">> [Bug 333] linstor rd c $RD -l drbd,luks,storage" err_file=$(mktemp) if ! "${LCTL[@]}" resource-definition create "$RD" \ diff --git a/tests/e2e/cli-matrix/luks-resize-encrypted.sh b/tests/e2e/cli-matrix/luks-resize-encrypted.sh index d1efe491..fd441f88 100755 --- a/tests/e2e/cli-matrix/luks-resize-encrypted.sh +++ b/tests/e2e/cli-matrix/luks-resize-encrypted.sh @@ -60,10 +60,6 @@ fi echo ">> [Bug 333] set cluster passphrase" "${LCTL[@]}" encryption create-passphrase --passphrase "$PASSPHRASE" >/dev/null 2>&1 || true -# LUKS-layered RDs additionally require the controller property -# DrbdOptions/EncryptPassphrase, else rd-create is rejected with -# "LUKS layer requires DrbdOptions/EncryptPassphrase to be set first". -"${LCTL[@]}" controller set-property DrbdOptions/EncryptPassphrase "$PASSPHRASE" >/dev/null 2>&1 || true echo ">> [Bug 333] linstor rd c $RD -l drbd,luks,storage + vd c 64M + r c --auto-place=2" "${LCTL[@]}" resource-definition create "$RD" --layer-list drbd,luks,storage >/dev/null diff --git a/tests/e2e/cli-matrix/luks-snapshot-restore-encrypted.sh b/tests/e2e/cli-matrix/luks-snapshot-restore-encrypted.sh index a83757e1..61549b1b 100755 --- a/tests/e2e/cli-matrix/luks-snapshot-restore-encrypted.sh +++ b/tests/e2e/cli-matrix/luks-snapshot-restore-encrypted.sh @@ -69,10 +69,6 @@ fi echo ">> [Bug 333] set cluster passphrase" "${LCTL[@]}" encryption create-passphrase --passphrase "$PASSPHRASE" >/dev/null 2>&1 || true -# LUKS-layered RDs additionally require the controller property -# DrbdOptions/EncryptPassphrase, else rd-create is rejected with -# "LUKS layer requires DrbdOptions/EncryptPassphrase to be set first". -"${LCTL[@]}" controller set-property DrbdOptions/EncryptPassphrase "$PASSPHRASE" >/dev/null 2>&1 || true echo ">> [Bug 333] create source encrypted RD" "${LCTL[@]}" resource-definition create "$RD_SRC" --layer-list drbd,luks,storage >/dev/null diff --git a/tests/e2e/drbd-luks-stack.sh b/tests/e2e/drbd-luks-stack.sh index cc7a8726..4f1fd5ac 100755 --- a/tests/e2e/drbd-luks-stack.sh +++ b/tests/e2e/drbd-luks-stack.sh @@ -8,10 +8,10 @@ # # Setup: # - 2-replica RD on workers 1+2 with LayerStack=["DRBD","LUKS","STORAGE"] -# - DrbdOptions/Encryption/passphrase set on the RD +# - the cluster passphrase Secret (ensure_passphrase_secret) # - write urandom on primary + capture md5 # Steps: -# 1. apply RD with explicit layerStack + passphrase +# 1. ensure the passphrase Secret, apply RD with explicit layerStack # 2. apply Resources on N1, N2 # 3. wait_uptodate — both peers reach disk:UpToDate over DRBD # 4. write 4 MiB on primary's /dev/drbdN, read back — md5 matches @@ -43,6 +43,8 @@ STORPOOL=${STORPOOL:-stand} trap 'delete_rd "$RD"' EXIT +ensure_passphrase_secret "$PASSPHRASE" + echo ">> apply ${RD} with LayerStack=[DRBD,LUKS,STORAGE]" cat </dev/null || true } + +# ensure_passphrase_secret — make sure the cluster LUKS +# master passphrase Secret exists, the one `linstor encryption +# create-passphrase` writes. It is the only place blockstor reads the +# passphrase from: a `DrbdOptions/EncryptPassphrase`, +# `DrbdOptions/Encryption/passphrase` or `LuksPassphrase` property is +# never read, and REST refuses to store one. An existing Secret is left +# alone, since replacing it would lock out every LUKS volume formatted +# with it; the scenario then runs on that cluster passphrase. +ensure_passphrase_secret() { + local pass=${1:?passphrase required} secret + secret=$(kubectl get controllerconfigs.blockstor.cozystack.io \ + -o jsonpath='{.items[0].spec.passphraseSecretRef.name}' 2>/dev/null || true) + [[ -n "$secret" ]] || secret=blockstor-cluster-passphrase + if kubectl -n "$NS" get secret "$secret" >/dev/null 2>&1; then + return 0 + fi + kubectl -n "$NS" create secret generic "$secret" \ + --from-literal=passphrase="$pass" >/dev/null +} diff --git a/tests/e2e/luks-layer.sh b/tests/e2e/luks-layer.sh index f135c4b7..99f24ffc 100755 --- a/tests/e2e/luks-layer.sh +++ b/tests/e2e/luks-layer.sh @@ -9,10 +9,10 @@ # # Setup: # - 1-replica RD on worker-1 with LayerStack=["LUKS","STORAGE"] -# - DrbdOptions/Encryption/passphrase set on the RD +# - the cluster passphrase Secret (ensure_passphrase_secret) # - write urandom + capture md5 # Steps: -# 1. apply RD with explicit layerStack + passphrase prop +# 1. ensure the passphrase Secret, apply RD with explicit layerStack # 2. apply Resource on N1 # 3. wait for satellite to: provision LV → luksFormat → luksOpen # 4. write 4 MiB pattern, read back — md5 must match @@ -44,6 +44,8 @@ STORPOOL=${STORPOOL:-stand} trap 'delete_rd "$RD"' EXIT +ensure_passphrase_secret "$PASSPHRASE" + echo ">> apply ${RD} with LayerStack=[LUKS,STORAGE]" cat </tmp/luks-toggle-leak-pf.log 2>&1 & PF_PID=$! -# Track passphrase state so cleanup doesn't strand -# DrbdOptions/EncryptPassphrase on a property the next scenario can't -# observe; harmless on shared stands but tidy. -SET_PASSPHRASE=0 - dump_diag() { echo "---- dump: linstor r l -r $RD ----" "${LCTL[@]}" r l -r "$RD" 2>&1 || true @@ -77,9 +72,6 @@ cleanup() { dump_diag fi delete_rd "$RD" 2>/dev/null || true - if (( SET_PASSPHRASE == 1 )); then - "${LCTL[@]}" controller set-property DrbdOptions/EncryptPassphrase "" 2>/dev/null || true - fi kill "$PF_PID" 2>/dev/null || true wait "$PF_PID" 2>/dev/null || true } @@ -96,12 +88,11 @@ LCTL=(linstor --controllers "http://localhost:$PF_PORT") delete_rd "$RD" 2>/dev/null || true -# Stamp the cluster-wide passphrase the LUKS layer needs. Best-effort: -# a previous scenario may already have it stamped and the second set -# is harmless on the apiserver but emits a warning on the CLI. -echo ">> stamping DrbdOptions/EncryptPassphrase via controller set-property" -"${LCTL[@]}" controller set-property DrbdOptions/EncryptPassphrase "bug-hunt-v2-luks-passphrase" 2>&1 | tail -5 -SET_PASSPHRASE=1 +# Make sure the cluster passphrase Secret the LUKS layer needs exists. +# A previous scenario may already have created it; that one is kept, +# since replacing it would lock out the volumes formatted with it. +echo ">> ensuring the cluster passphrase Secret" +ensure_passphrase_secret "bug-hunt-v2-luks-passphrase" # ---- STEP 1: 2 diskful with LUKS-stacked layer list ---------------- diff --git a/tests/e2e/resize-luks.sh b/tests/e2e/resize-luks.sh index bc98ce61..e7a8b647 100755 --- a/tests/e2e/resize-luks.sh +++ b/tests/e2e/resize-luks.sh @@ -35,6 +35,8 @@ SIZE_GROWN_KIB=131072 # 128 MiB trap 'delete_rd "$RD"' EXIT +ensure_passphrase_secret 'this-is-a-32-byte-test-passphrase!!' + echo ">> apply LUKS-encrypted RD ($SIZE_INITIAL_KIB KiB)" cat < is + REFUSED (exit 10): REST never stores the passphrase in a + plaintext property; the prop stays absent 3. rd create -l drbd,luks,storage -- pre-fix this step is rejected with "LUKS layer requires DrbdOptions/EncryptPassphrase to be set first" (exit 10); post-fix the Secret satisfies the @@ -16,9 +20,8 @@ description: | empty" and never converge) 5. legacy prop STILL absent after provisioning - Contrast with replay/luks-encrypted-rd.yaml, which sets the legacy - controller prop and keeps covering the deprecated path; this - workflow proves the prop is no longer required. + replay/luks-encrypted-rd.yaml runs the same refusal ahead of a + LUKS lifecycle with snapshots. NOTE: the runner only executes `linstor` argv, so the kernel-level asserts (cryptsetup luksDump / --test-passphrase on each replica's @@ -54,6 +57,19 @@ steps: # satisfies the post-fix LUKS gate. cmd: ["encryption", "create-passphrase", "-p", "replay-pp023-do-not-rely-on"] expect_exit: [0, 10] + - name: set-legacy-prop-refused + # The passphrase never goes into a property: the controller answers + # 400 ("refusing to store the LUKS master passphrase in property + # ...; run `linstor encryption create-passphrase`"), which the CLI + # surfaces as exit 10, and nothing is stored. A decoy value, so a + # regression leaves nothing worth having on the ControllerConfig. + cmd: ["controller", "set-property", "DrbdOptions/EncryptPassphrase", "replay-pp023-decoy"] + expect_exit: 10 + await: + kind: prop_value + obj: controller + key: DrbdOptions/EncryptPassphrase + timeout_s: 10 - name: create-rd-luks-no-legacy-prop # THE Bug-023 step: pre-fix exit 10 ("LUKS layer requires # DrbdOptions/EncryptPassphrase to be set first"), post-fix 0. diff --git a/tests/operator-harness/replay/luks-encrypted-rd.yaml b/tests/operator-harness/replay/luks-encrypted-rd.yaml index acb525a8..7a8f2b35 100644 --- a/tests/operator-harness/replay/luks-encrypted-rd.yaml +++ b/tests/operator-harness/replay/luks-encrypted-rd.yaml @@ -2,7 +2,8 @@ name: luks-encrypted-rd description: | Bug-333 catcher. Exercises an encrypted RD lifecycle: 1. encryption create-passphrase - 2. controller set-property DrbdOptions/EncryptPassphrase + 2. controller set-property DrbdOptions/EncryptPassphrase is + REFUSED (exit 10) and the prop stays absent 3. rd create -l drbd,luks,storage 4. vd create 5. r create --auto-place=2 @@ -10,14 +11,15 @@ description: | Validates that the LUKS layer is included in the actual layer stack and snapshot path doesn't crash python-linstor. - NOTE: a LUKS-layered RD requires BOTH the cluster passphrase - (encryption create-passphrase) AND the controller property - DrbdOptions/EncryptPassphrase (the LUKS layer reads per-volume key - material from it). Without the property the controller correctly - rejects rd-create with "LUKS layer requires DrbdOptions/EncryptPassphrase - to be set first" (exit 10) -- an INTENTIONAL guard, not a product gap. - The green L6 cell tests/e2e/cli-matrix/luks-rd-create-encrypted.sh - sets the property the same way; this replay mirrors it. + NOTE: the cluster passphrase Secret written by encryption + create-passphrase is the only thing a LUKS-layered RD needs, and the + only place the master passphrase is read from. The controller + property DrbdOptions/EncryptPassphrase is never read; the REST API + refuses to store it, since a property sits in plaintext on its CR. + This is an intentional divergence from upstream LINSTOR (see + docs/cli-parity-known-deltas.md). The L6 cell + tests/e2e/cli-matrix/luks-rd-create-encrypted.sh runs the same + Secret-only flow. prerequisites: min_nodes: 2 @@ -38,16 +40,17 @@ steps: # rest of the workflow, so we accept it. cmd: ["encryption", "create-passphrase", "-p", "fuzz-passphrase-do-not-rely-on"] expect_exit: [0, 10] - - name: set-encrypt-passphrase-property - # A LUKS-layered RD additionally requires the controller property - # DrbdOptions/EncryptPassphrase; the cluster passphrase alone is not - # enough. Without it, rd-create below is correctly rejected with - # "LUKS layer requires DrbdOptions/EncryptPassphrase to be set first" - # (exit 10). Setting a controller property is idempotent, so this is - # safe to re-run on the shared stand. Mirrors the green L6 cell - # tests/e2e/cli-matrix/luks-rd-create-encrypted.sh. - cmd: ["controller", "set-property", "DrbdOptions/EncryptPassphrase", "fuzz-passphrase-do-not-rely-on"] - expect_exit: 0 + - name: set-encrypt-passphrase-property-refused + # The passphrase never goes into a property: the controller answers + # 400 and the CLI exits 10; nothing is stored, and the rd-create + # below goes through on the Secret alone. + cmd: ["controller", "set-property", "DrbdOptions/EncryptPassphrase", "fuzz-passphrase-decoy"] + expect_exit: 10 + await: + kind: prop_value + obj: controller + key: DrbdOptions/EncryptPassphrase + timeout_s: 10 - name: create-rd-luks cmd: ["resource-definition", "create", "{{rd}}", "-l", "drbd,luks,storage"] expect_exit: 0 From f81b88537c0e05da3611a0cf2ac0e31206c5fe94 Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Mon, 26 Oct 2026 09:21:01 +0100 Subject: [PATCH 07/24] docs(luks): describe the Secret as the only passphrase source The layer-stack guide set the passphrase through a definition property and described a per-definition key and a wrapping master key, none of which matches the code. It now shows the Secret flow, states that a passphrase property is refused and never read, and warns that rotating the cluster passphrase leaves existing LUKS headers on the old key. The refused controller property is recorded as an intentional divergence from upstream LINSTOR in the parity accept-list, and stale code comments that still pointed at the property path are corrected. Assisted-by: LLM Signed-off-by: Andrei Kvapil --- docs/cli-parity-known-deltas.md | 1 + docs/layer-stack.md | 58 ++++++++++++++------ pkg/satellite/controllers/luks_passphrase.go | 6 +- pkg/satellite/controllers/resource.go | 12 ++-- pkg/satellite/reconciler.go | 5 +- 5 files changed, 54 insertions(+), 28 deletions(-) diff --git a/docs/cli-parity-known-deltas.md b/docs/cli-parity-known-deltas.md index 7581066f..127b8443 100644 --- a/docs/cli-parity-known-deltas.md +++ b/docs/cli-parity-known-deltas.md @@ -58,6 +58,7 @@ Row IDs match the command-catalogue indexes used by `cli-parity-refresh.sh` (see | 87 | `rd clone` / `s resource restore` retry over an unfinished target | BEHAVIOR | permanent | This is the data path: a clone of a source with volumes, and every restore. A clone of a volume-less source is a plain copy with no marker, so a repeat of it stays the already-exists refusal upstream gives. Both endpoints stamp `BlockstorRestoreFromSnapshot` with the target definition and hydrate its volumes and place its replicas afterwards, so a target carrying the marker says the operation STARTED, not that it finished. A repeat under the same name RESUMES it — every step tolerates an object a previous attempt already created — and says so (`… completed on retry`); upstream has no equivalent notion, a repeat there is a plain already-exists failure. This is what makes CSI's CreateVolume idempotent: external-provisioner has no other way to make progress after a partial failure, and answering success on the marker alone would leave CSI seeing a ready volume nothing ever finishes. A definition under that name that carries no marker is taken when it is in the state LINSTOR's own sequence leaves (`rd c`, `s vd restore`, then `s rsc restore`): its volumes exactly the snapshot's and no live replica. linstor-csi restores every volume that way, creating the definition itself first, and the marker is stamped on it at that point. What stays a refusal, each before anything is written to the definition: any other definition under that name this operation did not create and that is not prepared, meaning its volumes are not the snapshot's or it holds a live replica (409); a prepared definition whose layer stack is not the snapshot source's, compared as a set, since the restore would put the source's bytes under layers it never had or without layers it had (409); a prepared definition, or a leftover still to be finished or one the request places replicas into, whose every replica is still being deleted, which placing over would race (409; a bare REST restore's replay over a finished leftover still answers 201 while its snapshot exists, since it places none); a prepared definition or a leftover parented to a resource group that no longer exists (409, or 500 asking for a retry when the group cannot be read); this operation's own leftover carrying the DELETE flag, since finishing it races the tear-down reaping what it writes (409); a leftover whose earlier attempt's rollback gave up, which may hold less than the operation intended, with the way out named (409); a leftover holding a volume the snapshot never recorded beside one it did (409); and a leftover whose creating attempt has started rolling it back (409). Placing a replica, after the definition and its volumes are written, refuses an operator's diskless replica already on a node the request places onto, which holds no copy of the data and is not counted as placed: a 409 on a retry over this operation's own leftover, and on a fresh restore a failed placement, which rolls the new definition back and answers 500, neither with a FAIL_EXISTS band. The controller's tie-breaker witness there is promoted to the replica the request asked for instead, as autoplace promotes one, and its volumes are restored from the snapshot like any other replica of the target; a witness no storage pool can be found for is left as it is and refused the same way, naming `linstor resource create --storage-pool ` to give it one. Once the marker is on a prepared definition it stays: a concurrent retry that finds it resumes and places the replica, and the request that wrote it meets that replica as one already placed, never as a reason to take the marker back off. The window this leaves is a replica placed by hand between the judgement and the marker, which is then judged as part of the restore; on a target prepared for exactly this restore nothing else places one in that moment. A replica the operation would place that is still being deleted on its node fails the request rather than counting as placed (a 409 over a leftover this request adopted, and a 500 with the rollback's answer over one it created), with no FAIL_EXISTS_RSC band on either. Two restores of different snapshots into one prepared definition do not both take it: the second finds the first's marker and is refused. The CLI door settles the parent group, and the nodes and pools the replicas go to, before it writes the definition or the marker, so a request refused for its own shape, or over a prepared definition whose group is gone, leaves that definition unmarked; and a CLI placement that fails after the marker is finished by running the same command again, unless a replica already landed: the re-run then judges it finished, so the failure names the replicas that did not land and the `blockstor resource create` that places them. The CLI judges its own leftover with the same assessment and gate order the REST door uses (a tear-down, then a volume the restore would not have written, then a parent group that is gone, then a rollback that gave up): a leftover with the snapshot's volumes and one replica with a disk is finished and left alone rather than re-placed on a node the operator emptied, and only one with no such replica gets its placement finished, under the same adoption mark the REST door takes, so a rollback by the attempt that created it yields. A definition already under the name is judged before anything about the source is planned, so a finished restore is left alone even once the source can no longer be planned from. Replicas are counted past the cache on both doors, and a rollback still in progress is refused as one still running rather than as one that gave up. A clone resume whose internal snapshot is gone is refused once the leftover holds a volume with a replica that has a disk, since finishing it from a new snapshot would join two moments of the source; a leftover with no volume or no replica is finished from the new snapshot like a fresh clone, unless its volumes do not fit the source as it is now (one smaller than the source's, or one the source no longer has), which is refused with `delete and clone again` before any snapshot is taken. The volumes and replicas of a prepared definition, and a colliding replica, are read from the API server, not a cache, and so is the definition the request judges right after stamping its marker: linstor-csi's volume restore and the resource restore after it can be served by different replicas of the server. A finished clone or restore is judged by its volumes and, where the operation placed replicas, by holding one with a disk, never by where they are now: a replay after a node was evacuated or the target scaled down answers success and leaves placement alone. A finished clone is a copy of a point-in-time and its replay answers the same way whatever happens to the source afterwards — including a `vd set-size`. Pinned by `pkg/rest/rd_clone_idempotency_test.go` + `pkg/rest/rd_clone_replay_test.go` + `pkg/rest/rd_clone_leftover_shape_test.go` + `pkg/rest/snapshot_restore_idempotency_test.go` + `pkg/rest/snapshot_restore_replay_test.go` + `pkg/rest/snapshot_restore_csi_flow_test.go` + `pkg/rest/snapshot_restore_prepared_target_test.go` + `pkg/rest/snapshot_restore_prepared_cache_test.go` + `pkg/rest/snapshot_restore_resume_gates_test.go` + `pkg/rest/leftover_gates_test.go` + `pkg/rest/rollback_handshake_failclosed_test.go` + `pkg/store/restore_target_test.go` + `internal/cli/snapshot_restore_prepared_test.go` (L1) and `tests/e2e/cli-matrix/rd-clone-retry-semantics.sh` (L6, stand-only: `make e2e-list` covers `tests/e2e/*.sh` and no CI lane runs the cli-matrix cells) + `tests/operator-harness/replay/rd-clone-retry-semantics.yaml` (L7). Every marker is written with `Blockstor/RestoreVolumes`, the volumes the snapshot held, so a leftover whose snapshot is gone (an operator deleted it, or it was a clone's internal one and was reaped) is still judged against them: a clone that restored only some of its volumes is unfinished, not complete. A leftover from before the record existed carries none and is judged on its own volumes, as before. A CLI rollback that could not delete takes its in-progress mark back off so the next run finishes the definition, and a CLI re-run over a finished restore says so on stdout. The CLI's rollback of a definition it created holds the same handshake as REST before deleting: a definition another request adopted is left to it, and one whose mark could not be written or whose adoption could not be read is not deleted. A rollback after a failed clone or restore also leaves a definition that already reads as finished, holding every volume and, where the operation places replicas, one replica with a disk: the clone status poll answers such a definition complete without writing anything, which can happen while the attempt that created it is still stamping its last replica, so deleting it afterwards took a volume the driver had been told exists. A replay takes the adoption mark before its 201, so the rollback yields to it like to any adopter, whatever the request placed. The rollback over a group deleted under the operation is the exception: it removes a finished definition too, since one parented to a group that is gone is what it exists to remove, so a status poll that answered complete a moment before it starts is followed by the definition going away; a POST replay's mark makes it yield. That attempt fails, and its retry answers for what it left; a restore that asked for replicas on named nodes names the ones it never placed, with the `linstor resource create` that places them, since the retry judges the definition finished and places nothing. A clone's prop edits (`override_props`, `delete_props`, `delete_namespaces`) land with the definition when it is created, so no leftover carries a prop the caller asked to remove: linstor-csi removes `Aux/csi-provisioning-completed-by` on every clone and takes a definition carrying it, with volumes of the right size, as one already provisioned. The clone status poll asks a finished target what the POST's replay asks before its 201: one carrying the DELETE flag, parented to a group that is gone, or carrying a rollback in progress or one that gave up is answered 404, which sends the driver back to the POST that refuses it, and a group that cannot be read is a 500. A replay of a finished clone is answered before the source is read, so it survives the source being deleted or losing its volumes. A retry after the snapshot was deleted, the cleanup that follows a finished restore, is judged against the volumes the definition recorded when it was restored: a finished one answers 201 (the CLI exits 0) without reading the snapshot or the source, an unfinished one is refused with `delete and restore it from another snapshot`, and a target restored before the record existed keeps the 404 it got. A restored or cloned definition is stamped with the source's data-plane layer stack (its own stored stack, or DRBD over STORAGE when it has none): the dispatcher reads only the definition's stack, so a target group with another stack never changed how the bytes are brought up, but the control plane reads the group's stack for a definition stored without one, and the stamp keeps the two in step. A REST replay of a finished restore checks its node list only while the snapshot exists; once it is gone the replay is judged by the target alone. A bare restore counts as finished without a replica only while its snapshot exists: once the snapshot is gone, a target with no replica holds its data nowhere and is refused with `delete and restore it from another snapshot`, as the CLI refuses it. A clone that names no layer stack takes the source's and needs the cluster passphrase when that stack carries LUKS, as one that names it does; the check runs after the replay of a finished clone, which writes no new LUKS volume. A target already under the name is judged, hydrated and placed under the name it is stored with, whatever case the request spells it in: its replicas are selected by that name exactly, and judged under another spelling a finished clone or restore read as having none; a read of that name that fails is the 500 below. A target that cannot be read is answered 500 asking for a retry, not taken as absent: the create that followed collided and was adopted past every judgement of a leftover. The CLI refuses a snapshot that recorded no volumes before it writes or resumes anything, as the REST door does. A rollback that leaves a definition whole and cannot take its in-progress mark back off says so and names `linstor resource-definition set-property BlockstorRollbackAbandoned` with no value (python-linstor's delete; the blockstor CLI also has `delete-property`), which `rd modify` (`delete_props`) and the CLI accept only while the mark sits on a definition the rollback left whole (`in-progress`, or `snapshots` / `read-snapshots`, where it stopped before touching anything): one naming a later step sits on a definition the rollback took partly apart, and stays. It is one of three server-owned props an operator may delete by name, on both doors. The second is `Blockstor/RestoreAdopted`, which a claim whose patch landed and whose answer and read-back both failed leaves with no request answering for it; every later rollback yields on it, so where the clone or restore can never finish only the operator takes it off. The third is `BlockstorRestoreFromSnapshot` once the definition holds every volume and a diskful replica whose satellite reports every volume UpToDate: the marker keeps pinning placement to the snapshot's backend and nodes after the data is restored, and deleting it is the way to lift that pin, as it was on main; deleting it from an unfinished restore is refused, since a replica placed without it comes up blank. Setting any of them stays refused, and an empty `override_props` value is read as the delete. The refusal over an in-progress mark names the clear only once the rollback has had its time; a clear while it still runs stops that rollback before it deletes anything, since the rollback checks its own mark is still there before its first delete. A CLI rollback that leaves the target in place names the `blockstor snapshot resource restore` that finishes it, including for `rd clone`, whose own re-run is refused over a target that exists. | | 88 | `rg vg c` / `rg vg m` / `rg c` with volume-group `flags` | MISSING_FEATURE | permanent | Upstream's one volume-group flag is `GROSS_SIZE`, which sizes a spawned volume gross. Nothing in blockstor sizes a spawned volume gross, so both volume-group doors answer 501 to a body that sets a flag and write nothing, the props half included, rather than a 200 for a request that changes nothing. The volume-group create door and `rg c`, whose body can carry volume groups of its own, refuse any `flags`, so a flag no longer reaches the store; on main it was stored, listed by `rg vg l`, and did nothing. The modify door honours removals, spelled `-` as golinstor's `VolumeGroupModify` documents, since that is the only way to clear a flag a volume group created before this already carries; a body that mixes a removal with a set is refused whole. Pinned by `pkg/rest/vg_modify_golinstor_test.go`. | | 89 | `delete_namespaces` on every props-modify door | BEHAVIOR | permanent | Recorded so it is not reopened. As upstream's `PropsContainer.removeNamespace` does, a namespace removes the keys below it and leaves a key spelled exactly as the namespace: `DrbdOptions` takes `DrbdOptions/Net/protocol` and keeps `DrbdOptions`. Trailing slashes name the same namespace, however many there are. Against main this both widens and narrows the delete. Wider: a trailing-slash entry deleted nothing on most doors (the node-connection door trimmed one slash), and a multi-slash entry such as `Aux//` deleted nothing on any door; now both delete the keys below `Aux`. Narrower: the volume-definition, per-resource volume and node-connection doors deleted a key spelled exactly like the namespace, and now keep it. One spelling still differs from upstream: an entry of `/` alone, which upstream reads as the root namespace and so clears every prop, names nothing here and deletes nothing, the safe side of the difference. Every door and every guard in front of one read it through the same helper (`store.NamespaceCovers`), so a guard in front of a door refuses every spelling the delete honours. | +| 90 | `controller set-property DrbdOptions/EncryptPassphrase ` (and `DrbdOptions/Encryption/passphrase` / `LuksPassphrase` on any props door) | BEHAVIOR | permanent | BLOCKSTOR_STRICTER. Upstream LINSTOR accepts the LUKS master passphrase as a controller property. blockstor keeps it only in the Secret `encryption create-passphrase` writes and never reads it from a property: a property sits in plaintext on its CR, so any dump of the CRs for a backup would carry the key to every LUKS volume. Any POST/PUT/PATCH whose JSON body stores one of those keys, matched without regard to case, answers 400 naming `encryption create-passphrase`, and nothing is stored; the bundled CLI's `set-property` refuses the same write. Deleting such a property (a `delete_props` entry, or an override to the empty string) stays allowed. Pinned by `pkg/rest/passphrase_props_guard_test.go` and `replay/encryption-passphrase-luks-rd.yaml`. | | 07 | `rd l --resource-definitions ` (Layers column on a placed RD) | WIRE_SHAPE | permanent | Same `stampRDLayerDataFromStack` behaviour as row 81 — the flag-qualified `rd l --resource-definitions ` catalogue cell (harness index 07) the bare `rd l` whitelist string does not literally cover. BS re-synthesises `layer_data: [{"type":"DRBD"},{"type":"STORAGE"}]` from `Spec.LayerStack` on every RD read, so the python CLI's Layers column renders `DRBD,STORAGE` even for a fresh `parity-rd` with only a volume-definition; upstream 1.33.2 leaves the column blank until DRBD layer data is actually allocated. BLOCKSTOR_SUPERSET, operator-friendly, and linstor-csi / piraeus-operator do not gate on RD `layer_data`. See row 81 for the full rationale and the L1/L3 pins (`stampRDLayerDataFromStack`, `tests/contract/normalize_test.go::TestNormalizeRDLayerDataDropped`). | | 33 | `s d ` (idempotent snapshot delete) | WIRE_SHAPE | permanent | BLOCKSTOR idempotent-delete envelope. Deleting a snapshot definition that does not exist returns `SUCCESS: snapshot already absent: ` (the desired-state delete is satisfied); upstream LINSTOR returns `WARNING: Snapshot definition of resource not found`. Both exit 0. Deliberate: a CSI `DeleteSnapshot` / operator retry MUST be idempotent, so "already gone" is a success, not a warning. Mirrors the resource-delete idempotency in row 42. Pinned by the snapshot-delete REST handler's already-absent path (`pkg/rest/snapshots.go`). | diff --git a/docs/layer-stack.md b/docs/layer-stack.md index abf3656a..6b09fe54 100644 --- a/docs/layer-stack.md +++ b/docs/layer-stack.md @@ -63,8 +63,8 @@ curl -XPOST http://localhost:3370/v1/resource-definitions \ ### Per-RG via spawn template ```bash +linstor encryption create-passphrase --passphrase '<32-byte-secret>' linstor rg create encrypted-rg -linstor rg sp encrypted-rg DrbdOptions/Encryption/passphrase '<32-byte-secret>' linstor rg c --place-count 2 encrypted-rg linstor rg spawn encrypted-rg pvc-2 1G --layer-list LUKS DRBD ``` @@ -74,27 +74,49 @@ the REST shape mirrors upstream LINSTOR.) ### Per-RD via kubectl +The cluster passphrase Secret has to exist first; `linstor encryption +create-passphrase` writes it, or apply it yourself (GitOps): + ```yaml +apiVersion: v1 +kind: Secret +metadata: + name: blockstor-cluster-passphrase # or ControllerConfig spec.passphraseSecretRef.name + namespace: blockstor-system # the controller's namespace +type: Opaque +stringData: + passphrase: "32-byte-secret" +--- apiVersion: blockstor.cozystack.io/v1alpha1 kind: ResourceDefinition metadata: {name: pvc-encrypted} spec: layerStack: ["LUKS", "STORAGE"] - props: - DrbdOptions/Encryption/passphrase: "32-byte-secret" volumeDefinitions: - {volumeNumber: 0, sizeKib: 1048576} ``` ## LUKS specifics -- Passphrase is per-RD via `DrbdOptions/Encryption/passphrase`. The - upstream `linstor rd set-property` key. -- The dispatcher folds the resolver-resolved passphrase onto the wire - as `DesiredResource.Props["LuksPassphrase"]`; the satellite reads it - from there. -- Empty passphrase with `LUKS` in the stack fails the apply rather - than silently producing an unencrypted volume. +- The passphrase is the cluster master passphrase, and it lives only in + the Secret `linstor encryption create-passphrase` writes + (`blockstor-cluster-passphrase` in the controller's namespace, unless + ControllerConfig `spec.passphraseSecretRef` names another). Creating + an RD with `LUKS` in its stack is refused until that Secret exists. +- A passphrase is never read from a property. The REST API answers 400 + to any write that would store `DrbdOptions/EncryptPassphrase`, + `DrbdOptions/Encryption/passphrase` or `LuksPassphrase` (any case, on + any object), and so does the bundled CLI's `set-property`: a property + sits in plaintext on its CR, so any dump of the CRs would carry the + key to every LUKS volume. Deleting such a property is still allowed. + This differs from upstream LINSTOR, where `linstor controller + set-property DrbdOptions/EncryptPassphrase` is accepted. +- The satellite reads the Secret, drops any passphrase property that + reached a definition's props (one put there with kubectl, say), and + hands the Secret's value to the dispatcher, which puts it on the wire + as `DesiredResource.Props["LuksPassphrase"]` for the LUKS layer. +- No passphrase with `LUKS` in the stack fails the apply rather than + silently producing an unencrypted volume. - Mapper name: `--luks` → `/dev/mapper/--luks`. Stable across reconciles so reopen on satellite restart re-uses it. - Volume grow: the satellite runs `cryptsetup resize` on the mapper @@ -107,17 +129,19 @@ spec: rows in the table above. Arbitrary orderings (e.g. `["LUKS","DRBD","STORAGE"]` with LUKS-over-DRBD instead of DRBD-over-LUKS) aren't yet validated or rendered. -- **Per-volume LUKS keys**: every volume on a 2-volume RD currently - uses the same RD-level passphrase. Per-volume keys (with master-key +- **Per-volume LUKS keys**: every LUKS volume in the cluster currently + uses the cluster master passphrase. Per-volume keys (with master-key wrapping in the controller's KV store) is a follow-up. - **Mid-stack changes**: editing `layerStack` after a Resource is active doesn't re-encrypt or unwrap existing data. Treat it as set-once-per-RD until migration support lands. -- **Cluster-passphrase rotation**: the cluster passphrase - (`POST /v1/encryption/passphrase`) wraps RD passphrases in the - controller's KV store; rotating it doesn't re-encrypt LUKS headers. - Rotating the per-RD passphrase requires `cryptsetup luksChangeKey` - which isn't wired yet — operators should drop the RD and recreate. +- **Cluster-passphrase rotation**: the cluster passphrase is the LUKS + key itself, not a wrapping key. `linstor encryption + modify-passphrase` (`PUT /v1/encryption/passphrase`) rewrites the + Secret, but nothing runs `cryptsetup luksChangeKey` on the existing + headers, so volumes formatted with the old value stop opening the + next time the satellite opens them. Do not rotate while LUKS volumes + exist; drop and recreate them instead. ## Testing diff --git a/pkg/satellite/controllers/luks_passphrase.go b/pkg/satellite/controllers/luks_passphrase.go index f9552be9..7791f3aa 100644 --- a/pkg/satellite/controllers/luks_passphrase.go +++ b/pkg/satellite/controllers/luks_passphrase.go @@ -44,11 +44,11 @@ import ( // gate but every replica apply looped on `LUKS in layer stack but // Props.LuksPassphrase empty`. // -// The injection deliberately reuses the EXACT downstream channel the -// legacy prop travels: the value lands under the canonical +// The injection reuses the downstream channel the controller prop +// used to travel: the value lands under the canonical // `DrbdOptions/EncryptPassphrase` key in effectiveProps, so // BuildDesired's pickLUKSPassphrase lifts it onto `LuksPassphrase` -// and strips it from the rendered .res options exactly as before. +// and strips it from the rendered .res options. // The Secret is the only source. A passphrase an operator put in a // property, under the upstream key, the early per-RD alias or the wire // key itself, is dropped first: it sits in plaintext on its CR. diff --git a/pkg/satellite/controllers/resource.go b/pkg/satellite/controllers/resource.go index d9c2a2ca..14e1a732 100644 --- a/pkg/satellite/controllers/resource.go +++ b/pkg/satellite/controllers/resource.go @@ -1136,12 +1136,12 @@ func (r *ResourceReconciler) buildDesiredFromCRD(ctx context.Context, target *bl return nil, errors.Wrap(err, "resolve effective props") } - // Bug 023: when the master passphrase lives ONLY in the - // encryption Secret (`linstor encryption create-passphrase`), - // fold it into the props bag under the canonical - // DrbdOptions/EncryptPassphrase key so BuildDesired lifts it - // onto the `LuksPassphrase` wire prop exactly like the legacy - // controller-prop path. See luks_passphrase.go. + // Bug 023: the master passphrase lives only in the encryption + // Secret (`linstor encryption create-passphrase`). Drop any + // passphrase property from the bag and fold the Secret's value in + // under the canonical DrbdOptions/EncryptPassphrase key, so + // BuildDesired lifts it onto the `LuksPassphrase` wire prop. See + // luks_passphrase.go. r.injectLUKSMasterPassphrase(ctx, rd, effectiveProps) desired := dispatcher.BuildDesired(target, peers, nodeList.Items, poolList.Items, rd, effectiveProps) diff --git a/pkg/satellite/reconciler.go b/pkg/satellite/reconciler.go index cc7c18d9..d3a6163e 100644 --- a/pkg/satellite/reconciler.go +++ b/pkg/satellite/reconciler.go @@ -1061,8 +1061,9 @@ func needsLUKS(stack []string) bool { // out of cryptsetup luksFormat's geometry so the probe is skipped. // // Passphrase source for this slice: dr.Props["LuksPassphrase"]. The -// controller folds it in from the RD's `DrbdOptions/Encryption/passphrase` -// prop via the resolver. Empty passphrase fails the apply — explicit +// dispatcher stamps it from the cluster passphrase Secret alone +// (pkg/satellite/controllers/luks_passphrase.go); a passphrase in a +// property is never read. Empty passphrase fails the apply — explicit // rather than silently creating an unencrypted volume. func (r *Reconciler) applyLUKS(ctx context.Context, dr *intent.DesiredResource, devices map[int32]string, resized bool) (map[int32]string, bool, error) { if r.cfg.Cryptsetup == nil { From a60aff6a81546b664c21cd0a8f5622727e859ab8 Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Mon, 26 Oct 2026 09:39:27 +0100 Subject: [PATCH 08/24] fix(rest)!: refuse per-volume LUKS passphrases vd set-passphrase stored its value in the volume definition's properties, in plaintext on the CR, and nothing read it: every LUKS volume uses the cluster passphrase. The door now answers 400 pointing at encryption create-passphrase, and upstream's per-volume key is refused on every props door like the other passphrase keys. Assisted-by: LLM Signed-off-by: Andrei Kvapil --- pkg/passphrase/passphrase.go | 8 +- pkg/rest/passphrase_props_guard_test.go | 24 ++- pkg/rest/vd_passphrase_bug_233.go | 188 ++-------------------- pkg/rest/vd_passphrase_bug_233_test.go | 201 +++++------------------- 4 files changed, 81 insertions(+), 340 deletions(-) diff --git a/pkg/passphrase/passphrase.go b/pkg/passphrase/passphrase.go index 1d4cd65e..22219074 100644 --- a/pkg/passphrase/passphrase.go +++ b/pkg/passphrase/passphrase.go @@ -69,6 +69,12 @@ const PropKeyCanonical = "DrbdOptions/EncryptPassphrase" // refused and dropped the same way. const PropKeyLegacy = "DrbdOptions/Encryption/passphrase" +// PropKeyVolume is upstream LINSTOR's per-volume passphrase property, +// stamped by `linstor vd set-passphrase`. blockstor has no per-volume +// keys: every LUKS volume uses the cluster passphrase, so the door that +// wrote it is refused and the key is refused and dropped like the others. +const PropKeyVolume = "DrbdOptions/Encrypt/Passphrase" + // WirePropKey is the property the satellite's LUKS layer reads the // passphrase from. Only the dispatcher sets it, from the Secret; a // property of that name an operator set is refused and dropped too. @@ -78,7 +84,7 @@ const WirePropKey = "LuksPassphrase" // passphrase in a property. Matched without regard to case, so no // spelling slips through. func IsPlaintextPropKey(key string) bool { - for _, k := range []string{PropKeyCanonical, PropKeyLegacy, WirePropKey} { + for _, k := range []string{PropKeyCanonical, PropKeyLegacy, PropKeyVolume, WirePropKey} { if strings.EqualFold(key, k) { return true } diff --git a/pkg/rest/passphrase_props_guard_test.go b/pkg/rest/passphrase_props_guard_test.go index 2e9348d4..d233e93b 100644 --- a/pkg/rest/passphrase_props_guard_test.go +++ b/pkg/rest/passphrase_props_guard_test.go @@ -53,6 +53,12 @@ func startGuardServer(t *testing.T, objs ...client.Object) (string, store.Store, t.Fatalf("seed RD: %v", err) } + if err := st.VolumeDefinitions().Create(t.Context(), "rd-guard", &apiv1.VolumeDefinition{ + VolumeNumber: 0, SizeKib: 1024, + }); err != nil { + t.Fatalf("seed VD: %v", err) + } + cli := newFakeRESTClient(t) for _, obj := range objs { if err := cli.Create(t.Context(), obj); err != nil { @@ -88,7 +94,8 @@ func controllerExtraProps(t *testing.T, cli client.Client) map[string]string { // Every door that would store the master passphrase in a property answers // 400 and stores nothing: the controller set-property, a definition create // carrying the early per-RD alias, a definition modify carrying the wire key, -// and a spelling that differs only in case. +// a volume definition modify carrying upstream's per-volume key, and a +// spelling that differs only in case. func TestPlaintextPassphrasePropIsRefusedOnEveryDoor(t *testing.T) { t.Parallel() @@ -131,6 +138,21 @@ func TestPlaintextPassphrasePropIsRefusedOnEveryDoor(t *testing.T) { return rd.Props[passphrase.WirePropKey] != "" }, }, + { + name: "vd modify with the per-volume key", method: http.MethodPut, + path: "/v1/resource-definitions/rd-guard/volume-definitions/0", + body: `{"override_props":{"DrbdOptions/Encrypt/Passphrase":"` + guardCanary + `"}}`, + stored: func(t *testing.T, st store.Store, _ client.Client) bool { + t.Helper() + + vd, err := st.VolumeDefinitions().Get(t.Context(), "rd-guard", 0) + if err != nil { + t.Fatalf("get VD: %v", err) + } + + return vd.Props[passphrase.PropKeyVolume] != "" + }, + }, { name: "case variant", method: http.MethodPost, path: "/v1/controller/properties", body: `{"override_props":{"drbdoptions/ENCRYPTPASSPHRASE":"` + guardCanary + `"}}`, diff --git a/pkg/rest/vd_passphrase_bug_233.go b/pkg/rest/vd_passphrase_bug_233.go index 21e2a089..932ed783 100644 --- a/pkg/rest/vd_passphrase_bug_233.go +++ b/pkg/rest/vd_passphrase_bug_233.go @@ -19,77 +19,17 @@ limitations under the License. package rest import ( - "bytes" - "encoding/json" - "io" "net/http" "strconv" - - apiv1 "github.com/cozystack/blockstor/pkg/api/v1" ) -// vdPassphrasePropKey is the upstream-compatible per-VD LUKS -// passphrase property name. Stored on the VD's props bag so the -// satellite-side reconciler (once the cluster-side `cryptsetup -// luksChangeKey` orchestration lands in Phase 12) can pick it up -// via the same drbd_options channel ApplyResources already -// serialises. Mirrors upstream LINSTOR's -// `DrbdOptions/Encrypt/Passphrase` namespace so existing tooling and -// golinstor clients can read it back without translation. -// -//nolint:gosec // this is a property-name constant, not the secret value itself -const vdPassphrasePropKey = "DrbdOptions/Encrypt/Passphrase" - -// vdPassphraseRotateBody mirrors upstream Java's -// `JsonGenTypes.VolumeDefinitionModifyPassphrase` — a single -// `new_passphrase` field carrying the new per-VD LUKS key. We also -// accept the bare-string `PassPhraseEnter` body shape (Bug 173) so -// strict-OpenAPI clients posting `"…"` directly land here cleanly. -type vdPassphraseRotateBody struct { - NewPassphrase string `json:"new_passphrase,omitempty"` - - // Passphrase is the Bug 165 / 173 alias for the same field — - // `--curl` callers and W13-shape clients send this name. The - // canonical `new_passphrase` wins when both are populated. - Passphrase string `json:"passphrase,omitempty"` -} - -// proofOfKnowledge returns the operator-supplied passphrase honouring -// the dual-key wire surface. Canonical `new_passphrase` wins when -// both are set so a typo-defensive caller (sending both for -// belt-and-braces) lands on the upstream-canonical value. -func (b vdPassphraseRotateBody) proofOfKnowledge() string { - if b.NewPassphrase != "" { - return b.NewPassphrase - } - - return b.Passphrase -} - -// handleVDPassphraseRotate serves Bug 233. Validates the parent RD + -// VD exist (404 otherwise), then accepts a `{"new_passphrase":"…"}` -// wrapped object, a `{"passphrase":"…"}` alias, OR a bare JSON -// string `"…"` (the upstream `PassPhraseEnter` spec shape) and -// stamps the value onto the VD's props bag under the -// upstream-compatible `DrbdOptions/Encrypt/Passphrase` key. -// -// The actual satellite-side LUKS-header re-encryption (the -// `cryptsetup luksChangeKey` orchestration) is pending Phase 12; the -// wire-shape registration here is what unblocks -// `linstor vd set-passphrase`. Once the cluster-side rotation -// reconciler lands, this handler will additionally enqueue a -// rotation task — for now the persisted prop is the source-of-truth -// for the next reconcile pass. -// -// Status surface: -// - missing parent RD or VD → 404 (writeStoreError) -// - empty `new_passphrase` (any shape) → 400 + envelope (data-loss -// guard mirroring the Bug 172 cluster-passphrase contract — an -// empty rotation would erase the VD's LUKS key) -// - malformed body → 400 + envelope (Bug 158/161 typed-error path) -// - happy path → 200 + MASK_INFO envelope ("VD passphrase queued -// for rotation, cluster-side orchestration pending Phase 12"), -// so python-linstor's success line renders without confusion. +// handleVDPassphraseRotate serves `linstor vd set-passphrase` (PUT +// .../volume-definitions/{vlmNr}/encryption-passphrase) and refuses it. +// blockstor has no per-volume LUKS keys: every LUKS volume is formatted +// and opened with the cluster passphrase in the Secret. The door used to +// store the value in the volume definition's properties, in plaintext on +// the CR, where nothing read it. A missing definition or volume still +// answers 404 first. func (s *Server) handleVDPassphraseRotate(w http.ResponseWriter, r *http.Request) { rdName := r.PathValue("rd") vlmNrRaw := r.PathValue("vlmNr") @@ -102,8 +42,6 @@ func (s *Server) handleVDPassphraseRotate(w http.ResponseWriter, r *http.Request return } - // Verify the parent RD exists first so a missing RD surfaces 404, - // not 500 from the VD store's downstream chain. _, err = s.Store.ResourceDefinitions().Get(r.Context(), rdName) if err != nil { writeStoreError(w, err) @@ -111,9 +49,6 @@ func (s *Server) handleVDPassphraseRotate(w http.ResponseWriter, r *http.Request return } - // Probe for existence up-front so a missing VD surfaces 404 - // before the body validation's 400 — preserves the pre-Patch - // error-precedence contract. _, err = s.Store.VolumeDefinitions().Get(r.Context(), rdName, int32(vlmNr)) if err != nil { writeStoreError(w, err) @@ -121,110 +56,7 @@ func (s *Server) handleVDPassphraseRotate(w http.ResponseWriter, r *http.Request return } - body, ok := decodeVDPassphraseBody(w, r) - if !ok { - return - } - - want := body.proofOfKnowledge() - if want == "" { - // Bug 172-class data-loss guard: an empty rotation would - // stamp `""` into the VD's LUKS passphrase prop, and the - // next reconcile pass would re-encrypt the LUKS header with - // an empty key — silently erasing the operator-supplied - // per-VD secret while returning 200. Refuse loudly. - writeError(w, http.StatusBadRequest, - "new_passphrase is required: rotation must specify a non-empty value") - - return - } - - // Bug 204b shape: typed-Patch with retry-on-conflict so a - // reconciler write on the parent RD between the read and the - // write re-applies the rotation onto fresh state instead of - // surfacing a 409 to the operator. - err = s.Store.VolumeDefinitions().PatchVolumeDefinitionSpec(r.Context(), rdName, int32(vlmNr), - func(live *apiv1.VolumeDefinition) error { - if live.Props == nil { - live.Props = map[string]string{} - } - - live.Props[vdPassphrasePropKey] = want - - return nil - }) - if err != nil { - writeStoreError(w, err) - - return - } - - writeJSON(w, http.StatusOK, []apiv1.APICallRc{{ - RetCode: maskInfo, - Message: "VD passphrase stored; it is not applied to existing LUKS devices", - }}) -} - -// decodeVDPassphraseBody accepts BOTH the wrapped object shape -// (`{"new_passphrase":"…"}` / `{"passphrase":"…"}`) AND the bare -// JSON string shape `"…"` (upstream `PassPhraseEnter` spec). -// Mirrors the dual-shape decoder in -// `decodePassphraseEnterBody` (Bug 173) so the per-VD route shares -// one wire contract with the cluster-passphrase PATCH and the -// strict-spec golinstor client doesn't need to know which endpoint -// expects which envelope. -// -// Empty / truncated / malformed bodies fall through to the standard -// envelope via `decodeJSON` (wrapped path) or `writeDecodeError` -// (bare-string path). -func decodeVDPassphraseBody(w http.ResponseWriter, r *http.Request) (vdPassphraseRotateBody, bool) { - raw, err := io.ReadAll(r.Body) - if err != nil { - writeDecodeError(w, err) - - return vdPassphraseRotateBody{}, false - } - - // Re-wrap so the wrapped-object branch can keep using decodeJSON - // (Bug 158/161 envelope + DisallowUnknownFields). - r.Body = io.NopCloser(bytes.NewReader(raw)) - - first, ok := firstJSONToken(raw) - if !ok { - writeDecodeError(w, io.EOF) - - return vdPassphraseRotateBody{}, false - } - - if first == '"' { - var pass string - - dec := json.NewDecoder(bytes.NewReader(raw)) - - err = dec.Decode(&pass) - if err != nil { - writeDecodeError(w, err) - - return vdPassphraseRotateBody{}, false - } - - // Bug 203 parity: refuse residual bytes after the closing - // quote so a body of `"valid-pass"trailing` doesn't decode - // the partial value silently. - if dec.More() { - writeDecodeError(w, errTrailingJSONData) - - return vdPassphraseRotateBody{}, false - } - - return vdPassphraseRotateBody{NewPassphrase: pass}, true - } - - var body vdPassphraseRotateBody - - if !decodeJSON(w, r, &body) { - return vdPassphraseRotateBody{}, false - } - - return body, true + writeError(w, http.StatusBadRequest, + "per-volume LUKS passphrases are not supported: every LUKS volume uses the cluster passphrase, "+ + "kept in a Secret by `linstor encryption create-passphrase`") } diff --git a/pkg/rest/vd_passphrase_bug_233_test.go b/pkg/rest/vd_passphrase_bug_233_test.go index 927e991b..7ec77f05 100644 --- a/pkg/rest/vd_passphrase_bug_233_test.go +++ b/pkg/rest/vd_passphrase_bug_233_test.go @@ -22,32 +22,18 @@ import ( "encoding/json" "io" "net/http" + "strings" "testing" apiv1 "github.com/cozystack/blockstor/pkg/api/v1" "github.com/cozystack/blockstor/pkg/store" ) -// Bug 233 (P3) — `PUT /v1/resource-definitions/{rd}/volume-definitions/{vlmNr}/encryption-passphrase` -// returned 404. Upstream LINSTOR exposes per-VD LUKS passphrase -// rotation here (Java -// `VolumeDefinitions.java:modifyVolumeDefinitionPassphrase` at line -// 278; body shape is `JsonGenTypes.VolumeDefinitionModifyPassphrase`, -// i.e. `{"new_passphrase":"…"}`). -// -// blockstor's satellite-side `cryptsetup luksChangeKey` orchestration -// doesn't exist yet, so the REST endpoint accepts the body and -// stores the supplied passphrase on the VD's props under the -// upstream-compatible `DrbdOptions/Encrypt/Passphrase` key — the -// downstream reconciler will pick it up once the cluster-side -// rotation lands (Phase 12). The wire shape is what matters for -// upstream parity so `linstor vd set-passphrase` doesn't 404. -// -// The handler also accepts the bare-string `PassPhraseEnter` shape -// (Bug 173 dual-form), since golinstor and strict-OpenAPI clients -// posting `"…"` directly are common enough that rejecting them at the -// VD route while the cluster-passphrase PATCH accepts them would be -// a confusing asymmetry. +// `linstor vd set-passphrase` (PUT .../volume-definitions/{vlmNr}/ +// encryption-passphrase) is refused. blockstor has no per-volume LUKS +// keys: every LUKS volume uses the cluster passphrase in the Secret. The +// door used to store the value in the volume definition's properties, in +// plaintext on the CR, where nothing read it. // seedVDForBug233 seeds an RD + VD pair so the PUT under test has a // real target to rotate. Centralised so the test cases don't drift on @@ -71,113 +57,17 @@ func seedVDForBug233(t *testing.T, st store.Store) { } } -// TestBug233VDPassphraseRouteRegistered: PUT against a real RD+VD -// MUST NOT 404. Pre-fix the route was unregistered. Status code is -// 200 (happy path) or 501 (cluster-side orchestration pending) — -// anything that isn't 404 satisfies the wire-parity contract. -func TestBug233VDPassphraseRouteRegistered(t *testing.T) { - st := store.NewInMemory() - seedVDForBug233(t, st) - - base, stop := startServerWithStore(t, st) - defer stop() - - body, _ := json.Marshal(map[string]string{"new_passphrase": "rotated-secret"}) - - resp := httpPut(t, - base+"/v1/resource-definitions/rd-luks/volume-definitions/0/encryption-passphrase", body) - defer func() { _ = resp.Body.Close() }() - - if resp.StatusCode == http.StatusNotFound { - raw, _ := io.ReadAll(resp.Body) - t.Fatalf("PUT VD passphrase: got 404, want 200 or 501 (Bug 233 — route unregistered, body=%q)", - string(raw)) - } - - if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusNotImplemented { - raw, _ := io.ReadAll(resp.Body) - t.Fatalf("PUT VD passphrase: got %d, want 200 or 501 (body=%q)", - resp.StatusCode, string(raw)) - } - - // Whatever the status, the wire envelope MUST be a non-empty - // `[]APICallRc` so python-linstor renders the operator-visible - // line uniformly. - var rcs []apiv1.APICallRc - - if err := json.NewDecoder(resp.Body).Decode(&rcs); err != nil { - t.Fatalf("decode envelope: %v", err) - } - - if len(rcs) == 0 || rcs[0].Message == "" { - t.Errorf("envelope rcs=%+v; want non-empty []APICallRc with a message", rcs) - } -} - -// TestBug233VDPassphraseBareStringShape: golinstor and the upstream -// `PassPhraseEnter: type: string` strict-spec clients post a bare -// JSON string body. The handler must accept it symmetric with the -// Bug 173 cluster-passphrase PATCH so a `--curl` operator running -// `curl -d '"…"'` doesn't fall through to 400. -func TestBug233VDPassphraseBareStringShape(t *testing.T) { - st := store.NewInMemory() - seedVDForBug233(t, st) - - base, stop := startServerWithStore(t, st) - defer stop() - - body, _ := json.Marshal("rotated-bare-secret") - - resp := httpPut(t, - base+"/v1/resource-definitions/rd-luks/volume-definitions/0/encryption-passphrase", body) - defer func() { _ = resp.Body.Close() }() - - if resp.StatusCode == http.StatusNotFound { - raw, _ := io.ReadAll(resp.Body) - t.Fatalf("PUT VD passphrase bare string: got 404, want 200 or 501 (Bug 233, body=%q)", - string(raw)) - } - - if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusNotImplemented { - raw, _ := io.ReadAll(resp.Body) - t.Fatalf("PUT VD passphrase bare string: got %d, want 200 or 501 (body=%q)", - resp.StatusCode, string(raw)) - } -} - -// TestBug233VDPassphraseUnknownRD: a missing parent RD MUST 404 (not -// 405). Mirrors every other per-RD route's typed-not-found surface. -func TestBug233VDPassphraseUnknownRD(t *testing.T) { - base, stop := startServerWithStore(t, store.NewInMemory()) - defer stop() - - body, _ := json.Marshal(map[string]string{"new_passphrase": "rotated-secret"}) - - resp := httpPut(t, - base+"/v1/resource-definitions/ghost/volume-definitions/0/encryption-passphrase", body) - _ = resp.Body.Close() - - if resp.StatusCode != http.StatusNotFound { - t.Errorf("PUT unknown RD: got %d, want 404", resp.StatusCode) - } -} - -// TestBug233VDPassphraseEmptyValue: `{"new_passphrase":""}` MUST 400. -// Empty passphrase rotations would erase the per-VD LUKS key -// silently — the same Bug 172-class data-loss surface this codebase -// fences off everywhere. Bare empty string `""` MUST also 400 (same -// guard via proofOfKnowledge). -func TestBug233VDPassphraseEmptyValue(t *testing.T) { - cases := []struct { - name string - body []byte - }{ - {"wrapped-empty", mustMarshalBug233(map[string]string{"new_passphrase": ""})}, - {"bare-empty", mustMarshalBug233("")}, - } - - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { +// Every body shape is refused with a message that points at the cluster +// passphrase, and nothing lands in the volume definition's properties. +func TestBug233VDPassphraseIsRefused(t *testing.T) { + for name, raw := range map[string][]byte{ + "wrapped": mustMarshalBug233(map[string]string{"new_passphrase": "rotated-secret"}), + "alias": mustMarshalBug233(map[string]string{"passphrase": "rotated-secret"}), + "bare string": mustMarshalBug233("rotated-bare-secret"), + "empty": mustMarshalBug233(map[string]string{"new_passphrase": ""}), + "not json": []byte("not json"), + } { + t.Run(name, func(t *testing.T) { st := store.NewInMemory() seedVDForBug233(t, st) @@ -185,57 +75,48 @@ func TestBug233VDPassphraseEmptyValue(t *testing.T) { defer stop() resp := httpPut(t, - base+"/v1/resource-definitions/rd-luks/volume-definitions/0/encryption-passphrase", tc.body) + base+"/v1/resource-definitions/rd-luks/volume-definitions/0/encryption-passphrase", raw) defer func() { _ = resp.Body.Close() }() + got, _ := io.ReadAll(resp.Body) if resp.StatusCode != http.StatusBadRequest { - raw, _ := io.ReadAll(resp.Body) - t.Fatalf("PUT empty passphrase: got %d, want 400 (Bug 233 data-loss guard, body=%q)", - resp.StatusCode, string(raw)) + t.Fatalf("PUT VD passphrase: got %d, want 400 (body=%q)", resp.StatusCode, got) } var rcs []apiv1.APICallRc + if err := json.Unmarshal(got, &rcs); err != nil || len(rcs) == 0 || + !strings.Contains(rcs[0].Message, "encryption create-passphrase") { + t.Errorf("envelope %q, want one pointing at encryption create-passphrase", got) + } - if err := json.NewDecoder(resp.Body).Decode(&rcs); err != nil { - t.Fatalf("decode envelope: %v", err) + vd, err := st.VolumeDefinitions().Get(t.Context(), "rd-luks", 0) + if err != nil { + t.Fatalf("get VD: %v", err) } - if len(rcs) == 0 || rcs[0].Message == "" { - t.Errorf("envelope rcs=%+v; want non-empty []APICallRc with a message", rcs) + for key := range vd.Props { + if strings.Contains(strings.ToLower(key), "passphrase") { + t.Errorf("the VD carries %s after a refused PUT", key) + } } }) } } -// TestBug233VDPassphraseMalformedBody: non-JSON garbage MUST 400 + -// standard envelope. Mirrors the Bug 158/161 contract every -// write-side endpoint upholds via decodeJSON. -func TestBug233VDPassphraseMalformedBody(t *testing.T) { - st := store.NewInMemory() - seedVDForBug233(t, st) - - base, stop := startServerWithStore(t, st) +// TestBug233VDPassphraseUnknownRD: a missing parent RD MUST 404 (not +// 405). Mirrors every other per-RD route's typed-not-found surface. +func TestBug233VDPassphraseUnknownRD(t *testing.T) { + base, stop := startServerWithStore(t, store.NewInMemory()) defer stop() - resp := httpPut(t, - base+"/v1/resource-definitions/rd-luks/volume-definitions/0/encryption-passphrase", - []byte("not json")) - defer func() { _ = resp.Body.Close() }() - - if resp.StatusCode != http.StatusBadRequest { - raw, _ := io.ReadAll(resp.Body) - t.Fatalf("PUT malformed: got %d, want 400 (body=%q)", - resp.StatusCode, string(raw)) - } - - var rcs []apiv1.APICallRc + body, _ := json.Marshal(map[string]string{"new_passphrase": "rotated-secret"}) - if err := json.NewDecoder(resp.Body).Decode(&rcs); err != nil { - t.Fatalf("decode envelope: %v", err) - } + resp := httpPut(t, + base+"/v1/resource-definitions/ghost/volume-definitions/0/encryption-passphrase", body) + _ = resp.Body.Close() - if len(rcs) == 0 || rcs[0].Message == "" { - t.Errorf("error envelope rcs=%+v; want non-empty []APICallRc with a message", rcs) + if resp.StatusCode != http.StatusNotFound { + t.Errorf("PUT unknown RD: got %d, want 404", resp.StatusCode) } } From 5de1a077432a7e3a84826d7cd742b3432154ef22 Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Mon, 26 Oct 2026 09:39:27 +0100 Subject: [PATCH 09/24] fix(rest): refuse to change the cluster passphrase while LUKS volumes exist The cluster passphrase is the key every LUKS volume was formatted with and nothing re-keys a volume, so a rotation left every existing LUKS volume unable to open. It now answers 409 naming one of them. Assisted-by: LLM Signed-off-by: Andrei Kvapil --- pkg/rest/encryption.go | 51 +++++++++++++++-- pkg/rest/encryption_rotation_luks_test.go | 70 +++++++++++++++++++++++ 2 files changed, 117 insertions(+), 4 deletions(-) create mode 100644 pkg/rest/encryption_rotation_luks_test.go diff --git a/pkg/rest/encryption.go b/pkg/rest/encryption.go index aa950f0d..3ac23bbe 100644 --- a/pkg/rest/encryption.go +++ b/pkg/rest/encryption.go @@ -23,6 +23,7 @@ import ( "context" "crypto/subtle" "encoding/json" + "fmt" "io" "net/http" "time" @@ -597,10 +598,7 @@ func (s *Server) handlePassphraseModify(w http.ResponseWriter, r *http.Request) return } - err = s.writePassphrase(ctx, want) - if err != nil { - writeError(w, http.StatusInternalServerError, err.Error()) - + if !s.rotatePassphrase(ctx, w, want) { return } @@ -618,6 +616,51 @@ func (s *Server) handlePassphraseModify(w http.ResponseWriter, r *http.Request) }}) } +// rotatePassphrase writes the new cluster passphrase unless LUKS volumes +// would lose their key to it, answering the error itself; false means the +// handler stops. +func (s *Server) rotatePassphrase(ctx context.Context, w http.ResponseWriter, want string) bool { + if !s.refuseRotationOverLUKSVolumes(ctx, w) { + return false + } + + err := s.writePassphrase(ctx, want) + if err != nil { + writeError(w, http.StatusInternalServerError, err.Error()) + + return false + } + + return true +} + +// refuseRotationOverLUKSVolumes answers 409 and returns false while any +// definition has LUKS in its stack. The cluster passphrase is the key +// every LUKS volume was formatted with, and nothing re-keys a volume yet +// (no `cryptsetup luksChangeKey`), so a new value in the Secret would +// leave every existing LUKS volume unable to open on its next start. +func (s *Server) refuseRotationOverLUKSVolumes(ctx context.Context, w http.ResponseWriter) bool { + rds, err := s.Store.ResourceDefinitions().List(ctx) + if err != nil { + writeStoreError(w, err) + + return false + } + + for i := range rds { + if apiv1.LayerInStack(rds[i].LayerStack, apiv1.LayerKindLUKS) { + writeError(w, http.StatusConflict, fmt.Sprintf( + "refusing to change the cluster passphrase while LUKS volumes exist (%s among them): "+ + "it is the key they were formatted with, and they are not re-keyed, so they would no longer open", + rds[i].Name)) + + return false + } + } + + return true +} + // readPassphrase reads the current cluster passphrase. Empty // string (not error) means "not yet set". Secret-backed only — // the legacy KVEntry fallback was retired in Phase 10.6. diff --git a/pkg/rest/encryption_rotation_luks_test.go b/pkg/rest/encryption_rotation_luks_test.go new file mode 100644 index 00000000..4d9a24a8 --- /dev/null +++ b/pkg/rest/encryption_rotation_luks_test.go @@ -0,0 +1,70 @@ +// 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 ( + "encoding/json" + "io" + "net/http" + "strings" + "testing" + + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/store" +) + +// The cluster passphrase is the key every LUKS volume was formatted with, +// and nothing re-keys a volume, so changing it while LUKS volumes exist is +// refused and the old value keeps unlocking. +func TestPassphraseModifyIsRefusedWhileLUKSVolumesExist(t *testing.T) { + st := store.NewInMemory() + if err := st.ResourceDefinitions().Create(t.Context(), &apiv1.ResourceDefinition{ + Name: "pvc-enc", LayerStack: []string{apiv1.LayerKindDRBD, apiv1.LayerKindLUKS, apiv1.LayerKindStorage}, + }); err != nil { + t.Fatalf("seed RD: %v", err) + } + + base, stop := startServerWithStore(t, st) + defer stop() + + createBody, _ := json.Marshal(map[string]string{"new_passphrase": "old-secret"}) + resp := httpPost(t, base+"/v1/encryption/passphrase", createBody) + _ = resp.Body.Close() + + if resp.StatusCode != http.StatusCreated { + t.Fatalf("seed create: got %d, want 201", resp.StatusCode) + } + + modifyBody, _ := json.Marshal(map[string]string{"old_passphrase": "old-secret", "new_passphrase": "new-secret"}) + modifyResp := httpPut(t, base+"/v1/encryption/passphrase", modifyBody) + got, _ := io.ReadAll(modifyResp.Body) + _ = modifyResp.Body.Close() + + if modifyResp.StatusCode != http.StatusConflict || !strings.Contains(string(got), "pvc-enc") { + t.Fatalf("modify: got %d %s, want 409 naming the LUKS volume", modifyResp.StatusCode, got) + } + + oldEnter, _ := json.Marshal(map[string]string{"new_passphrase": "old-secret"}) + oldResp := httpPatch(t, base+"/v1/encryption/passphrase", oldEnter) + _ = oldResp.Body.Close() + + if oldResp.StatusCode != http.StatusOK { + t.Errorf("the old passphrase no longer unlocks after a refused rotation: %d", oldResp.StatusCode) + } +} From 88336201a521253e494675173d008d17f4f5fcb5 Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Mon, 26 Oct 2026 09:39:27 +0100 Subject: [PATCH 10/24] fix(dispatcher): drop passphrase keys from a copy of the replica's props Without a resolved bag the wire props are the replica's own map, so dropping the passphrase keys edited the caller's object. Assisted-by: LLM Signed-off-by: Andrei Kvapil --- pkg/dispatcher/dispatcher.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/dispatcher/dispatcher.go b/pkg/dispatcher/dispatcher.go index 6f7fe41f..2462de18 100644 --- a/pkg/dispatcher/dispatcher.go +++ b/pkg/dispatcher/dispatcher.go @@ -28,6 +28,7 @@ package dispatcher import ( "crypto/sha256" "encoding/binary" + "maps" "slices" "strconv" "strings" @@ -228,6 +229,9 @@ func assembleDesired(target *blockstoriov1alpha1.Resource, peers []blockstoriov1 // in the .res options. A wire key that came in with the replica's // own properties is dropped first, so nothing but the Secret reaches // the LUKS layer. + // mergeEffectiveProps hands back the replica's own map when there is + // no resolved bag; clone it so the drop never edits the caller's CR. + wireProps = maps.Clone(wireProps) passphrase.DropPlaintextProps(wireProps) if pass := pickLUKSPassphrase(drbdOpts); pass != "" { From 900e34e1a6d59b171f5d3c3d8ef44a191fce40f3 Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Mon, 26 Oct 2026 09:39:27 +0100 Subject: [PATCH 11/24] docs(luks): name the refused per-volume key and the rotation guard Assisted-by: LLM Signed-off-by: Andrei Kvapil --- docs/cli-parity-known-deltas.md | 2 ++ docs/layer-stack.md | 16 +++++++++------- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/docs/cli-parity-known-deltas.md b/docs/cli-parity-known-deltas.md index 127b8443..16bf667c 100644 --- a/docs/cli-parity-known-deltas.md +++ b/docs/cli-parity-known-deltas.md @@ -59,6 +59,8 @@ Row IDs match the command-catalogue indexes used by `cli-parity-refresh.sh` (see | 88 | `rg vg c` / `rg vg m` / `rg c` with volume-group `flags` | MISSING_FEATURE | permanent | Upstream's one volume-group flag is `GROSS_SIZE`, which sizes a spawned volume gross. Nothing in blockstor sizes a spawned volume gross, so both volume-group doors answer 501 to a body that sets a flag and write nothing, the props half included, rather than a 200 for a request that changes nothing. The volume-group create door and `rg c`, whose body can carry volume groups of its own, refuse any `flags`, so a flag no longer reaches the store; on main it was stored, listed by `rg vg l`, and did nothing. The modify door honours removals, spelled `-` as golinstor's `VolumeGroupModify` documents, since that is the only way to clear a flag a volume group created before this already carries; a body that mixes a removal with a set is refused whole. Pinned by `pkg/rest/vg_modify_golinstor_test.go`. | | 89 | `delete_namespaces` on every props-modify door | BEHAVIOR | permanent | Recorded so it is not reopened. As upstream's `PropsContainer.removeNamespace` does, a namespace removes the keys below it and leaves a key spelled exactly as the namespace: `DrbdOptions` takes `DrbdOptions/Net/protocol` and keeps `DrbdOptions`. Trailing slashes name the same namespace, however many there are. Against main this both widens and narrows the delete. Wider: a trailing-slash entry deleted nothing on most doors (the node-connection door trimmed one slash), and a multi-slash entry such as `Aux//` deleted nothing on any door; now both delete the keys below `Aux`. Narrower: the volume-definition, per-resource volume and node-connection doors deleted a key spelled exactly like the namespace, and now keep it. One spelling still differs from upstream: an entry of `/` alone, which upstream reads as the root namespace and so clears every prop, names nothing here and deletes nothing, the safe side of the difference. Every door and every guard in front of one read it through the same helper (`store.NamespaceCovers`), so a guard in front of a door refuses every spelling the delete honours. | | 90 | `controller set-property DrbdOptions/EncryptPassphrase ` (and `DrbdOptions/Encryption/passphrase` / `LuksPassphrase` on any props door) | BEHAVIOR | permanent | BLOCKSTOR_STRICTER. Upstream LINSTOR accepts the LUKS master passphrase as a controller property. blockstor keeps it only in the Secret `encryption create-passphrase` writes and never reads it from a property: a property sits in plaintext on its CR, so any dump of the CRs for a backup would carry the key to every LUKS volume. Any POST/PUT/PATCH whose JSON body stores one of those keys, matched without regard to case, answers 400 naming `encryption create-passphrase`, and nothing is stored; the bundled CLI's `set-property` refuses the same write. Deleting such a property (a `delete_props` entry, or an override to the empty string) stays allowed. Pinned by `pkg/rest/passphrase_props_guard_test.go` and `replay/encryption-passphrase-luks-rd.yaml`. | +| 91 | `vd set-passphrase ` (`PUT .../volume-definitions/{vlmNr}/encryption-passphrase`) | BEHAVIOR | until per-volume keys land | BLOCKSTOR_STRICTER. Upstream keeps a per-volume LUKS key wrapped by the master passphrase. blockstor has no per-volume keys, every LUKS volume uses the cluster passphrase, and the door used to store the value in plaintext in the volume definition's properties where nothing read it. It now answers 400 naming `encryption create-passphrase`, and `DrbdOptions/Encrypt/Passphrase` is refused on every props door like row 90. Pinned by `pkg/rest/vd_passphrase_bug_233_test.go`. | +| 92 | `encryption modify-passphrase` while LUKS volumes exist | BEHAVIOR | until re-keying lands | BLOCKSTOR_STRICTER. Upstream rotates the master passphrase that wraps the volume keys, so volumes keep opening. In blockstor the cluster passphrase is the LUKS key itself and nothing runs `cryptsetup luksChangeKey`, so a rotation would leave every existing LUKS volume unable to open. It answers 409 naming one LUKS definition while any exists. Pinned by `pkg/rest/encryption_rotation_luks_test.go`. | | 07 | `rd l --resource-definitions ` (Layers column on a placed RD) | WIRE_SHAPE | permanent | Same `stampRDLayerDataFromStack` behaviour as row 81 — the flag-qualified `rd l --resource-definitions ` catalogue cell (harness index 07) the bare `rd l` whitelist string does not literally cover. BS re-synthesises `layer_data: [{"type":"DRBD"},{"type":"STORAGE"}]` from `Spec.LayerStack` on every RD read, so the python CLI's Layers column renders `DRBD,STORAGE` even for a fresh `parity-rd` with only a volume-definition; upstream 1.33.2 leaves the column blank until DRBD layer data is actually allocated. BLOCKSTOR_SUPERSET, operator-friendly, and linstor-csi / piraeus-operator do not gate on RD `layer_data`. See row 81 for the full rationale and the L1/L3 pins (`stampRDLayerDataFromStack`, `tests/contract/normalize_test.go::TestNormalizeRDLayerDataDropped`). | | 33 | `s d ` (idempotent snapshot delete) | WIRE_SHAPE | permanent | BLOCKSTOR idempotent-delete envelope. Deleting a snapshot definition that does not exist returns `SUCCESS: snapshot already absent: ` (the desired-state delete is satisfied); upstream LINSTOR returns `WARNING: Snapshot definition of resource not found`. Both exit 0. Deliberate: a CSI `DeleteSnapshot` / operator retry MUST be idempotent, so "already gone" is a success, not a warning. Mirrors the resource-delete idempotency in row 42. Pinned by the snapshot-delete REST handler's already-absent path (`pkg/rest/snapshots.go`). | diff --git a/docs/layer-stack.md b/docs/layer-stack.md index 6b09fe54..f41322ba 100644 --- a/docs/layer-stack.md +++ b/docs/layer-stack.md @@ -129,19 +129,21 @@ spec: rows in the table above. Arbitrary orderings (e.g. `["LUKS","DRBD","STORAGE"]` with LUKS-over-DRBD instead of DRBD-over-LUKS) aren't yet validated or rendered. -- **Per-volume LUKS keys**: every LUKS volume in the cluster currently - uses the cluster master passphrase. Per-volume keys (with master-key - wrapping in the controller's KV store) is a follow-up. +- **Per-volume LUKS keys**: every LUKS volume in the cluster uses the + cluster master passphrase. `linstor vd set-passphrase` answers 400: + it used to store the value in the volume definition's properties, + in plaintext, where nothing read it. Per-volume keys (with master-key + wrapping) are a follow-up. - **Mid-stack changes**: editing `layerStack` after a Resource is active doesn't re-encrypt or unwrap existing data. Treat it as set-once-per-RD until migration support lands. - **Cluster-passphrase rotation**: the cluster passphrase is the LUKS key itself, not a wrapping key. `linstor encryption modify-passphrase` (`PUT /v1/encryption/passphrase`) rewrites the - Secret, but nothing runs `cryptsetup luksChangeKey` on the existing - headers, so volumes formatted with the old value stop opening the - next time the satellite opens them. Do not rotate while LUKS volumes - exist; drop and recreate them instead. + Secret, and nothing runs `cryptsetup luksChangeKey` on the existing + headers, so volumes formatted with the old value would stop opening. + It answers 409 while any definition has LUKS in its stack, naming + one of them; rotate only once there are none. ## Testing From 3e7e056ac71cfbc295a3ff87d7b8f29bcab1cf45 Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Mon, 26 Oct 2026 09:59:40 +0100 Subject: [PATCH 12/24] fix(rest): read the whole body the way the handlers do in the passphrase guard The guard parsed the body with json.Unmarshal, which fails on bytes after the first value, and let a body it could not parse through. The handlers decode the first value and ignore or tolerate what follows, so a closing brace or any trailing text stored the passphrase anyway. The guard now decodes value by value with a json.Decoder. Assisted-by: LLM Signed-off-by: Andrei Kvapil --- pkg/rest/passphrase_props_guard.go | 23 +++++++---- pkg/rest/passphrase_props_guard_test.go | 52 +++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 7 deletions(-) diff --git a/pkg/rest/passphrase_props_guard.go b/pkg/rest/passphrase_props_guard.go index 2e93b52a..d2ca8a47 100644 --- a/pkg/rest/passphrase_props_guard.go +++ b/pkg/rest/passphrase_props_guard.go @@ -68,16 +68,25 @@ func isWriteMethod(method string) bool { } // plaintextPassphraseKeyIn finds an object key, at any depth of a JSON body, -// that would carry the master passphrase. A body that is not JSON has none; -// the handler answers it as before. +// that would carry the master passphrase. It reads the body the way the +// handlers do, value by value with a json.Decoder, so bytes after the first +// value cannot hide it: a handler that decodes one value and ignores the +// rest still sees the same first value here. A body that is not JSON has +// none; the handler answers it as before. func plaintextPassphraseKeyIn(body []byte) (string, bool) { - var doc any + dec := json.NewDecoder(bytes.NewReader(body)) - if json.Unmarshal(body, &doc) != nil { - return "", false - } + for { + var doc any + + if dec.Decode(&doc) != nil { + return "", false + } - return findPlaintextPassphraseKey(doc) + if key, found := findPlaintextPassphraseKey(doc); found { + return key, true + } + } } func findPlaintextPassphraseKey(node any) (string, bool) { diff --git a/pkg/rest/passphrase_props_guard_test.go b/pkg/rest/passphrase_props_guard_test.go index d233e93b..57b3da25 100644 --- a/pkg/rest/passphrase_props_guard_test.go +++ b/pkg/rest/passphrase_props_guard_test.go @@ -300,3 +300,55 @@ func sendGuardRequest(t *testing.T, method, url, body string) *http.Response { return resp } + +// Bytes after the JSON value do not hide the key: the handlers decode the +// first value and ignore or tolerate what follows, so the guard reads the +// same first value. +func TestPlaintextPassphraseGuardSeesPastTrailingBytes(t *testing.T) { + t.Parallel() + + for _, tail := range []string{"}", "]", "garbage", ` {"x":1}`} { + for name, door := range map[string]struct{ method, path, body string }{ + "controller props": { + http.MethodPost, "/v1/controller/properties", + `{"override_props":{"DrbdOptions/EncryptPassphrase":"` + guardCanary + `"}}`, + }, + "rg modify": { + http.MethodPut, "/v1/resource-groups/rg-guard", + `{"override_props":{"DrbdOptions/EncryptPassphrase":"` + guardCanary + `"}}`, + }, + } { + t.Run(name+" tail "+tail, func(t *testing.T) { + t.Parallel() + + base, st, cli, stop := startGuardServer(t) + defer stop() + + if err := st.ResourceGroups().Create(t.Context(), &apiv1.ResourceGroup{Name: "rg-guard"}); err != nil { + t.Fatalf("seed RG: %v", err) + } + + resp := sendGuardRequest(t, door.method, base+door.path, door.body+tail) + body, _ := readAllBody(resp) + _ = resp.Body.Close() + + if resp.StatusCode != http.StatusBadRequest { + t.Fatalf("status = %d, want 400; body: %s", resp.StatusCode, body) + } + + if controllerExtraProps(t, cli)[passphrase.PropKeyCanonical] != "" { + t.Error("the controller props carry the passphrase") + } + + rg, err := st.ResourceGroups().Get(t.Context(), "rg-guard") + if err != nil { + t.Fatalf("get RG: %v", err) + } + + if rg.Props[passphrase.PropKeyCanonical] != "" { + t.Error("the group props carry the passphrase") + } + }) + } + } +} From a057acff1560e93e004a5142e5f3ae1f75612b02 Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Mon, 26 Oct 2026 09:59:40 +0100 Subject: [PATCH 13/24] test(dispatcher): pin that dropping the wire key leaves the replica alone Assisted-by: LLM Signed-off-by: Andrei Kvapil --- pkg/dispatcher/dispatcher_luks_wire_prop_test.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/pkg/dispatcher/dispatcher_luks_wire_prop_test.go b/pkg/dispatcher/dispatcher_luks_wire_prop_test.go index 03cf6ff2..5d83c6f7 100644 --- a/pkg/dispatcher/dispatcher_luks_wire_prop_test.go +++ b/pkg/dispatcher/dispatcher_luks_wire_prop_test.go @@ -61,3 +61,18 @@ func TestLUKSReplicaWirePropNeverReachesTheWire(t *testing.T) { } }) } + +// With no resolved props the wire props start as the replica's own map; +// dropping the passphrase key from the wire must leave the replica, an +// object out of the caller's cache, as it was. +func TestLUKSWirePropDropLeavesTheReplicaAlone(t *testing.T) { + const name = "pvc-luks-replica-untouched" + + target := luksTestTarget(name, map[string]string{passphrase.WirePropKey: "from-a-replica-prop", "Aux/keep": "yes"}) + + _ = dispatcher.BuildDesired(target, nil, nil, nil, luksTestRD(name), nil) + + if target.Spec.Props[passphrase.WirePropKey] != "from-a-replica-prop" || target.Spec.Props["Aux/keep"] != "yes" { + t.Errorf("the replica's props were edited: %v", target.Spec.Props) + } +} From 6dcf641a4501b27c0e6b555b13d9e2339a223a59 Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Mon, 26 Oct 2026 09:59:40 +0100 Subject: [PATCH 14/24] fix(linstor-migrate): leave passphrase properties out and name the real LUKS step The converter copied every property, so a passphrase LINSTOR kept in one landed in plaintext in YAML that is applied past the REST guard. The props index now leaves those keys out. The LUKS warning sent operators to spec.encryption, which nothing reads; it now says to add the cluster passphrase to each volume as a key slot. Assisted-by: LLM Signed-off-by: Andrei Kvapil --- docs/linstor-migration.md | 8 +-- pkg/linstormigrate/convert.go | 10 ++-- pkg/linstormigrate/convert_test.go | 4 +- pkg/linstormigrate/props.go | 11 +++- pkg/linstormigrate/props_passphrase_test.go | 50 +++++++++++++++++++ pkg/linstormigrate/testdata/golden/report.txt | 2 +- 6 files changed, 74 insertions(+), 11 deletions(-) create mode 100644 pkg/linstormigrate/props_passphrase_test.go diff --git a/docs/linstor-migration.md b/docs/linstor-migration.md index f9aead48..58ea491b 100644 --- a/docs/linstor-migration.md +++ b/docs/linstor-migration.md @@ -10,7 +10,7 @@ Migrated: nodes, storage pools, resource groups, resource definitions (+ volume **NOT migrated — read the report on stderr for the exact list per cluster:** -- **LUKS passphrases.** LUKS-encrypted volumes convert with their layer stack intact (an RD whose `layer_stack` column is empty takes the stack its replicas were built with, read from `LAYER_RESOURCE_IDS`), but the per-volume passphrase (encrypted with the LINSTOR master key) is not decrypted. You must provision `spec.encryption` for those RDs by hand before the encrypted volumes can be opened. Each affected RD is reported. +- **LUKS passphrases.** LUKS-encrypted volumes convert with their layer stack intact (an RD whose `layer_stack` column is empty takes the stack its replicas were built with, read from `LAYER_RESOURCE_IDS`), but the per-volume key (encrypted with the LINSTOR master key) is not decrypted. blockstor opens every LUKS volume with the cluster passphrase kept in its Secret, so add that passphrase to each such volume as a key slot (`cryptsetup luksAddKey`) before adopting it. Each affected RD is reported. - **Definitions whose replicas disagree on the layer stack** are skipped with their replicas and reported: replicas built two ways, or with their layers in another order, under a definition with no stack of its own; a diskful replica the dump shows no layer chain for, under such a definition; or a replica built otherwise than the stack the definition names. Adopted as they are, such a replica would be brought up under a stack it was not built with, DRBD metadata over a LUKS volume's ciphertext for one. Settle the stack in LINSTOR and convert again. - **Backup-shipping remotes**, unknown/runtime flag bits, and DELETE'd / FAILED_DEPLOYMENT rows are skipped and reported. None affect live data availability. - **DRBD TCP ports** are absent from most LINSTOR 1.33 dumps — see "Capture live DRBD ports" below. @@ -74,7 +74,7 @@ Migrated: nodes, storage pools, resource groups, resource definitions (+ volume linstor-migrate -in ./dump -drbd-ports ./drbd-ports.txt -out ./blockstor-manifests.yaml ``` -Read the report on stderr end to end. Every `warning:` line is a thing the converter refused to guess — decide per line whether it matters for your data before continuing. In particular resolve every `LUKS passphrase NOT migrated` and every `no DRBD port` line (the latter should be empty if the port capture was complete). +Read the report on stderr end to end. Every `warning:` line is a thing the converter refused to guess — decide per line whether it matters for your data before continuing. In particular resolve every `LUKS key NOT migrated` and every `no DRBD port` line (the latter should be empty if the port capture was complete). ## Cutover @@ -104,7 +104,7 @@ The manifests do not migrate a cluster by themselves — the control plane must - Spot-check ≥5 RDs: size, per-volume `/dev/drbd`, per-replica node-id and port match the pre-migration values. A changed minor or node-id is a stop-the-line defect. - Every diskful replica is `UpToDate`; no replica is unexpectedly `SyncTarget`. - Adopted snapshots list as present without any suspend-io having fired on the parent RDs (check the satellites did not log `suspend-io`). -- For LUKS RDs: their volumes stay closed until you provision `spec.encryption`; confirm consumers of those volumes are quiesced until then. +- For LUKS RDs: their volumes stay closed until the cluster passphrase is added to them as a key slot; confirm consumers of those volumes are quiesced until then. ## Rollback @@ -114,7 +114,7 @@ Until CSI is repointed and confirmed, rollback is: scale blockstor to 0, scale ` | Item | Impact | Mitigation | | --- | --- | --- | -| LUKS passphrases not migrated | Encrypted volumes cannot be opened until `spec.encryption` is provisioned | Provision by hand; keep those consumers quiesced during migration | +| LUKS keys not migrated | Encrypted volumes cannot be opened until the cluster passphrase is a key slot on them | Add it with `cryptsetup luksAddKey`; keep those consumers quiesced during migration | | DRBD port not in dump (LINSTOR 1.33) | Adoption reconnects the mesh on a new port if not captured | Capture with the pre-flight command and pass `-drbd-ports` | | Unknown flag bits dropped | Cosmetic runtime bookkeeping only | Reported; verified not to carry availability semantics | | Placement/data unchanged | The converter never moves data | Rebalance with blockstor after migration if desired | diff --git a/pkg/linstormigrate/convert.go b/pkg/linstormigrate/convert.go index 8bedf5e9..ab295e03 100644 --- a/pkg/linstormigrate/convert.go +++ b/pkg/linstormigrate/convert.go @@ -1364,9 +1364,13 @@ func (c *converter) attachRDLayerFields(def *crdv1alpha1.ResourceDefinition, row // operator's master passphrase and LINSTOR's KDF, which this // converter does not implement yet. The layer stack carries // LUKS (rdLayerStack reads it off the replicas when the RD - // row leaves it empty), but the volume cannot be opened until - // the passphrase is provisioned into blockstor by hand. - c.warnf("resource definition %s: LUKS passphrase NOT migrated (encrypted with the LINSTOR master key) — provision spec.encryption manually before adopting", dsp) + // row leaves it empty), but blockstor opens every LUKS volume with the + // cluster passphrase in its Secret, which is not this volume's + // key, so the volume cannot be opened until that passphrase is + // added to it as a key slot. + c.warnf("resource definition %s: LUKS key NOT migrated (encrypted with the LINSTOR master key); "+ + "blockstor opens LUKS volumes with the cluster passphrase, so add it to each volume with "+ + "`cryptsetup luksAddKey` before adopting", dsp) } } diff --git a/pkg/linstormigrate/convert_test.go b/pkg/linstormigrate/convert_test.go index 18f5ee73..d94db9be 100644 --- a/pkg/linstormigrate/convert_test.go +++ b/pkg/linstormigrate/convert_test.go @@ -794,8 +794,8 @@ func TestLuksPassphraseWarning(t *testing.T) { t.Errorf("pvc-vol3 layerStack = %v, want [DRBD LUKS STORAGE]", vol3.Spec.LayerStack) } - if !hasWarning(res, "pvc-vol3: LUKS passphrase NOT migrated") { - t.Errorf("LUKS passphrase warning missing; warnings: %v", res.Warnings) + if !hasWarning(res, "pvc-vol3: LUKS key NOT migrated") || !hasWarning(res, "cryptsetup luksAddKey") { + t.Errorf("LUKS key warning missing; warnings: %v", res.Warnings) } } diff --git a/pkg/linstormigrate/props.go b/pkg/linstormigrate/props.go index 8d85360c..004f2600 100644 --- a/pkg/linstormigrate/props.go +++ b/pkg/linstormigrate/props.go @@ -21,6 +21,8 @@ package linstormigrate import ( "maps" "strings" + + "github.com/cozystack/blockstor/pkg/passphrase" ) // PropsIndex groups the flat PROPS_CONTAINERS rows by owner object. @@ -56,7 +58,10 @@ type PropsIndex struct { // every pool that does not override it. const PropsInstanceController = "/CTRL" -// NewPropsIndex builds the lookup from the raw table rows. +// NewPropsIndex builds the lookup from the raw table rows. A row that would +// carry a LUKS passphrase in a property is left out: blockstor never reads +// one, and the generated YAML is applied with kubectl, past the REST guard, +// so the value would otherwise land in plaintext on a CR. func NewPropsIndex(rows []PropsContainerRow) *PropsIndex { idx := &PropsIndex{ Controller: map[string]string{}, @@ -64,6 +69,10 @@ func NewPropsIndex(rows []PropsContainerRow) *PropsIndex { } for _, row := range rows { + if passphrase.IsPlaintextPropKey(row.PropKey) { + continue + } + if row.PropsInstance == PropsInstanceController { idx.Controller[row.PropKey] = row.PropValue diff --git a/pkg/linstormigrate/props_passphrase_test.go b/pkg/linstormigrate/props_passphrase_test.go new file mode 100644 index 00000000..3e3e4646 --- /dev/null +++ b/pkg/linstormigrate/props_passphrase_test.go @@ -0,0 +1,50 @@ +// 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 linstormigrate + +import "testing" + +// The generated YAML is applied with kubectl, past the REST guard, so a +// passphrase LINSTOR kept in a property never makes it into the index the +// converter copies properties from, under any of its keys. +func TestPropsIndexLeavesPassphrasePropertiesOut(t *testing.T) { + t.Parallel() + + idx := NewPropsIndex([]PropsContainerRow{ + {PropsInstance: PropsInstanceController, PropKey: "DrbdOptions/EncryptPassphrase", PropValue: "ctrl-secret"}, + {PropsInstance: PropsInstanceController, PropKey: "DrbdOptions/Net/max-buffers", PropValue: "8000"}, + {PropsInstance: "/RSC_DFNS/PVC-A", PropKey: "DrbdOptions/Encryption/passphrase", PropValue: "rd-secret"}, + {PropsInstance: "/VLM_DFNS/PVC-A/0", PropKey: "DrbdOptions/Encrypt/Passphrase", PropValue: "vd-secret"}, + {PropsInstance: "/RSCS/NODE-A/PVC-A", PropKey: "LuksPassphrase", PropValue: "rsc-secret"}, + }) + + if _, ok := idx.Controller["DrbdOptions/EncryptPassphrase"]; ok || idx.Controller["DrbdOptions/Net/max-buffers"] != "8000" { + t.Errorf("controller props = %v, want the passphrase left out and the rest kept", idx.Controller) + } + + for name, bag := range map[string]map[string]string{ + "definition": idx.ResourceDefinition("PVC-A"), + "volume": idx.VolumeDefinition("PVC-A", "0"), + "replica": idx.Resource("NODE-A", "PVC-A"), + } { + if len(bag) != 0 { + t.Errorf("%s props = %v, want the passphrase left out", name, bag) + } + } +} diff --git a/pkg/linstormigrate/testdata/golden/report.txt b/pkg/linstormigrate/testdata/golden/report.txt index d23815d0..40ca0f9f 100644 --- a/pkg/linstormigrate/testdata/golden/report.txt +++ b/pkg/linstormigrate/testdata/golden/report.txt @@ -1,7 +1,7 @@ converted: 3 nodes, 4 storage pools, 3 resource groups, 6 resource definitions, 9 resources, 1 snapshots warning: volume definition pvc-vol1/0: unhandled flags bitmask 32 dropped warning: resource definition pvc-vol3: no DRBD port in the dump or live map — blockstor will allocate a fresh port; the adopted mesh will reconnect on the new port. Capture live ports (see runbook) to avoid the blip. -warning: resource definition pvc-vol3: LUKS passphrase NOT migrated (encrypted with the LINSTOR master key) — provision spec.encryption manually before adopting +warning: resource definition pvc-vol3: LUKS key NOT migrated (encrypted with the LINSTOR master key); blockstor opens LUKS volumes with the cluster passphrase, so add it to each volume with `cryptsetup luksAddKey` before adopting warning: resource definition pvc-vol4: marked DELETE in the source cluster — skipped warning: resource definition pvc-vol5: no DRBD port in the dump or live map — blockstor will allocate a fresh port; the adopted mesh will reconnect on the new port. Capture live ports (see runbook) to avoid the blip. warning: resource definition pvc-vol6: resource group "SC-GHOST" was not migrated — reference cleared From 41fee60fc2c642d14c0d51a307fd0fd11f736f06 Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Mon, 26 Oct 2026 09:59:40 +0100 Subject: [PATCH 15/24] test(e2e): refuse vd modify-passphrase and rotation over a LUKS volume The L6 cell and the L7 replay now assert both refusals, and the cell checks that the passphrase still opens the volume afterwards. The vd door is named modify-passphrase, as python-linstor spells it. Assisted-by: LLM Signed-off-by: Andrei Kvapil --- docs/cli-parity-known-deltas.md | 2 +- docs/layer-stack.md | 2 +- pkg/passphrase/passphrase.go | 2 +- pkg/rest/vd_passphrase_bug_233.go | 2 +- pkg/rest/vd_passphrase_bug_233_test.go | 2 +- pkg/rest/volume_definitions.go | 2 +- .../encryption-passphrase-luks-rd.sh | 18 +++++++++++++++ .../replay/encryption-passphrase-luks-rd.yaml | 22 +++++++++++++++++++ 8 files changed, 46 insertions(+), 6 deletions(-) diff --git a/docs/cli-parity-known-deltas.md b/docs/cli-parity-known-deltas.md index 16bf667c..879639d8 100644 --- a/docs/cli-parity-known-deltas.md +++ b/docs/cli-parity-known-deltas.md @@ -59,7 +59,7 @@ Row IDs match the command-catalogue indexes used by `cli-parity-refresh.sh` (see | 88 | `rg vg c` / `rg vg m` / `rg c` with volume-group `flags` | MISSING_FEATURE | permanent | Upstream's one volume-group flag is `GROSS_SIZE`, which sizes a spawned volume gross. Nothing in blockstor sizes a spawned volume gross, so both volume-group doors answer 501 to a body that sets a flag and write nothing, the props half included, rather than a 200 for a request that changes nothing. The volume-group create door and `rg c`, whose body can carry volume groups of its own, refuse any `flags`, so a flag no longer reaches the store; on main it was stored, listed by `rg vg l`, and did nothing. The modify door honours removals, spelled `-` as golinstor's `VolumeGroupModify` documents, since that is the only way to clear a flag a volume group created before this already carries; a body that mixes a removal with a set is refused whole. Pinned by `pkg/rest/vg_modify_golinstor_test.go`. | | 89 | `delete_namespaces` on every props-modify door | BEHAVIOR | permanent | Recorded so it is not reopened. As upstream's `PropsContainer.removeNamespace` does, a namespace removes the keys below it and leaves a key spelled exactly as the namespace: `DrbdOptions` takes `DrbdOptions/Net/protocol` and keeps `DrbdOptions`. Trailing slashes name the same namespace, however many there are. Against main this both widens and narrows the delete. Wider: a trailing-slash entry deleted nothing on most doors (the node-connection door trimmed one slash), and a multi-slash entry such as `Aux//` deleted nothing on any door; now both delete the keys below `Aux`. Narrower: the volume-definition, per-resource volume and node-connection doors deleted a key spelled exactly like the namespace, and now keep it. One spelling still differs from upstream: an entry of `/` alone, which upstream reads as the root namespace and so clears every prop, names nothing here and deletes nothing, the safe side of the difference. Every door and every guard in front of one read it through the same helper (`store.NamespaceCovers`), so a guard in front of a door refuses every spelling the delete honours. | | 90 | `controller set-property DrbdOptions/EncryptPassphrase ` (and `DrbdOptions/Encryption/passphrase` / `LuksPassphrase` on any props door) | BEHAVIOR | permanent | BLOCKSTOR_STRICTER. Upstream LINSTOR accepts the LUKS master passphrase as a controller property. blockstor keeps it only in the Secret `encryption create-passphrase` writes and never reads it from a property: a property sits in plaintext on its CR, so any dump of the CRs for a backup would carry the key to every LUKS volume. Any POST/PUT/PATCH whose JSON body stores one of those keys, matched without regard to case, answers 400 naming `encryption create-passphrase`, and nothing is stored; the bundled CLI's `set-property` refuses the same write. Deleting such a property (a `delete_props` entry, or an override to the empty string) stays allowed. Pinned by `pkg/rest/passphrase_props_guard_test.go` and `replay/encryption-passphrase-luks-rd.yaml`. | -| 91 | `vd set-passphrase ` (`PUT .../volume-definitions/{vlmNr}/encryption-passphrase`) | BEHAVIOR | until per-volume keys land | BLOCKSTOR_STRICTER. Upstream keeps a per-volume LUKS key wrapped by the master passphrase. blockstor has no per-volume keys, every LUKS volume uses the cluster passphrase, and the door used to store the value in plaintext in the volume definition's properties where nothing read it. It now answers 400 naming `encryption create-passphrase`, and `DrbdOptions/Encrypt/Passphrase` is refused on every props door like row 90. Pinned by `pkg/rest/vd_passphrase_bug_233_test.go`. | +| 91 | `vd modify-passphrase ` (`PUT .../volume-definitions/{vlmNr}/encryption-passphrase`) | BEHAVIOR | until per-volume keys land | BLOCKSTOR_STRICTER. Upstream keeps a per-volume LUKS key wrapped by the master passphrase. blockstor has no per-volume keys, every LUKS volume uses the cluster passphrase, and the door used to store the value in plaintext in the volume definition's properties where nothing read it. It now answers 400 naming `encryption create-passphrase`, and `DrbdOptions/Encrypt/Passphrase` is refused on every props door like row 90. Pinned by `pkg/rest/vd_passphrase_bug_233_test.go`. | | 92 | `encryption modify-passphrase` while LUKS volumes exist | BEHAVIOR | until re-keying lands | BLOCKSTOR_STRICTER. Upstream rotates the master passphrase that wraps the volume keys, so volumes keep opening. In blockstor the cluster passphrase is the LUKS key itself and nothing runs `cryptsetup luksChangeKey`, so a rotation would leave every existing LUKS volume unable to open. It answers 409 naming one LUKS definition while any exists. Pinned by `pkg/rest/encryption_rotation_luks_test.go`. | | 07 | `rd l --resource-definitions ` (Layers column on a placed RD) | WIRE_SHAPE | permanent | Same `stampRDLayerDataFromStack` behaviour as row 81 — the flag-qualified `rd l --resource-definitions ` catalogue cell (harness index 07) the bare `rd l` whitelist string does not literally cover. BS re-synthesises `layer_data: [{"type":"DRBD"},{"type":"STORAGE"}]` from `Spec.LayerStack` on every RD read, so the python CLI's Layers column renders `DRBD,STORAGE` even for a fresh `parity-rd` with only a volume-definition; upstream 1.33.2 leaves the column blank until DRBD layer data is actually allocated. BLOCKSTOR_SUPERSET, operator-friendly, and linstor-csi / piraeus-operator do not gate on RD `layer_data`. See row 81 for the full rationale and the L1/L3 pins (`stampRDLayerDataFromStack`, `tests/contract/normalize_test.go::TestNormalizeRDLayerDataDropped`). | diff --git a/docs/layer-stack.md b/docs/layer-stack.md index f41322ba..7a257a96 100644 --- a/docs/layer-stack.md +++ b/docs/layer-stack.md @@ -130,7 +130,7 @@ spec: with LUKS-over-DRBD instead of DRBD-over-LUKS) aren't yet validated or rendered. - **Per-volume LUKS keys**: every LUKS volume in the cluster uses the - cluster master passphrase. `linstor vd set-passphrase` answers 400: + cluster master passphrase. `linstor vd modify-passphrase` answers 400: it used to store the value in the volume definition's properties, in plaintext, where nothing read it. Per-volume keys (with master-key wrapping) are a follow-up. diff --git a/pkg/passphrase/passphrase.go b/pkg/passphrase/passphrase.go index 22219074..e721c3a6 100644 --- a/pkg/passphrase/passphrase.go +++ b/pkg/passphrase/passphrase.go @@ -70,7 +70,7 @@ const PropKeyCanonical = "DrbdOptions/EncryptPassphrase" const PropKeyLegacy = "DrbdOptions/Encryption/passphrase" // PropKeyVolume is upstream LINSTOR's per-volume passphrase property, -// stamped by `linstor vd set-passphrase`. blockstor has no per-volume +// stamped by `linstor vd modify-passphrase`. blockstor has no per-volume // keys: every LUKS volume uses the cluster passphrase, so the door that // wrote it is refused and the key is refused and dropped like the others. const PropKeyVolume = "DrbdOptions/Encrypt/Passphrase" diff --git a/pkg/rest/vd_passphrase_bug_233.go b/pkg/rest/vd_passphrase_bug_233.go index 932ed783..5601309c 100644 --- a/pkg/rest/vd_passphrase_bug_233.go +++ b/pkg/rest/vd_passphrase_bug_233.go @@ -23,7 +23,7 @@ import ( "strconv" ) -// handleVDPassphraseRotate serves `linstor vd set-passphrase` (PUT +// handleVDPassphraseRotate serves `linstor vd modify-passphrase` (PUT // .../volume-definitions/{vlmNr}/encryption-passphrase) and refuses it. // blockstor has no per-volume LUKS keys: every LUKS volume is formatted // and opened with the cluster passphrase in the Secret. The door used to diff --git a/pkg/rest/vd_passphrase_bug_233_test.go b/pkg/rest/vd_passphrase_bug_233_test.go index 7ec77f05..7a24c8b3 100644 --- a/pkg/rest/vd_passphrase_bug_233_test.go +++ b/pkg/rest/vd_passphrase_bug_233_test.go @@ -29,7 +29,7 @@ import ( "github.com/cozystack/blockstor/pkg/store" ) -// `linstor vd set-passphrase` (PUT .../volume-definitions/{vlmNr}/ +// `linstor vd modify-passphrase` (PUT .../volume-definitions/{vlmNr}/ // encryption-passphrase) is refused. blockstor has no per-volume LUKS // keys: every LUKS volume uses the cluster passphrase in the Secret. The // door used to store the value in the volume definition's properties, in diff --git a/pkg/rest/volume_definitions.go b/pkg/rest/volume_definitions.go index 4eb98d9e..95eda7d2 100644 --- a/pkg/rest/volume_definitions.go +++ b/pkg/rest/volume_definitions.go @@ -104,7 +104,7 @@ func (s *Server) registerVolumeDefinitions(mux *http.ServeMux) { // (`{"new_passphrase":"…"}`). We also accept the bare-string // `PassPhraseEnter` shape symmetric with the Bug 173 dual-form // cluster-passphrase PATCH. Pre-fix this 404'd, breaking - // `linstor vd set-passphrase` entirely. Path uses `{vlmNr}` to + // `linstor vd modify-passphrase` entirely. Path uses `{vlmNr}` to // match the upstream OpenAPI spec. mux.HandleFunc( "PUT /v1/resource-definitions/{rd}/volume-definitions/{vlmNr}/encryption-passphrase", diff --git a/tests/e2e/cli-matrix/encryption-passphrase-luks-rd.sh b/tests/e2e/cli-matrix/encryption-passphrase-luks-rd.sh index 6bcf0bf5..ae0f01c8 100755 --- a/tests/e2e/cli-matrix/encryption-passphrase-luks-rd.sh +++ b/tests/e2e/cli-matrix/encryption-passphrase-luks-rd.sh @@ -212,4 +212,22 @@ for node in "$N1" "$N2"; do fi done +echo ">> [Bug 023] vd modify-passphrase is REFUSED (no per-volume keys)" +if "${LCTL[@]}" volume-definition modify-passphrase "$RD" 0 'cli-matrix-023-vd-decoy' >/dev/null 2>&1; then + echo "FAIL (Bug 023): vd modify-passphrase was accepted; it stored a plaintext per-volume key nothing reads" >&2 + exit 1 +fi + +echo ">> [Bug 023] encryption modify-passphrase is REFUSED while the LUKS volume exists" +if "${LCTL[@]}" encryption modify-passphrase --old-passphrase "$PASSPHRASE" \ + --new-passphrase 'cli-matrix-023-rotated' >/dev/null 2>&1; then + echo "FAIL (Bug 023): the cluster passphrase rotated under a LUKS volume nothing re-keys" >&2 + exit 1 +fi +backing=$(luks_backing_device "$RD" "$N1" 0) +if ! assert_luks_passphrase_opens "$N1" "$backing" "$PASSPHRASE"; then + echo "FAIL (Bug 023): the passphrase no longer opens $N1:$backing after the refused rotation" >&2 + exit 1 +fi + echo ">> encryption-passphrase-luks-rd OK (Bug 023: Secret-only passphrase provisions LUKS end-to-end, no legacy prop)" diff --git a/tests/operator-harness/replay/encryption-passphrase-luks-rd.yaml b/tests/operator-harness/replay/encryption-passphrase-luks-rd.yaml index 66ea4d1d..954988bb 100644 --- a/tests/operator-harness/replay/encryption-passphrase-luks-rd.yaml +++ b/tests/operator-harness/replay/encryption-passphrase-luks-rd.yaml @@ -18,6 +18,10 @@ description: | the LuksPassphrase wire prop (a gate-only patch would pass step 3 but loop on "LUKS in layer stack but Props.LuksPassphrase empty" and never converge) + 4a. vd modify-passphrase is REFUSED (exit 10): no per-volume keys, + and nothing lands in the volume definition's properties + 4b. encryption modify-passphrase is REFUSED (exit 10) while the + LUKS definition exists: nothing re-keys its volume 5. legacy prop STILL absent after provisioning replay/luks-encrypted-rd.yaml runs the same refusal ahead of a @@ -78,6 +82,24 @@ steps: - name: create-vd cmd: ["volume-definition", "create", "{{rd}}", "32M"] expect_exit: 0 + - name: vd-passphrase-refused + # No per-volume keys: every LUKS volume uses the cluster passphrase, + # and the door used to store this value in plaintext in the volume + # definition's properties. It answers 400 (exit 10) and stores + # nothing. + cmd: ["volume-definition", "modify-passphrase", "{{rd}}", "0", "replay-pp023-vd-decoy"] + # The runner's prop_value reads no volume definitions; the L1 test + # pkg/rest/vd_passphrase_bug_233_test.go pins that nothing is stored. + expect_exit: 10 + - name: cluster-passphrase-rotation-refused + # The cluster passphrase is the key this LUKS volume was formatted + # with and nothing re-keys it, so a rotation is refused while the + # definition exists: 409, exit 10. On a shared stand whose + # passphrase predates this workflow the old value does not match + # and the answer is 401, also exit 10; either way nothing rotates. + cmd: ["encryption", "modify-passphrase", "--old-passphrase", "replay-pp023-do-not-rely-on", + "--new-passphrase", "replay-pp023-rotated"] + expect_exit: 10 - name: auto-place-2-encrypted cmd: ["resource", "create", "--auto-place", "2", "--storage-pool={{sp}}", "{{rd}}"] expect_exit: 0 From 3eb902f1ec24e27fcfb492cd1f429d6a2f64f17a Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Mon, 26 Oct 2026 10:47:50 +0100 Subject: [PATCH 16/24] fix(satellite): log the passphrase properties it ignores on a LUKS definition A volume formatted with a property's key fails to open with the Secret's value, and nothing named the property. The satellite now logs the keys it dropped, never their values. Assisted-by: LLM Signed-off-by: Andrei Kvapil --- pkg/passphrase/passphrase.go | 18 ++++++-- pkg/passphrase/passphrase_props_test.go | 47 ++++++++++++++++++++ pkg/satellite/controllers/luks_passphrase.go | 10 ++++- 3 files changed, 71 insertions(+), 4 deletions(-) create mode 100644 pkg/passphrase/passphrase_props_test.go diff --git a/pkg/passphrase/passphrase.go b/pkg/passphrase/passphrase.go index e721c3a6..a85048f5 100644 --- a/pkg/passphrase/passphrase.go +++ b/pkg/passphrase/passphrase.go @@ -35,6 +35,7 @@ package passphrase import ( "context" + "slices" "strings" "github.com/cockroachdb/errors" @@ -94,13 +95,24 @@ func IsPlaintextPropKey(key string) bool { } // DropPlaintextProps removes every property that would carry the -// master passphrase from props. -func DropPlaintextProps(props map[string]string) { - for key := range props { +// master passphrase from props, and returns the keys it removed that +// held a value, sorted, for the caller to name in a log line. +func DropPlaintextProps(props map[string]string) []string { + var dropped []string + + for key, value := range props { if IsPlaintextPropKey(key) { delete(props, key) + + if value != "" { + dropped = append(dropped, key) + } } } + + slices.Sort(dropped) + + return dropped } // SecretName resolves the passphrase Secret's name from the diff --git a/pkg/passphrase/passphrase_props_test.go b/pkg/passphrase/passphrase_props_test.go new file mode 100644 index 00000000..ff76e22f --- /dev/null +++ b/pkg/passphrase/passphrase_props_test.go @@ -0,0 +1,47 @@ +// 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 passphrase + +import ( + "slices" + "testing" +) + +// Every passphrase key goes, in any case; the keys that held a value come +// back sorted for the log line, and an empty one is dropped silently. +func TestDropPlaintextPropsNamesWhatItDropped(t *testing.T) { + t.Parallel() + + props := map[string]string{ + "drbdoptions/ENCRYPTPASSPHRASE": "a", + PropKeyLegacy: "b", + PropKeyVolume: "", + "DrbdOptions/Net/max-buffers": "8000", + } + + got := DropPlaintextProps(props) + + if want := []string{PropKeyLegacy, "drbdoptions/ENCRYPTPASSPHRASE"}; !slices.Equal(got, want) { + t.Errorf("dropped %v, want %v", got, want) + } + + if len(props) != 1 || props["DrbdOptions/Net/max-buffers"] != "8000" { + t.Errorf("props left %v, want only the unrelated key", props) + } +} diff --git a/pkg/satellite/controllers/luks_passphrase.go b/pkg/satellite/controllers/luks_passphrase.go index 7791f3aa..5e7145f2 100644 --- a/pkg/satellite/controllers/luks_passphrase.go +++ b/pkg/satellite/controllers/luks_passphrase.go @@ -67,12 +67,20 @@ func (r *ResourceReconciler) injectLUKSMasterPassphrase(ctx context.Context, rd // A passphrase in a property never counts: whatever an operator put // there under any of its keys goes, so only the Secret's value can // reach the LUKS layer. - passphrase.DropPlaintextProps(effectiveProps) + ignored := passphrase.DropPlaintextProps(effectiveProps) if rd == nil || effectiveProps == nil || !layerStackHasLUKS(rd.Spec.LayerStack) { return } + if len(ignored) > 0 { + // Key names only, never the values. A volume formatted with one of + // these keys fails to open with the Secret's value; this line is + // what names the property it was formatted with. + log.FromContext(ctx).Info("ignoring LUKS passphrase properties; the passphrase Secret is the only source", + "resourceDefinition", rd.Name, "properties", ignored) + } + if r.Config.Namespace == "" { return } From 4b82209d117b0546cb0ee52c614d3702f8db9389 Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Mon, 26 Oct 2026 10:47:50 +0100 Subject: [PATCH 17/24] docs(luks): give the upgrade steps for a passphrase kept in a property Assisted-by: LLM Signed-off-by: Andrei Kvapil --- docs/layer-stack.md | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/docs/layer-stack.md b/docs/layer-stack.md index 7a257a96..c1bb3cf2 100644 --- a/docs/layer-stack.md +++ b/docs/layer-stack.md @@ -123,6 +123,39 @@ spec: after the storage layer has resized the underlying LV, before DRBD resizes the replicated device. +### Upgrading a cluster that kept the passphrase in a property + +Before this release a passphrase property won over the Secret: `DrbdOptions/EncryptPassphrase` on the controller, or `DrbdOptions/Encryption/passphrase` on a group or a definition. LUKS volumes created while one was set were formatted with its value. From this release on the satellite opens them with the Secret's value only, and logs `ignoring LUKS passphrase properties` with the keys it dropped. Do this before the upgrade. + +1. Find the properties. Their values are redacted on the REST API, so read the CRs: + + ```sh + kubectl get controllerconfigs.blockstor.cozystack.io -o jsonpath='{.items[*].spec.extraProps}' + kubectl get resourcegroups.blockstor.cozystack.io,resourcedefinitions.blockstor.cozystack.io -o json \ + | jq -r '.items[] | select(.spec.props // {} | keys | any(test("passphrase|Passphrase"))) | .metadata.name' + ``` + +2. Make the Secret open every LUKS volume: + - **A controller property and no Secret**: run `linstor encryption create-passphrase` with the property's exact value. + - **A controller property set from the start, and a Secret with another value**: no LUKS volume was formatted with the Secret's value, so overwrite the Secret with the property's value. The API refuses this (create answers 409 because a passphrase exists, modify answers 409 because LUKS volumes exist), so do it with kubectl: + + ```sh + kubectl -n blockstor-system create secret generic blockstor-cluster-passphrase \ + --from-literal=passphrase='' --dry-run=client -o yaml | kubectl apply -f - + ``` + + - **Per-definition or per-group values, or a Secret that some volumes were formatted with**: the volumes use different keys, so no single Secret value opens them all. Add the Secret's value to each LUKS device that was formatted with another key, on every node with a replica, with `cryptsetup luksAddKey `; it asks for the key the device has now. + +3. Delete the properties, so the plaintext value leaves the CRs and the next backup: + + ```sh + linstor controller set-property DrbdOptions/EncryptPassphrase "" + linstor resource-group set-property DrbdOptions/Encryption/passphrase "" + linstor resource-definition set-property DrbdOptions/Encryption/passphrase "" + ``` + +A volume missed in step 2 fails to open on the next satellite restart or mapper close; the satellite log line names the property it was formatted with. + ## What blockstor doesn't yet support - **Pluggable layer ordering**: the stack must be one of the four From 24d0f7ec9ce2436bd3bc7fbb2b95ffd922c6d3eb Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Tue, 27 Oct 2026 10:12:55 +0100 Subject: [PATCH 18/24] fix(store): keep passphrase properties out of every server-side copy Clone, spawn, snapshot and restore copy a definition's or a group's properties through TravellingProps, which dropped only the restore markers. A group that kept a passphrase property put the plaintext key into every definition spawned from it. TravellingProps now drops the passphrase keys too. Assisted-by: LLM Signed-off-by: Andrei Kvapil --- pkg/rest/group_server_owned_props_test.go | 49 +++++++++++++++++++++++ pkg/store/local_props.go | 7 ++++ 2 files changed, 56 insertions(+) diff --git a/pkg/rest/group_server_owned_props_test.go b/pkg/rest/group_server_owned_props_test.go index 5f153c33..70fa577f 100644 --- a/pkg/rest/group_server_owned_props_test.go +++ b/pkg/rest/group_server_owned_props_test.go @@ -24,6 +24,7 @@ import ( "testing" apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/passphrase" "github.com/cozystack/blockstor/pkg/store" ) @@ -78,6 +79,54 @@ func TestSpawnDoesNotInheritServerOwnedGroupProps(t *testing.T) { } } +// A passphrase property a group kept from before the Secret was the only +// source is not copied into what it spawns: nothing reads it, and the copy +// would put the plaintext key into every new definition and its backups. +func TestSpawnDoesNotCopyAGroupPassphraseProp(t *testing.T) { + t.Parallel() + + st := store.NewInMemory() + if err := st.ResourceGroups().Create(t.Context(), &apiv1.ResourceGroup{ + Name: "rg-pass", + Props: map[string]string{ + passphrase.PropKeyLegacy: "left-over-plaintext", + "DrbdOptions/Net/protocol": "C", + }, + }); err != nil { + t.Fatalf("seed the group: %v", err) + } + + base, stop := startServerWithStore(t, st) + defer stop() + + body, err := json.Marshal(apiv1.ResourceGroupSpawn{ResourceDefinitionName: "pvc-pass"}) + if err != nil { + t.Fatalf("marshal: %v", err) + } + + resp := httpPost(t, base+"/v1/resource-groups/rg-pass/spawn", body) + _ = resp.Body.Close() + + if resp.StatusCode != http.StatusCreated { + t.Fatalf("spawn = %d, want 201", resp.StatusCode) + } + + rd, err := st.ResourceDefinitions().Get(t.Context(), "pvc-pass") + if err != nil { + t.Fatalf("read the spawned definition: %v", err) + } + + for key := range rd.Props { + if passphrase.IsPlaintextPropKey(key) { + t.Errorf("spawned definition carries %s from its group", key) + } + } + + if rd.Props["DrbdOptions/Net/protocol"] != "C" { + t.Errorf("spawned definition lost an ordinary group prop: %v", rd.Props) + } +} + func TestGroupCreateRefusesServerOwnedProps(t *testing.T) { t.Parallel() diff --git a/pkg/store/local_props.go b/pkg/store/local_props.go index 630008d1..1dc85ce5 100644 --- a/pkg/store/local_props.go +++ b/pkg/store/local_props.go @@ -26,6 +26,7 @@ import ( "strings" apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/passphrase" ) // RestoreFromSnapshotProp is the marker a restored or cloned definition @@ -166,6 +167,12 @@ func TravellingProps(props map[string]string) map[string]string { delete(out, CloneSnapshotOwnerProp) delete(out, CloneSnapshotReapingProp) + // A passphrase property left on a group or a definition from before the + // Secret was the only source is never read, and copying it would put the + // plaintext key into every definition a clone, a spawn, a snapshot or a + // restore creates, and into every backup of those. + passphrase.DropPlaintextProps(out) + return out } From 94c95438900c95e2f1207d2b847e3b82efab08c5 Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Tue, 27 Oct 2026 10:12:55 +0100 Subject: [PATCH 19/24] test(satellite): pin the log line that names an ignored passphrase property Assisted-by: LLM Signed-off-by: Andrei Kvapil --- .../luks_passphrase_internal_test.go | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/pkg/satellite/controllers/luks_passphrase_internal_test.go b/pkg/satellite/controllers/luks_passphrase_internal_test.go index 5349df22..13d95783 100644 --- a/pkg/satellite/controllers/luks_passphrase_internal_test.go +++ b/pkg/satellite/controllers/luks_passphrase_internal_test.go @@ -19,8 +19,11 @@ limitations under the License. package controllers import ( + "strings" "testing" + "github.com/go-logr/logr" + "github.com/go-logr/logr/funcr" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -276,3 +279,27 @@ func TestBug023InjectionHonoursSecretRef(t *testing.T) { t.Errorf("SecretRef lookup: got %q, want custom-secret", got) } } + +// The operator docs send someone whose LUKS volume no longer opens to the +// satellite log, so the line naming the ignored properties has to be there, +// with the keys and never the value. +func TestBug023IgnoredPassphrasePropsAreLogged(t *testing.T) { + t.Parallel() + + var lines []string + + logger := funcr.New(func(prefix, args string) { lines = append(lines, args) }, funcr.Options{}) + ctx := logr.NewContext(t.Context(), logger) + + r := newPassphraseTestReconciler(t) + r.injectLUKSMasterPassphrase(ctx, luksRD(), map[string]string{passphrase.PropKeyLegacy: "plaintext-canary"}) + + joined := strings.Join(lines, "\n") + if !strings.Contains(joined, "ignoring LUKS passphrase properties") || !strings.Contains(joined, passphrase.PropKeyLegacy) { + t.Errorf("log %q does not name the ignored property", joined) + } + + if strings.Contains(joined, "plaintext-canary") { + t.Errorf("log %q carries the passphrase value", joined) + } +} From 673256d51b8d0d6c08a1780884e5978b8b47c47b Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Tue, 27 Oct 2026 10:12:55 +0100 Subject: [PATCH 20/24] docs(luks): find passphrase properties where the store keeps them The upgrade search read only spec.props, and the store keeps a DrbdOptions key in spec.extraProps, so it found nothing on groups and definitions. It now reads both maps on groups, definitions and replicas, and the cleanup deletes every key it found, the controller's alias included. Assisted-by: LLM Signed-off-by: Andrei Kvapil --- docs/layer-stack.md | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/docs/layer-stack.md b/docs/layer-stack.md index c1bb3cf2..1bdefbb0 100644 --- a/docs/layer-stack.md +++ b/docs/layer-stack.md @@ -127,12 +127,12 @@ spec: Before this release a passphrase property won over the Secret: `DrbdOptions/EncryptPassphrase` on the controller, or `DrbdOptions/Encryption/passphrase` on a group or a definition. LUKS volumes created while one was set were formatted with its value. From this release on the satellite opens them with the Secret's value only, and logs `ignoring LUKS passphrase properties` with the keys it dropped. Do this before the upgrade. -1. Find the properties. Their values are redacted on the REST API, so read the CRs: +1. Find the properties. Their values are redacted on the REST API, so read the CRs. The store keeps a `DrbdOptions/*` key in `spec.extraProps`, not in `spec.props`, so read both maps, on groups, definitions and replicas, and the controller's own: ```sh kubectl get controllerconfigs.blockstor.cozystack.io -o jsonpath='{.items[*].spec.extraProps}' - kubectl get resourcegroups.blockstor.cozystack.io,resourcedefinitions.blockstor.cozystack.io -o json \ - | jq -r '.items[] | select(.spec.props // {} | keys | any(test("passphrase|Passphrase"))) | .metadata.name' + kubectl get resourcegroups.blockstor.cozystack.io,resourcedefinitions.blockstor.cozystack.io,resources.blockstor.cozystack.io -o json \ + | jq -r '.items[] | select((.spec.props // {}) + (.spec.extraProps // {}) | keys | any(test("passphrase"; "i"))) | "\(.kind)/\(.metadata.name)"' ``` 2. Make the Secret open every LUKS volume: @@ -144,16 +144,20 @@ Before this release a passphrase property won over the Secret: `DrbdOptions/Encr --from-literal=passphrase='' --dry-run=client -o yaml | kubectl apply -f - ``` - - **Per-definition or per-group values, or a Secret that some volumes were formatted with**: the volumes use different keys, so no single Secret value opens them all. Add the Secret's value to each LUKS device that was formatted with another key, on every node with a replica, with `cryptsetup luksAddKey `; it asks for the key the device has now. + - **Per-group, per-definition or per-replica values, or a Secret that some volumes were formatted with**: the volumes use different keys, so no single Secret value opens them all. Add the Secret's value to each LUKS device that was formatted with another key, on every node with a replica, with `cryptsetup luksAddKey `; it asks for the key the device has now. -3. Delete the properties, so the plaintext value leaves the CRs and the next backup: +3. Delete every key step 1 found, on the object it was found on, so the plaintext value leaves the CRs and the next backup. The controller can hold the canonical key and the alias both: ```sh linstor controller set-property DrbdOptions/EncryptPassphrase "" - linstor resource-group set-property DrbdOptions/Encryption/passphrase "" - linstor resource-definition set-property DrbdOptions/Encryption/passphrase "" + linstor controller set-property DrbdOptions/Encryption/passphrase "" + linstor resource-group set-property "" + linstor resource-definition set-property "" + linstor resource set-property "" ``` + Groups and definitions no longer pass such a key on to what is cloned, spawned, snapshotted or restored from them, but the copies made before the upgrade carry it, so step 1 finds those too. + A volume missed in step 2 fails to open on the next satellite restart or mapper close; the satellite log line names the property it was formatted with. ## What blockstor doesn't yet support From 534fcdf84b79cfada406e0956f83ae66d109e8c1 Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Tue, 27 Oct 2026 19:56:08 +0100 Subject: [PATCH 21/24] fix(rest): ask the API server for LUKS volumes before a passphrase rotation The guard listed definitions from the cache, so one created a moment ago could be missed and the rotation, which cannot be undone, let through. It now lists them through the server's direct reader. The migration runbook gains the step that gives each LUKS volume the blockstor cluster passphrase while LINSTOR still has it open, reading the volume key from the open mapping, and says what to do when the key sits in the kernel keyring and cannot be read back. Assisted-by: LLM Signed-off-by: Andrei Kvapil --- docs/linstor-migration.md | 21 +++++++++- pkg/rest/encryption.go | 51 +++++++++++++++++++---- pkg/rest/encryption_rotation_luks_test.go | 41 ++++++++++++++++++ pkg/rest/server.go | 3 +- 4 files changed, 106 insertions(+), 10 deletions(-) diff --git a/docs/linstor-migration.md b/docs/linstor-migration.md index 58ea491b..321ef459 100644 --- a/docs/linstor-migration.md +++ b/docs/linstor-migration.md @@ -10,7 +10,7 @@ Migrated: nodes, storage pools, resource groups, resource definitions (+ volume **NOT migrated — read the report on stderr for the exact list per cluster:** -- **LUKS passphrases.** LUKS-encrypted volumes convert with their layer stack intact (an RD whose `layer_stack` column is empty takes the stack its replicas were built with, read from `LAYER_RESOURCE_IDS`), but the per-volume key (encrypted with the LINSTOR master key) is not decrypted. blockstor opens every LUKS volume with the cluster passphrase kept in its Secret, so add that passphrase to each such volume as a key slot (`cryptsetup luksAddKey`) before adopting it. Each affected RD is reported. +- **LUKS passphrases.** LUKS-encrypted volumes convert with their layer stack intact (an RD whose `layer_stack` column is empty takes the stack its replicas were built with, read from `LAYER_RESOURCE_IDS`), but the per-volume key (encrypted with the LINSTOR master key) is not decrypted. blockstor opens every LUKS volume with the cluster passphrase kept in its Secret, so add that passphrase to each such volume as a key slot before adopting it: pre-flight step 7 says how. Each affected RD is reported. - **Definitions whose replicas disagree on the layer stack** are skipped with their replicas and reported: replicas built two ways, or with their layers in another order, under a definition with no stack of its own; a diskful replica the dump shows no layer chain for, under such a definition; or a replica built otherwise than the stack the definition names. Adopted as they are, such a replica would be brought up under a stack it was not built with, DRBD metadata over a LUKS volume's ciphertext for one. Settle the stack in LINSTOR and convert again. - **Backup-shipping remotes**, unknown/runtime flag bits, and DELETE'd / FAILED_DEPLOYMENT rows are skipped and reported. None affect live data availability. - **DRBD TCP ports** are absent from most LINSTOR 1.33 dumps — see "Capture live DRBD ports" below. @@ -68,6 +68,25 @@ Migrated: nodes, storage pools, resource groups, resource definitions (+ volume 6. **Rehearse on staging first.** Do a full dry-run adoption of a COPY of the pools (or a representative subset) on a non-production cluster: apply the manifests, confirm every StoragePool registers a provider (no `provider requires … in props` errors in the satellite log), every Resource reaches `UpToDate` with `out-of-sync=0` (adopted, not resynced), and no new empty zvols appear. Only after a clean staging rehearsal proceed to production. +7. **Give every LUKS volume the blockstor cluster passphrase, while LINSTOR still has it open.** LINSTOR formats each LUKS volume with a key of its own, kept in its database encrypted with the master passphrase, and the converter does not decrypt it. blockstor opens every LUKS volume with the cluster passphrase in its Secret, so that passphrase has to be a key slot on each volume before adoption. Choose the cluster passphrase now, and put that exact value into blockstor's passphrase Secret before adoption (`linstor encryption create-passphrase` against blockstor, or the Secret by hand). Then, on a node where LINSTOR has the volume open: + + ```bash + dmsetup ls --target crypt # find the mapper of the volume + dmsetup table --showkeys # field 5 is the volume key + ``` + + If field 5 is a hex string, it is the volume key itself. Write it as binary to a file on tmpfs and add the cluster passphrase with it; `luksAddKey` asks for the new passphrase: + + ```bash + dmsetup table --showkeys | awk '{print $5}' | xxd -r -p > /run/vk.bin + cryptsetup luksAddKey --volume-key-file /run/vk.bin + shred -u /run/vk.bin + ``` + + The key slot lives in the LUKS header on that replica's backing device, so do this on every node with a diskful replica of the volume. Then check it, for each replica: `cryptsetup open --test-passphrase ` with the cluster passphrase. + + If field 5 starts with `:` (`:64:logon:cryptsetup:...`), the volume was opened through the kernel keyring, which cryptsetup 2 does for LUKS2 by default, and the key cannot be read back from userspace. Such a volume cannot be given the passphrase this way: copy its data to a new volume created under blockstor instead, or keep it on LINSTOR. The volume key is the secret that decrypts the volume, so treat the file and the shell history like the passphrase itself. + ## Convert ```bash diff --git a/pkg/rest/encryption.go b/pkg/rest/encryption.go index 3ac23bbe..56df76c6 100644 --- a/pkg/rest/encryption.go +++ b/pkg/rest/encryption.go @@ -34,6 +34,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" + blockstoriov1alpha1 "github.com/cozystack/blockstor/api/v1alpha1" apiv1 "github.com/cozystack/blockstor/pkg/api/v1" "github.com/cozystack/blockstor/pkg/passphrase" ) @@ -640,25 +641,59 @@ func (s *Server) rotatePassphrase(ctx context.Context, w http.ResponseWriter, wa // (no `cryptsetup luksChangeKey`), so a new value in the Secret would // leave every existing LUKS volume unable to open on its next start. func (s *Server) refuseRotationOverLUKSVolumes(ctx context.Context, w http.ResponseWriter) bool { - rds, err := s.Store.ResourceDefinitions().List(ctx) + name, err := s.aLUKSDefinition(ctx) if err != nil { writeStoreError(w, err) return false } + if name == "" { + return true + } + + writeError(w, http.StatusConflict, fmt.Sprintf( + "refusing to change the cluster passphrase while LUKS volumes exist (%s among them): "+ + "it is the key they were formatted with, and they are not re-keyed, so they would no longer open", + name)) + + return false +} + +// aLUKSDefinition names one definition with LUKS in its stack, or "" when +// there is none. It reads the API server directly when the server has a +// direct reader: a definition created a moment ago may not be in the cache +// yet, and the rotation it would let through cannot be undone. +func (s *Server) aLUKSDefinition(ctx context.Context) (string, error) { + if s.SecretReader != nil { + var list blockstoriov1alpha1.ResourceDefinitionList + + err := s.SecretReader.List(ctx, &list) + if err != nil { + return "", fmt.Errorf("list resource definitions: %w", err) + } + + for i := range list.Items { + if apiv1.LayerInStack(list.Items[i].Spec.LayerStack, apiv1.LayerKindLUKS) { + return list.Items[i].Name, nil + } + } + + return "", nil + } + + rds, err := s.Store.ResourceDefinitions().List(ctx) + if err != nil { + return "", err //nolint:wrapcheck // the store names what it failed to read + } + for i := range rds { if apiv1.LayerInStack(rds[i].LayerStack, apiv1.LayerKindLUKS) { - writeError(w, http.StatusConflict, fmt.Sprintf( - "refusing to change the cluster passphrase while LUKS volumes exist (%s among them): "+ - "it is the key they were formatted with, and they are not re-keyed, so they would no longer open", - rds[i].Name)) - - return false + return rds[i].Name, nil } } - return true + return "", nil } // readPassphrase reads the current cluster passphrase. Empty diff --git a/pkg/rest/encryption_rotation_luks_test.go b/pkg/rest/encryption_rotation_luks_test.go index 4d9a24a8..4d05f5ab 100644 --- a/pkg/rest/encryption_rotation_luks_test.go +++ b/pkg/rest/encryption_rotation_luks_test.go @@ -25,6 +25,9 @@ import ( "strings" "testing" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + blockstoriov1alpha1 "github.com/cozystack/blockstor/api/v1alpha1" apiv1 "github.com/cozystack/blockstor/pkg/api/v1" "github.com/cozystack/blockstor/pkg/store" ) @@ -68,3 +71,41 @@ func TestPassphraseModifyIsRefusedWhileLUKSVolumesExist(t *testing.T) { t.Errorf("the old passphrase no longer unlocks after a refused rotation: %d", oldResp.StatusCode) } } + +// The guard asks the API server, not the cache: a LUKS definition the +// cache has not seen yet still stops the rotation. +func TestPassphraseModifyAsksTheAPIServerForLUKSVolumes(t *testing.T) { + cli := newFakeRESTClient(t) + ctx := t.Context() + + if err := cli.Create(ctx, testPassphraseSecret("old-secret")); err != nil { + t.Fatalf("seed the Secret: %v", err) + } + + if err := cli.Create(ctx, &blockstoriov1alpha1.ResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{Name: "pvc-new-luks"}, + Spec: blockstoriov1alpha1.ResourceDefinitionSpec{ + LayerStack: []string{apiv1.LayerKindDRBD, apiv1.LayerKindLUKS, apiv1.LayerKindStorage}, + }, + }); err != nil { + t.Fatalf("seed the definition: %v", err) + } + + base, stop := startServerCustom(t, &Server{ + Addr: pickFreeAddr(t), + Store: store.NewInMemory(), + Client: cli, + SecretReader: cli, + Namespace: testRESTNamespace, + }) + defer stop() + + body, _ := json.Marshal(map[string]string{"old_passphrase": "old-secret", "new_passphrase": "new-secret"}) + resp := httpPut(t, base+"/v1/encryption/passphrase", body) + got, _ := io.ReadAll(resp.Body) + _ = resp.Body.Close() + + if resp.StatusCode != http.StatusConflict || !strings.Contains(string(got), "pvc-new-luks") { + t.Fatalf("modify: got %d %s, want 409 naming the definition the cache has not seen", resp.StatusCode, got) + } +} diff --git a/pkg/rest/server.go b/pkg/rest/server.go index d6eef190..97d6326f 100644 --- a/pkg/rest/server.go +++ b/pkg/rest/server.go @@ -83,7 +83,8 @@ type Server struct { // Client, the first Get starts an informer over every Secret in the // cluster, which needs cluster-wide list and watch on Secrets and holds // all of them in memory, for the one Secret this server reads. Nil - // falls back to Client, which tests rely on. + // falls back to Client, which tests rely on. It is the manager's direct + // reader, so the passphrase rotation also lists definitions through it. SecretReader client.Reader // TLS, when non-nil, makes Start also bring up a mutual-TLS From 5e3599e3225f95fb84740c0f2a5a69c4e330f903 Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Mon, 2 Nov 2026 11:11:45 +0100 Subject: [PATCH 22/24] docs(luks): find a passphrase on volume props and snapshots too The upgrade search missed the per-volume key vd modify-passphrase wrote into a definition's volume props, and the alias a snapshot of such a definition copied into its own props. It now reads both, the cleanup deletes the volume key with vd set-property and the snapshot's with a kubectl patch, and step 2 says to use the Secret name and namespace the ControllerConfig points at. Assisted-by: LLM Signed-off-by: Andrei Kvapil --- docs/layer-stack.md | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/docs/layer-stack.md b/docs/layer-stack.md index 1bdefbb0..9078779f 100644 --- a/docs/layer-stack.md +++ b/docs/layer-stack.md @@ -127,12 +127,12 @@ spec: Before this release a passphrase property won over the Secret: `DrbdOptions/EncryptPassphrase` on the controller, or `DrbdOptions/Encryption/passphrase` on a group or a definition. LUKS volumes created while one was set were formatted with its value. From this release on the satellite opens them with the Secret's value only, and logs `ignoring LUKS passphrase properties` with the keys it dropped. Do this before the upgrade. -1. Find the properties. Their values are redacted on the REST API, so read the CRs. The store keeps a `DrbdOptions/*` key in `spec.extraProps`, not in `spec.props`, so read both maps, on groups, definitions and replicas, and the controller's own: +1. Find the properties. Their values are redacted on the REST API, so read the CRs. The store keeps a `DrbdOptions/*` key in `spec.extraProps`, not in `spec.props`, and a volume's own props, where `vd modify-passphrase` wrote `DrbdOptions/Encrypt/Passphrase`, in `spec.volumeDefinitions[].props` of its definition. Snapshots taken of a definition that carried the alias copied it into their own `spec.props`. So read all three on groups, definitions, replicas and snapshots, and the controller's own: ```sh kubectl get controllerconfigs.blockstor.cozystack.io -o jsonpath='{.items[*].spec.extraProps}' - kubectl get resourcegroups.blockstor.cozystack.io,resourcedefinitions.blockstor.cozystack.io,resources.blockstor.cozystack.io -o json \ - | jq -r '.items[] | select((.spec.props // {}) + (.spec.extraProps // {}) | keys | any(test("passphrase"; "i"))) | "\(.kind)/\(.metadata.name)"' + kubectl get resourcegroups.blockstor.cozystack.io,resourcedefinitions.blockstor.cozystack.io,resources.blockstor.cozystack.io,snapshots.blockstor.cozystack.io -o json \ + | jq -r '.items[] | select([(.spec.props // {}), (.spec.extraProps // {}), (.spec.volumeDefinitions // [] | .[].props // {})] | add | keys | any(test("passphrase"; "i"))) | "\(.kind)/\(.metadata.name)"' ``` 2. Make the Secret open every LUKS volume: @@ -144,6 +144,8 @@ Before this release a passphrase property won over the Secret: `DrbdOptions/Encr --from-literal=passphrase='' --dry-run=client -o yaml | kubectl apply -f - ``` + The Secret is `blockstor-cluster-passphrase` in the controller's namespace unless the ControllerConfig's `spec.passphraseSecretRef` names another; use that name, and the controller's namespace, if you changed either. + - **Per-group, per-definition or per-replica values, or a Secret that some volumes were formatted with**: the volumes use different keys, so no single Secret value opens them all. Add the Secret's value to each LUKS device that was formatted with another key, on every node with a replica, with `cryptsetup luksAddKey `; it asks for the key the device has now. 3. Delete every key step 1 found, on the object it was found on, so the plaintext value leaves the CRs and the next backup. The controller can hold the canonical key and the alias both: @@ -154,6 +156,14 @@ Before this release a passphrase property won over the Secret: `DrbdOptions/Encr linstor resource-group set-property "" linstor resource-definition set-property "" linstor resource set-property "" + linstor volume-definition set-property "" + ``` + + A snapshot has no set-property. Remove the key from its CR, with `/` in the key written as `~1`, or delete the snapshot if it is no longer needed: + + ```sh + kubectl patch snapshots.blockstor.cozystack.io --type json \ + -p '[{"op":"remove","path":"/spec/props/DrbdOptions~1Encryption~1passphrase"}]' ``` Groups and definitions no longer pass such a key on to what is cloned, spawned, snapshotted or restored from them, but the copies made before the upgrade carry it, so step 1 finds those too. From ef42997dc677b6d97a20797de8ab56bf57157cab Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Mon, 2 Nov 2026 20:20:54 +0100 Subject: [PATCH 23/24] fix(spawn): keep passphrase properties off spawned volumes and CLI spawns A spawn filtered the group's own props but copied each volume group's props verbatim onto the volume definition it created, and the bundled CLI's spawn cloned the group's props whole. Either path put a passphrase property left from before the Secret was the only source onto every new definition. Both go through TravellingProps now. Assisted-by: LLM Signed-off-by: Andrei Kvapil --- internal/cli/place.go | 3 +- internal/cli/place_test.go | 38 ++++++++++++++++ pkg/rest/group_server_owned_props_test.go | 53 +++++++++++++++++++++++ pkg/rest/spawn.go | 8 ++-- 4 files changed, 96 insertions(+), 6 deletions(-) diff --git a/internal/cli/place.go b/internal/cli/place.go index e227029a..ee57c72c 100644 --- a/internal/cli/place.go +++ b/internal/cli/place.go @@ -22,7 +22,6 @@ import ( "context" "errors" "fmt" - "maps" "slices" "strings" @@ -323,7 +322,7 @@ func spawnDefinition(ctx context.Context, run *runContext, group *apiv1.Resource } if len(group.Props) > 0 { - def.Props = maps.Clone(group.Props) + def.Props = store.TravellingProps(group.Props) } err := run.Store.ResourceDefinitions().Create(ctx, def) diff --git a/internal/cli/place_test.go b/internal/cli/place_test.go index 135038e6..aca7eed8 100644 --- a/internal/cli/place_test.go +++ b/internal/cli/place_test.go @@ -24,6 +24,7 @@ import ( "testing" apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/passphrase" "github.com/cozystack/blockstor/pkg/store" "github.com/cozystack/blockstor/internal/cli" @@ -185,6 +186,43 @@ func TestResourceGroupSpawn(t *testing.T) { } } +// A passphrase property a group kept from before the Secret was the only +// source stays off what `rg spawn` creates, as it does on the REST spawn. +func TestResourceGroupSpawnDoesNotCopyAPassphraseProp(t *testing.T) { + t.Parallel() + + app, _, errBuf := newApp(t, func(ctx context.Context, backend store.Store) { + seedCluster(2)(ctx, backend) + _ = backend.ResourceGroups().Create(ctx, &apiv1.ResourceGroup{ + Name: "grp", + SelectFilter: apiv1.AutoSelectFilter{PlaceCount: 2, StoragePool: "data"}, + Props: map[string]string{ + passphrase.PropKeyLegacy: "left-over-plaintext", + "DrbdOptions/Net/protocol": "C", + }, + }) + }) + + if got := app.Run(t.Context(), []string{"rg", "spawn-resources", "grp", "pvc-x", "32M"}); got != 0 { + t.Fatalf("spawn exit = %d (stderr: %s)", got, errBuf.String()) + } + + def, err := appStore(t, app).ResourceDefinitions().Get(t.Context(), "pvc-x") + if err != nil { + t.Fatalf("get definition: %v", err) + } + + for key := range def.Props { + if passphrase.IsPlaintextPropKey(key) { + t.Errorf("spawned definition carries %s from its group", key) + } + } + + if def.Props["DrbdOptions/Net/protocol"] != "C" { + t.Errorf("spawned definition lost an ordinary group prop: %v", def.Props) + } +} + // Spawning from an over-committed group SUCCEEDS, and reports. Here // the group's place count is a target the rebalance reconciler keeps // working towards, so failing would break provisioning ahead of the diff --git a/pkg/rest/group_server_owned_props_test.go b/pkg/rest/group_server_owned_props_test.go index 70fa577f..46d7a5b4 100644 --- a/pkg/rest/group_server_owned_props_test.go +++ b/pkg/rest/group_server_owned_props_test.go @@ -127,6 +127,59 @@ func TestSpawnDoesNotCopyAGroupPassphraseProp(t *testing.T) { } } +// The same holds for a volume group's props, which a spawn copies onto the +// volume definition it creates for that volume number. +func TestSpawnDoesNotCopyAVolumeGroupPassphraseProp(t *testing.T) { + t.Parallel() + + st := store.NewInMemory() + if err := st.ResourceGroups().Create(t.Context(), &apiv1.ResourceGroup{ + Name: "rg-vgpass", + VolumeGroups: []apiv1.VolumeGroup{{ + VolumeNumber: 0, + Props: map[string]string{ + passphrase.PropKeyVolume: "left-over-plaintext", + "DrbdOptions/Disk/al-extents": "6007", + }, + }}, + }); err != nil { + t.Fatalf("seed the group: %v", err) + } + + base, stop := startServerWithStore(t, st) + defer stop() + + body, err := json.Marshal(apiv1.ResourceGroupSpawn{ + ResourceDefinitionName: "pvc-vgpass", + VolumeSizes: []int64{4096 * 1024}, + }) + if err != nil { + t.Fatalf("marshal: %v", err) + } + + resp := httpPost(t, base+"/v1/resource-groups/rg-vgpass/spawn", body) + _ = resp.Body.Close() + + if resp.StatusCode != http.StatusCreated { + t.Fatalf("spawn = %d, want 201", resp.StatusCode) + } + + vd, err := st.VolumeDefinitions().Get(t.Context(), "pvc-vgpass", 0) + if err != nil { + t.Fatalf("read the spawned volume: %v", err) + } + + for key := range vd.Props { + if passphrase.IsPlaintextPropKey(key) { + t.Errorf("spawned volume carries %s from its volume group", key) + } + } + + if vd.Props["DrbdOptions/Disk/al-extents"] != "6007" { + t.Errorf("spawned volume lost an ordinary volume group prop: %v", vd.Props) + } +} + func TestGroupCreateRefusesServerOwnedProps(t *testing.T) { t.Parallel() diff --git a/pkg/rest/spawn.go b/pkg/rest/spawn.go index d80a2f1a..0eba3f30 100644 --- a/pkg/rest/spawn.go +++ b/pkg/rest/spawn.go @@ -310,10 +310,10 @@ func copyVolumeGroupProps(vgs []apiv1.VolumeGroup, volNumber int32) map[string]s return nil } - out := make(map[string]string, len(vgs[i].Props)) - maps.Copy(out, vgs[i].Props) - - return out + // The same filter as the group's own props: a passphrase + // property left on a volume group is never read, and copying it + // would put the plaintext key on every spawned volume. + return store.TravellingProps(vgs[i].Props) } return nil From 71cc110e537e9059ea8009c3e145e0dfdbf6e5bb Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Mon, 2 Nov 2026 20:20:55 +0100 Subject: [PATCH 24/24] docs(luks): find and delete a passphrase on volume group props A volume group keeps its props in spec.volumeGroups[].props of its group, which the upgrade query did not read. Read it, and delete the key there with volume-group set-property. Assisted-by: LLM Signed-off-by: Andrei Kvapil --- docs/layer-stack.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/docs/layer-stack.md b/docs/layer-stack.md index 9078779f..efe31623 100644 --- a/docs/layer-stack.md +++ b/docs/layer-stack.md @@ -127,12 +127,12 @@ spec: Before this release a passphrase property won over the Secret: `DrbdOptions/EncryptPassphrase` on the controller, or `DrbdOptions/Encryption/passphrase` on a group or a definition. LUKS volumes created while one was set were formatted with its value. From this release on the satellite opens them with the Secret's value only, and logs `ignoring LUKS passphrase properties` with the keys it dropped. Do this before the upgrade. -1. Find the properties. Their values are redacted on the REST API, so read the CRs. The store keeps a `DrbdOptions/*` key in `spec.extraProps`, not in `spec.props`, and a volume's own props, where `vd modify-passphrase` wrote `DrbdOptions/Encrypt/Passphrase`, in `spec.volumeDefinitions[].props` of its definition. Snapshots taken of a definition that carried the alias copied it into their own `spec.props`. So read all three on groups, definitions, replicas and snapshots, and the controller's own: +1. Find the properties. Their values are redacted on the REST API, so read the CRs. The store keeps a `DrbdOptions/*` key in `spec.extraProps`, not in `spec.props`, and a volume's own props, where `vd modify-passphrase` wrote `DrbdOptions/Encrypt/Passphrase`, in `spec.volumeDefinitions[].props` of its definition, and a volume group's in `spec.volumeGroups[].props` of its group. Snapshots taken of a definition that carried the alias copied it into their own `spec.props`. So read all of them on groups, definitions, replicas and snapshots, and the controller's own: ```sh kubectl get controllerconfigs.blockstor.cozystack.io -o jsonpath='{.items[*].spec.extraProps}' kubectl get resourcegroups.blockstor.cozystack.io,resourcedefinitions.blockstor.cozystack.io,resources.blockstor.cozystack.io,snapshots.blockstor.cozystack.io -o json \ - | jq -r '.items[] | select([(.spec.props // {}), (.spec.extraProps // {}), (.spec.volumeDefinitions // [] | .[].props // {})] | add | keys | any(test("passphrase"; "i"))) | "\(.kind)/\(.metadata.name)"' + | jq -r '.items[] | select([(.spec.props // {}), (.spec.extraProps // {}), (.spec.volumeDefinitions // [] | .[].props // {}), (.spec.volumeGroups // [] | .[].props // {})] | add | keys | any(test("passphrase"; "i"))) | "\(.kind)/\(.metadata.name)"' ``` 2. Make the Secret open every LUKS volume: @@ -157,6 +157,7 @@ Before this release a passphrase property won over the Secret: `DrbdOptions/Encr linstor resource-definition set-property "" linstor resource set-property "" linstor volume-definition set-property "" + linstor volume-group set-property "" ``` A snapshot has no set-property. Remove the key from its CR, with `/` in the key written as `~1`, or delete the snapshot if it is no longer needed: @@ -166,7 +167,7 @@ Before this release a passphrase property won over the Secret: `DrbdOptions/Encr -p '[{"op":"remove","path":"/spec/props/DrbdOptions~1Encryption~1passphrase"}]' ``` - Groups and definitions no longer pass such a key on to what is cloned, spawned, snapshotted or restored from them, but the copies made before the upgrade carry it, so step 1 finds those too. + Groups, their volume groups and definitions no longer pass such a key on to what is cloned, spawned, snapshotted or restored from them, but the copies made before the upgrade carry it, so step 1 finds those too. A volume missed in step 2 fails to open on the next satellite restart or mapper close; the satellite log line names the property it was formatted with.