many: update secboot and use new key format #14304

valentindavid · 2024-08-05T11:02:32Z

This is the second part of moving to new secboot. This one is where we start using the new format.

~~This PR is draft as it is on top of #14282 (being split into smaller PRs) and #14253 (the first part of updating secboot).~~

valentindavid · 2024-08-26T11:34:10Z

~~This updates to secboot snapcore/secboot@8dd835e19590 because later version use a broken go dependency.~~

valentindavid · 2024-08-26T12:31:07Z

go.mod

 	golang.org/x/term v0.8.0 // indirect
 	maze.io/x/crypto v0.0.0-20190131090603-9b94c9afe066 // indirect
 )
+
+// github.com/intel-go was taken over
+replace github.com/intel-go/cpuid => github.com/aregm/cpuid v0.0.0-20220614022739-219e067757cb


We need a fix in secboot for this. @chrisccoulson ?

Yes, I'm going to push this to the @canonical org.

valentindavid · 2024-08-27T17:11:50Z

gadget/install/install.go

+		// TODO: when save partition does not have slots, then
+		// we need to create new partition the old fashion
+		// way.  Or, we should upgrade the save partition
+		// (which should be safe since the seed should not be
+		// considered safe to downgrade).


I think I need to create a spread test for that case first.

Install with an old snapd.

Remodel with a newer snapd.

Factory reset with the new snapd.

It is possible that it is already working, but we are left with some non-slot keys that we should remove.

pedronis

did an initial pass

secboot/encrypt_sb.go

secboot/secboot_hooks.go

secboot/bootstrap_container.go

secboot/secboot_sb.go

pedronis · 2024-08-28T11:46:46Z

overlord/devicestate/handlers_install_sb.go

+	renames := map[string]string{
+		"default":          "factory-reset-old",
+		"default-fallback": "factory-reset-old-fallback",
+		"save":             "factory-reset-old-save",
+	}
+	if err := secboot.RenameOrDeleteKeys(saveNode, renames); err != nil {
+		return nil, err
+	}


what this bit of code is doing probably needs a comment

I have added a comment.

I am wondering if we should remove all the other keys (that is not "default" or "default-fallback"). We do remove the recovery keys. But in the future we will might have other keys. And it is not obvious which ones will need to be removed. Maybe all?

should probably leave a TODO somewhere about the fact that factory reset should indeed remove all extra keys when done?

overlord/devicestate/handlers_install_sb.go

secboot/secboot_sb.go

overlord/devicestate/handlers_install_sb.go

overlord/install/install.go

secboot/secboot_sb.go

secboot/encrypt_sb.go

secboot/secboot_tpm.go

pedronis

thank you, notice that I also replied to some discussions from the previous pass

pedronis · 2024-09-02T14:24:29Z

secboot/bootstrap_container_sb.go

