storage: Confirmation before erasing non-empty filesystems #20272

mvollmer · 2024-04-08T11:51:59Z

But only in Anaconda mode.

Storage: Cockpit asks for extra confirmation before deleting files in Anaconda mode

When using Cockpit to configure storage manually during installation with Anaconda, one normally only operates on empty filesystems. You might create a couple of partitions with filesystems and then change your mind and delete those partitions again, maybe to create them in a slightly different way. These are all pretty harmless operations since no actual data gets deleted.

However, there might also be filesystems present that have not been created during installation. They might contain other operating systems and old, valueable files. In order to protect against deleting these by accident, Cockpit now checks whether a filesystem contains actual files before deleting it, and asks for extra confirmation if it does.

garrett · 2024-04-10T14:13:43Z

Here's some revised mockups that streamline things:

Don't show two or three options in a dropdown (that should really be for 5 or more items only, or a dynamic amount that can be 5 or more); I changed it to radios instead
I renamed it to "modern" and "legacy" with GPT and MBR specified in parenthesis
The 2 TB limit is specified on the legacy MBR option, as it's a limitation of the old method, not a feature of the new
The warning is not a warning (as it is not the result of an action), but a helper text
The table is without much else, so it's just the possibly affected partitions with data
The table contains the filesystem label as well as the size (used & total)
I dropped the /dev/ to be more consistent with the rest of Cockpit and Anaconda (as /dev/ is redundant for devices, as every device is in /dev/)
Use PatternFly suggestions (for the heading, for example)
Remove overwrite with zeros option, as per our few discussions about it, as it's problematic for all kinds of reasons (false sense of security, isn't consistently supported, physically damages drives, can destroy drives in some cases, and the actual solution for ensuring sensitive data is not on the drive is to have it encrypted from the start).

This is all assuming we detect data like we talked about yesterday. That is, show the warning only if:

It's not a partition created in this Anaconda and/or Cockpit session
It has data
- Ideally without mounting the partition
- We could check the used size; if it's over a certain small threshold, we know there are files
- If it's encrypted

Examples:

If someone created an encrypted partition within the session, it wouldn't get flagged as containing data, as it was created in the session.
If someone created a partition with another tool ahead of time, but it does not contain data (as in, the used size is barely anything), then it wouldn't be flagged as having data.

This would let Cockpit not get in the way when someone is adding and removing partitions, but it also would protect someone from accidentally losing data.

We should use this general pattern for initializing the disk, formatting a partition, and anything else that would probably cause someone to lose data on a partition like this. Although we should rework the warning area a little with regard to the text (and probably table too).

Open questions:

Should we not even include an option of GPT vs. MBR for disks that are 2TB or more?
Should we really include options for GPT vs. MBR at all? (Is it actually important in 2024?)

mvollmer · 2024-04-11T10:01:16Z

Here's some revised mockups that streamline things:

Thanks!

I think we should split the parts out that are general improvements to the "Initialize disk" dialog. They might even happen as part of #19882.

mvollmer · 2024-04-11T10:02:56Z

I think "a partition on this disk" is too specific, there are other cases like LVM2 logical volumes. What about "Important data might be deleted" as the header?

mvollmer · 2024-04-11T11:53:07Z

@garrett, looks like this now:

The text of the "important data" section pretty small. This is the result of using HelperText and a compact Table...

The gap between the table and the checkbox line is because the table is in the dialog body, while the checkbox line is in the footer.

mvollmer · 2024-04-12T09:17:43Z

Don't show two or three options in a dropdown (that should really be for 5 or more items only, or a dynamic amount that can be 5 or more); I changed it to radios instead

I renamed it to "modern" and "legacy" with GPT and MBR specified in parenthesis

The 2 TB limit is specified on the legacy MBR option, as it's a limitation of the old method, not a feature of the new

Remove overwrite with zeros option, as per our few discussions about it, as it's problematic for all kinds of reasons (false sense of security, isn't consistently supported, physically damages drives, can destroy drives in some cases, and the actual solution for ensuring sensitive data is not on the drive is to have it encrypted from the start).

This has been implemented in #19882.

The warning is not a warning (as it is not the result of an action), but a helper text

The table is without much else, so it's just the possibly affected partitions with data

The table contains the filesystem label as well as the size (used & total)

This has been implemented here.

I dropped the /dev/ to be more consistent with the rest of Cockpit and Anaconda (as /dev/ is redundant for devices, as every device is in /dev/)

This has been partly implemented here (for the "important data" table), and will be completed in #19882.

Use PatternFly suggestions (for the heading, for example)

@garrett, I don't understand this, can you elaborate?

mvollmer · 2024-04-16T06:49:03Z

Should we not even include an option of GPT vs. MBR for disks that are 2TB or more?

I did this in #19882.

