overlord/devicestate/handlers_reprovision.go: do not remove keyring keys

Author: valentindavid

core-initrd/build-source-pkgs.sh

@@ -60,6 +60,7 @@ EOF
 if [ -n "${TEST_BUILD-}" ]; then
     # Use local code for test builds
     printf "\nreplace github.com/snapcore/snapd => ../../\n" >> go.mod
+    echo "replace github.com/snapcore/secboot v0.0.0-20260814094831-dd95d855ad64 => github.com/valentindavid/secboot v0.0.0-20260923082132-b24039e2d038" >> go.mod
 fi
 # solve dependencies
 go mod tidy

go.mod

@@ -56,3 +56,5 @@ require (
 	golang.org/x/term v0.20.0 // indirect
 	maze.io/x/crypto v0.0.0-20190131090603-9b94c9afe066 // indirect
 )
+
+replace github.com/snapcore/secboot v0.0.0-20260814094831-dd95d855ad64 => github.com/valentindavid/secboot v0.0.0-20260923082132-b24039e2d038

go.sum

@@ -61,9 +61,9 @@ github.com/seccomp/libseccomp-golang v0.9.2-0.20220502024300-f57e1d55ea18 h1:A15
 github.com/seccomp/libseccomp-golang v0.9.2-0.20220502024300-f57e1d55ea18/go.mod h1:JA8cRccbGaA1s33RQf7Y1+q9gHmZX1yB/z9WDN1C6fg=
 github.com/snapcore/maze.io-x-crypto v0.0.0-20190131090603-9b94c9afe066 h1:InG0EmriMOiI4YgtQNOo+6fNxzLCYioo3Q3BCVLdMCE=
 github.com/snapcore/maze.io-x-crypto v0.0.0-20190131090603-9b94c9afe066/go.mod h1:VuAdaITF1MrGzxPU+8GxagM1HW2vg7QhEFEeGHbmEMU=
-github.com/snapcore/secboot v0.0.0-20260814094831-dd95d855ad64 h1:jZBwQI4+0/dypcKdQk9v5yi5pfKMVvXs/KG2XQlNjyI=
-github.com/snapcore/secboot v0.0.0-20260814094831-dd95d855ad64/go.mod h1:/J5bNHw8HkyxuUZRrk2LUzkne3zO/kEwJlm+/oB16SE=
 github.com/stretchr/testify v1.8.1 h1:w7B6lhMri9wdJUVmEZPGGhZzrYTPvgJArz7wNPgYKsk=
+github.com/valentindavid/secboot v0.0.0-20260923082132-b24039e2d038 h1:0w1akpvJms7A05lLdOdv5GHGul9THQgo1AJV1MWH0uY=
+github.com/valentindavid/secboot v0.0.0-20260923082132-b24039e2d038/go.mod h1:/J5bNHw8HkyxuUZRrk2LUzkne3zO/kEwJlm+/oB16SE=
 go.etcd.io/bbolt v1.3.9 h1:8x7aARPEXiXbHmtUwAIv7eV2fQFHrLLavdiJ3uzJXoI=
 go.etcd.io/bbolt v1.3.9/go.mod h1:zaO32+Ti0PK1ivdPtgMESzuzL2VPoIG1PCQNvOdo/dE=
 golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w=

overlord/devicestate/handlers_reprovision.go