+func (bc *bootstrappedContainer) AddKey(slotName string, newKey []byte, token bool) (KeyDataWriter, error) {
+	if slotName == "" {
+		slotName = "default"
+	}
+	if err := sb.AddLUKS2ContainerUnlockKey(bc.devicePath, slotName, sb.DiskUnlockKey(bc.key), sb.DiskUnlockKey(newKey)); err != nil {
+		return nil, err
+	}
+	if !token {
+		return nil, nil
+	}
+	return nil, fmt.Errorf("not implemented")
+}
+
+func (bc *bootstrappedContainer) RemoveBootstrapKey() error {


these don't seem exercised by tests

bboozzoo · 2024-09-04T11:14:20Z

boot/seal.go

+	} {
+		if container != nil {
+			if err := container.RemoveBootstrapKey(); err != nil {
+				// This could be a warning


shall we log it then?

If we change it from an error to a warning, yes we should log it. But I do not think we should log here yet since we do return an error.

bboozzoo · 2024-09-04T11:24:07Z

secboot/secboot_sb.go

+
+// AddBootstrapKeyOnExistingDisk will add a new bootstrap key to on
+// an existing encrypted disk. The disk is expected to be unlocked and
+// they key is available on . The bootstrap key is temporary and is


available on ?

I t was the keyring. I have pushed a fix.

bboozzoo · 2024-09-04T12:03:24Z

data/systemd/snapd.service.in

@@ -27,6 +27,8 @@ NotifyAccess=all
 SuccessExitStatus=42
 RestartPreventExitStatus=42
 KillMode=process
+#TODO: test with "shared"
+KeyringMode=inherit


wouldn't every process we spawn how have access to the same keyring we inherited from systemd? I suppose with this we could simply call snapd-fde-keymgr without systemd-run too?

Yes, we will call everything else with access to the keyring. But we should really not call anything we cannot trust anyway as we run as root and anything we call could just dispatch anything to systemd. Let's say there is a vulnerability in sfdisk, we are already vulnerable then. Unless we start dropping privileges on whatever we execute.

I can follow up with removing systemd-run from calling snapd-fde-keymgr. But it is a lot of changes in tests for very little.

No, it's fine, I don't think there's an urgent need to do anything about snapd-fde-keymgr right now. I think we should look into ensuring that the external things we run are isolated from us in a reasonable manner, and preferably invoked through systemd-run where possible.

bboozzoo

LGTM

bboozzoo · 2024-09-10T07:56:51Z

secboot/secboot_sb.go

+	if err != nil {
+		return fmt.Errorf("cannot list slots in partition save partition: %v", err)
+	}
+	for _, slot := range slots {


is there a possible ordering issue when the target name repeats again as the name to rename from?

Is that if renames contains for example A -> B, B -> C. Then maybe A is renamed to B, or to C depending on the order?

I have added checks for 2 cases:

As a precondition, give an internal error if we have something like a -> b, b -> c. We cannot use the same name as a target or source.

Before we do the renaming, we verify that no target name exists in the luks2 container already. This is not an internal error.

I have added also 2 FIXME notes in the caller part of the code. I do not think we need to save the "default" key. And I think we need to delete "default-fallback" if "factory-reset-old-fallback" exists, and not do the rename.

bboozzoo · 2024-09-10T07:58:57Z

secboot/encrypt_sb.go

+		slots, err := sbListLUKS2ContainerUnlockKeyNames(dev)
+		if err != nil {
+			return keys.RecoveryKey{}, fmt.Errorf("cannot find list keys for: %v", err)
+		}
+		if len(slots) == 0 {


wonder if this maybe could be refactored into a more explicit function testing for the legacy setup?

or could use a comment maybe

What about adding hasOnlyLegacyKeys := len(slots) == 0. And use that? I do not see anywhere else in the code where we need that.

that's fine, with a comment that key slot names were not used previously and lack of them indicates a legacy setup

chrisccoulson

I've left a small number of comments, but this seems to be ok

chrisccoulson · 2024-09-16T14:18:13Z

cmd/snap-bootstrap/cmd_initramfs_mounts.go

@@ -100,7 +100,7 @@ var (
 	secbootMeasureSnapSystemEpochWhenPossible    func() error
 	secbootMeasureSnapModelWhenPossible          func(findModel func() (*asserts.Model, error)) error
 	secbootUnlockVolumeUsingSealedKeyIfEncrypted func(disk disks.Disk, name string, encryptionKeyFile string, opts *secboot.UnlockVolumeUsingSealedKeyOptions) (secboot.UnlockResult, error)
-	secbootUnlockEncryptedVolumeUsingKey         func(disk disks.Disk, name string, key []byte) (secboot.UnlockResult, error)
+	secbootUnlockEncryptedVolumeUsingPlatformKey func(disk disks.Disk, name string, key []byte) (secboot.UnlockResult, error)


At some point, then intention is that snap-bootstrap calls plainkey.SetProtectorKeys, and then there will just be a single API for unlocking data and save.

Right. Though that will still be here for the key data in files. That is for backward compliance.

I do not think we need that rename in this PR. That was an intermediate state of the spike, but later. I will check if I can revert this.

chrisccoulson · 2024-09-16T17:53:20Z

data/systemd/snapd.service.in

@@ -27,6 +27,8 @@ NotifyAccess=all
 SuccessExitStatus=42
 RestartPreventExitStatus=42
 KillMode=process
+#TODO: test with "shared"
+KeyringMode=inherit


shared should work as well. The main difference is that in inherit mode, systemd leaves the session keyring pointer untouched (in this case, NULL, because PID1 does nothing to initialise a session keyring), so the kernel initialises it to the kernel created user session keyring associated with the root user on first access. In shared mode, systemd creates a session keyring with a link to the user keyring in it, so that the user keyring becomes "possessed" and possessor permissions apply when accessing it. In private mode, the link to the user keyring is missing so the service can access the user keyring with user permissions but not posseessor permissions. The security benefits of this are limited though - the user keyring can be linked in to a private mode session keyring because the permissions of the user keyring permit user linking. So, even the default private works here, but snapd would need to add a link to the user keyring from its session keyring, which is just extra work.

I have set it to shared.

chrisccoulson · 2024-09-16T17:58:17Z

gadget/install/install_test.go

@@ -377,13 +362,20 @@ fi
 	if opts.encryption {
 		runOpts.EncryptionType = secboot.EncryptionTypeLUKS
 	}
+
+	defer install.MockCryptsetupOpen(func(key secboot.DiskUnlockKey, node, name string) error {
+		return nil


Should these mocks test that any of the arguments are the expected values?

chrisccoulson · 2024-09-16T18:00:31Z

go.mod

 	golang.org/x/term v0.8.0 // indirect
 	maze.io/x/crypto v0.0.0-20190131090603-9b94c9afe066 // indirect
 )
+
+// github.com/intel-go was taken over
+replace github.com/intel-go/cpuid => github.com/aregm/cpuid v0.0.0-20220614022739-219e067757cb


Yes, I'm going to push this to the @canonical org.

valentindavid added Run nested The PR also runs tests inluded in nested suite FDE Manager Pull requests that target FDE manager branch labels Aug 5, 2024

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

valentindavid force-pushed the valentindavid/update-secboot-part2-new-keyformat branch 9 times, most recently from 7183b85 to 63e5383 Compare August 8, 2024 14:50

valentindavid force-pushed the valentindavid/update-secboot-part2-new-keyformat branch 3 times, most recently from 948f2b3 to 3060fd6 Compare August 16, 2024 11:07

valentindavid force-pushed the valentindavid/update-secboot-part2-new-keyformat branch from 3060fd6 to 71aa49c Compare August 26, 2024 08:48

valentindavid marked this pull request as ready for review August 26, 2024 08:49

valentindavid force-pushed the valentindavid/update-secboot-part2-new-keyformat branch 2 times, most recently from 0940e30 to b9e785f Compare August 26, 2024 11:32

valentindavid force-pushed the valentindavid/update-secboot-part2-new-keyformat branch from b9e785f to d742f96 Compare August 26, 2024 11:54

valentindavid commented Aug 26, 2024

View reviewed changes

valentindavid force-pushed the valentindavid/update-secboot-part2-new-keyformat branch 6 times, most recently from 6a8f879 to dcb925e Compare August 27, 2024 17:01

valentindavid commented Aug 27, 2024

View reviewed changes

valentindavid requested a review from pedronis August 27, 2024 17:13

valentindavid force-pushed the valentindavid/update-secboot-part2-new-keyformat branch 4 times, most recently from d274f23 to eb18dca Compare August 28, 2024 10:57

pedronis reviewed Aug 28, 2024

View reviewed changes

secboot/secboot_sb.go Outdated Show resolved Hide resolved

valentindavid commented Aug 30, 2024

View reviewed changes

secboot/encrypt_sb.go Show resolved Hide resolved

valentindavid commented Aug 30, 2024

View reviewed changes

secboot/encrypt_sb.go Show resolved Hide resolved

valentindavid commented Aug 30, 2024

View reviewed changes

secboot/secboot_tpm.go Show resolved Hide resolved

valentindavid requested review from pedronis and chrisccoulson August 30, 2024 09:09

pedronis approved these changes Sep 2, 2024

View reviewed changes

valentindavid requested a review from bboozzoo September 4, 2024 08:21

bboozzoo reviewed Sep 4, 2024

View reviewed changes

valentindavid requested a review from bboozzoo September 4, 2024 12:51

valentindavid mentioned this pull request Sep 5, 2024

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

Draft

valentindavid force-pushed the valentindavid/update-secboot-part2-new-keyformat branch from bbcdecb to f5591f5 Compare September 9, 2024 15:12

bboozzoo approved these changes Sep 10, 2024

View reviewed changes

valentindavid force-pushed the valentindavid/update-secboot-part2-new-keyformat branch from 89166a0 to 7178818 Compare September 13, 2024 11:19

chrisccoulson approved these changes Sep 16, 2024

View reviewed changes

valentindavid added 2 commits September 19, 2024 16:22

many: update secboot and use new key format

6380e6e

tests/lib/muinstaller: fix snapcraft.yaml syntax

85f269e

valentindavid force-pushed the valentindavid/update-secboot-part2-new-keyformat branch from 1b74e68 to 85f269e Compare September 19, 2024 14:22

ernestl merged commit 5afd0bc into canonical:fde-manager-features Sep 20, 2024
41 of 51 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

many: update secboot and use new key format #14304

many: update secboot and use new key format #14304

valentindavid commented Aug 5, 2024 •

edited

Loading

valentindavid commented Aug 26, 2024 •

edited

Loading

valentindavid Aug 26, 2024

chrisccoulson Sep 16, 2024

valentindavid Aug 27, 2024 •

edited

Loading

pedronis left a comment

pedronis Aug 28, 2024

valentindavid Aug 29, 2024

pedronis Sep 2, 2024

pedronis left a comment

pedronis Sep 2, 2024 •

edited

Loading

bboozzoo Sep 4, 2024

valentindavid Sep 4, 2024

bboozzoo Sep 4, 2024

valentindavid Sep 4, 2024

bboozzoo Sep 4, 2024

valentindavid Sep 4, 2024

bboozzoo Sep 4, 2024

bboozzoo left a comment

bboozzoo Sep 10, 2024

valentindavid Sep 10, 2024

valentindavid Sep 10, 2024

valentindavid Sep 10, 2024

bboozzoo Sep 10, 2024

valentindavid Sep 10, 2024

bboozzoo Sep 10, 2024

chrisccoulson left a comment

chrisccoulson Sep 16, 2024

valentindavid Sep 17, 2024

chrisccoulson Sep 16, 2024 •

edited

Loading

valentindavid Sep 17, 2024

chrisccoulson Sep 16, 2024

valentindavid Sep 17, 2024

chrisccoulson Sep 16, 2024

many: update secboot and use new key format #14304

many: update secboot and use new key format #14304

Conversation

valentindavid commented Aug 5, 2024 • edited Loading

valentindavid commented Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentindavid Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

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

pedronis left a comment

Choose a reason for hiding this comment

pedronis Sep 2, 2024 • edited Loading

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

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

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

chrisccoulson Sep 16, 2024 • edited Loading

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 Aug 5, 2024 •

edited

Loading

valentindavid commented Aug 26, 2024 •

edited

Loading

valentindavid Aug 27, 2024 •

edited

Loading

pedronis Sep 2, 2024 •

edited

Loading

chrisccoulson Sep 16, 2024 •

edited

Loading