boot: reseal when changing kernel #9331

bboozzoo · 2020-09-11T15:12:14Z

stacked on #9337 now

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

adjust tests ATM we should never have sealed keys and not a TrustedAssetsBootloader

pedronis · 2020-09-12T18:18:50Z

@mvo5 I rebased this now on #9337 and fixed/changed some things, unit tests should pass in boot

pedronis

a bunch of comments, I have addressed some in here already, and some I'm addressing in #9340

pedronis · 2020-09-13T06:17:56Z

boot/bootstate20.go

@@ -93,6 +96,9 @@ type bootStateUpdate20 struct {

 	// tasks to run after the modeenv has been written
 	postModeenvTasks []bootCommitTask
+
+	// device
+	dev Device


I think we should store just the model atm (when we will have to support remodeling it will get more complex but let's do things one at a time), something like resealModel

This is done in #9340

pedronis · 2020-09-13T06:18:20Z

boot/bootstate20.go

@@ -103,6 +109,10 @@ func (u20 *bootStateUpdate20) postModeenv(task bootCommitTask) {
 	u20.postModeenvTasks = append(u20.postModeenvTasks, task)
 }

+func (u20 *bootStateUpdate20) setDevice(dev Device) {


this then could have a meaningful name like resealForModel(model)

pedronis · 2020-09-13T06:18:38Z

boot/bootstate20.go

-	//            in between, we are still able to boot properly
+	if u20.dev != nil {
+		model := u20.dev.Model()
+		if model.Grade() != asserts.ModelGradeUnset {


this should always be true in our case

pedronis · 2020-09-13T06:20:06Z

boot/bootstate20.go

+		model := u20.dev.Model()
+		if model.Grade() != asserts.ModelGradeUnset {
+			// reseal if needed
+			if err := resealKeyToModeenv(model, u20.writeModeenv); err != nil {


we'll have to think whether to use the fact we have written the modeenv in the if block before as hint to expect or not a reseal, maybe it's useful with unasserted kernel, anyway this can be a follow up

pedronis · 2020-09-13T06:20:22Z

boot/bootstate20.go

+		if model.Grade() != asserts.ModelGradeUnset {
+			// reseal if needed
+			if err := resealKeyToModeenv(model, u20.writeModeenv); err != nil {
+				return fmt.Errorf("cannot reseal encryption key: %v", err)


should be just return err to avoid repetitive errors

pedronis · 2020-09-13T06:21:35Z

boot/boottest/device.go

+	return d.model
+}
+
+func (d *mockDevice) SetModel(model *asserts.Model) { d.model = model }


we discussed this, but in hindsight it's confusing because then Kernel and Base return values get out of sync vs Model. I proposed #9339 instead

pedronis · 2020-09-13T06:22:24Z

boot/seal.go

-	recoveryBootChains, err := recoveryBootChainsForSystems(modeenv.CurrentRecoverySystems, rbl, model, modeenv)
+	tbl, ok := rbl.(bootloader.TrustedAssetsBootloader)
+	if !ok {
+		// nothing to do


I made this an internal error now that we know whether we have sealed keys at all or not

pedronis · 2020-09-13T06:22:49Z

boot/seal.go

@@ -59,8 +59,13 @@ func sealKeyToModeenv(key secboot.EncryptionKey, model *asserts.Model, modeenv *
 	if err != nil {
 		return fmt.Errorf("cannot find the recovery bootloader: %v", err)
 	}
+	tbl, ok := rbl.(bootloader.TrustedAssetsBootloader)
+	if !ok {
+		// nothing to do


this should be an internal error I think...

pedronis · 2020-09-13T06:29:19Z

boot/bootstate20.go

@@ -640,7 +664,9 @@ func genericInitramfsSelectSnap(bs bootState20, modeenv *Modeenv, expectedTrySta

 // bootState20BootAssets implements the successfulBootState interface for trusted
 // boot assets UC20.
-type bootState20BootAssets struct{}
+type bootState20BootAssets struct {
+	dev Device


same about this being something like resealModel

pedronis · 2020-09-13T06:30:41Z

boot/boot_test.go

+	tab.BootChainList = []bootloader.BootFile{
+		bootloader.NewBootFile("", "asset", bootloader.RoleRunMode),
+		// TODO:UC20: fix mocked trusted assets bootloader to actually
+		// geenerate kernel boot files


this TODO is interesting, we'll have to think how to avoid having the mock code guess

pedronis · 2020-09-13T07:54:35Z

I noew addressed most of my own comments with the changes proposed via #9340

… not in use

anonymouse64

I didn't get a chance to look at the tests, but the rest of the main code looks good to me

anonymouse64 · 2020-09-13T12:11:00Z

boot/seal.go

+		logger.Debugf("reseal not necessary")
+		return nil
+	}
+	pbcJSON, _ := json.Marshal(pbc)


same comment as on other PR, but why no error checking here?

…eal-with-kernel

…eal-with-kernel Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

…eal-with-kernel

This test adds checks that resealing happens after the kernel gets updated.

pedronis · 2020-09-15T13:47:00Z

tests/nested/core20/kernel-reseal/task.yaml

+  . "$TESTSLIB/nested.sh"
+
+  # Wait for snapd to be seeded
+  nested_exec sudo snap wait system seed.loaded


later we can drop this

pedronis

lgtm assuming we will land #9340 after

anonymouse64

One question about the unit tests in boot, otherwise lgtm

anonymouse64 · 2020-09-15T14:12:37Z

boot/boot_test.go

+		"snap_kernel":     s.kern2.Filename(),
+		"snap_try_kernel": boot.DefaultStatus,
+	}
+	c.Assert(tab.BootVars, DeepEquals, expected)


this is fine for now, but in general I'd like to move away from accessing BootVars directly like this and instead use the GetBootVars method from the Bootloader interface as it makes it easier to make changes to the MockBootloader interface and such

anonymouse64 · 2020-09-15T14:13:05Z

boot/boot_test.go

+	m2, err := boot.ReadModeenv("")
+	c.Assert(err, IsNil)
+	c.Assert(m2.CurrentKernels, DeepEquals, []string{s.kern1.Filename(), s.kern2.Filename()})
+


we should check the kernel_status bl var as well here

If the run is green, I'll push that in a followup.

this should get covered in #9340 I suppose

anonymouse64 · 2020-09-15T14:16:55Z

boot/boot_test.go

+	tab.BootChainList = []bootloader.BootFile{
+		bootloader.NewBootFile("", "shim", bootloader.RoleRecovery),
+		bootloader.NewBootFile("", "asset", bootloader.RoleRecovery),
+		runKernelBf,
+	}


I see this is the same as other tests, but it now occurs to me whether this is the right chain for run mode?

I thought with run mode our chain was something like:
recovery shim -> recovery grub -> run grub -> run kernel

or are these tests just being simplistic because it's easier than building the full chain? or perhaps I have again misunderstood how the chain is used when resealing?

as discussed on IRC with @pedronis, this is simplistic but it's fine for now, we have more realistic tests in seal_test.go so this is fine for now

bboozzoo · 2020-09-15T16:06:19Z

Spread run finished, the following failed on core20 nested:

2020-09-15T15:30:25.0076160Z 2020-09-15 15:30:25 Failed tasks: 2
2020-09-15T15:30:25.0076705Z     - google-nested:ubuntu-20.04-64:tests/nested/core20/basic
2020-09-15T15:30:25.0077412Z     - google-nested:ubuntu-20.04-64:tests/nested/manual/minimal-smoke

tests/nested/core20/basic which expected test-snapd-sh to be there but looking at the log, snap install command hit a timeout:

2020-09-15T14:42:59.9007179Z + nested_exec 'sudo snap install test-snapd-sh'
2020-09-15T14:42:59.9008238Z + sshpass -p ubuntu ssh -p 8022 -o ConnectTimeout=10 -o UserKnownHostsFile=/dev/null -o StrictHostKeyChecking=no user1@localhost 'sudo snap install test-snapd-sh'
2020-09-15T14:42:59.9009297Z Warning: Permanently added '[localhost]:8022' (ECDSA) to the list of known hosts.
2020-09-15T14:42:59.9009958Z error: cannot communicate with server: timeout exceeded while waiting for response
2020-09-15T14:42:59.9010369Z + true
2020-09-15T14:42:59.9011254Z + echo 'Ensure '\''snap list'\'' works and test-snapd-sh snap is installed'

And the minimal got the a point where 3 out of 4 tests have already executed successfuly and it got killed (?):

2020-09-15T15:30:24.5270786Z 2020-09-15 15:24:47 Restoring external:ubuntu-core-20-64:tests/smoke/remove (external:ubuntu-core-20-64)...
2020-09-15T15:30:24.5271732Z 2020-09-15 15:24:56 Preparing external:ubuntu-core-20-64:tests/smoke/install (external:ubuntu-core-20-64)...
2020-09-15T15:30:24.5272686Z 2020-09-15 15:25:50 Executing external:ubuntu-core-20-64:tests/smoke/install (external:ubuntu-core-20-64) (3/4)...
2020-09-15T15:30:24.5273644Z 2020-09-15 15:28:28 Restoring external:ubuntu-core-20-64:tests/smoke/install (external:ubuntu-core-20-64)...
2020-09-15T15:30:24.5275718Z 2020-09-15 15:28:51 Preparing external:ubuntu-core-20-64:tests/smoke/find-info (external:ubuntu-core-20-64)...
2020-09-15T15:30:24.5276373Z -----

pedronis · 2020-09-15T16:11:34Z

kernel-reseal itself passed, merging

bboozzoo added the UC20 label Sep 11, 2020

bboozzoo requested review from cmatsuoka, pedronis and anonymouse64 September 11, 2020 15:12

bboozzoo added the Run nested The PR also runs tests inluded in nested suite label Sep 11, 2020

bboozzoo closed this Sep 11, 2020

bboozzoo reopened this Sep 11, 2020

pedronis force-pushed the bboozzoo/uc20-reseal-with-kernel branch from 401d31f to a31f6dd Compare September 12, 2020 07:46

bboozzoo and others added 4 commits September 12, 2020 20:12

boot: add model to the Device

58e5be1

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

boot: reseal when changing kernels

2066fff

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

overlord/snapstate: Model() is defined on boot.Device now

9c69969

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

boot: rebase on uc20-conditional-reseal, adjust

60ee9aa

adjust tests ATM we should never have sealed keys and not a TrustedAssetsBootloader

pedronis force-pushed the bboozzoo/uc20-reseal-with-kernel branch from a31f6dd to 60ee9aa Compare September 12, 2020 18:17

mvo5 mentioned this pull request Sep 12, 2020

tests: add nested core20 kernel reseal test #9338

Closed

pedronis mentioned this pull request Sep 12, 2020

boot: streamline bootstate20.go reseal and tests changes #9340

Merged

pedronis reviewed Sep 13, 2020

View reviewed changes

boot: sealKeyToModeenv should error if a trusted assets bootloader is…

8614069

… not in use

pedronis marked this pull request as ready for review September 13, 2020 08:05

mvo5 mentioned this pull request Sep 13, 2020

tests: add nested core20 gadget reseal test #9341

Merged

anonymouse64 reviewed Sep 13, 2020

View reviewed changes

bboozzoo added 2 commits September 14, 2020 08:24

Merge remote-tracking branch 'upstream/master' into bboozzoo/uc20-res…

0004210

…eal-with-kernel

Merge remote-tracking branch 'upstream/master' into bboozzoo/uc20-res…

87586fa

…eal-with-kernel Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

bboozzoo mentioned this pull request Sep 14, 2020

tests: more logging for UC20 kernel test #9343

Closed

mvo5 added this to the 2.47 milestone Sep 15, 2020

bboozzoo and others added 2 commits September 15, 2020 15:29

Merge remote-tracking branch 'upstream/master' into bboozzoo/uc20-res…

5f92a59

…eal-with-kernel

tests: add simple core20 kernel reseal test

3019bda

This test adds checks that resealing happens after the kernel gets updated.

pedronis reviewed Sep 15, 2020

View reviewed changes

pedronis approved these changes Sep 15, 2020

View reviewed changes

anonymouse64 reviewed Sep 15, 2020

View reviewed changes

bboozzoo requested a review from mvo5 September 15, 2020 16:06

anonymouse64 approved these changes Sep 15, 2020

View reviewed changes

pedronis merged commit 556884a into snapcore:master Sep 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

boot: reseal when changing kernel #9331

boot: reseal when changing kernel #9331

bboozzoo commented Sep 11, 2020 •

edited by pedronis

pedronis commented Sep 12, 2020

pedronis left a comment

pedronis Sep 13, 2020

bboozzoo Sep 14, 2020

pedronis Sep 13, 2020

pedronis Sep 13, 2020

pedronis Sep 13, 2020 •

edited

pedronis Sep 13, 2020

pedronis Sep 13, 2020 •

edited

pedronis Sep 13, 2020

pedronis Sep 13, 2020

pedronis Sep 13, 2020

pedronis Sep 13, 2020

pedronis Sep 13, 2020

pedronis commented Sep 13, 2020

anonymouse64 left a comment

anonymouse64 Sep 13, 2020

pedronis Sep 15, 2020

pedronis left a comment

anonymouse64 left a comment

anonymouse64 Sep 15, 2020

anonymouse64 Sep 15, 2020

bboozzoo Sep 15, 2020

pedronis Sep 15, 2020

anonymouse64 Sep 15, 2020

anonymouse64 Sep 15, 2020

bboozzoo commented Sep 15, 2020

pedronis commented Sep 15, 2020

boot: reseal when changing kernel #9331

boot: reseal when changing kernel #9331

Conversation

bboozzoo commented Sep 11, 2020 • edited by pedronis

pedronis commented Sep 12, 2020

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

Choose a reason for hiding this comment

pedronis Sep 13, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis Sep 13, 2020 • edited

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

pedronis commented Sep 13, 2020

anonymouse64 left a 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

anonymouse64 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

bboozzoo commented Sep 15, 2020

pedronis commented Sep 15, 2020

bboozzoo commented Sep 11, 2020 •

edited by pedronis

pedronis Sep 13, 2020 •

edited

pedronis Sep 13, 2020 •

edited