snap-bootstrap: load partition information from a manifest file for cloudimg-rootfs mode #14713

sespiros · 2024-11-12T09:52:24Z

~~Rebased on top of #14178 which is approved but not merged. New commits start at commit 2035d4d.~~

~~Rebased on top of #14789 which is approved but not merged. New commits start at commit c55f509.~~

This refactors cloudimg-rootfs mode and adds support for externally providing a dm-verity root hash for the rootfs that is mounted. This root hash is part of a new manifest file that resides on the ESP.

Subsequent PRs will add support for measuring the manifest to the TPM for remote attestation purposes.

Jira: https://warthogs.atlassian.net/browse/FR-9480

alfonsosanchezbeato · 2024-11-20T18:55:31Z

@sespiros probably it would be good to rebase this now that the prerequisite has been merged

pedronis

@sespiros I think we discussed this at the sprint, my strong preference would be to first move the existing CVM support to a _cvm.go and _cmv_test.go file, and then do this change

codecov · 2024-12-03T13:53:35Z

Codecov Report

Attention: Patch coverage is 78.52349% with 32 lines in your changes missing coverage. Please review.

Project coverage is 78.22%. Comparing base (24a0034) to head (a43c494).
Report is 89 commits behind head on master.

Files with missing lines	Patch %	Lines
cmd/snap-bootstrap/cmd_initramfs_mounts_cvm.go	78.52%	21 Missing and 11 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #14713      +/-   ##
==========================================
+ Coverage   78.20%   78.22%   +0.01%     
==========================================
  Files        1151     1156       +5     
  Lines      151396   152997    +1601     
==========================================
+ Hits       118402   119684    +1282     
- Misses      25662    25919     +257     
- Partials     7332     7394      +62

Flag	Coverage Δ
unittests	`78.22% <78.52%> (+0.01%)`	⬆️

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.

This is a preparatory commit which modifies the existing CVM mode to support mounting multiple overlaid partitions for the rootfs.

In order to provide integrity protection for the root fs, the root fs will be mounted read-only and will be accompanied by dm-verity data from a new auto-discovered partition. To support writable state for the ephemeral CVM scenario, an overlayfs mount is created with an upper layer residing on tmpfs. A writable layer residing on disk can be also supported but it is currently disabled. The filesystem layout in the initramfs will look like: . /run/mnt ├── ... ├── data (where the final merged overlay will be mounted) ├── cloudimg-rootfs (read-only mount of the root fs, folder name is the fs label) ├── .. (future WIP, other partition mounts as separate mounts) └── cloudimg-rootfs-writable (tmpfs mount for the writable upper layer) ├── work └── upper An example final overlay mount command will look like: mount -t overlay overlay -olowerdir=/run/mnt/cloudimg-rootfs,\ upperdir=/run/mnt/cloudimg-rootfs-writable/upper,\ workdir=/run/mnt/cloudimg-rootfs-writable/work \ /run/mnt/data

…verity partition instead

v1 CVM images use the filesystem label to auto-discover an encrypted rootfs partition based on whether it has the '-enc' suffix. The new images use the GPT label to auto-discover the verity partition. In order to keep backwards compatibility, the customization tool will use set both labels to the same one. This commit clarifies that the field in the new structs actually contain GPT label information.

creating the upper and work folders used for setting up the overlayfs mount, was initially handled by snap-bootstrap whether it set up a tmpfs or detected a writable partition on disk but in order to support the writable partition case, it was later discovered that these folders might need to be created during the customization step and might already exist. snap-bootstrap will now create those folders only if it has detected no writable partition in the manifest and has auto-created a tmpfs-based writable partition.

trying to boot an image on a system without a TPM present was failing due to not-finding a TPM device. The source of the error was not the provision step as secbotProvisionForCVM already ignores errors (returns nil) if a TPM device is not found or a template file for provision the TPM is not found.

CVM mode used to mount the rootfs partition with the Ephemeral flag as cloud images contain a fixed fstab entry.

This is a legacy check that is not actually needed now as secbootUnlockVolumeUsingSealedKeyIfEncrypted will only find partitions residing on the disk that the ESP is from.

alfonsosanchezbeato

Thanks, I have some comments

alfonsosanchezbeato · 2024-12-05T16:42:11Z

cmd/snap-bootstrap/cmd_initramfs_mounts_cvm.go

+	Opts     *systemdMountOptions
+}
+
+type ImageManifestPartition struct {


It looks like this and other new types like ImageManifest or ManifestError do not need to be exported.

Made them private.

alfonsosanchezbeato · 2024-12-05T16:45:37Z

cmd/snap-bootstrap/export_test.go

@@ -94,6 +94,14 @@ func MockOsutilIsMounted(f func(string) (bool, error)) (restore func()) {
 	}
 }

+func MockOsReadFile(f func(string) ([]byte, error)) (restore func()) {


In tests we tend to create the files instead of mocking the read, note that dirs.GlobalRootDir is set to a temporary folder so this is in principle easy to do.

Implemented the suggestion, thanks.

alfonsosanchezbeato · 2024-12-05T16:47:20Z

cmd/snap-bootstrap/cmd_initramfs_mounts_cvm.go

+	GptLabel string `json:"label"`
+	RootHash string `json:"root_hash"`
+	Overlay  string `json:"overlay"`
+}


A description on what is each of these fields would be helpful, or maybe a link to a description of the format if available.

Added descriptions for all the proposed fields.

alfonsosanchezbeato · 2024-12-05T16:47:55Z

cmd/snap-bootstrap/cmd_initramfs_mounts_cvm.go

+}
+
+type ImageManifest struct {
+	Partitions []ImageManifestPartition `json:"partitions"`


same: re describing the field

Added a description although I feel it is a bit self-describing.

alfonsosanchezbeato · 2024-12-05T16:59:19Z

cmd/snap-bootstrap/cmd_initramfs_mounts_cvm.go

+	return im, nil
+}
+
+// generateMountsFromManifest is used to parse a manifest file which contains information about which


strictly speaking, the manifest passed around parsed already, right?

That's true, I modified my comment here.

alfonsosanchezbeato · 2024-12-05T17:03:39Z

cmd/snap-bootstrap/cmd_initramfs_mounts_cvm.go

+		if p.Overlay == "lowerdir" {
+			// XXX: currently only one lower layer is permitted. The rest, if found, are ignored.
+			if foundReadOnlyPartition != "" {
+				continue


Should not this as a minimum be logged? Or should it be considered an error in the manifest?

We should at least log something here, yes. Initially I thought to support potential future cases where I might have an older snap-bootstrap (which only supports a single lowerdir for instance) be supplied with a manifest that might be richer in functionality (multiple lowerdirs) and still allow it to boot.

This might be too complicated of a use case so I decided to throw an error instead.

alfonsosanchezbeato · 2024-12-05T17:06:28Z

cmd/snap-bootstrap/cmd_initramfs_mounts_cvm.go

+		} else {
+			// Only one writable partition is permitted.
+			if foundWritablePartition != "" {
+				continue


Same regarding log/error out

See previous comment. This will now be stricter and throw an error.

alfonsosanchezbeato · 2024-12-05T17:12:04Z

cmd/snap-bootstrap/cmd_initramfs_mounts_cvm.go

+		GptLabel: "cloudimg-rootfs",
+		Opts: &systemdMountOptions{
+			Overlayfs: true,
+			LowerDirs: []string{filepath.Join(boot.InitramfsRunMntDir, partitionMounts[0].GptLabel)},


is using partitionMounts[0] assuming an order in the definitions of the partitions in the file? should this always be the read-only partition?

I fixed this to use the foundReadOnlyPartition variable. I also reworked some bits a little bit:

I no longer require any order (dropped some sanity checks from parseImageManifest)

I renamed the Overlay field which was a bit confusing to ReadOnly. I described the change in the generateMountsFromManifest comment.

alfonsosanchezbeato · 2024-12-05T17:14:53Z

cmd/snap-bootstrap/cmd_initramfs_mounts_cvm.go

+	pm.Opts.UpperDir = filepath.Join(boot.InitramfsRunMntDir, foundWritablePartition, "upper")
+	pm.Opts.WorkDir = filepath.Join(boot.InitramfsRunMntDir, foundWritablePartition, "work")


This could be done when pm is defined, splitting the initialization of Opts does not seem necessary.

alfonsosanchezbeato · 2024-12-05T17:17:10Z

cmd/snap-bootstrap/cmd_initramfs_mounts_cvm_test.go

+	restore = main.MockOsReadFile(func(path string) ([]byte, error) {
+		// check attempt to read manifest from specific path
+		c.Assert(path, Equals, filepath.Join(boot.InitramfsUbuntuSeedDir, "EFI/ubuntu", "manifest.json"))
+		return []byte(fmt.Sprintf(`{"partitions":[{"label":"cloudimg-rootfs","root_hash":%q,"overlay":"lowerdir"}]}`, expectedRootHash)), nil


Do we need a test case for when more than one partition is defined?

I modified the existing one to accommodate the changes to the manifest and also added extra test cases for the private generateMountsFromManifest to test multiple unsupported partitions scenarios.

…he read

pedronis

did a pass

pedronis · 2024-12-17T15:27:18Z

cmd/snap-bootstrap/cmd_initramfs_mounts_cvm.go

+	if err != nil {
+		return err
+	}
+	if err := os.Mkdir(path, fi.Mode()); err != nil && !os.IsExist(err) {


won't we always get an IsExist error, otherwise how can the previous os.Stat work?

I reworked that bit.

pedronis · 2024-12-17T15:27:28Z

cmd/snap-bootstrap/cmd_initramfs_mounts_cvm.go

+	return partitionMounts, nil
+}
+
+var createOverlayDirs = func(path string) error {


this is not tested

Added several test cases.

pedronis · 2024-12-17T15:28:54Z

cmd/snap-bootstrap/cmd_initramfs_mounts_cvm.go

-		// then we need to specify that the data mountpoint is
-		// expected to be a decrypted device
-		diskOpts.IsDecryptedDevice = true
+	if imageManifestFile != nil {


shouldn't this be an "else" of the previous "if"?

I think it is the same logically (could os.ReadFile return both an error and non nil imageManifestFile?). I changed it and put it in the else block.

alfonsosanchezbeato

Thanks for the changes, I have some minor comments

alfonsosanchezbeato · 2024-12-17T18:36:59Z

cmd/snap-bootstrap/cmd_initramfs_mounts_cvm.go

+		} else {
+			// Only one writable partition is permitted.
+			if foundWritablePartition != "" {
+				return nil, errors.New("manifest contains multiple non read-only partitions")


nitpick: maybe "manifest contains multiple writable partitions"

alfonsosanchezbeato · 2024-12-17T18:44:54Z

cmd/snap-bootstrap/export_test.go

@@ -241,3 +247,11 @@ func MockBuildInstallObserver(f func(model *asserts.Model, gadgetDir string, use
 		installBuildInstallObserver = old
 	}
 }
+
+func MockCreateOverlayDirs(f func(path string) error) (restore func()) {


My comment about mocking also applies here, you do not need to mock, you could use osutil.IsDirectory() to check if the folder has been created.

Thanks, I replaced that with a call to osutil.IsDirectory().

alfonsosanchezbeato · 2024-12-17T18:48:35Z

cmd/snap-bootstrap/cmd_initramfs_mounts_cvm.go

-		// then we need to specify that the data mountpoint is
-		// expected to be a decrypted device
-		diskOpts.IsDecryptedDevice = true
+		// XXX: if a manifest file is not found fall-back to CVM v1 behaviour


nitpick: why the XXX in the comment? There is nothing to fix now or in the future here, right?

I tagged with XXX the comments where either I made assumptions about which behavior would be actually most desirable (this case here) or where I foresee things will be changed. For this particular one, I believe this behavior is most likely what we will stick with so I removed the XXX.

alfonsosanchezbeato · 2024-12-17T18:49:15Z

cmd/snap-bootstrap/cmd_initramfs_mounts_cvm.go

+		pm.Where = filepath.Join(boot.InitramfsRunMntDir, p.GptLabel)
+
+		if p.ReadOnly {
+			// XXX: currently only a single read-only partition/overlay fs lowerdir is permitted.


Is this something expected to change in the future? Maybe the comment should clarify this.

We have discussed about having support for multiple lowerdirs in the future to support varying degrees of customization. I clarified this in my comment.

alfonsosanchezbeato · 2024-12-17T18:55:33Z

cmd/snap-bootstrap/cmd_initramfs_mounts_cvm.go

+		// Create overlayfs' upperdir and workdir in the writable tmpfs layer.
+		if pm.Opts.Tmpfs {
+			if err := createOverlayDirs(pm.Where); err != nil {


Are these directories then always expected to exist in a non-temporary writable partition? Maybe the comment could clarify this.

I clarified the comment a bit for now. Currently we mostly care about the ephemeral case that doesn't have a persistent writable layer. If a persistent writable layer exists, I expect these directories to be created by the image creation tool and not by snap-bootstrap.

This was developed in conjunction with canonical/encrypt-cloud-image#26 (the image creation tool) which hasn't been reviewed yet. I might change this to make it actually a responsibility of snap-bootstrap once this gets some reviews.

alfonsosanchezbeato · 2024-12-17T18:56:37Z

cmd/snap-bootstrap/cmd_initramfs_mounts_cvm.go

-		// comes from the same disk as ESP
-		return fmt.Errorf("cannot validate boot: cloudimg-rootfs mountpoint is expected to be from disk %s but is not", disk.Dev())
+
+	// Mount partitions


This assumes that the overlayfs mount is last, right? Maybe it could be good to add that to the comment.

Yes, generateMountsFromManifest should create the partitionMounts array in a specific order (ro, rw, overlayfs mount). I clarified the comment.

alfonsosanchezbeato

LGTM, thanks!

pedronis

+1

alfonsosanchezbeato · 2025-01-06T19:39:51Z

@sespiros there are some static checks failing atm

pedronis requested review from pedronis, valentindavid and alfonsosanchezbeato November 12, 2024 09:58

alfonsosanchezbeato added the Run nested The PR also runs tests inluded in nested suite label Nov 20, 2024

sespiros force-pushed the snap-bootstrap-manifest branch from 5c312d1 to 45d1ecd Compare November 21, 2024 17:28

pedronis requested changes Dec 2, 2024

View reviewed changes

sespiros mentioned this pull request Dec 3, 2024

c/snap-bootstrap: split CVM related functionality in separate files #14789

Merged

sespiros force-pushed the snap-bootstrap-manifest branch from 45d1ecd to b4d37ea Compare December 3, 2024 13:36

sespiros requested a review from pedronis December 3, 2024 13:36

sespiros force-pushed the snap-bootstrap-manifest branch from b4d37ea to 6b717d2 Compare December 4, 2024 11:53

sespiros added 12 commits December 5, 2024 12:48

c/snap-bootstrap: refactor CVM mode

6e0520c

This is a preparatory commit which modifies the existing CVM mode to support mounting multiple overlaid partitions for the rootfs.

c/snap-bootstrap: use partition label to auto-discover the rootfs dm-…

979f766

…verity partition instead

c/snap-bootstrap: refactor manifest parsing code for CVM mode

7c04d9e

c/snap-bootstrap: change path name of writable tmpfs partition

375d3ee

c/snap-bootstrap: make comment more generic

0d15271

c/snap-bootstrap: fix test for CVM mode

54a9460

CVM mode used to mount the rootfs partition with the Ephemeral flag as cloud images contain a fixed fstab entry.

c/snap-bootstrap: remove redundant disk check

7543b7e

This is a legacy check that is not actually needed now as secbootUnlockVolumeUsingSealedKeyIfEncrypted will only find partitions residing on the disk that the ESP is from.

c/snap-bootstrap: add test for CVM ephemeral mode with manifest

c4ab893

sespiros force-pushed the snap-bootstrap-manifest branch from 6b717d2 to c4ab893 Compare December 5, 2024 10:48

alfonsosanchezbeato reviewed Dec 5, 2024

View reviewed changes

sespiros added 3 commits December 16, 2024 14:34

c/snap-bootstrap: make cvm structures private

0d0e20f

c/snap-bootstrap: create a manifest file in test instead of mocking t…

66a6022

…he read

c/snap-bootstrap: simplify manifest and add extra tests

81e0cbe

sespiros requested a review from alfonsosanchezbeato December 17, 2024 13:40

pedronis reviewed Dec 17, 2024

View reviewed changes

sespiros added 3 commits December 17, 2024 20:13

c/snap-bootstrap: minor comment changes in test cases for cvm

88b9b4e

c/snap-bootstrap: address comment wrt to manifest handling in cvm mode

7fd84a6

c/snap-bootstrap: add tests for createOverlayDirs

74993cc

sespiros requested a review from pedronis December 17, 2024 18:16

alfonsosanchezbeato reviewed Dec 17, 2024

View reviewed changes

sespiros added 2 commits December 17, 2024 21:24

cmd/snap-bootstrap: add TODO notice for manifest measurement

c493ddc

c/snap-bootstrap: address comments for cvm mode

08d5943

sespiros requested a review from alfonsosanchezbeato December 17, 2024 19:53

alfonsosanchezbeato approved these changes Dec 17, 2024

View reviewed changes

pedronis approved these changes Dec 18, 2024

View reviewed changes

sespiros added 2 commits January 7, 2025 17:57

cmd/snap-bootstrap: fix comment for inclusive language

5dd550a

cmd/snap-bootstrap: remove unused manifestError

a43c494

		pm.Opts.UpperDir = filepath.Join(boot.InitramfsRunMntDir, foundWritablePartition, "upper")
		pm.Opts.WorkDir = filepath.Join(boot.InitramfsRunMntDir, foundWritablePartition, "work")

snap-bootstrap: load partition information from a manifest file for cloudimg-rootfs mode #14713

Are you sure you want to change the base?

snap-bootstrap: load partition information from a manifest file for cloudimg-rootfs mode #14713

Conversation

sespiros commented Nov 12, 2024 • edited Loading

alfonsosanchezbeato commented Nov 20, 2024

pedronis left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 3, 2024 • edited Loading

Codecov Report

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

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

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

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

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

alfonsosanchezbeato left a comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

alfonsosanchezbeato commented Jan 6, 2025

sespiros commented Nov 12, 2024 •

edited

Loading

codecov bot commented Dec 3, 2024 •

edited

Loading