@@ -53,6 +53,7 @@ var (
 	secbootDeleteContainerKey            = secboot.DeleteContainerKey
 	secbootSaveCheckResult               = (*secboot.PreinstallCheckContext).SaveCheckResult
 	secbootCheckResult                   = (*secboot.PreinstallCheckContext).CheckResult
+	secbootIsKeyUsedByKeyring            = secboot.IsKeyUsedByKeyring
 
 	keysNewProtectorKey    = keys.NewProtectorKey
 	keysCreateProtectedKey = (keys.ProtectorKey).CreateProtectedKey
@@ -87,6 +88,41 @@ func hookKeyProtectorFactoryImpl(m *DeviceManager, kernelInfo *snap.Info) (secbo
 
 var hookKeyProtectorFactory = hookKeyProtectorFactoryImpl
 
+func removeKeyringBackupIfUnused(devicePath, keyslot string) error {
+	usedByKeyring, err := secbootIsKeyUsedByKeyring(devicePath, keyslot)
+	if err != nil {
+		return fmt.Errorf("cannot verify if key %s:%s was used to unlock disk: %v", devicePath, keyslot, err)
+	} else if !usedByKeyring {
+		if err := secbootDeleteContainerKey(devicePath, keyslot); err != nil {
+			if !errors.Is(err, secboot.ErrKeyslotNameNotExist) {
+				return err
+			}
+		}
+	}
+	return nil
+}
+
+func safelyRemoveKey(devicePath, keyslot, backup string) error {
+	usedByKeyring, err := secbootIsKeyUsedByKeyring(devicePath, keyslot)
+	if err != nil {
+		return fmt.Errorf("cannot verify if key %s:%s was used to unlock disk: %v", devicePath, keyslot, err)
+	} else if usedByKeyring {
+		if err := removeKeyringBackupIfUnused(devicePath, backup); err != nil {
+			return err
+		}
+		if err := secbootRenameContainerKey(devicePath, keyslot, backup); err != nil {
+			return err
+		}
+	} else {
+		if err := secbootDeleteContainerKey(devicePath, keyslot); err != nil {
+			if !errors.Is(err, secboot.ErrKeyslotNameNotExist) {
+				return err
+			}
+		}
+	}
+	return nil
+}
+
 func (m *DeviceManager) doReprovision(t *state.Task, _ *tomb.Tomb) error {
 	renames := []struct {
 		// new is the name of latest keyslot if there are 2
@@ -148,6 +184,8 @@ func (m *DeviceManager) doReprovision(t *state.Task, _ *tomb.Tomb) error {
 		return err
 	}
 
+	const backupName = "snapd-active-key-backup"
+
 	// revertReprovisionAttempt is called either if we detect that we have already
 	// called reprovision but did not reach step 6. Or if we
 	// return with error before step 6.
@@ -179,6 +217,10 @@ func (m *DeviceManager) doReprovision(t *state.Task, _ *tomb.Tomb) error {
 				hasKeySlot[k] = true
 			}
 
+			if err := removeKeyringBackupIfUnused(disk, backupName); err != nil {
+				logger.Debugf("cannot remove keyring backup key for %s: %v", disk, err)
+			}
+
 			for _, rename := range renames {
 				if hasPlatformKeyslot[rename.old] && hasPlatformKeyslot[rename.new] {
 					nv, err := secbootGetPCRHandleFromToken(disk, rename.new)
@@ -203,7 +245,7 @@ func (m *DeviceManager) doReprovision(t *state.Task, _ *tomb.Tomb) error {
 				if rename.new == "default" && disk == saveDisk.DevPath() {
 					continue
 				}
-				if err := secbootDeleteContainerKey(disk, rename.new); err != nil {
+				if err := safelyRemoveKey(disk, rename.new, backupName); err != nil {
 					logger.Debugf("cannot remove %s on %s: %v", rename.new, disk, err)
 				}
 				if err := secbootRenameContainerKey(disk, rename.old, rename.new); err != nil {
@@ -231,7 +273,7 @@ func (m *DeviceManager) doReprovision(t *state.Task, _ *tomb.Tomb) error {
 				// be removed before we try to do step 1 (rename existing key slots). But those
 				// "new" keys are wrong only if the "snapd-reprovision-default" of save matches
 				// the protector key file.
-				if err := secbootDeleteContainerKey(saveDisk.DevPath(), "default"); err != nil {
+				if err := safelyRemoveKey(saveDisk.DevPath(), "default", backupName); err != nil {
 					logger.Debugf("could not remove default on %s: %v", saveDisk.DevPath(), err)
 				}
 				if err := secbootRenameContainerKey(saveDisk.DevPath(), "snapd-reprovision-default", "default"); err != nil {
@@ -324,7 +366,7 @@ func (m *DeviceManager) doReprovision(t *state.Task, _ *tomb.Tomb) error {
 		}
 
 		for _, rename := range renames {
-			if err := secbootDeleteContainerKey(disk, rename.old); err != nil {
+			if err := safelyRemoveKey(disk, rename.old, backupName); err != nil {
 				// We do not expect it to exist, we should not fail on error.
 				// For example due to previous run not cleaned up.
 				// We know it is not a key that is still in use because we
@@ -500,6 +542,11 @@ func (m *DeviceManager) doReprovision(t *state.Task, _ *tomb.Tomb) error {
 	removedNvIndices := map[uint32]bool{}
 
 	for _, disk := range []string{dataDisk.DevPath(), saveDisk.DevPath()} {
+		// Since we committed the keyring, we should be able to clean up the keyring backup keys
+		if err := removeKeyringBackupIfUnused(disk, backupName); err != nil {
+			return err
+		}
+
 		recoveryKeyNames, err := secbootListContainerRecoveryKeyNames(disk)
 		if err != nil {
 			return err
@@ -508,7 +555,7 @@ func (m *DeviceManager) doReprovision(t *state.Task, _ *tomb.Tomb) error {
 			if key == "default-recovery" {
 				continue
 			}
-			if err := secbootDeleteContainerKey(disk, key); err != nil {
+			if err := safelyRemoveKey(disk, key, backupName); err != nil {
 				return err
 			}
 		}
@@ -539,13 +586,13 @@ func (m *DeviceManager) doReprovision(t *state.Task, _ *tomb.Tomb) error {
 				continue
 			}
 
-			if err := secbootDeleteContainerKey(disk, key); err != nil {
+			if err := safelyRemoveKey(disk, key, backupName); err != nil {
 				return err
 			}
 		}
 	}
 
-	if err := secbootDeleteContainerKey(saveDisk.DevPath(), "snapd-reprovision-default"); err != nil {
+	if err := safelyRemoveKey(saveDisk.DevPath(), "snapd-reprovision-default", backupName); err != nil {
 		return err
 	}
 

secboot/secboot_sb.go

@@ -50,16 +50,17 @@ func sbNewLUKS2KeyDataReaderImpl(device, slot string) (sb.KeyDataReader, error)
 }
 
 var (
-	sbFindStorageContainer       = sb.FindStorageContainer
-	sbAddLUKS2ContainerUnlockKey = sb.AddLUKS2ContainerUnlockKey
-	sbRenameLUKS2ContainerKey    = sb.RenameLUKS2ContainerKey
-	sbNewLUKS2KeyDataReader      = sbNewLUKS2KeyDataReaderImpl
-	sbSetProtectorKeys           = sb_plainkey.SetProtectorKeys
-	sbGetPrimaryKeyFromKernel    = sb.GetPrimaryKeyFromKernel
-	sbTestLUKS2ContainerKey      = sb.TestLUKS2ContainerKey
-	sbCheckPassphraseEntropy     = sb.CheckPassphraseEntropy
-	disksDevlinks                = disks.Devlinks
-	sbNewActivateContext         = sb.NewActivateContext
+	sbFindStorageContainer            = sb.FindStorageContainer
+	sbAddLUKS2ContainerUnlockKey      = sb.AddLUKS2ContainerUnlockKey
+	sbRenameLUKS2ContainerKey         = sb.RenameLUKS2ContainerKey
+	sbNewLUKS2KeyDataReader           = sbNewLUKS2KeyDataReaderImpl
+	sbSetProtectorKeys                = sb_plainkey.SetProtectorKeys
+	sbGetPrimaryKeyFromKernel         = sb.GetPrimaryKeyFromKernel
+	sbTestLUKS2ContainerKey           = sb.TestLUKS2ContainerKey
+	sbTestLUKS2ContainerKeyForKeyslot = sb.TestLUKS2ContainerKeyForKeyslot
+	sbCheckPassphraseEntropy          = sb.CheckPassphraseEntropy
+	disksDevlinks                     = disks.Devlinks
+	sbNewActivateContext              = sb.NewActivateContext
 
 	sbKeyDataChangePassphrase = (*sb.KeyData).ChangePassphrase
 	sbKeyDataChangePIN        = (*sb.KeyData).ChangePIN
@@ -74,6 +75,8 @@ var (
 	sbWithPassphraseTries                  = sb.WithPassphraseTries
 	sbWithPINTries                         = sb.WithPINTries
 	sbWithAuthRequestorUserVisibleName     = sb.WithAuthRequestorUserVisibleName
+
+	ErrKeyslotNameNotExist = sb.ErrKeyslotNameNotExist
 )
 
 func init() {
@@ -1107,3 +1110,54 @@ func validatePINImpl(pin string) error {
 	_, err := secboot.ParsePIN(pin)
 	return err
 }
+
+func findUnlockKey(devicePath string) ([]byte, error) {
+	const remove = false
+	key, err := sbGetDiskUnlockKeyFromKernel(keyringPrefix, devicePath, remove)
+	if err == nil {
+		return key, nil
+	}
+	if !errors.Is(err, sb.ErrKernelKeyNotFound) {
+		return nil, err
+	}
+
+	// Old kernels will use "by-partuuid" symlinks. So let's
+	// look at all the symlinks of the device.
+	devlinks, errDevlinks := disksDevlinks(devicePath)
+	if errDevlinks != nil {
+		return nil, err
+	}
+	var errDevlink error
+	for _, devlink := range devlinks {
+		if !strings.HasPrefix(devlink, "/dev/disk/by-partuuid/") {
+			continue
+		}
+		key, errDevlink = sbGetDiskUnlockKeyFromKernel(keyringPrefix, devlink, remove)
+		if errDevlink == nil {
+			return key, nil
+		}
+	}
+	return nil, err
+}
+
+func IsKeyUsedByKeyring(devicePath string, name string) (bool, error) {
+	unlockKey, err := findUnlockKey(devicePath)
+	if err != nil {
+		if errors.Is(err, sb.ErrKernelKeyNotFound) {
+			return false, nil
+		} else {
+			return false, err
+		}
+	}
+
+	isUsed, err := sbTestLUKS2ContainerKeyForKeyslot(devicePath, name, unlockKey)
+	if err != nil {
+		if errors.Is(err, sb.ErrKeyslotNameNotExist) {
+			return false, nil
+		} else {
+			return false, err
+		}
+	}
+
+	return isUsed, err
+}

tests/nested/manual/hybrid-fde-reprovision/task.yaml

@@ -164,34 +164,7 @@ execute: |
   reprovision_change_3="$(gojq --raw-output '.change' <reprovision-3.resp)"
   retry -n 100 --wait 5 sh -c "remote.exec sudo snap changes | MATCH '^${reprovision_change_3}\s+(Done|Undone|Error)'"
 
-  if remote.exec sudo snap changes | NOMATCH "^${reprovision_change_3}\s+Done"; then
-    # TODO: for now if we got in a broken state where we unlocked some disks with new keys, then we will not be able to
-    # finish reprovision. However after a attempt and a reboot, we should be able to fix that.
-    # Verify we are in such a case
-    remote.exec sudo cat /run/snapd/snap-bootstrap/unlocked.json >unlocked-post-last-reprovision.json
-    test "$(gojq -r '."ubuntu-data"."unlock-key"' <unlocked-post-last-reprovision.json)" = run
-
-    boot_id="$(tests.nested boot-id)"
-    tests.nested vm stop
-    tests.nested vm start
-    remote.wait-for reboot "${boot_id}"
-
-    remote.wait-for snap-command
-
-    wait_for_auto_repair_state autorepair-just-reboot.json not-attempted
-    api_get_v2_system_info_storage_encrypted | gojq -c '.result.recommendations' | MATCH 'require-reprovision'
-
-    remote.exec "sudo snap debug api /v2/systems?running=true 2>/dev/null" | gojq '.result["storage-encryption"].support' | MATCH "available"
-
-    echo '{"action": "generate-recovery-key"}' | remote.exec "sudo snap debug api -X POST -H 'Content-Type: application/json' /v2/systems" >rkey-reprovision-4.resp
-    gojq --raw-output '.result."recovery-key"' < rkey-reprovision-4.resp > rkey-reprovision-4.out
-
-    echo '{"action":"reprovision"}' | remote.exec "sudo snap debug api -X POST -H 'Content-Type: application/json' /v2/systems" >reprovision-4.resp
-
-    reprovision_change_4="$(gojq --raw-output '.change' <reprovision-4.resp)"
-    retry -n 100 --wait 5 sh -c "remote.exec sudo snap changes | MATCH '^${reprovision_change_4}\s+(Done|Undone|Error)'"
-    remote.exec sudo snap changes | MATCH "^${reprovision_change_4}\s+Done"
-  fi
+  remote.exec sudo snap changes | MATCH "^${reprovision_change_3}\s+Done"
 
   tests.nested vm set-recovery-key ""