secboot: fix enrolling of recovery key

canonical · Jun 5, 2024 · 217935a · 217935a
1 parent 3fa33ec
commit 217935a
Show file tree

Hide file tree

Showing 4 changed files with 82 additions and 15 deletions.
diff --git a/secboot/encrypt_sb.go b/secboot/encrypt_sb.go
@@ -39,11 +39,12 @@ import (
 )
 
 var (
-	sbInitializeLUKS2Container         = sb.InitializeLUKS2Container
-	sbGetDiskUnlockKeyFromKernel       = sb.GetDiskUnlockKeyFromKernel
-	sbAddLUKS2ContainerRecoveryKey     = sb.AddLUKS2ContainerRecoveryKey
-	sbListLUKS2ContainerUnlockKeyNames = sb.ListLUKS2ContainerUnlockKeyNames
-	sbDeleteLUKS2ContainerKey          = sb.DeleteLUKS2ContainerKey
+	sbInitializeLUKS2Container           = sb.InitializeLUKS2Container
+	sbGetDiskUnlockKeyFromKernel         = sb.GetDiskUnlockKeyFromKernel
+	sbAddLUKS2ContainerRecoveryKey       = sb.AddLUKS2ContainerRecoveryKey
+	sbListLUKS2ContainerUnlockKeyNames   = sb.ListLUKS2ContainerUnlockKeyNames
+	sbDeleteLUKS2ContainerKey            = sb.DeleteLUKS2ContainerKey
+	sbListLUKS2ContainerRecoveryKeyNames = sb.ListLUKS2ContainerRecoveryKeyNames
 )
 
 const keyslotsAreaKiBSize = 2560 // 2.5MB
