secboot: use uuid of luks2 instead of partition #14400

valentindavid · 2024-08-21T10:37:23Z

by-partuuid does not make much sense because it uselessly assumes that it is a partition. Conceptually we should not care about it. It also makes the resolution more complex as we need to fetch information about the device which we do not really need at this point.

It is more common to resolve by filesystem UUID than part UUID. For instance cryptsetup accepts path as UUID=deadbeef-dead-dead-dead-deaddeafbeef. But it does not accept this kind of syntax for partitions.

We will also need to register the old paths with something like canonical/secboot#331

bboozzoo

Small suggestion about a possible name tweak

bboozzoo · 2024-09-23T08:15:22Z

osutil/disks/disks_linux.go

-func PartitionUUIDFromMountPoint(mountpoint string, opts *Options) (string, error) {
+var dmUUIDRe = regexp.MustCompile(`^CRYPT-(?P<type>.*)-(?P<uuid1>[0-9a-f]{8})(?P<uuid2>[0-9a-f]{4})(?P<uuid3>[0-9a-f]{4})(?P<uuid4>[0-9a-f]{4})(?P<uuid5>[0-9a-f]{12})-(?P<name>.*)$`)
+
+func DmCryptUUIDFromMountPoint(mountpoint string) (string, error) {


maybe MapperCryptUUID... or DeviceMapper.., or simply DMCrypt... ?

bboozzoo · 2024-09-23T08:17:10Z

osutil/disks/disks_linux.go

@@ -963,41 +964,40 @@ func AllPhysicalDisks() ([]Disk, error) {
 	return disks, nil
 }

-// PartitionUUIDFromMountPoint returns the UUID of the partition which is a
-// source of a given mount point.
-func PartitionUUIDFromMountPoint(mountpoint string, opts *Options) (string, error) {


This doesn't seem to have been used in non-encryption contexts, so the change should be fine

alfonsosanchezbeato

Some minor comments, but besides that I think that the PR/commit needs a more detailed explanation on why we want to switch to using filesystem/dm-crypt uuids instead of the partitions uuids.

alfonsosanchezbeato · 2024-09-25T20:27:24Z

osutil/disks/disks_linux.go

-func PartitionUUIDFromMountPoint(mountpoint string, opts *Options) (string, error) {
+var dmUUIDRe = regexp.MustCompile(`^CRYPT-(?P<type>.*)-(?P<uuid1>[0-9a-f]{8})(?P<uuid2>[0-9a-f]{4})(?P<uuid3>[0-9a-f]{4})(?P<uuid4>[0-9a-f]{4})(?P<uuid5>[0-9a-f]{12})-(?P<name>.*)$`)
+
+func DmCryptUUIDFromMountPoint(mountpoint string) (string, error) {


This would need a function description

alfonsosanchezbeato · 2024-09-25T20:29:49Z

osutil/disks/disks_linux.go

 }

-// PartitionUUID returns the UUID of a given partition
-func PartitionUUID(node string) (string, error) {
+func FilesystemUUID(node string) (string, error) {


This would need tests in the package, and a description

still doesn't seem to have tests

alfonsosanchezbeato · 2024-09-25T20:30:56Z

osutil/disks/disks_darwin.go

Wouldn't this need a definition of the new package functions?

I have opened #14557 to enable the tests.

I was expecting a failure but nothing happened.

pedronis

thanks, couple of questions/comments

pedronis · 2024-09-30T16:30:14Z

secboot/secboot_sb.go

 		if err != nil {
 			return res, fmt.Errorf("error enumerating partitions for disk to find unencrypted device %q: %v", name, err)
 		}
 	}

-	partDevice := filepath.Join("/dev/disk/by-partuuid", partUUID)
+	partDevice := filepath.Join("/dev/disk/by-partuuid", part.PartitionUUID)


is it intentional to keep using by-partuuid here?

That requires more changes in snap-bootstrap. For now it is not needed. The main point here is to simplify everywhere where we need to get keys from keyring.

We will probably have to refactor snap-bootstrap to match primary keys, and we can do that refactoring in the same time.

pedronis · 2024-09-30T16:30:46Z

osutil/disks/disks_linux.go

 }

-// PartitionUUID returns the UUID of a given partition
-func PartitionUUID(node string) (string, error) {
+func FilesystemUUID(node string) (string, error) {


still doesn't seem to have tests

alfonsosanchezbeato

LGTM, thanks for the changes and clarifications

`by-partuuid` does not make much sense because it uselessly assumes that it is a partition. Conceptually we should not care about it. It also makes the resolution more complex as we need to fetch information about the device which we do not really need at this point. It is more common to resolve by filesystem UUID than part UUID. For instance cryptsetup accepts path as `UUID=deadbeef-dead-dead-dead-deaddeafbeef`. But it does not accept this kind of syntax for partitions.

valentindavid · 2024-10-02T11:16:21Z

tests/nested/manual/core-factory-reset-new-secboot will fail until we have canonical/secboot#331

valentindavid added FDE Manager Pull requests that target FDE manager branch Run nested The PR also runs tests inluded in nested suite labels Aug 21, 2024

valentindavid closed this Aug 21, 2024

valentindavid reopened this Aug 21, 2024

valentindavid force-pushed the valentindavid/use-luks2-uuid branch from 88e9db9 to 195df00 Compare August 23, 2024 12:48

valentindavid force-pushed the valentindavid/use-luks2-uuid branch from 195df00 to fef4bf4 Compare September 11, 2024 13:59

valentindavid force-pushed the valentindavid/use-luks2-uuid branch 5 times, most recently from c9cb79d to 6491f5f Compare September 20, 2024 11:57

valentindavid marked this pull request as ready for review September 20, 2024 14:15

valentindavid requested a review from pedronis September 20, 2024 14:16

valentindavid mentioned this pull request Sep 20, 2024

overlord/fdestate: keep FDE state up to date #14516

Merged

pedronis requested review from bboozzoo and alfonsosanchezbeato September 20, 2024 16:40

bboozzoo approved these changes Sep 23, 2024

View reviewed changes

alfonsosanchezbeato reviewed Sep 25, 2024

View reviewed changes

valentindavid force-pushed the valentindavid/use-luks2-uuid branch from 825af20 to c53f1be Compare September 30, 2024 14:21

pedronis approved these changes Sep 30, 2024

View reviewed changes

valentindavid force-pushed the valentindavid/use-luks2-uuid branch from c53f1be to 1921cff Compare October 1, 2024 07:20

valentindavid requested a review from alfonsosanchezbeato October 1, 2024 10:46

alfonsosanchezbeato approved these changes Oct 1, 2024

View reviewed changes

valentindavid force-pushed the valentindavid/use-luks2-uuid branch from 39026f3 to 8df73a3 Compare October 2, 2024 08:20

pedronis merged commit 663bc3b into canonical:fde-manager-features Oct 2, 2024
42 of 52 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

secboot: use uuid of luks2 instead of partition #14400

secboot: use uuid of luks2 instead of partition #14400

valentindavid commented Aug 21, 2024 •

edited

Loading

bboozzoo left a comment

bboozzoo Sep 23, 2024

bboozzoo Sep 23, 2024

alfonsosanchezbeato left a comment

alfonsosanchezbeato Sep 25, 2024

alfonsosanchezbeato Sep 25, 2024

pedronis Sep 30, 2024

alfonsosanchezbeato Sep 25, 2024

valentindavid Sep 30, 2024

valentindavid Oct 1, 2024

pedronis left a comment

pedronis Sep 30, 2024

valentindavid Oct 1, 2024

pedronis Sep 30, 2024

alfonsosanchezbeato left a comment

valentindavid commented Oct 2, 2024

secboot: use uuid of luks2 instead of partition #14400

secboot: use uuid of luks2 instead of partition #14400

Conversation

valentindavid commented Aug 21, 2024 • edited Loading

bboozzoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alfonsosanchezbeato left a 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

pedronis left a 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

alfonsosanchezbeato left a comment

Choose a reason for hiding this comment

valentindavid commented Oct 2, 2024

valentindavid commented Aug 21, 2024 •

edited

Loading