overlord/devicestate,secboot: fix factory reset on older versions #14471

valentindavid · 2024-09-05T13:21:56Z

Until now, we were just delete old keys on factory-reset instead of
renaming if cryptsetup was too old. But this is a bit too dangerous.
Instead we copy-delete them.

Second, we need to convert old keyslots to have a name so we can
remove them after.

codecov · 2024-10-18T12:10:40Z

Codecov Report

Attention: Patch coverage is 17.77778% with 37 lines in your changes missing coverage. Please review.

Please upload report for BASE (fde-manager-features@ca5418e). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
secboot/secboot_sb.go	13.04%	20 Missing ⚠️
overlord/devicestate/handlers_install.go	31.25%	7 Missing and 4 partials ⚠️
secboot/secboot_dummy.go	0.00%	5 Missing ⚠️
secboot/secboot_tpm.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@                   Coverage Diff                   @@
##             fde-manager-features   #14471   +/-   ##
=======================================================
  Coverage                        ?   77.67%           
=======================================================
  Files                           ?     1168           
  Lines                           ?   155564           
  Branches                        ?        0           
=======================================================
  Hits                            ?   120830           
  Misses                          ?    27239           
  Partials                        ?     7495

Flag	Coverage Δ
unittests	`77.67% <17.77%> (?)`

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.

valentindavid · 2024-11-19T11:38:40Z

This might still need #14718 to work. We will see.

valentindavid · 2024-11-20T16:00:07Z

We are missing a piece, I have forgotten about that. We need to be able to convert LUKS2 disk without tokens to ones with tokens, just for the names, so we can rename and delete them.

valentindavid · 2024-11-20T16:36:09Z

I have opened canonical/secboot#349 to have a way to write factory reset in a way that works.

valentindavid · 2024-12-10T10:47:06Z

This also depends on canonical/secboot#355

valentindavid · 2024-12-10T10:53:40Z

Draft, because it needs PRs to be merged on secboot.

pedronis

some questions/comments

pedronis · 2025-01-24T16:44:38Z

secboot/secboot_sb.go

@@ -348,7 +348,7 @@ func AddBootstrapKeyOnExistingDisk(node string, newKey keys.EncryptionKey) error
 // Rename key slots on LUKS2 container. If the key slot does not
 // exist, it is ignored. If cryptsetup does not support renaming, then
 // the key slots are instead removed.
