Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

storage: Improve handling of LUKS backed btrfs #20070

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mvollmer
Copy link
Member

@mvollmer mvollmer commented Feb 21, 2024

@mvollmer
Copy link
Member Author

Current plan: have a function that computes the correct crypttab options from the existing ChildConfiguration entries, without any arguments needed from the dialog action functions. Plus whatever is necessary for the direct Lock and Unlock actions.

@mvollmer mvollmer added the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Feb 29, 2024
@mvollmer mvollmer force-pushed the storage-correct-crypttab-options-for-subvols branch 2 times, most recently from 984efa4 to dc33b9c Compare February 29, 2024 13:28
@mvollmer
Copy link
Member Author

mvollmer commented Feb 29, 2024

To make this complete, we also need to show subvolumes of btrfs volumes that are currently locked, like we show mountpoints for filesystems that are locked. (This only needs to work well for single -device volumes, I think.)

@mvollmer mvollmer force-pushed the storage-correct-crypttab-options-for-subvols branch from f9df53c to 6f9190d Compare March 1, 2024 11:15
@mvollmer mvollmer added the blocked Don't land until something else happens first (see task list) label Mar 1, 2024
pkg/storaged/filesystem/mounting-dialog.jsx Fixed Show fixed Hide fixed
pkg/storaged/btrfs/subvolume.jsx Fixed Show fixed Hide fixed
pkg/storaged/btrfs/subvolume.jsx Fixed Show fixed Hide fixed
pkg/storaged/block/create-pages.jsx Fixed Show fixed Hide fixed
pkg/storaged/btrfs/subvolume.jsx Fixed Show fixed Hide fixed
@mvollmer mvollmer force-pushed the storage-correct-crypttab-options-for-subvols branch 7 times, most recently from a0863fa to f65ba0d Compare March 5, 2024 14:37
@mvollmer mvollmer changed the title WIP - storage: Compute crypttab options from all subvolumes storage: Improve handling of LUKS backed btrfs Mar 5, 2024
@mvollmer mvollmer force-pushed the storage-correct-crypttab-options-for-subvols branch 5 times, most recently from d68368d to cfc1805 Compare March 20, 2024 12:15
@mvollmer mvollmer removed the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Mar 20, 2024
@mvollmer mvollmer force-pushed the storage-correct-crypttab-options-for-subvols branch from cfc1805 to e115dd4 Compare March 20, 2024 12:33
@mvollmer mvollmer marked this pull request as ready for review March 20, 2024 12:33
@mvollmer mvollmer requested a review from jelly March 20, 2024 12:43
@mvollmer mvollmer force-pushed the storage-correct-crypttab-options-for-subvols branch from e115dd4 to 9571892 Compare March 25, 2024 07:46
@jelly
Copy link
Member

jelly commented Mar 26, 2024

This is now unblocked as readonly crypto was merged.

