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

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.
canonical · Jan 13, 2025 · f503fb1 · f503fb1
1 parent dd8293f
commit f503fb1
Show file tree

Hide file tree

Showing 7 changed files with 89 additions and 46 deletions.
diff --git a/overlord/devicestate/devicestate_install_mode_test.go b/overlord/devicestate/devicestate_install_mode_test.go
@@ -2333,7 +2333,7 @@ func (s *deviceMgrInstallModeSuite) doRunFactoryResetChange(c *C, model *asserts
 		return fmt.Errorf("unexpected call")
 	})()
 
-	defer devicestate.MockSecbootRenameOrDeleteKeys(func(node string, renames map[string]string) error {
+	defer devicestate.MockSecbootRenameKeys(func(node string, renames map[string]string) error {
 		if tc.encrypt {
 			c.Check(node, Equals, "/dev/disk/by-uuid/570faa3d-e3bc-49db-979b-e7814b6bd390")
 			c.Check(renames, DeepEquals, map[string]string{

diff --git a/overlord/devicestate/export_test.go b/overlord/devicestate/export_test.go
@@ -610,11 +610,11 @@ func MockSecbootAddBootstrapKeyOnExistingDisk(f func(node string, newKey keys.En
 	}
 }
 
-func MockSecbootRenameOrDeleteKeys(f func(node string, renames map[string]string) error) (restore func()) {
-	old := secbootRenameOrDeleteKeys
-	secbootRenameOrDeleteKeys = f
+func MockSecbootRenameKeys(f func(node string, renames map[string]string) error) (restore func()) {
+	old := secbootRenameKeys
+	secbootRenameKeys = f
 	return func() {
-		secbootRenameOrDeleteKeys = old
+		secbootRenameKeys = old
 	}
 }
 

diff --git a/overlord/devicestate/handlers_install.go b/overlord/devicestate/handlers_install.go
@@ -1313,7 +1313,7 @@ func (m *DeviceManager) doInstallSetupStorageEncryption(t *state.Task, _ *tomb.T
 
 var (
 	secbootAddBootstrapKeyOnExistingDisk = secboot.AddBootstrapKeyOnExistingDisk
-	secbootRenameOrDeleteKeys            = secboot.RenameOrDeleteKeys
+	secbootRenameKeys                    = secboot.RenameKeys
 	secbootCreateBootstrappedContainer   = secboot.CreateBootstrappedContainer
 	secbootDeleteKeys                    = secboot.DeleteKeys
 )
@@ -1352,8 +1352,12 @@ func createSaveBootstrappedContainer(saveNode string) (secboot.BootstrappedConta
 		"default":          "factory-reset-old",
 		"default-fallback": "factory-reset-old-fallback",
 	}
-	if err := secbootRenameOrDeleteKeys(saveNode, renames); err != nil {
-		return nil, err
+	if err := secbootRenameKeys(saveNode, renames); err != nil {
+		return nil, fmt.Errorf("cannot rename existing keys: %w", err)
+	}
+
+	if err := secboot.ConvertOldDisk(saveNode); err != nil {
+		return nil, fmt.Errorf("cannot convert old keys: %w", err)
 	}
 
 	return secbootCreateBootstrappedContainer(secboot.DiskUnlockKey(saveEncryptionKey), saveNode), nil
@@ -1378,5 +1382,12 @@ func deleteOldSaveKey(saveMntPnt string) error {
 		"factory-reset-old-fallback": true,
 	}
 
-	return secbootDeleteKeys(diskPath, toDelete)
+
+	if err := secbootDeleteKeys(diskPath, toDelete); err != nil {
+		return fmt.Errorf("cannot delete previous keys: %w", err)
+	}
+	if err := secboot.RemoveOldDiskKeys(diskPath); err != nil {
+		return fmt.Errorf("cannot remove old disk keys: %w", err)
+	}
+	return nil
 }
diff --git a/secboot/export_sb_test.go b/secboot/export_sb_test.go
@@ -330,6 +330,14 @@ func MockRenameLUKS2ContainerKey(f func(devicePath, keyslotName, renameTo string
 	}
 }
 
+func MockCopyAndRemoveLUKS2ContainerKey(f func(devicePath, keyslotName, renameTo string) error) (restore func()) {
+	old := sbCopyAndRemoveLUKS2ContainerKey
+	sbCopyAndRemoveLUKS2ContainerKey = f
+	return func() {
+		sbCopyAndRemoveLUKS2ContainerKey = old
+	}
+}
+
 func MockSbNewFileKeyDataReader(f func(path string) (*sb.FileKeyDataReader, error)) (restore func()) {
 	old := sbNewFileKeyDataReader
 	sbNewFileKeyDataReader = f

diff --git a/secboot/secboot_dummy.go b/secboot/secboot_dummy.go
@@ -79,7 +79,7 @@ func AddBootstrapKeyOnExistingDisk(node string, newKey keys.EncryptionKey) error
 	return errBuildWithoutSecboot
 }
 
-func RenameOrDeleteKeys(node string, renames map[string]string) error {
+func RenameKeys(node string, renames map[string]string) error {
 	return errBuildWithoutSecboot
 }
 

diff --git a/secboot/secboot_sb.go b/secboot/secboot_sb.go
@@ -44,14 +44,15 @@ func sbNewLUKS2KeyDataReaderImpl(device, slot string) (sb.KeyDataReader, error)
 }
 
 var (
-	sbActivateVolumeWithKey         = sb.ActivateVolumeWithKey
-	sbActivateVolumeWithKeyData     = sb.ActivateVolumeWithKeyData
-	sbActivateVolumeWithRecoveryKey = sb.ActivateVolumeWithRecoveryKey
-	sbDeactivateVolume              = sb.DeactivateVolume
-	sbAddLUKS2ContainerUnlockKey    = sb.AddLUKS2ContainerUnlockKey
-	sbRenameLUKS2ContainerKey       = sb.RenameLUKS2ContainerKey
-	sbNewLUKS2KeyDataReader         = sbNewLUKS2KeyDataReaderImpl
-	sbSetProtectorKeys              = sb_plainkey.SetProtectorKeys
+	sbActivateVolumeWithKey          = sb.ActivateVolumeWithKey
+	sbActivateVolumeWithKeyData      = sb.ActivateVolumeWithKeyData
+	sbActivateVolumeWithRecoveryKey  = sb.ActivateVolumeWithRecoveryKey
+	sbDeactivateVolume               = sb.DeactivateVolume
+	sbAddLUKS2ContainerUnlockKey     = sb.AddLUKS2ContainerUnlockKey
+	sbRenameLUKS2ContainerKey        = sb.RenameLUKS2ContainerKey
+	sbCopyAndRemoveLUKS2ContainerKey = sb.CopyAndRemoveLUKS2ContainerKey
+	sbNewLUKS2KeyDataReader          = sbNewLUKS2KeyDataReaderImpl
+	sbSetProtectorKeys               = sb_plainkey.SetProtectorKeys
 )
 
 func init() {
@@ -348,7 +349,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(node string, renames map[string]string) error {
 	targets := make(map[string]bool)
 
 	for _, renameTo := range renames {
@@ -378,8 +379,8 @@ func RenameOrDeleteKeys(node string, renames map[string]string) error {
 		if found {
 			if err := sbRenameLUKS2ContainerKey(node, slot, renameTo); err != nil {
 				if errors.Is(err, sb.ErrMissingCryptsetupFeature) {
-					if err := sbDeleteLUKS2ContainerKey(node, slot); err != nil {
-						return fmt.Errorf("cannot remove old container key: %v", err)
+					if err := sbCopyAndRemoveLUKS2ContainerKey(node, slot, renameTo); err != nil {
+						return fmt.Errorf("cannot rename old container key: %v", err)
 					}
 				} else {
 					return fmt.Errorf("cannot rename container key: %v", err)
@@ -456,3 +457,25 @@ func (key *SealKeyRequest) getWriter() (sb.KeyDataWriter, error) {
 		return key.BootstrappedContainer.GetTokenWriter(key.SlotName)
 	}
 }
+
+func ConvertOldDisk(devicePath string) error {
+	if err := sb.NameLegacyLUKS2ContainerKey(devicePath, 0, "old-default-key"); err != nil && !errors.Is(err, sb.KeyslotAlreadyHasAName) {
+		return err
+	}
+	if err := sb.NameLegacyLUKS2ContainerKey(devicePath, 1, "old-recovery-key"); err != nil && !errors.Is(err, sb.KeyslotAlreadyHasAName) {
+		return err
+	}
+	if err := sb.NameLegacyLUKS2ContainerKey(devicePath, 2, "old-temporary-key"); err != nil && !errors.Is(err, sb.KeyslotAlreadyHasAName) {
+		return err
+	}
+	return nil
+}
+
+func RemoveOldDiskKeys(devicePath string) error {
+	toDelete := map[string]bool{
+		"old-default-key":   true,
+		"old-recovery-key":  true,
+		"old-temporary-key": true,
+	}
+	return DeleteKeys(devicePath, toDelete)
+}
diff --git a/secboot/secboot_sb_test.go b/secboot/secboot_sb_test.go
@@ -2540,7 +2540,7 @@ func (s *secbootSuite) TestAddBootstrapKeyOnExistingDiskLUKS2Error(c *C) {
 	c.Check(err, ErrorMatches, `cannot enroll new installation key: some error`)
 }
 
-func (s *secbootSuite) TestRenameOrDeleteKeys(c *C) {
+func (s *secbootSuite) TestRenameKeys(c *C) {
 	defer secboot.MockListLUKS2ContainerUnlockKeyNames(func(devicePath string) ([]string, error) {
 		c.Check(devicePath, Equals, "/dev/foo")
 		return []string{"slot-a", "slot-b", "slot-c"}, nil
@@ -2566,13 +2566,13 @@ func (s *secbootSuite) TestRenameOrDeleteKeys(c *C) {
 		"slot-d": "new-slot-d",
 	}
 
-	err := secboot.RenameOrDeleteKeys("/dev/foo", toRename)
+	err := secboot.RenameKeys("/dev/foo", toRename)
 	c.Assert(err, IsNil)
 
 	c.Check(expectedRenames, HasLen, 0)
 }
 
-func (s *secbootSuite) TestRenameOrDeleteKeysBadInput(c *C) {
+func (s *secbootSuite) TestRenameKeysBadInput(c *C) {
 	defer secboot.MockListLUKS2ContainerUnlockKeyNames(func(devicePath string) ([]string, error) {
 		c.Errorf("unexpected call")
 		return []string{"slot-a", "slot-b", "slot-c"}, nil
@@ -2588,11 +2588,11 @@ func (s *secbootSuite) TestRenameOrDeleteKeysBadInput(c *C) {
 		"slot-b": "slot-c",
 	}
 
-	err := secboot.RenameOrDeleteKeys("/dev/foo", toRename)
+	err := secboot.RenameKeys("/dev/foo", toRename)
 	c.Assert(err, ErrorMatches, `internal error: keyslot name slot-b used as source and target of a rename`)
 }
 
-func (s *secbootSuite) TestRenameOrDeleteKeysNameExists(c *C) {
+func (s *secbootSuite) TestRenameKeysNameExists(c *C) {
 	defer secboot.MockListLUKS2ContainerUnlockKeyNames(func(devicePath string) ([]string, error) {
 		c.Check(devicePath, Equals, "/dev/foo")
 		return []string{"slot-a", "slot-b", "slot-c"}, nil
@@ -2607,31 +2607,32 @@ func (s *secbootSuite) TestRenameOrDeleteKeysNameExists(c *C) {
 		"slot-a": "slot-b",
 	}
 
-	err := secboot.RenameOrDeleteKeys("/dev/foo", toRename)
+	err := secboot.RenameKeys("/dev/foo", toRename)
 	c.Assert(err, ErrorMatches, `slot name slot-b is already in use`)
 }
 
-func (s *secbootSuite) TestRenameOrDeleteKeysNoRename(c *C) {
+func (s *secbootSuite) TestRenameKeysNoRename(c *C) {
 	defer secboot.MockListLUKS2ContainerUnlockKeyNames(func(devicePath string) ([]string, error) {
 		c.Check(devicePath, Equals, "/dev/foo")
 		return []string{"slot-a", "slot-b", "slot-c"}, nil
 	})()
 
-	expectedRemovals := map[string]bool{
-		"slot-b": true,
-		"slot-c": true,
+	expectedRenames := map[string]string{
+		"slot-b": "new-slot-b",
+		"slot-c": "new-slot-c",
 	}
 
 	defer secboot.MockRenameLUKS2ContainerKey(func(devicePath, slotName, renameTo string) error {
 		c.Check(devicePath, Equals, "/dev/foo")
 		return sb.ErrMissingCryptsetupFeature
 	})()
 
-	defer secboot.MockDeleteLUKS2ContainerKey(func(devicePath, slotName string) error {
+	defer secboot.MockCopyAndRemoveLUKS2ContainerKey(func(devicePath, slotName, renameTo string) error {
 		c.Check(devicePath, Equals, "/dev/foo")
-		_, expected := expectedRemovals[slotName]
-		c.Check(expected, Equals, true)
-		delete(expectedRemovals, slotName)
+		expectedRename, expected := expectedRenames[slotName]
+		c.Assert(expected, Equals, true)
+		c.Check(renameTo, Equals, expectedRename)
+		delete(expectedRenames, slotName)
 		return nil
 	})()
 
@@ -2641,13 +2642,13 @@ func (s *secbootSuite) TestRenameOrDeleteKeysNoRename(c *C) {
 		"slot-d": "new-slot-d",
 	}
 
-	err := secboot.RenameOrDeleteKeys("/dev/foo", toRename)
+	err := secboot.RenameKeys("/dev/foo", toRename)
 	c.Assert(err, IsNil)
 
-	c.Check(expectedRemovals, HasLen, 0)
+	c.Check(expectedRenames, HasLen, 0)
 }
 
-func (s *secbootSuite) TestRenameOrDeleteKeysListError(c *C) {
+func (s *secbootSuite) TestRenameKeysListError(c *C) {
 	defer secboot.MockListLUKS2ContainerUnlockKeyNames(func(devicePath string) ([]string, error) {
 		c.Check(devicePath, Equals, "/dev/foo")
 		return nil, fmt.Errorf("some error")
@@ -2669,11 +2670,11 @@ func (s *secbootSuite) TestRenameOrDeleteKeysListError(c *C) {
 		"slot-d": "new-slot-d",
 	}
 
-	err := secboot.RenameOrDeleteKeys("/dev/foo", toRename)
+	err := secboot.RenameKeys("/dev/foo", toRename)
 	c.Assert(err, ErrorMatches, `cannot list slots in partition save partition: some error`)
 }
 
-func (s *secbootSuite) TestRenameOrDeleteKeysRenameError(c *C) {
+func (s *secbootSuite) TestRenameKeysRenameError(c *C) {
 	defer secboot.MockListLUKS2ContainerUnlockKeyNames(func(devicePath string) ([]string, error) {
 		c.Check(devicePath, Equals, "/dev/foo")
 		return []string{"slot-a", "slot-b", "slot-c"}, nil
@@ -2683,7 +2684,7 @@ func (s *secbootSuite) TestRenameOrDeleteKeysRenameError(c *C) {
 		return fmt.Errorf("some other error")
 	})()
 
-	defer secboot.MockDeleteLUKS2ContainerKey(func(devicePath, slotName string) error {
+	defer secboot.MockCopyAndRemoveLUKS2ContainerKey(func(devicePath, slotName, renameTo string) error {
 		c.Errorf("unexpected call")
 		return nil
 	})()
@@ -2694,11 +2695,11 @@ func (s *secbootSuite) TestRenameOrDeleteKeysRenameError(c *C) {
 		"slot-d": "new-slot-d",
 	}
 
-	err := secboot.RenameOrDeleteKeys("/dev/foo", toRename)
+	err := secboot.RenameKeys("/dev/foo", toRename)
 	c.Assert(err, ErrorMatches, `cannot rename container key: some other error`)
 }
 
-func (s *secbootSuite) TestRenameOrDeleteKeysDeleteError(c *C) {
+func (s *secbootSuite) TestRenameKeysDeleteError(c *C) {
 	defer secboot.MockListLUKS2ContainerUnlockKeyNames(func(devicePath string) ([]string, error) {
 		c.Check(devicePath, Equals, "/dev/foo")
 		return []string{"slot-a", "slot-b", "slot-c"}, nil
@@ -2708,7 +2709,7 @@ func (s *secbootSuite) TestRenameOrDeleteKeysDeleteError(c *C) {
 		return sb.ErrMissingCryptsetupFeature
 	})()
 
-	defer secboot.MockDeleteLUKS2ContainerKey(func(devicePath, slotName string) error {
+	defer secboot.MockCopyAndRemoveLUKS2ContainerKey(func(devicePath, slotName, renameTo string) error {
 		return fmt.Errorf("some error")
 	})()
 
@@ -2718,8 +2719,8 @@ func (s *secbootSuite) TestRenameOrDeleteKeysDeleteError(c *C) {
 		"slot-d": "new-slot-d",
 	}
 
-	err := secboot.RenameOrDeleteKeys("/dev/foo", toRename)
-	c.Assert(err, ErrorMatches, `cannot remove old container key: some error`)
+	err := secboot.RenameKeys("/dev/foo", toRename)
+	c.Assert(err, ErrorMatches, `cannot rename old container key: some error`)
 }
 
 func (s *secbootSuite) TestDeleteKeys(c *C) {