overload/fdestate/backend: implement FDE hook resealing #14305

valentindavid · 2024-08-05T11:07:23Z

This also adds:

tests/lib/fakestore: add support for tracking channels
tests: add remodeling test using fakestore

pedronis

thanks, did a first pass on this

pedronis · 2024-10-04T16:53:09Z

tests/lib/fakestore/store/store.go

@@ -394,7 +413,7 @@ func (s *Store) detailsEndpoint(w http.ResponseWriter, req *http.Request) {
 	w.Write(out)
 }

-func (s *Store) collectSnaps() (map[string]string, error) {
+func (s *Store) collectSnaps(cs *ChannelRepository) (map[string]string, error) {


why is cs passed in when is also on s?

I didn't see an answer or change related to this question

tests/lib/fakestore/store/store.go

tests/nested/manual/remodel-uc20-to-uc22-fakestore/task.yaml

overlord/fdestate/backend/reseal.go

pedronis

thanks, looking good, small comment/questions and I'm not sure a question from the previous review about the fakestore changes was addressed

pedronis · 2024-10-09T12:16:25Z

tests/nested/manual/remodel-uc20-to-uc22-fakestore/task.yaml

+summary: verify remodel from UC20 to UC22
 details: |
-  Blah
+  Verify remodel from UC20 to UC22. This verifies unencrypted, tpm and
+  fde hook modes.


shouldn't this mention the fakestore? do we have a not fakestore variant?

pedronis · 2024-10-09T12:18:37Z

tests/lib/fakestore/store/store.go

@@ -394,7 +413,7 @@ func (s *Store) detailsEndpoint(w http.ResponseWriter, req *http.Request) {
 	w.Write(out)
 }

-func (s *Store) collectSnaps() (map[string]string, error) {
+func (s *Store) collectSnaps(cs *ChannelRepository) (map[string]string, error) {


I didn't see an answer or change related to this question

pedronis

thank you

alfonsosanchezbeato

Thanks, l have some comments / questions

alfonsosanchezbeato · 2024-10-16T19:06:22Z

tests/lib/fakestore/store/store.go

+		if a.Channel != "" {
+			snapPath, foundSnap = snaps[fmt.Sprintf("%s|%s", name, a.Channel)]
+		}
+		if !foundSnap {
+			snapPath, foundSnap = snaps[name]
+		}


Maybe a nitpick as this is test code, but do we want to look with the normal name if a channel has been specified? Shouldn't we just return with error/empty result? Unless channel is latest/stable maybe.

Very good question. I suppose I did it like that because other tests are not setting channels correctly, that are matching the model, or the --channel=... parameter to refresh or install. And I suppose it is rarely "latest/stable" the tests actually look for. But I do not remember. That is probably a task to look at at some point to clean-up the broken tests.

I will add a comment

Great, thanks for adding the comment

alfonsosanchezbeato · 2024-10-16T19:12:44Z

tests/nested/manual/remodel-uc20-to-uc22-fakestore/repack-kernel.sh

+
+mkdir "${tmpd}/early"
+mkdir "${tmpd}/main"
+( (cd "${tmpd}/early"; cpio -id) ; (cd "${tmpd}/main"; zstdcat | cpio -id) ) <"${tmpd}/initrd"


cannot we just use unmkinitramfs?

I am not a fan of initramfs-tools in general. And also the overcomplicated way unmkinitramfs tries to split the initramfs.

Ok, but this will fail for older initramfs not using zstd, or newer using maybe something else. So maybe we need to chang in the future.

alfonsosanchezbeato · 2024-10-16T19:16:01Z

tests/nested/manual/remodel-uc20-to-uc22-fakestore/repack-kernel.sh

+if [ "${BUILD_FDE_HOOK-}" = 1 ]; then
+    go build -o "${tmpd}/pc-kernel/meta/hooks/fde-setup" /project/tests/lib/fde-setup-hook
+fi


isn't this compiling the same as fde-reveal-key? why not just copy around the already compiled binary?

go has a build cache, right?

Tbh I was not that concerned about build time but about something changing and then forgetting to change the thing in two places, but not really very important anyway.

alfonsosanchezbeato · 2024-10-16T19:18:35Z

tests/nested/manual/remodel-uc20-to-uc22-fakestore/repack-kernel.sh

This is very useful, maybe it should go to tests/lib/. As a side point, we do similar things in different places, it would be great to unify at some point. Maybe this script could be the reference for repacking kernels.

I agree. But I would like to go through tests/lib/nested.sh and rewrite some of the code to use some generic script. And then use the same script here.

alfonsosanchezbeato · 2024-10-16T19:26:54Z

tests/nested/manual/remodel-uc20-to-uc22-fakestore/task.yaml

+    mkdir -p ./extra-initrd/usr/bin/
+    go build -o ./extra-initrd/usr/bin/fde-reveal-key "$TESTSLIB"/fde-setup-hook/fde-setup.go
+    mkdir -p ./extra-kernel-snap/meta/hooks
+    go build -o ./extra-kernel-snap/meta/hooks/fde-setup "$TESTSLIB"/fde-setup-hook/fde-setup.go


Same nitpick about double building the same binary instead of copying

alfonsosanchezbeato · 2024-10-16T19:30:07Z

tests/nested/manual/remodel-uc20-to-uc22-fakestore/task.yaml

+  for f in  updates/{core22,pc-22,pc-kernel-22}.{snap,assert} updates/pc-22.{snap,assert} updates/pc-kernel-22.{snap,assert}; do
+    remote.push "${f}"
+  done


Is pushing the snaps needed?

It is probably the remains some old attempts without changing the fakestore. I will remove that.

tests/nested/manual/remodel-uc20-to-uc22-fakestore/task.yaml

alfonsosanchezbeato · 2024-10-16T19:48:32Z

boot/seal.go

As far as I can see the main changes in this file are that things that in the past were done only for TPM-backed FDE are done also when using hooks. As in principle we dot not care about boot chains / sealing for hooks, what is the purpose of these changes? Maybe there is some spec that would help me understand this?

On FDE hooks, we do care about what model is used depending of the key we unseal. This is a boot chain. That is much smaller than TPM, but it is still a boot chain.

We should probably have some abstractions though here. But it is yet obvious what it should look like. There will be the run mode that will be introduce in the future, that is not here. And maybe it will start to make more sense.

Maybe we should rename it to "boot context" at some point.

Thanks for the offline clarification

alfonsosanchezbeato · 2024-10-16T20:02:33Z

overlord/fdestate/backend/reseal.go

+type comparableModel struct {
+	BrandID   string
+	SignKeyID string
+	Model     string
+	Classic   bool
+	Grade     asserts.ModelGrade
+	Series    string
+}


do we need this just because there is no Series field in modelForSealing? Maybe worth adding a comment explaining.

There should be Series in modelForSealing.

The problem here is that it is not useable as an index for a map, because it is not comparable.

alfonsosanchezbeato · 2024-10-16T20:22:23Z

secboot/secboot_sb_test.go

+	return "some-key"
+}
+
+func (s *secbootSuite) TestResealKeysWithFDESetupHookV1(c *C) {


What is v1 in this context? Which other formats for keyslots do we have?

Actually this is a mistake here. It tests v2. The new format is v3.

We have 2 format of keydata. Keydata are json. v1 is not json. But this does not need resealing.

Note, the version here is not related to the key slots. But the key format. Which might be stored in the token for the key slot, or stored in a file.

codecov · 2024-10-21T09:03:37Z

Codecov Report

Attention: Patch coverage is 63.46154% with 76 lines in your changes missing coverage. Please review.

Please upload report for BASE (fde-manager-features@98d30f1). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
tests/lib/fakestore/store/store.go	57.33%	26 Missing and 6 partials ⚠️
secboot/secboot_hooks.go	42.50%	16 Missing and 7 partials ⚠️
boot/seal.go	72.91%	9 Missing and 4 partials ⚠️
overlord/fdestate/backend/reseal.go	86.04%	4 Missing and 2 partials ⚠️
secboot/secboot_dummy.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@                   Coverage Diff                   @@
##             fde-manager-features   #14305   +/-   ##
=======================================================
  Coverage                        ?   78.83%           
=======================================================
  Files                           ?     1093           
  Lines                           ?   147706           
  Branches                        ?        0           
=======================================================
  Hits                            ?   116449           
  Misses                          ?    23992           
  Partials                        ?     7265

Flag	Coverage Δ
unittests	`78.83% <63.46%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

valentindavid · 2024-10-25T13:39:05Z

I have implemented the update of the state as part of #14674 that will go on top of this.

alfonsosanchezbeato

LGTM, thanks for the changes

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/remodel-fakestore branch from 2695fa0 to 9cd28d9 Compare August 30, 2024 11:44

valentindavid mentioned this pull request Sep 5, 2024

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

Open

valentindavid force-pushed the valentindavid/remodel-fakestore branch from 9cd28d9 to d219e4d Compare September 11, 2024 09:35

valentindavid force-pushed the valentindavid/remodel-fakestore branch 2 times, most recently from b0bd57d to 825c315 Compare September 20, 2024 14:33

valentindavid force-pushed the valentindavid/remodel-fakestore branch 5 times, most recently from 185cc1d to 083087c Compare October 3, 2024 09:03

valentindavid marked this pull request as ready for review October 3, 2024 11:54

valentindavid requested review from pedronis and alfonsosanchezbeato October 3, 2024 11:54

valentindavid changed the title ~~tests: add remodeling test using fakestore~~ overload/fdestate/backend: implement FDE hook resealing Oct 3, 2024

pedronis reviewed Oct 4, 2024

View reviewed changes

valentindavid requested a review from pedronis October 9, 2024 10:56

pedronis reviewed Oct 9, 2024

View reviewed changes

valentindavid requested a review from pedronis October 10, 2024 08:24

pedronis approved these changes Oct 10, 2024

View reviewed changes

valentindavid force-pushed the valentindavid/remodel-fakestore branch from efc06fa to 9b0551f Compare October 16, 2024 15:58

alfonsosanchezbeato reviewed Oct 16, 2024

View reviewed changes

pedronis force-pushed the fde-manager-features branch from fcc5e2e to 24018ee Compare October 18, 2024 11:27

valentindavid force-pushed the valentindavid/remodel-fakestore branch 4 times, most recently from e0ce10a to dac3674 Compare October 21, 2024 08:46

valentindavid force-pushed the valentindavid/remodel-fakestore branch 5 times, most recently from a20dc03 to 73ea788 Compare October 21, 2024 16:45

tests/lib/fakestore: add support for tracking channels

848f97b

valentindavid force-pushed the valentindavid/remodel-fakestore branch from 73ea788 to 6ad2708 Compare October 24, 2024 08:00

valentindavid added 2 commits October 24, 2024 10:02

tests: add remodeling test using fakestore

8523f60

overload/fdestate/backend: implement FDE hook resealing

0d72241

valentindavid force-pushed the valentindavid/remodel-fakestore branch from 6ad2708 to 0d72241 Compare October 24, 2024 08:02

valentindavid requested a review from alfonsosanchezbeato October 24, 2024 08:04

bboozzoo self-requested a review October 25, 2024 11:25

valentindavid mentioned this pull request Oct 25, 2024

overlord/fdestate/backend/reseal.go: factor FDE hook and TPM resealing #14674

Open

alfonsosanchezbeato approved these changes Oct 28, 2024

View reviewed changes

pedronis merged commit 58e1ac7 into canonical:fde-manager-features Nov 5, 2024
42 of 58 checks passed

overload/fdestate/backend: implement FDE hook resealing #14305

overload/fdestate/backend: implement FDE hook resealing #14305

Conversation

valentindavid commented Aug 5, 2024 • edited Loading

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

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

pedronis left a comment

Choose a reason for hiding this comment

alfonsosanchezbeato 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 21, 2024 • edited Loading

Codecov Report

valentindavid commented Oct 25, 2024

alfonsosanchezbeato left a comment

Choose a reason for hiding this comment

valentindavid commented Aug 5, 2024 •

edited

Loading

codecov bot commented Oct 21, 2024 •

edited

Loading