snap-bootstrap: add scan-disk subcommand

[valentindavid]

This command is expected to be called from udev. This creates
symlinks /dev/ubuntu/ubuntu-{seed,boot,data,save}.

Then we can bind snap-bootstrap initramfs-mounts to
dev-ubuntu-ubuntu-seed.device.

[valentindavid]

This works with snapcore/core-initrd#107

[mardy]

Would it be possible to get the same information just by running blkid instead of using the library?

[xnox]

if someone appends unencrypted ubuntu-data to a hard-drive, then it will be mounted instead of ubuntu-data-enc? surely we should try to find -enc paritions, and not search for non-encrypted ones afterwords?

Also I thought udev database already has all of this information inside it's database, which maybe can be queried directly in the udev rule alone without this helper binary? Apart from the LoaderDevicePartUUID bit, which i thought there should be a udev symlink for.

[valentindavid]

if someone appends unencrypted ubuntu-data to a hard-drive, then it will be mounted instead of ubuntu-data-enc? surely we should try to find -enc paritions, and not search for non-encrypted ones afterwords?

Maybe I mixed up, but I think the filesystem labels have "-enc" suffix. But the partition name does not. See for example:

/dev/vda4: UUID="2eeba654-032f-4384-8a46-075be2b415b5" LABEL="ubuntu-save-enc" TYPE="crypto_LUKS" PARTLABEL="ubuntu-save" PARTUUID="a154643d-4398-714a-9704-71335e5ea856"

PARTLABEL is not the same as LABEL

[xnox]

Inside ubuntu-save-enc there is ubuntu-save. Outside of ubuntu-save-enc there can be a malicious second partition with an FS ubuntu-save. Does above code handle this fine? As far as I can tell, after scanning and finding ubuntu-data-enc and using it; it will then find unencrypted ubuntu-data and override it.

Somehow this feels odd that after the first one is detected, it can be detected again, and override previously found result.

[valentindavid]

Also I thought udev database already has all of this information inside it's database, which maybe can be queried directly in the udev rule alone without this helper binary? Apart from the LoaderDevicePartUUID bit, which i thought there should be a udev symlink for.

Good question. I have tried before to use more rules and less code. But there were issues. And I do not remember. I will try again to document why this is not possible.

I am guessing it was the fallback for non-uefi boot that was a bit tricky.

gpt-auto-generator can create a symlink. But it has to be a partition with a specific uuid on the same disk as the booting partition. We do not use that uuid. And we should not use it. It is not made for that.

[valentindavid]

Inside ubuntu-save-enc there is ubuntu-save. Outside of ubuntu-save-enc there can be a malicious second partition with an FS ubuntu-save.

I am not sure I understood this.

This binary only looks at partition names. Not filesystem labels. If there are other partitions with conflicting filesystem labels, we will not look at them, because we only look at the partition names.

[valentindavid]

Today, someone can remove an encrypted ubuntu-save and replace it by a non encrypted one. snap-bootstrap will then fallback to it. What would be the difference?

[valentindavid]

Also I thought udev database already has all of this information inside it's database, which maybe can be queried directly in the udev rule alone without this helper binary? Apart from the LoaderDevicePartUUID bit, which i thought there should be a udev symlink for.

Good question. I have tried before to use more rules and less code. But there were issues. And I do not remember. I will try again to document why this is not possible.

Now I remember. It is possible to not call it on ENV{DEVTYPE}=="partition". Though we need to make sure that IMPORT{builtin}="blkid [...]" has been called yet, so post 60-persistent-storage.rules. Then what we have to do is compare ID_PART_ENTRY_UUID with UBUNTU_SEED (or the 3 others).

However, for ENV{DEVTYPE}=="disk" we do need to scan the partitions and find the booting one. And this we cannot do with just udev rules. We have LoaderDevicePartUUID which points to the boot partition. But we are processing the disk at this point.

[valentindavid]

gpt-auto-generator can create a symlink.

By the way, I was wrong about it. It is in an udev rule that the symlink is created. gpt-auto-generator only makes sysroot.mount and -.mount to use that symlink.

[valentindavid]

Would it be possible to get the same information just by running blkid instead of using the library?

We cannot list partitions from blkid.

[xnox]

Given SEED is ESP; and we bind the initramfs-mounts unit to the SEED; i am wondering if we can instead turn on / patch the systemd's gpt-auto-generator which can create esp mount in /efi and use that instead?

Looking more at this, I actually don't quite like the behaviour of gpt-auto-generator. Because the good bits of it are not what we need here.

Ideally we would only read efi_loader_get_device_part_uuid (aka LoaderDevicePartUUID) and generate .device unit for it, and bind initramfs-mounts service unit to it. Before any drives appear.

Thus without waiting udev to exec scan-disk to process them all either.

[valentindavid]

Ideally we would only read efi_loader_get_device_part_uuid (aka LoaderDevicePartUUID) and generate .device unit for it, and bind initramfs-mounts service unit to it. Before any drives appear.

Thus without waiting udev to exec scan-disk to process them all either.

So almost what I have done, but a symlink on the whole disk rather than the partitions?

[pedronis]

@valentindavid can the description or some comment in the code include the actual udev rules that this will be used from? it's a bit unclear just from the code how things go from the code in scan-disk to have /dev/ubuntu/disk symlinked?

What's the plan for testing the cmd_scan_disk code?

[valentindavid]

@valentindavid can the description or some comment in the code include the actual udev rules that this will be used from? it's a bit unclear just from the code how things go from the code in scan-disk to have /dev/ubuntu/disk symlinked?

Sure. We could even add the rules to snapd and install them into core-initrd.

What's the plan for testing the cmd_scan_disk code?

I think I will need to mock libblkid somehow to test it.

Loop devices need root to be setup, and I do not think there is as this point any nice user land block devices. I think only character devices can be emulated with CUSE. Not block.

[valentindavid]

What's the plan for testing the cmd_scan_disk code?

I have added tests.

I still have to add tests for the changes in cmd_initramfs_mounts.go (when /dev/ubuntu/disk exists). But that should be straight forward.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 62.35294% with 96 lines in your changes are missing coverage. Please review.

Project coverage is 78.84%. Comparing base (f7fea7f) to head (6ee943e).
Report is 44 commits behind head on master.

Files	Patch %	Lines
cmd/snap-bootstrap/cmd_scan_disk.go	68.42%	46 Missing and 20 partials ⚠️
osutil/disks/disks_linux.go	0.00%	12 Missing ⚠️
osutil/disks/mockdisk.go	0.00%	12 Missing ⚠️
cmd/snap-bootstrap/cmd_initramfs_mounts.go	72.72%	4 Missing and 2 partials ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #12006      +/-   ##
==========================================
- Coverage   78.88%   78.84%   -0.04%     
==========================================
  Files        1037     1039       +2     
  Lines      132881   133744     +863     
==========================================
+ Hits       104817   105454     +637     
- Misses      21523    21692     +169     
- Partials     6541     6598      +57     

Flag	Coverage Δ
unittests	78.84% <62.35%> (-0.04%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.