@mvollmer mvollmer removed the blocked Don't land until something else happens first (see task list) label Mar 27, 2024
@mvollmer mvollmer force-pushed the storage-correct-crypttab-options-for-subvols branch from 9571892 to 027e1f6 Compare March 27, 2024 13:45
@mvollmer mvollmer force-pushed the storage-correct-crypttab-options-for-subvols branch 2 times, most recently from 210ab78 to 7c985f0 Compare March 27, 2024 14:08
for (const c of block_crypto.ChildConfiguration) {
if (c[0] == "fstab") {
const fsname = decode_filename(c[1].fsname.v);
const uuid_match = fsname.match(/^UUID=(?<uuid>[A-Fa-f0-9-]+)/);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this depends on fstab being like:

UUID=11e176e1-8cd9-42b7-85e0-46bbb9e9bc35 /           btrfs       rw,compress=lzo,ssd,space_cache,discard=async,subvolid=256,subvol=/root 0 0

However I have something like:

LABEL=system            /           btrfs       rw,compress=lzo,ssd,space_cache,discard=async,subvolid=256,subvol=/root 0 0

Copy link
Member Author

@mvollmer mvollmer Mar 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this complexity is actually worth it. A fstab entry will only have one x-parent entry, and thus only one of the backing devices of a multi-device btrfs volume will have that entry in its ChildConfiguration. I wonder how the tests actually reach this code...

Copy link
Member Author

@mvollmer mvollmer Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have removed this and improved the docs a bit. It was the correct idea, but doesn't really work in practice.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, back to where we started! :-)

Looking into ChildConfiguration is important when: There is a multi-device btrfs volume on encrypted devices, one of them ("a") has a x-parent option in fstab and is locked, and another one ("b") is unlocked.

In that case we don't want to treat the "a" device as a locked single-device btrfs filesystem. "b" is already open and a "btrfs volume" is created for it. We either need to associate "a" with that volume, or show it as generic "locked data". We shouldn't show it as another copy of the same btrfs volume.

We could associate "a" with the volume and create a "btrfs device" card for it. But then we also need to put a "unlock" action on it, etc. I would like to finish #19882 instead, I think.

if (single_device_volume)
card = make_btrfs_filesystem_card(card, block, null);
else
card = make_locked_encrypted_data_card(card, block);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This condition confuses me as a reader, if it's not a single device volume it is encrypted?

I understand that we don't know if a locked without fstab?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's always encrypted and locked when we get here (content_block == null, is_crypto == true), but if we know (or guess) that it's a single device btrfs, we can make a filesystem card for it even when it is locked.

pkg/storaged/utils.js Outdated Show resolved Hide resolved
pkg/storaged/utils.js Outdated Show resolved Hide resolved
// Thus, while we should try to not let the user
// run into this situation here as much as
// possible, we will probably not be able to rule
// it out completely.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: btrfs filesystem show /dev/mapper/system should work fine for luks unlocked device members but parsing that and then offering to unlock all members is quite some work. Rather have udisks figure that out.

@mvollmer mvollmer force-pushed the storage-correct-crypttab-options-for-subvols branch from 7c985f0 to 76fe4d6 Compare April 4, 2024 11:15
if (fstab_config.length > 0) {
if (is_btrfs) {
if (single_device_volume)
card = make_btrfs_filesystem_card(card, block, null);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this requires investigation.

@mvollmer mvollmer marked this pull request as draft April 5, 2024 09:54
@mvollmer
Copy link
Member Author

mvollmer commented Apr 5, 2024

Alright back to draft, there is more coming...

@mvollmer mvollmer force-pushed the storage-correct-crypttab-options-for-subvols branch from 76fe4d6 to 77ace98 Compare April 5, 2024 10:35
@mvollmer mvollmer marked this pull request as ready for review April 8, 2024 06:41
@mvollmer mvollmer requested a review from jelly April 8, 2024 06:41
@mvollmer mvollmer force-pushed the storage-correct-crypttab-options-for-subvols branch 2 times, most recently from 92729b4 to 02ad089 Compare June 20, 2024 12:34
Single-device btrfs volumes now behave like other filesystems:
Unmounting all subvolumes will automatically lock the LUKS device, and
mounting the first subvolume will automatically unlock it.

Options in /etc/crypttab are now maintained correctly for btrfs
volumes with multiple subvolumes. For example, the LUKS device is only
"noauto" when all of the subvolumes are also "noauto", and it is
readonly exactly when all of the subvolumes are readonly.

Cockpit can't unfortunately always know whether a locked LUKS device
is part of a single-device or multi-device btrfs volume.  (Because
UDisks2 only tracks one parent of a fstab entry, we would need to fix
that.) We assume that they are single-device volumes (because they are
more common, probably) and let the user mount their subvolumes
directly.

If the assumption turns out to have been wrong, the mount operation is
cleanly aborted.
@mvollmer mvollmer force-pushed the storage-correct-crypttab-options-for-subvols branch from 02ad089 to 5714def Compare July 16, 2024 12:45

if (client.in_anaconda_mode()) {
actions.push({
title: _("Edit mount point"),
action: () => edit_mount_point(block, forced_options, subvol),
action: () => edit_mount_point(content_block || backing_block, forced_options, subvol, subvols),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This added line is not executed by any test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants