fixup! many: rework the way we allocate counter handles

overlord/devicestate/devicestate_test.go

@@ -2245,15 +2245,24 @@ func (s *deviceMgrSuite) TestDeviceManagerEnsurePostFactoryResetEncrypted(c *C)
 	defer restore()
 
 	removedOldHandles := 0
-	restore = devicestate.MockSecbootRemoveOldCounterHandles(func(node string, possibleOldKeys map[string]bool, possibleKeyFiles []string) error {
+	restore = devicestate.MockSecbootRemoveOldCounterHandles(func(node string, possibleOldKeys map[string]bool, possibleKeyFiles []string, hintExpectFDEHook bool) error {
 		removedOldHandles++
+		c.Check(hintExpectFDEHook, Equals, false)
 		c.Check(node, Equals, "/dev/disk/by-uuid/FOOUUID")
 		c.Check(possibleOldKeys, DeepEquals, map[string]bool{
 			"factory-reset-old-fallback": true,
 		})
-		c.Check(possibleKeyFiles, DeepEquals, []string{
-			filepath.Join(boot.InitramfsSeedEncryptionKeyDir, "ubuntu-save.recovery.sealed-key"),
-		})
+		switch removedOldHandles {
+		case 1:
+			c.Check(possibleKeyFiles, DeepEquals, []string{
+				filepath.Join(boot.InitramfsSeedEncryptionKeyDir, "ubuntu-save.recovery.sealed-key"),
+			})
+		case 2:
+			c.Check(possibleKeyFiles, IsNil)
+		default:
+			c.Errorf("unexpected call")
+			return fmt.Errorf("unexpected call")
+		}
 		return nil
 	})
 	defer restore()
@@ -2281,15 +2290,14 @@ func (s *deviceMgrSuite) TestDeviceManagerEnsurePostFactoryResetEncrypted(c *C)
 
 	transitionCalls = 0
 	deleteOldSaveKey = 0
-	removedOldHandles = 0
 	// try again, no marker, nothing should happen
 	devicestate.SetPostFactoryResetRan(s.mgr, false)
 	err = s.mgr.Ensure()
 	c.Assert(err, IsNil)
 	// nothing was called
 	c.Check(transitionCalls, Equals, 0)
 	c.Check(deleteOldSaveKey, Equals, 0)
-	c.Check(removedOldHandles, Equals, 0)
+	c.Check(removedOldHandles, Equals, 1)
 
 	// have the marker, but keep the keys rotated as if boot code would do it and
 	// try again, in this setup the marker hash matches the migrated key
@@ -2300,7 +2308,7 @@ func (s *deviceMgrSuite) TestDeviceManagerEnsurePostFactoryResetEncrypted(c *C)
 	c.Assert(err, IsNil)
 	c.Check(transitionCalls, Equals, 0)
 	c.Check(deleteOldSaveKey, Equals, 1)
-	c.Check(removedOldHandles, Equals, 1)
+	c.Check(removedOldHandles, Equals, 2)
 	// the marker was again removed
 	c.Check(filepath.Join(dirs.SnapDeviceDir, "factory-reset"), testutil.FileAbsent)
 }

overlord/devicestate/export_test.go

@@ -640,7 +640,7 @@ func VolumesAuthOptionsKeyByLabel(label string) volumesAuthOptionsKey {
 	return volumesAuthOptionsKey{label}
 }
 
-func MockSecbootRemoveOldCounterHandles(f func(node string, possibleOldKeys map[string]bool, possibleKeyFiles []string) error) (restore func()) {
+func MockSecbootRemoveOldCounterHandles(f func(node string, possibleOldKeys map[string]bool, possibleKeyFiles []string, hintExpectFDEHook bool) error) (restore func()) {
 	old := secbootRemoveOldCounterHandles
 	secbootRemoveOldCounterHandles = f
 	return func() {

overlord/devicestate/handlers_install.go

@@ -1360,7 +1360,18 @@ func createSaveBootstrappedContainer(saveNode string) (secboot.BootstrappedConta
 	return secbootCreateBootstrappedContainer(secboot.DiskUnlockKey(saveEncryptionKey), saveNode), nil
 }
 
+
+// deleteOldKeys remove old keys that were used in previous installation after successful factory reset.
+//  * Key files ubuntu-save.recovery.sealed-key has to be replaced by key file ubuntu-save.recovery.sealed-key.factory-reset
+//  * Keyslots factory-reset-* have to be removed
+//  * TPM handles used by the removed keys have to be released
 func deleteOldKeys(saveMntPnt string) error {
+	hasHook, err := boot.HasFDESetupHook(nil)
+	if err != nil {
+		logger.Noticef("WARNING: cannot figure out if FDE hooks are in use: %v", err)
+		hasHook = false
+	}
+
 	uuid, err := disksDMCryptUUIDFromMountPoint(saveMntPnt)
 	if err != nil {
 		return fmt.Errorf("cannot find save partition: %v", err)
@@ -1372,21 +1383,29 @@ func deleteOldKeys(saveMntPnt string) error {
 		"factory-reset-old-fallback": true,
 	}
 
-	oldKey := device.FallbackSaveSealedKeyUnder(boot.InitramfsSeedEncryptionKeyDir)
+	defaultSaveKey := device.FallbackSaveSealedKeyUnder(boot.InitramfsSeedEncryptionKeyDir)
+	saveFallbackKeyFactory := device.FactoryResetFallbackSaveSealedKeyUnder(boot.InitramfsSeedEncryptionKeyDir)
+
+	var oldKeys []string
+	renameKey := false
+	if osutil.FileExists(saveFallbackKeyFactory) {
+		oldKeys = append(oldKeys, defaultSaveKey)
+		renameKey = true
+	}
 
 	// FIXME: we are missing a bit of context here. We should do this only if we are using TPM2.
 	err = secbootRemoveOldCounterHandles(
 		diskPath,
 		oldPossiblyTPMKeySlots,
-		[]string{oldKey},
+		oldKeys,
+		hasHook,
 	)
 	if err != nil {
 		return fmt.Errorf("could not clean up old counter handles: %v", err)
 	}
 
-	saveFallbackKeyFactory := device.FactoryResetFallbackSaveSealedKeyUnder(boot.InitramfsSeedEncryptionKeyDir)
-	if err := os.Rename(saveFallbackKeyFactory, oldKey); err != nil {
-		if !os.IsNotExist(err) {
+	if renameKey {
+		if err := os.Rename(saveFallbackKeyFactory, defaultSaveKey); err != nil {
 			return fmt.Errorf("cannot rotate fallback key: %v", err)
 		}
 	}

secboot/secboot_dummy.go

@@ -113,6 +113,6 @@ func GetPCRHandle(node, keySlot, keyFile string) (uint32, error) {
 	return 0, errBuildWithoutSecboot
 }
 
-func RemoveOldCounterHandles(node string, possibleOldKeys map[string]bool, possibleKeyFiles []string) error {
+func RemoveOldCounterHandles(node string, possibleOldKeys map[string]bool, possibleKeyFiles []string, hintExpectFDEHook bool) error {
 	return errBuildWithoutSecboot
 }

secboot/secboot_sb_test.go

@@ -3298,6 +3298,7 @@ func (s *secbootSuite) TestRemoveOldCounterHandles(c *C) {
 			"just-ignore",
 			"does-not-exist",
 		},
+		false,
 	)
 
 	c.Assert(err, IsNil)
@@ -3309,6 +3310,79 @@ func (s *secbootSuite) TestRemoveOldCounterHandles(c *C) {
 	})
 }
 
+func (s *secbootSuite) TestRemoveOldCounterHandlesFDEHint(c *C) {
+	restore := secboot.MockListLUKS2ContainerUnlockKeyNames(func(devicePath string) ([]string, error) {
+		c.Check(devicePath, Equals, "foo")
+		return []string{"some-other-key", "some-key"}, nil
+	})
+	defer restore()
+
+	restore = secboot.MockSbNewLUKS2KeyDataReader(func(device, slot string) (sb.KeyDataReader, error) {
+		c.Check(device, Equals, "foo")
+		switch slot {
+		case "some-key":
+			return newFakeKeyDataReader(slot, []byte(`tpm2`)), nil
+		case "some-other-key":
+			return newFakeKeyDataReader(slot, []byte(`other`)), nil
+		default:
+			c.Errorf("unexpected call")
+			return nil, fmt.Errorf("unexpected")
+		}
+	})
+	defer restore()
+
+	restore = secboot.MockReadKeyData(func(reader sb.KeyDataReader) (secboot.MockableKeyData, error) {
+		switch reader.ReadableName() {
+		case "some-key":
+			return &myFakeKeyData{platformName: "not-tpm2-for-sure", handle: secboot.PCRPolicyCounterHandleStart + 1}, nil
+		case "some-other-key":
+			return &myNonTPMFakeKeyData{platformName: "other"}, nil
+		default:
+			c.Errorf("unexpected call")
+			return nil, fmt.Errorf("unexpected")
+		}
+	})
+	defer restore()
+
+	restore = secboot.MockMockableReadKeyFile(func(keyFile string, kl *secboot.MockableKeyLoader, hintExpectFDEHook bool) error {
+		c.Check(hintExpectFDEHook, Equals, true)
+		switch keyFile {
+		case "new-format":
+			kl.KeyData = &myFakeKeyData{platformName: "not-tpm2-for-sure", handle: secboot.PCRPolicyCounterHandleStart + 2}
+		case "just-ignore":
+		case "does-not-exist":
+			return os.ErrNotExist
+		default:
+			c.Errorf("unexpected call")
+			return fmt.Errorf("unexpected")
+		}
+		return nil
+	})
+	defer restore()
+
+	restore = secboot.MockTPMReleaseResources(func(tpm *sb_tpm2.Connection, handle tpm2.Handle) error {
+		c.Errorf("unexpected call")
+		return fmt.Errorf("unexpected call")
+	})
+	defer restore()
+
+	err := secboot.RemoveOldCounterHandles(
+		"foo",
+		map[string]bool{
+			"some-key":       true,
+			"some-other-key": true,
+		},
+		[]string{
+			"new-format",
+			"just-ignore",
+			"does-not-exist",
+		},
+		true,
+	)
+
+	c.Assert(err, IsNil)
+}
+
 func (s *secbootSuite) TestFindFreeHandle(c *C) {
 	_, restore := mockSbTPMConnection(c, nil)
 	defer restore()

secboot/secboot_tpm.go

@@ -936,6 +936,9 @@ func mockableReadKeyFileImpl(keyFile string, keyLoader *mockableKeyLoader, hintE
 
 var mockableReadKeyFile = mockableReadKeyFileImpl
 
+// GetPCRHandle returns the handle used by a key.  The key will be
+// search on as token on node in the keySlot. If keySlot has no
+// KeyData, then the key will be loaded at path keyFile.
 func GetPCRHandle(node, keySlot, keyFile string) (uint32, error) {
 	slots, err := sbListLUKS2ContainerUnlockKeyNames(node)
 	if err != nil {
@@ -972,7 +975,10 @@ func GetPCRHandle(node, keySlot, keyFile string) (uint32, error) {
 	if err != nil {
 		if os.IsNotExist(err) {
 			if readKeyDataErr != nil {
-				// FIXME: we need to check for
+				// FIXME: secboot should tell us if
+				// Data was nil, in that case we
+				// should be silent, otherwise we
+				// should return the error.
 				logger.Noticef("WARNING: cannot read key data for slot %s: %v", keySlot, readKeyDataErr)
 				return 0, nil
 			}
@@ -999,7 +1005,12 @@ func GetPCRHandle(node, keySlot, keyFile string) (uint32, error) {
 	return 0, nil
 }
 
-func RemoveOldCounterHandles(node string, possibleOldKeys map[string]bool, possibleKeyFiles []string) error {
+// RemoveOldCounterHandles tracks and release TPM2 handles used by
+// given keys node and possibleOldKeys point to keyslot tokens. And
+// possibleKeyFiles is a list to paths.  hintExpectFDEHook helps
+// reading old key object files.  If not TPM2 key is found, nothing
+// happens.
+func RemoveOldCounterHandles(node string, possibleOldKeys map[string]bool, possibleKeyFiles []string, hintExpectFDEHook bool) error {
 	slots, err := sbListLUKS2ContainerUnlockKeyNames(node)
 	if err != nil {
 		return fmt.Errorf("cannot list slots in partition save partition: %v", err)
@@ -1011,10 +1022,10 @@ func RemoveOldCounterHandles(node string, possibleOldKeys map[string]bool, possi
 		if possibleOldKeys[slot] {
 			reader, err := sbNewLUKS2KeyDataReader(node, slot)
 			if err != nil {
-				// FIXME: secboot should tell use if
+				// FIXME: secboot should tell us if
 				// Data was nil, in that case we
 				// should be silent, otherwise we
-				// should print an error.
+				// should return the error.
 				continue
 			}
 			keyData, err := mockableReadKeyData(reader)
@@ -1032,7 +1043,6 @@ func RemoveOldCounterHandles(node string, possibleOldKeys map[string]bool, possi
 		}
 	}
 
-	const hintExpectFDEHook = false
 	for _, keyFile := range possibleKeyFiles {
 		loadedKey := &mockableKeyLoader{}
 		err := mockableReadKeyFile(keyFile, loadedKey, hintExpectFDEHook)
@@ -1084,11 +1094,15 @@ func RemoveOldCounterHandles(node string, possibleOldKeys map[string]bool, possi
 		}
 	}
 
-	// FIXME: if we know we are using TPM2, then we should fail if no handle was found
+	if len(oldHandlesList) == 0 {
+		return nil
+	}
 
 	return releasePCRResourceHandles(oldHandlesList...)
 }
 
+// FindFreeHandle finds and unused handle on the TPM.
+// The handle will be arbitrary in range 0x01880005-0x0188000F.
 func FindFreeHandle() (uint32, error) {
 	tpm, err := sbConnectToDefaultTPM()
 	if err != nil {