mvollmer · 2024-04-16T07:25:55Z

This is all assuming we detect data like we talked about yesterday. That is, show the warning only if:

It's not a partition created in this Anaconda and/or Cockpit session

I am reluctant to implement this. We would have to identify all places in Cockpit where things get created. That's of course possible, but I hope we can find a more straighforward rule to determine whether to show the warning or not, a rule that doesn't require us to gather information as the user is using Cockpit. That would be more robust.

Right now the rule is:

If it is a filesystem of a recognized type (xfs, ext2, ext3, ext4, btrfs, vfat, or ntfs), we check whether it has anything in it that is not a directory. This might mount the filesystem temporarily. We say "N used, M total".
If it is a encrypted device that is locked, we say "Locked encrypted device might contain data" and warn about it regardless of what is in it.
If it is something not recognized by Cockpit in general or a filesystem not in the list above, we say "Device contains unrecognized data"

My hope is that encrypted devices that are created by Cockpit will remain open during the installation process.

The idea behind the list of recognized filesystem types is to avoid trying to mount unusual filesystems that might be supported by the kernel, but are maybe buggy.

jelly · 2024-05-14T08:23:24Z

pkg/storaged/dialog.jsx

+                                                           [decode_filename(u.block.PreferredDevice)],
+                                                           { superuser: true, err: "message" });
+                        if (empty.trim() != "yes") {
+                            console.log("INFO", empty);


Should this be a debug message?

Let's just remove this!

jelly · 2024-05-14T08:25:39Z

pkg/storaged/dialog.jsx

+                }
+                if (client.in_anaconda_mode() && !expect_single_unmount && u.block) {
+                    if (u.block.IdUsage == "filesystem" &&
+                        ["xfs", "ext2", "ext3", "ext4", "btrfs", "vfat", "ntfs"].indexOf(u.block.IdType) >= 0) {


Are these all the filesystems udisks supports? If so Cockpit should already have a list of them somewhere right? Or can't we get this from udisks? Like http://storaged.org/doc/udisks2-api/latest/gdbus-org.freedesktop.UDisks2.Manager.html#gdbus-property-org-freedesktop-UDisks2-Manager.SupportedFilesystems

Are these all the filesystems udisks supports?

No, the idea is to only mount a small number of well-known filesystems, instead of just throwing everything that we find at the kernel.

jelly · 2024-05-14T08:27:39Z

pkg/storaged/dialog.jsx

+
+            if (have_data) {
+                usage.Teardown = true;
+                dlg.need_confirmation(_("I confirm I want to lose this data forever"));


Sounds a bit dramatic 😆

It's a dramatic situation! :-)

jelly · 2024-05-14T08:39:34Z

pkg/storaged/fsys-is-empty.sh

+dev=$1
+
+need_unmount=""
+mp=$(findmnt -no TARGET "$dev" | cat)


That cat seems unnecessary:

[jelle@t14s][~/projects/cockpit/main]%x=$(findmnt -no TARGET /dev/nvme0n1p1) [jelle@t14s][~/projects/cockpit/main]%echo $x /boot

It protects against the exit code of findmnt. When $dev is not mounted, findmnt exits with code 1, which would stop the script because of set -e. In a pipeline, the last sub-command determines the exit code of the while pipeline, and cat always exits with 0.

jelly · 2024-05-14T08:43:16Z

pkg/storaged/fsys-is-empty.sh

+# A filesystem is empty if it only has directories in it.
+
+first=$(find "$mp" -not -type d | head -1)
+info=$(stat -f -c '{ "unit": %S, "free": %f, "total": %b }' "$mp")


Neither of these commands is allowed to fail as then we will exit and never unmount if needed. So maybe we should use a trap for that.

(I can only get find to fail when not running as root for example but maybe it can fail on broken symlinks or pipes as root?)

So maybe we should use a trap for that.

Good point!

jelly · 2024-05-14T08:45:13Z

test/verify/check-storage-anaconda

+        m.execute(f"echo einszweidrei | cryptsetup luksFormat --pbkdf-memory 32768 {disk}")
+        m.execute(f"echo einszweidrei | cryptsetup luksOpen {disk} dm-test")
+        m.execute("mkfs.ext4 /dev/mapper/dm-test; mount /dev/mapper/dm-test /mnt; echo Hi >/mnt/hello; umount /mnt")
+        m.execute("cryptsetup close dm-test")


A test for a Filesystem with only directories would be nice as well.

Yes, agreed.

garrett · 2024-05-14T15:39:40Z

Are the screenshots up to date? They still vary wildly from the mockups in several ways.

mvollmer · 2024-05-15T11:46:20Z

Are the screenshots up to date?

Yes.

They still vary wildly from the mockups in several ways.

Many of the changes are in #19882.

I have changed some of your UI text:

I think "a partition on this disk" is too specific, there are other cases like LVM2 logical volumes. What about "Important data might be deleted" as the header?

Also, I left the action out of the confirmation text since the confirmation is used with more than just initializing disks: "I confirm I want to lose this data forever".

But only in Anaconda mode.

cockpituous · 2024-05-15T15:35:04Z

pkg/storaged/dialog.jsx

+                            } catch {
+                                u.data_warning = _("Device contains unrecognized data");


These 2 added lines are not executed by any test.

cockpituous · 2024-05-15T15:35:04Z

pkg/storaged/legacy-vdo/legacy-vdo.jsx

@@ -68,7 +68,7 @@ export function make_legacy_vdo_page(parent, vdo, backing_block, next_card) {
                    }
                },
                Inits: [
-                    init_active_usage_processes(client, usage)
+                    init_teardown_usage(client, usage)


This added line is not executed by any test.

cockpituous · 2024-05-15T15:35:04Z

pkg/storaged/legacy-vdo/legacy-vdo.jsx

@@ -130,7 +130,7 @@
                }
            },
            Inits: [
-                init_active_usage_processes(client, usage)
+                init_teardown_usage(client, usage)


This added line is not executed by any test.

mvollmer added the release-note label Apr 8, 2024

mvollmer force-pushed the storage-extra-warning-for-non-empty-filesystems branch 7 times, most recently from 5803fbe to 1acb017 Compare April 10, 2024 12:13

mvollmer force-pushed the storage-extra-warning-for-non-empty-filesystems branch from 1acb017 to e75d76d Compare April 11, 2024 11:39

mvollmer force-pushed the storage-extra-warning-for-non-empty-filesystems branch 2 times, most recently from 61d25c4 to 6aa6559 Compare April 12, 2024 09:10

mvollmer force-pushed the storage-extra-warning-for-non-empty-filesystems branch from 6aa6559 to bcecc43 Compare April 16, 2024 07:45

mvollmer force-pushed the storage-extra-warning-for-non-empty-filesystems branch 2 times, most recently from dd4eff0 to 219c595 Compare May 13, 2024 11:06

mvollmer marked this pull request as ready for review May 13, 2024 11:10

mvollmer requested review from garrett and jelly May 13, 2024 11:10

jelly reviewed May 14, 2024

View reviewed changes

storage: Confirmation before erasing actual data

70f3eb3

But only in Anaconda mode.

mvollmer force-pushed the storage-extra-warning-for-non-empty-filesystems branch from 219c595 to 70f3eb3 Compare May 15, 2024 14:26

cockpituous reviewed May 15, 2024

View reviewed changes

mvollmer requested a review from jelly May 17, 2024 08:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

storage: Confirmation before erasing non-empty filesystems #20272

storage: Confirmation before erasing non-empty filesystems #20272

mvollmer commented Apr 8, 2024 •

edited

garrett commented Apr 10, 2024

mvollmer commented Apr 11, 2024

mvollmer commented Apr 11, 2024

mvollmer commented Apr 11, 2024 •

edited

mvollmer commented Apr 12, 2024 •

edited

mvollmer commented Apr 16, 2024 •

edited

mvollmer commented Apr 16, 2024 •

edited

jelly May 14, 2024

mvollmer May 15, 2024

jelly May 14, 2024

mvollmer May 15, 2024

jelly May 14, 2024

mvollmer May 15, 2024

jelly May 14, 2024

mvollmer May 15, 2024

jelly May 14, 2024

mvollmer May 15, 2024

jelly May 14, 2024

mvollmer May 15, 2024

garrett commented May 14, 2024

mvollmer commented May 15, 2024

cockpituous May 15, 2024

cockpituous May 15, 2024

cockpituous May 15, 2024

		} catch {
		u.data_warning = _("Device contains unrecognized data");

storage: Confirmation before erasing non-empty filesystems #20272

Are you sure you want to change the base?

storage: Confirmation before erasing non-empty filesystems #20272

Conversation

mvollmer commented Apr 8, 2024 • edited

Storage: Cockpit asks for extra confirmation before deleting files in Anaconda mode

garrett commented Apr 10, 2024

mvollmer commented Apr 11, 2024

mvollmer commented Apr 11, 2024

mvollmer commented Apr 11, 2024 • edited

mvollmer commented Apr 12, 2024 • edited

mvollmer commented Apr 16, 2024 • edited

mvollmer commented Apr 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garrett commented May 14, 2024

mvollmer commented May 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvollmer commented Apr 8, 2024 •

edited

mvollmer commented Apr 11, 2024 •

edited

mvollmer commented Apr 12, 2024 •

edited

mvollmer commented Apr 16, 2024 •

edited

mvollmer commented Apr 16, 2024 •

edited