Repository navigation
fix(luks)!: read the master passphrase from the Secret only - #222
Andrei Kvapil (kvaps) wants to merge 20 commits into
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 44 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Andrei Kvapil (@kvaps) NOT LGTM — the code holds up, but nothing tells an operator how to upgrade a cluster whose LUKS volumes were formatted with a property passphrase, and the release note covers only one of the ways that could happen.
Business context: a LUKS master passphrase kept in a property sits in plaintext on its CR, so this PR makes the passphrase Secret the only source and refuses to store the key in any property.
I broke each guard once and every mutation fails a test: the satellite drop, the dispatcher drop and its maps.Clone, the REST middleware and its empty-value delete, the case-insensitive match, the CLI set-property, the migrate index, the rotation guard, the vd door and the CLI rd create gate. A wrong key does not turn into a reformat, because luks.Format probes isLuks first and the satellite fails on Open. Tests of the touched packages pass on the head and on a merge with #206, apart from the known pkg/rest 2s shutdown flake. The merge with #206 conflicts in pkg/linstormigrate/convert.go and docs/linstor-migration.md, both on the same LUKS warning text, so whichever PR lands second has to merge the two wordings.
Blocker: no upgrade step for volumes formatted with a property passphrase
Before this PR a property won over the Secret. The base luks_passphrase.go says "an operator-set controller prop (either spelling) wins over the Secret", and the base docs/layer-stack.md told operators to set DrbdOptions/Encryption/passphrase per definition and through rg sp encrypted-rg. The site text in #205 says the same for released versions. So a cluster can have LUKS volumes formatted with a property key in three ways:
- A controller property and no Secret.
- A property and a Secret with a different value.
- Per-definition aliases, each with its own value.
After this PR all three get the Secret's value, which TestBug023SecretReplacesEveryPassphraseProp pins. The release note says only "A passphrase set only in a property no longer unlocks LUKS volumes", and that sentence describes the first case.
The API has no way out of the second case. I checked it with a throwaway test in pkg/rest. With a Secret and a LUKS definition present, POST /v1/encryption/passphrase answers 409 "cluster passphrase already set; PUT to modify". A PUT to the property's value answers 409 "it is the key they were formatted with", which is not true on such a cluster. With no Secret, POST still answers 201 while a LUKS definition exists, so the first case has a path. An empty override_props value for DrbdOptions/EncryptPassphrase deletes the property (201).
What happens if it ships as is: on the next satellite restart or mapper close those volumes fail with Props.LuksPassphrase empty or a luks open error, and neither message names the property that was ignored. Nothing deletes the existing properties either, so the plaintext key this PR is about stays in every CR dump until someone removes it by hand.
The fix is text. Put the operator action into the release note and the docs, to be done before the upgrade:
- No Secret: run
encryption create-passphrasewith the property's exact value. - A Secret with another value, or per-definition aliases: add the Secret's value to every LUKS device as a key slot with
cryptsetup luksAddKey, or overwrite the Secret object with kubectl. The API refuses both create and modify there. - Then delete the properties with
set-property <key> "".
The release-note sentence also needs to cover a property that was set next to the Secret.
Non-blocking
pkg/satellite/controllers/luks_passphrase.go:70 drops a non-empty property silently. One log line or Warning Event naming the ignored key would make the failure above visible from the Resource.
pkg/rest/encryption.go:643 lists definitions and then writes the Secret, and nothing orders that against a concurrent LUKS rd create, whose own check only asks whether the Secret exists. A definition created and formatted inside that window keeps the old key. The window is narrow and rotation is rare, so I'm only noting it.
E2E lanes 1 to 6 are green on this head and the piraeus interop lane is still running. The L6/L7 cells were not run, as the PR body says.
|
Thanks, head a6ab601.
The satellite now logs the property keys it drops on a LUKS definition, names only. The rotation race I left as is. |
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 <kvapss@gmail.com>
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 <kvapss@gmail.com>
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 <kvapss@gmail.com>
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 <kvapss@gmail.com>
The guard refused any body that named a passphrase key as an object key, so `set-property <key> ""`, 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 <kvapss@gmail.com>
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 <kvapss@gmail.com>
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 <kvapss@gmail.com>
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 <kvapss@gmail.com>
… 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 <kvapss@gmail.com>
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 <kvapss@gmail.com>
Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
…ase 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 <kvapss@gmail.com>
…lone Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
…al 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 <kvapss@gmail.com>
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 <kvapss@gmail.com>
…finition 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 <kvapss@gmail.com>
Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
a6ab601 to
4772f98
Compare
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Andrei Kvapil (@kvaps) NOT LGTM. The new upgrade section has the right steps, but its search finds none of the group or definition properties set through the API, and it skips replicas. Also, four server-side copy paths still write a passphrase property into new CRs.
Points from my previous review
- Upgrade path for volumes formatted with a property passphrase: partly fixed. The release note, the PR body and the new section in
docs/layer-stack.mdcover the three cases and the cleanup. The search step does not work, see B1. - Log naming the ignored key: fixed, key names only. The helper is pinned by a test, the log call is not: with the
len(ignored) > 0branch disabled,./pkg/satellite/...stays green. The docs now send operators to this line, so a test that it fires would be good to have. - Rotation racing a concurrent LUKS
rd create: unchanged, as you said. Still non-blocking for me.
B1: the search in step 1 finds nothing on groups and definitions
The query reads only .spec.props, and the store never puts a DRBD option there. wireToCRDRDSpec and wireToCRDRGSpec store stripDRBDProps(in.Props) as Spec.Props, and propsToTyped sends an unknown DrbdOptions/* key to Spec.ExtraProps. The CLI set-property writes through the same store. I ran an RD and an RG carrying DrbdOptions/Encryption/passphrase through wireToCRDRD and wireToCRDRG. Both came out as {"extraProps":{"DrbdOptions/Encryption/passphrase":"..."}}, and the query from the docs printed nothing.
So an operator who follows the section gets an empty list and upgrades. Then the volumes formatted with a group or definition value fail to open, which is the failure the section exists to prevent.
Replicas are missing too. On the base, effectiveprops.Resolve merges the Resource scope last, so DrbdOptions/EncryptPassphrase on a replica beat the controller value. A LuksPassphrase in a replica's spec.props reached the wire when nothing else was set, and the PR body lists it as a source. Step 1 does not look at resources.blockstor.cozystack.io, and step 3 deletes nothing there. Step 3 also deletes only the canonical key on the controller, though the controller can hold the alias as well.
This query reads both maps, in any case, on all three kinds. I checked it against the same CR JSON:
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)"'Then step 3 can say: delete every key step 1 found, on the object it was found on, with linstor resource set-property <node> <resource> <key> "" for replicas.
B2: clone, spawn, snapshot and restore copy the passphrase property
The PR body says REST refuses any write that would store such a key. The guard only checks request bodies, and four handlers copy properties on the server side: rd_clone.go:2128 (volume-less clone), spawn.go:248 (group props into each spawned definition), snapshots.go:954 and snapshot_restore.go:1349. All of them go through store.TravellingProps, which drops the restore markers and nothing else. A group that still has DrbdOptions/Encryption/passphrase after the upgrade puts the plaintext key into every definition spawned from it, and spawn is how linstor-csi creates volumes. The satellite ignores those copies, but the key lands in more CRs and more backups. With B1 in place, that is the likely state of an upgraded cluster.
All four paths share TravellingProps, so dropping the passphrase keys there with passphrase.DropPlaintextProps covers them. A test that spawns from a group with the alias and checks the new definition has no such key would pin it.
Not code
GitHub still shows this PR as conflicting, but git merge-tree against main at 1652387 is clean and gives exactly the head's tree. The base was switched from the #190 branch to main one second after the force-push, and both the mergeability and DCO look computed against the old base. DCO counted 18 commits, including the #190 squash on main, whose author address differs from its sign-off. All 17 commits here are signed off fine. Because of the conflict state no workflow ran on this head at all, and a fresh push should recompute both. "Stacked on #190" in the body is out of date now that #190 is merged.
The conflict with #206 is the same as before, the LUKS warning in pkg/linstormigrate/convert.go and docs/linstor-migration.md. The #206 side still says to provision spec.encryption, which this PR says nothing reads. Whichever lands second should keep this PR's wording and add the layer-stack sentence from #206.
Tests of the touched packages pass on the head, apart from the known 2s server-shutdown flake in pkg/rest. Every failure there is server did not stop within 2s after cancel.
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 <kvapss@gmail.com>
…operty Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
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 <kvapss@gmail.com>
|
Thanks, head 9261ab7.
"Stacked on #190" is gone from the body, and this push should recompute mergeability and run CI. On the #206 conflict I'll keep this PR's wording and add #206's layer-stack sentence. |
The LUKS master passphrase could also live in a property:
DrbdOptions/EncryptPassphraseon the controller, the oldDrbdOptions/Encryption/passphrasealias on a definition, orLuksPassphraseon a replica. A property sits 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. The operations docs say the key lives in the Secret only.There are no users yet, so this drops the property path instead of migrating it.
encryption create-passphraseis the only source. The satellite drops every passphrase property before it adds the Secret's value, and the dispatcher reads that value alone. The LUKS checks onrd create, in REST and in the CLI, look at the Secret only.set-property. Deleting such a property stays allowed.vd modify-passphraseanswers 400. blockstor has no per-volume keys, and the door stored the value in plaintext where nothing read it. Upstream's per-volume key is refused like the others.encryption modify-passphraseanswers 409 while any definition has LUKS. The cluster passphrase is the LUKS key itself and nothing runscryptsetup luksChangeKey, so a rotation left every existing LUKS volume unable to open. That was already true before this PR.linstor-migrateleaves passphrase properties out of the YAML it generates, which is applied past the REST guard. Its LUKS warning pointed atspec.encryption, which nothing reads; it now says to add the cluster passphrase to each volume withcryptsetup luksAddKey.Upgrading needs an operator step on a cluster whose LUKS volumes were formatted with a property's value, since before this a property won over the Secret.
docs/layer-stack.mdlists the cases and what to do in each, and the satellite logs the property keys it ignores on a LUKS definition.The intentional divergences from upstream are rows 90 to 92 in
docs/cli-parity-known-deltas.md.Testing
set-property, the satellite injection, the dispatcher, the vd door, the rotation guard and the migrate index. Reverting each guard fails its test.go test ./...with envtest: green apart from the knownpkg/restshutdown flake.go test -tags=integration ./tests/integration/...: green.cli-matrix/encryption-passphrase-luks-rd.shand the L7 replaysencryption-passphrase-luks-rd.yamlandluks-encrypted-rd.yamlare updated but not run: I had no stand.