Conversation
f62894b to
0b12dbd
Compare
Add Volume.encrypted for save_on_stop, writable, dom0-backed volumes (mutex with ephemeral). Format/unlock with LUKS2 and an in-memory passphrase passed to cryptsetup on stdin; never write the passphrase to disk or qubes.xml. luksFormat on empty volumes; in-place reencrypt for existing data. Refuse dirty volumes and volumes with revisions. Unlock on start, close on stop; roll back vol.start() if open/resize fails so a wrong passphrase can be retried. Clone copies the LUKS container raw. QubesOS/qubes-issues#1293
Expose encrypted in admin.vm.volume.Info. Add Set.encrypted, SetPassphrase (memory only), and ChangePassphrase (old\nnew). Disabling encryption is rejected. Refuse encrypting a snapshot source. Register the new calls in the dom0 spec. QubesOS/qubes-issues#1293
Unit tests for property constraints, passphrase handling, dirty/revision refusal, start rollback after failed open, import/clone rules, and the new Admin API methods. Document Volume.encrypted. QubesOS/qubes-issues#1293
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #871 +/- ##
==========================================
+ Coverage 70.51% 71.14% +0.63%
==========================================
Files 61 61
Lines 14371 14799 +428
==========================================
+ Hits 10133 10529 +396
- Misses 4238 4270 +32
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
CC @marmarek |
marmarek
left a comment
marmarek
left a comment
There was a problem hiding this comment.
Nice work :)
Besides the comments below, this could also use an integration test, see qubes/tests/integ/storage.py. It doesn't need to be too detailed, but should cover basic cases:
- enabling encryption
- starting / stopping volume (and checking if data properly survives)
- maybe also encrypting volume with existing data (and again, checking if data is intact)
test_003_snapshot might be a good inspiration.
|
CC @marmarek |
|
Black complains about one line in tests |
|
openQArun MACHINE=64bit TEST=system_tests_gui_interactive,system_tests_basic_vm_qrexec_gui,system_tests_pvgrub_salt_storage,system_tests_gui_interactive,system_tests_gui_tools,system_tests_backup |
OpenQA test summaryComplete test suite and dependencies: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=202609121440-devel&flavor=pull-requests Test run included the following: New failures, excluding unstableCompared to: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2026050504-devel&flavor=update
Failed tests4 failures
Fixed failuresCompared to: https://openqa.qubes-os.org/tests/176874#dependencies 5 fixed
Unstable testsDetails
Performance TestsPerformance degradation:No issues Remaining performance tests:No remaining performance tests |
This is definitely related to this PR. I'm not 100% sure if this is more about issue in tests, or implementation, but needs some fixes.... |
Keep no revisions in the integ test so File/LVM can encrypt after the first shutdown. Use the committed reflink image when the dirty file is gone, and re-resolve the backend path after resize.
There was a problem hiding this comment.
🟡 Changes recommended
It introduces security-sensitive passphrase handling and LUKS path validation where the current implementation leaves avoidable in-memory passphrase copies and uses assert for runtime checks that can be disabled.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
qubes/storage/init.py:616
- stop_luks() also relies on an assert for runtime validation of the mapper path. Asserts may be disabled, so this should be a normal conditional check that raises a regular exception.
assert name.startswith("/dev/mapper/"), (
"invalid encrypted volume path %r" % name
)
doc/qubes-storage.rst:169
- This change removes the existing "Module contents" /
.. automodule:: qubes.storagesection at the end of the page, while other module docs in doc/ keep it. Unless this was intentionally moved elsewhere, keeping the automodule section helps ensure the public API is documented consistently.
See specific methods documentation for details.
- Files reviewed: 14/14 changed files
- Comments generated: 2
- Review effort level: Lite
| assert name.startswith("/dev/mapper/"), ( | ||
| "Invalid path %r passed to cryptsetup" % name | ||
| ) | ||
| mapper_name = name[12:] | ||
| assert "/" not in mapper_name, "Invalid name passed to cryptsetup" |
| if isinstance(passphrase, bytearray): | ||
| return bytes(passphrase) | ||
| if isinstance(passphrase, str): | ||
| return passphrase.encode("utf-8") | ||
| return passphrase |
| candidates = [] | ||
| if self.path: | ||
| candidates.append(self.path) | ||
| clean = getattr(self, "_path_clean", None) |
There was a problem hiding this comment.
Accessing driver-specific field here is a layering violation. I'm not sure why reflink driver behaves this way, might be a bug (@rustybird can you help?). But if that's intentional, better override the luks_backend_path function in the reflink driver with driver-specific logic (and not mark it as private anymore).
There was a problem hiding this comment.
Yeah, that’s intentional in Reflink. Fixed by moving the clean/dirty path handling into the public luks_backend_path() override.
Keep generic path/block-device lookup on Volume. Reflink overrides it so the base class does not read _path_clean.
|
openQArun MACHINE=64bit TEST=system_tests_gui_interactive,system_tests_basic_vm_qrexec_gui,system_tests_pvgrub_salt_storage,system_tests_gui_interactive,system_tests_gui_tools,system_tests_backup |
Now it fails this way |
LUKS2 resize needs the volume key; without it start_luks failed on every backend after unlock.
@marmarek Could you please rerun system_tests_pvgrub_salt_storage and check test_010_persistent_luks with the latest fix in 11b7064? |
|
openQArun MACHINE=64bit TEST=system_tests_basic_vm_qrexec_gui,system_tests_pvgrub_salt_storage,system_tests_backup |
Now the test says data doesn't survive encrypting. I have not checked what actually happens (is it empty? scrambled? something else?) |
A bare cryptsetup resize after in-place encrypt grows the guest disk by 16M and breaks the persist check.
@marmarek , I want to confirm the expected device size after in-place LUKS2 re-encryption. If the original VM volume is 128 MiB and we temporarily grow it by 32 MiB for --reduce-device-size 32M, after re-encryption completes, what size should cryptsetup open expose to the guest? Should it be: If the expected guest size is 128 MiB, should we shrink the unused tail after re-encryption, or is there a preferred way to make cryptsetup open expose exactly the original 128 MiB? I’m asking because OpenQA compares the whole /dev/xvde against the original 128 MiB image, so exposing 144 MiB causes the persistence test to fail even though the original data itself is intact. |
|
I'd say encryption should be transparent for the VM, meaning the size should not change. So, 128M in this example. |
Summary
First slice of QubesOS/qubes-issues#1293: persistent per-volume encryption on the existing storage stack (same approach as ephemeral volatile encryption in #396).
Behavior
Volume.encryptedonsave_on_stop+rw+ dom0 volumes (private on AppVM/Standalone, root on Template/Standalone). Mutually exclusive withephemeral.luksFormat/open --type=luks2). Passphrase stays in qubesd memory only (admin.vm.volume.SetPassphrase); not XML, notvolume.Info.encrypted=is visible inadmin.vm.volume.Infoso start tools know to prompt first.luksFormat; volume with data → grow 32 MiB +reencrypt --encrypt. Dirty volumes and volumes with revisions are refused.vol.start()rolls backvol.stop()and keeps the passphrase for retry.admin.vm.volume.ChangePassphrasepayload isold\nnew.Out of scope
Test plan
On a real system: enable on a new private volume, start/stop, wrong passphrase then retry, clone, refuse encrypt on a template that already has AppVMs.