-func RenameOrDeleteKeys(node string, renames map[string]string) error {
+func RenameKeys(nonAtomic *NonAtomicOperationAllowedFlag, node string, renames map[string]string) error {


are there calls to RenameKeys that will not pass nonAtomic?

At this point we have only one call.

then the nonAtomic bit here is probably overkill, we can just have a comment

OK, I will simplify it then.

pedronis · 2025-01-24T16:47:49Z

overlord/devicestate/handlers_install.go

+	if err := secbootDeleteKeys(diskPath, toDelete); err != nil {
+		return fmt.Errorf("cannot delete previous keys: %w", err)
+	}
+	if err := secbootRemoveOldDiskKeys(diskPath); err != nil {


it a bit strange that we both call DeleteKeys and then call a helper that does a bit of that as well, at a minimum these need comments about what each does

It is also difficult to explain the name of the function. One remove old keys if you reset from a new version. The other one is if you reset from an old version where snapd was updated in the seed with a remodel.

I have added comments before the calls to explain what both are doing.

secboot/secboot_sb.go

bboozzoo · 2025-01-27T08:13:24Z

secboot/secboot_sb.go

@@ -348,7 +348,7 @@ func AddBootstrapKeyOnExistingDisk(node string, newKey keys.EncryptionKey) error
 // Rename key slots on LUKS2 container. If the key slot does not
 // exist, it is ignored. If cryptsetup does not support renaming, then
 // the key slots are instead removed.
-func RenameOrDeleteKeys(node string, renames map[string]string) error {
+func RenameKeys(nonAtomic *NonAtomicOperationAllowedFlag, node string, renames map[string]string) error {


this is a very weird API tbh. The call sites where this isn't passed will see a bare nil as the first argument which is even more confusing than bare true/false. I understand canonical/secboot does this, but it doesn't make it any less weird and it doesn't need to be exposed outside of the secboot package.

If this needs an argument then pass it in a struct:

type RenameKeysFlags struct { AllowNonAtomic bool } func RenameKeys(node string, renames map[string]string, flags RenameKeysFlags) error { ..

It is on purpose weird because should avoid using this mode. Chris was concerned that would get used when we do not need to.

As requested by @pedronis this was simplified.

github-actions · 2025-01-29T12:17:26Z

Tue Feb 4 00:34:40 UTC 2025
The following results are from: https://github.com/canonical/snapd/actions/runs/13119332245

Failures:

Preparing:

openstack:debian-12-64
openstack:debian-sid-64
google-nested:ubuntu-20.04-64:tests/nested/manual/remodel-uc20-to-uc22:encrypted
google-nested:ubuntu-22.04-64:tests/nested/manual/recovery-system-reboot:recover
google-nested:ubuntu-22.04-64:tests/nested/manual/recovery-system-reboot:factory_reset
google-nested:ubuntu-24.04-64:tests/nested/manual/recovery-system-reboot:recover
google-nested:ubuntu-24.04-64:tests/nested/manual/recovery-system-reboot:factory_reset
google-nested:ubuntu-24.04-64:tests/nested/manual/build-with-kernel-modules-components:encrypted
google-nested:ubuntu-24.04-64:tests/nested/manual/core20-fault-inject-on-install-component:kernel_panic_prepare_kernel_components
google-nested:ubuntu-24.04-64:tests/nested/manual/kernel-modules-components:encrypted
google-nested:ubuntu-24.04-64:tests/nested/manual/uc20-fde-hooks:tokens
openstack:opensuse-15.6-64:tests/main/snapctl

Executing:

openstack:debian-sid-64:tests/main/microk8s-smoke:edge
openstack:debian-12-64:tests/main/microk8s-smoke:edge
google-nested:ubuntu-20.04-64:tests/nested/manual/update-snapd-seed-and-factory-reset:tpm
google-nested:ubuntu-20.04-64:tests/nested/manual/remodel-offline:local_assertions
google-nested:ubuntu-20.04-64:tests/nested/manual/remodel-offline:installed_snaps
google-nested:ubuntu-20.04-64:tests/nested/manual/remodel-offline:local_and_installed_snaps
google-nested:ubuntu-24.04-64:tests/nested/manual/remodel-min-size
openstack:opensuse-tumbleweed-64:tests/main/auto-refresh-retry
openstack:opensuse-tumbleweed-64:tests/main/auto-refresh-gating-from-snap
openstack:opensuse-tumbleweed-64:tests/main/auto-refresh:regular
openstack:opensuse-tumbleweed-64:tests/main/refresh-app-awareness-notify
openstack:opensuse-tumbleweed-64:tests/main/auto-refresh:parallel
openstack:opensuse-tumbleweed-64:tests/main/auto-refresh-pre-download:ignore
openstack:opensuse-tumbleweed-64:tests/main/snap-refresh-hold
openstack:opensuse-15.6-64:tests/main/microk8s-smoke:edge
openstack:opensuse-tumbleweed-64:tests/main/auto-refresh-pre-download:close
openstack:opensuse-tumbleweed-64:tests/main/auto-refresh-gating
google:ubuntu-25.04-64:tests/main/security-device-cgroups-required-or-optional
google:ubuntu-25.04-64:tests/main/security-device-cgroups-helper
google:ubuntu-25.04-64:tests/main/cgroup-devices-v2
google:ubuntu-25.04-64:tests/main/security-device-cgroups-serial-port
google:ubuntu-25.04-64:tests/main/security-device-cgroups:uinput
google:ubuntu-25.04-64:tests/main/security-device-cgroups-strict-enforced
google:ubuntu-25.04-64:tests/main/security-device-cgroups-self-manage
google:ubuntu-25.04-64:tests/main/security-device-cgroups:kmsg
google:ubuntu-16.04-64:tests/unit/go:static

Restoring:

openstack:debian-12-64:tests/unit/
openstack:debian-12-64:tests/unit/
openstack:opensuse-tumbleweed-64:tests/main/refresh-app-awareness-notify
openstack:opensuse-15.6-64:tests/main/stale-base-snap
openstack:opensuse-15.6-64:tests/main/
openstack:opensuse-15.6-64
openstack:opensuse-15.6-64:tests/main/snapctl
openstack:opensuse-15.6-64:tests/main/
openstack:opensuse-15.6-64
openstack:opensuse-tumbleweed-64:tests/main/snap-session-agent-unavailable-to-snaps
openstack:opensuse-tumbleweed-64:tests/main/services-after-before-install
openstack:opensuse-tumbleweed-64:tests/main/
openstack:opensuse-tumbleweed-64
openstack:opensuse-tumbleweed-64:tests/main/
openstack:opensuse-tumbleweed-64
google:ubuntu-25.04-64:tests/main/security-device-cgroups-strict-enforced

pedronis

thanks, this looks good, a bunch of naming/commenting remarks.

Can you remind what happens with all the rename if the factory reset doesn't quite finish?

pedronis · 2025-01-29T15:13:16Z

secboot/secboot_sb.go

+// ConvertOldDisk takes on disk using legacy keyslots 0, 1, 2 and add
+// names to those keyslots. This is needed to convert the save disk
+// during a factory reset.
+func ConvertOldDisk(devicePath string) error {


s/Convert/TemporaryNameOldKeys/ would probably describe better what this does

Also this should mention that it is for migrating during factory-reset and that it is a noop if not unamed legacy keyslots exists.

pedronis · 2025-01-29T15:14:12Z

secboot/secboot_sb.go

@@ -348,7 +348,8 @@ func AddBootstrapKeyOnExistingDisk(node string, newKey keys.EncryptionKey) error
 // Rename key slots on LUKS2 container. If the key slot does not
 // exist, it is ignored. If cryptsetup does not support renaming, then
 // the key slots are instead removed.
-func RenameOrDeleteKeys(node string, renames map[string]string) error {
+// WARNING: this function is not atomic. Please avoid using it.


is not always atomic?

Sure, it is not atomic only if cryptsetup is too old. I will amend that.

pedronis · 2025-01-29T15:16:24Z

secboot/secboot_sb.go

+
+// RemoveOldDiskKeys removes key slots from an old installation that
+// had names created by ConvertOldDisk.
+func RemoveOldDiskKeys(devicePath string) error {


wouldn't DeleteOldKeys be more consistent with other naming here?

this will not fail if the key don't exist right?

It is the keys of an old disk.

DeleteOldKeys is fine, I will change to it.

overlord/devicestate/handlers_install.go

pedronis · 2025-01-29T15:20:37Z

overlord/devicestate/handlers_install.go

+		return nil, fmt.Errorf("cannot rename existing keys: %w", err)
+	}
+
+	if err := secbootConvertOldDisk(saveNode); err != nil {


// deal as needed instead with naming unamed keyslots, they will be removed at the end of factory reset

valentindavid · 2025-01-29T16:04:52Z

@pedronis

Can you remind what happens with all the rename if the factory reset doesn't quite finish?

For the preparation of the factory reset, if the preparation is not finished, the keys are already named or renamed.

If the factory reset clean up does not finish, it should run again. Because we have a flag that needs to be removed last.

pedronis

thank you

bboozzoo

LGTM

bboozzoo · 2025-01-30T12:53:45Z

secboot/secboot_sb.go

+// old, it will try to copy and delete keys instead. Please avoid
+// using this function.


Suggested change

// old, it will try to copy and delete keys instead. Please avoid

// using this function.

// old, it will try to copy and delete keys instead. Avoid

// using this function in new code.

bboozzoo · 2025-01-30T12:55:28Z

secboot/secboot_sb.go

+// add names to those keyslots. This is needed to convert the save
+// disk during a factory reset. This is a no-operation if no keyslot
+// are unnamed.


Suggested change

// add names to those keyslots. This is needed to convert the save

// disk during a factory reset. This is a no-operation if no keyslot

// are unnamed.

// adds names to those keyslots. This is needed to convert the save

// disk during a factory reset. This is a no-operation if keyslots

// are already named.

bboozzoo · 2025-01-30T12:57:33Z

tests/nested/manual/update-snapd-seed-and-factory-reset/task.yaml

+
+prepare: |
+    # Install what is needed before using the fake store
+    snap install jq remarshal


shouldn't be needed anymore, nested.sh was updated to use gojq

Suggested change

snap install jq remarshal

chrisccoulson

This looks mostly ok - I've left a few comments, including a suggestion for TemporaryNameOldKeys.

I know we're trying to deliver a MVP for this cycle, but it feels a bit like factory-reset and reprovision are getting tangled together quite a bit. I just want to keep in mind that as per FO166, reprovision should be able to operate independently of factory reset eventually, and it will be required to do so for the post-recovery repair functionality in any case.

chrisccoulson · 2025-01-31T12:05:25Z

overlord/devicestate/handlers_install.go

-		return nil, err
+	// Temporarily rename keyslots across the factory reset to
+	// allow to create the new ones.
+	if err := secbootRenameKeys(saveNode, renames); err != nil {


FO166, which describes reprovisioning for FDE, suggests renaming existing keyslots early on to reprovision-XX (with XX being a numeric identifier).

I can change that.

Not sure though what "persist the mapping of temporary to old key slot names" means this case. I wonder if maybe we can add a FIXME here and change the names once we have this persisting of the mapping.

I could prefix with "reprovision-" but keep the name instead of using an id for now.

chrisccoulson · 2025-01-31T12:15:58Z

secboot/secboot_sb.go

+// disk during a factory reset. This is a no-operation if all keyslots
+// are already named.
+func TemporaryNameOldKeys(devicePath string) error {
+	if err := sb.NameLegacyLUKS2ContainerKey(devicePath, 0, "old-default-key"); err != nil && !errors.Is(err, sb.KeyslotAlreadyHasANameErr) {


As this function is making assumption that the container structure is unmodified from the original install, I think it could be improved a bit to make it safer.

If the error is secboot.KeyslotAlreadyHasANameErr, should you check that the keyslot has the expected name before ignoring the error? I think you can probably do that by listing all names with secboot.ListLUKS2ContainerUnlockKeyNames, and then creating a secboot.LUKSKeyDataReader from each name and calling the KeyslotID method to verify that no unexpected name points to each of the keyslots 0-2.

Or an alternative way might be to turn secboot.KeyslotAlreadyHasANameErr into a structure that contains the keyslot's existing name, or define the type of the error based on a string.

I would prefer doing the removal of other keys that are named out of this function. They are named, so it is part of normal factory reset.

chrisccoulson · 2025-01-31T12:21:37Z

secboot/secboot_tpm.go

@@ -837,7 +837,7 @@ func PCRHandleOfSealedKey(p string) (uint32, error) {
 func tpmReleaseResourcesImpl(tpm *sb_tpm2.Connection, handle tpm2.Handle) error {
 	rc, err := tpm.CreateResourceContextFromTPM(handle)
 	if err != nil {
-		if _, ok := err.(tpm2.ResourceUnavailableError); ok {
+		if errors.Is(err, &tpm2.ResourceUnavailableError{}) {


There is also tpm2.IsResourceUnavailableError

Until now, we were just delete old keys on factory-reset instead of renaming if cryptsetup was too old. But this is a bit too dangerous. Instead we copy-delete them. Second, we need to convert old keyslots to have a name so we can remove them after.

github-actions bot added the Run Nested -auto- Label automatically added in case nested tests need to be executed label Sep 5, 2024

valentindavid added the FDE Manager Pull requests that target FDE manager branch label Sep 10, 2024

valentindavid force-pushed the valentindavid/remodel-then-factory-reset branch from e820f9a to 21ed272 Compare September 11, 2024 10:11

valentindavid force-pushed the valentindavid/remodel-then-factory-reset branch from 21ed272 to 4b3991d Compare September 20, 2024 08:41

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

valentindavid closed this Sep 20, 2024

valentindavid reopened this Sep 20, 2024

pedronis force-pushed the fde-manager-features branch from fcc5e2e to 24018ee Compare October 18, 2024 11:27

valentindavid force-pushed the valentindavid/remodel-then-factory-reset branch from 4b3991d to f463a03 Compare October 18, 2024 11:53

valentindavid force-pushed the valentindavid/remodel-then-factory-reset branch 2 times, most recently from 3fe46ac to 29dd378 Compare November 19, 2024 11:38

valentindavid marked this pull request as ready for review November 19, 2024 11:38

valentindavid force-pushed the valentindavid/remodel-then-factory-reset branch 4 times, most recently from c0efc73 to d43620a Compare November 20, 2024 09:30

valentindavid force-pushed the valentindavid/remodel-then-factory-reset branch from d43620a to bfbb339 Compare December 10, 2024 10:46

This was referenced Dec 10, 2024

crypt.go: allow adding names to legacy keyslots canonical/secboot#349

Merged

crypt.go: allow renaming keys without cryptsetup features canonical/secboot#355

Merged

valentindavid marked this pull request as draft December 10, 2024 10:53

pedronis force-pushed the fde-manager-features branch from 5802840 to 5ec2e43 Compare January 13, 2025 09:50

valentindavid force-pushed the valentindavid/remodel-then-factory-reset branch from bfbb339 to f503fb1 Compare January 13, 2025 11:59

valentindavid mentioned this pull request Jan 21, 2025

tests/nested/manual/core20-new-snapd-does-not-break-old-initrd: remove #14946

Closed

valentindavid force-pushed the valentindavid/remodel-then-factory-reset branch from f503fb1 to 288d176 Compare January 24, 2025 14:55

valentindavid changed the title ~~tests: verify that factory reset after remodeling to a new snapd works~~ overlord/devicestate,secboot: fix factory reset on older versions Jan 24, 2025

valentindavid marked this pull request as ready for review January 24, 2025 14:56

valentindavid force-pushed the valentindavid/remodel-then-factory-reset branch from 288d176 to 7c2e31f Compare January 24, 2025 15:00

valentindavid requested review from bboozzoo and pedronis January 24, 2025 15:42

pedronis reviewed Jan 24, 2025

View reviewed changes

bboozzoo reviewed Jan 27, 2025

View reviewed changes

pedronis force-pushed the fde-manager-features branch from 7005c64 to 25f9119 Compare January 29, 2025 10:49

valentindavid force-pushed the valentindavid/remodel-then-factory-reset branch from 91da819 to 1e6820c Compare January 29, 2025 10:57

valentindavid requested review from pedronis and bboozzoo January 29, 2025 13:23

pedronis reviewed Jan 29, 2025

View reviewed changes

valentindavid force-pushed the valentindavid/remodel-then-factory-reset branch from 5750e24 to b835a09 Compare January 29, 2025 15:37

valentindavid requested a review from pedronis January 29, 2025 16:01

pedronis approved these changes Jan 30, 2025

View reviewed changes

bboozzoo approved these changes Jan 30, 2025

View reviewed changes

valentindavid force-pushed the valentindavid/remodel-then-factory-reset branch from b835a09 to 9a27837 Compare January 31, 2025 08:53

chrisccoulson self-requested a review January 31, 2025 11:47

chrisccoulson reviewed Jan 31, 2025

View reviewed changes

valentindavid force-pushed the valentindavid/remodel-then-factory-reset branch from 9a27837 to 9b8bfaf Compare February 3, 2025 14:36

valentindavid added 2 commits February 3, 2025 18:33

tests: verify that factory reset after remodeling to a new snapd works

d784faa

valentindavid force-pushed the valentindavid/remodel-then-factory-reset branch from 9b8bfaf to d784faa Compare February 3, 2025 17:33

		// old, it will try to copy and delete keys instead. Please avoid
		// using this function.

overlord/devicestate,secboot: fix factory reset on older versions #14471

Are you sure you want to change the base?

overlord/devicestate,secboot: fix factory reset on older versions #14471

Conversation

valentindavid commented Sep 5, 2024 • edited Loading

codecov bot commented Oct 18, 2024 • edited Loading

Codecov Report

valentindavid commented Nov 19, 2024

valentindavid commented Nov 20, 2024

valentindavid commented Nov 20, 2024

valentindavid commented Dec 10, 2024

valentindavid commented Dec 10, 2024

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

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

github-actions bot commented Jan 29, 2025 • edited Loading

Failures:

Preparing:

Executing:

Restoring:

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentindavid commented Jan 29, 2025

pedronis left a comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

chrisccoulson 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

chrisccoulson Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentindavid commented Sep 5, 2024 •

edited

Loading

codecov bot commented Oct 18, 2024 •

edited

Loading

github-actions bot commented Jan 29, 2025 •

edited

Loading

chrisccoulson Jan 31, 2025 •

edited

Loading