@@ -134,10 +135,31 @@ func EnsureRecoveryKey(keyFile string, rkeyDevs []RecoveryKeyDevice) (keys.Recov
 		return keys.RecoveryKey{}, fmt.Errorf("some encrypted partitions use new slots, whereas other use legacy slots")
 	}
 	if len(legacyCmdline) == 0 {
-		recoveryKey, err := keys.NewRecoveryKey()
+		var recoveryKey keys.RecoveryKey
+
+		f, err := os.OpenFile(keyFile, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0600)
 		if err != nil {
-			return keys.RecoveryKey{}, fmt.Errorf("cannot create new recovery key: %v", err)
+			if os.IsExist(err) {
+				readKey, err := keys.RecoveryKeyFromFile(keyFile)
+				if err != nil {
+					return keys.RecoveryKey{}, fmt.Errorf("cannot read recovery key file %s: %v", keyFile, err)
+				}
+				recoveryKey = *readKey
+			} else {
+				return keys.RecoveryKey{}, fmt.Errorf("cannot open recovery key file %s: %v", keyFile, err)
+			}
+		} else {
+			defer f.Close()
+			newKey, err := keys.NewRecoveryKey()
+			if err != nil {
+				return keys.RecoveryKey{}, fmt.Errorf("cannot create new recovery key: %v", err)
+			}
+			recoveryKey = newKey
+			if _, err := f.Write(recoveryKey[:]); err != nil {
+				return keys.RecoveryKey{}, fmt.Errorf("cannot create write recovery key %s: %v", keyFile, err)
+			}
 		}
+
 		for _, device := range newDevices {
 			var unlockKey []byte
 			if device.keyFile != "" {
@@ -155,8 +177,22 @@ func EnsureRecoveryKey(keyFile string, rkeyDevs []RecoveryKeyDevice) (keys.Recov
 				unlockKey = key
 			}
 
-			if err := sbAddLUKS2ContainerRecoveryKey(device.node, "default-recovery", sb.DiskUnlockKey(unlockKey), sb.RecoveryKey(recoveryKey)); err != nil {
-				return keys.RecoveryKey{}, fmt.Errorf("cannot enroll new recovery key: %v", err)
+			// FIXME: we should try to enroll the key and check the error instead of verifying the key is there
+			slots, err := sbListLUKS2ContainerRecoveryKeyNames(device.node)
+			if err != nil {
+				return keys.RecoveryKey{}, fmt.Errorf("cannot list keys on disk %s: %v", device.node, err)
+			}
+			keyExists := false
+			for _, slot := range slots {
+				if slot == "default-recovery" {
+					keyExists = true
+					break
+				}
+			}
+			if !keyExists {
+				if err := sbAddLUKS2ContainerRecoveryKey(device.node, "default-recovery", sb.DiskUnlockKey(unlockKey), sb.RecoveryKey(recoveryKey)); err != nil {
+					return keys.RecoveryKey{}, fmt.Errorf("cannot enroll new recovery key for %s: %v", device.node, err)
+				}
 			}
 		}
 
@@ -197,6 +233,8 @@ func devByPartUUIDFromMount(mp string) (string, error) {
 func RemoveRecoveryKeys(rkeyDevToKey map[RecoveryKeyDevice]string) error {
 	var legacyCmdline []string
 	var newDevices []string
+	var keyFiles []string
+
 	for rkeyDev, keyFile := range rkeyDevToKey {
 		dev, err := devByPartUUIDFromMount(rkeyDev.Mountpoint)
 		if err != nil {
@@ -219,6 +257,7 @@ func RemoveRecoveryKeys(rkeyDevToKey map[RecoveryKeyDevice]string) error {
 			}...)
 		} else {
 			newDevices = append(newDevices, dev)
+			keyFiles = append(keyFiles, keyFile)
 		}
 	}
 
@@ -231,6 +270,11 @@ func RemoveRecoveryKeys(rkeyDevToKey map[RecoveryKeyDevice]string) error {
 				return fmt.Errorf("cannot remove recovery key: %v", err)
 			}
 		}
+		for _, keyFile := range keyFiles {
+			if err := os.Remove(keyFile); err != nil && !os.IsNotExist(err) {
+				return fmt.Errorf("cannot remove key file %s: %v", keyFile, err)
+			}
+		}
 
 		return nil
 

diff --git a/secboot/encrypt_sb_test.go b/secboot/encrypt_sb_test.go
@@ -348,6 +348,9 @@ func (s *keymgrSuite) TestEnsureRecoveryKey(c *C) {
 	defer secboot.MockListLUKS2ContainerUnlockKeyNames(func(devicePath string) ([]string, error) {
 		return []string{"default"}, nil
 	})()
+	defer secboot.MockListLUKS2ContainerRecoveryKeyNames(func(devicePath string) ([]string, error) {
+		return []string{}, nil
+	})()
 	defer secboot.MockGetDiskUnlockKeyFromKernel(func(prefix string, devicePath string, remove bool) (sb.DiskUnlockKey, error) {
 		return []byte{1, 2, 3, 4, 1, 2, 3, 4, 1, 2, 3, 4, 1, 2, 3, 4}, nil
 	})()

diff --git a/secboot/export_sb_test.go b/secboot/export_sb_test.go
@@ -260,6 +260,14 @@ func MockListLUKS2ContainerUnlockKeyNames(f func(devicePath string) ([]string, e
 	}
 }
 
+func MockListLUKS2ContainerRecoveryKeyNames(f func(devicePath string) ([]string, error)) (restore func()) {
+	old := sbListLUKS2ContainerRecoveryKeyNames
+	sbListLUKS2ContainerRecoveryKeyNames = f
+	return func() {
+		sbListLUKS2ContainerRecoveryKeyNames = old
+	}
+}
+
 func MockGetDiskUnlockKeyFromKernel(f func(prefix string, devicePath string, remove bool) (sb.DiskUnlockKey, error)) (restore func()) {
 	old := sbGetDiskUnlockKeyFromKernel
 	sbGetDiskUnlockKeyFromKernel = f

diff --git a/tests/nested/core/core20-basic/task.yaml b/tests/nested/core/core20-basic/task.yaml
@@ -43,8 +43,12 @@ execute: |
     fi
 
     # single key for ubuntu-data and ubuntu-save
-    test "$(remote.exec "sudo cryptsetup luksDump /dev/vda4 |grep Key:" | wc -l)" = "1"
-    test "$(remote.exec "sudo cryptsetup luksDump /dev/vda5 |grep Key:" | wc -l)" = "1"
+    # FIXME: for now save partition has 2 keys, one which is sealed in
+    # tpm, one that is data partition plain.
+    test "$(remote.exec "sudo cryptsetup luksDump /dev/vda4 |grep Key:" | wc -l)" = "2"
+    # FIXME: for now data partition has 2 keys, default, and
+    # default-fallback. Both are sealed in tpm.
+    test "$(remote.exec "sudo cryptsetup luksDump /dev/vda5 |grep Key:" | wc -l)" = "2"
 
     echo "Ensure 'snap debug show-keys' works as root"
     remote.exec "sudo snap recovery --show-keys" > show-keys.out
@@ -59,8 +63,16 @@ execute: |
     remote.exec "test -f /var/lib/snapd/device/fde/recovery.key"
     remote.exec "test ! -f /var/lib/snapd/device/fde/reinstall.key"
     # and each partition has 2 keys now
-    test "$(remote.exec "sudo cryptsetup luksDump /dev/vda4 |grep Key:" | wc -l)" = "2"
-    test "$(remote.exec "sudo cryptsetup luksDump /dev/vda5 |grep Key:" | wc -l)" = "2"
+    echo "luksDump for /dev/vda4"
+    remote.exec "sudo cryptsetup luksDump /dev/vda4"
+    echo "luksDump for /dev/vda5"
+    remote.exec "sudo cryptsetup luksDump /dev/vda5"
+    # FIXME: for now save partition has 3 keys, one which is sealed in
+    # tpm, one that is data partition plain. Then one recovery key.
+    test "$(remote.exec "sudo cryptsetup luksDump /dev/vda4 |grep Key:" | wc -l)" = "3"
+    # FIXME: for now data partition has 3 keys, default, and
+    # default-fallback. Both are sealed in tpm. Then one that is recovery.
+    test "$(remote.exec "sudo cryptsetup luksDump /dev/vda5 |grep Key:" | wc -l)" = "3"
 
     echo "But not as user (normal file permissions prevent this)"
     if remote.exec "snap recovery --show-keys"; then
@@ -76,8 +88,8 @@ execute: |
     remote.exec "test ! -f /var/lib/snapd/device/fde/recovery.key"
     remote.exec "test ! -f /var/lib/snapd/device/fde/reinstall.key"
     # back to having just one key
-    test "$(remote.exec "sudo cryptsetup luksDump /dev/vda4 |grep Key:" | wc -l)" = "1"
-    test "$(remote.exec "sudo cryptsetup luksDump /dev/vda5 |grep Key:" | wc -l)" = "1"
+    test "$(remote.exec "sudo cryptsetup luksDump /dev/vda4 |grep Key:" | wc -l)" = "2"
+    test "$(remote.exec "sudo cryptsetup luksDump /dev/vda5 |grep Key:" | wc -l)" = "2"
 
     echo "Check that the serial backed up to save is as expected"
     remote.exec 'cat /var/lib/snapd/save/device/asserts-v0/serial/'"$(tests.nested get model-authority)"'/pc/*/active' >serial.saved