secboot: re-introduce v1 fde hook format #14638

valentindavid · 2024-10-17T10:22:36Z

No description provided.

bboozzoo

LGTM

secboot/secboot_hooks.go

valentindavid · 2024-10-18T13:20:02Z

This was manually tested with

snapd 2.49.2
removing the assume in the pc gadget from latest/edge
rebuild pc-kernel latest/edge with core-initrd core20 branch and snap-bootstrap from this PR
core20 from latest/edge

codecov · 2024-10-18T13:34:27Z

Codecov Report

Attention: Patch coverage is 76.14679% with 26 lines in your changes missing coverage. Please review.

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

Files with missing lines	Patch %	Lines
secboot/secboot_tpm.go	75.75%	13 Missing and 3 partials ⚠️
secboot/secboot_hooks.go	61.53%	5 Missing and 5 partials ⚠️

Additional details and impacted files

@@                   Coverage Diff                   @@
##             fde-manager-features   #14638   +/-   ##
=======================================================
  Coverage                        ?   78.83%           
=======================================================
  Files                           ?     1093           
  Lines                           ?   147894           
  Branches                        ?        0           
=======================================================
  Hits                            ?   116594           
  Misses                          ?    24022           
  Partials                        ?     7278

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

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.

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

bboozzoo · 2024-10-18T15:50:45Z

secboot/secboot_sb.go

@@ -161,6 +161,22 @@ func UnlockVolumeUsingSealedKeyIfEncrypted(disk disks.Disk, name string, sealedE
 		return res, fmt.Errorf("internal error: cannot build an auth requestor: %v", err)
 	}

+	if loadedKey.SealedKeyV1 != nil {


this feels like it's a leaky abstraction which only muddies the whole picture, could we do something more explicit, eg:

// legacy fde-hooks keys which did not carry any metadata var legacyV1Key []byte // more modern keys with metadata var keys []*sb.KeyData // isLegacyV1Key - simply checks the header if fdeHasRevealKey() && isLegacyV1Key(sealedEncryptioKeyFile) { legacyV1Key, err = os.ReadFile() ... } else { if err := readKeyFile(sealedEncryptionKeyFile, loadedKey); err != nil { return res, err } } ... if len(legacyV1KeyFile) != 0 { } ... // continue with ActivateVolumeWithKeyData

and then simply drop the loader thingy

code changed

bboozzoo

LGTM

(although I added some changes here, so it should likely count as +0.5)

valentindavid · 2024-10-23T13:47:50Z

I have retested. And it is broken.

valentindavid · 2024-10-23T14:55:24Z

It is working again.

bboozzoo

LGTM, just some minor comments

bboozzoo · 2024-11-04T11:42:29Z

secboot/secboot_tpm.go

+	LoadKeyData(kd *sb.KeyData)
+	LoadSealedKeyObject(sko *sb_tpm2.SealedKeyObject)
+	LoadFDEHookKeyV1(sk []byte)


Suggested change

LoadKeyData(kd *sb.KeyData)

LoadSealedKeyObject(sko *sb_tpm2.SealedKeyObject)

LoadFDEHookKeyV1(sk []byte)

// LoadKeyData is expected to keep track of key data wrappers

LoadKeyData(kd *sb.KeyData)

// LoadSealedKeyObject is expected to keep track of a TPM sealed key

LoadSealedKeyObject(sko *sb_tpm2.SealedKeyObject)

// LoadFDEHookV1Key is expected to keep track of a FDE hook v1 sealed key

LoadFDEHookKeyV1(sk []byte)

bboozzoo · 2024-11-04T11:44:04Z

secboot/secboot_tpm.go

+	case oldSealedKey && hintExpectFDEHook:
+		// FDE hook key v1
+		//
+		// It has the same magic header has sealed key object,


Suggested change

// It has the same magic header has sealed key object,

// It has the same magic header as sealed key object,

secboot/secboot_tpm.go

ZeyadYasser

Small comments

ZeyadYasser · 2024-11-01T11:21:14Z

secboot/secboot_hooks.go

-	sb.RegisterPlatformKeyDataHandler(legacyFdeHooksPlatformName, handler)
+	v2Handler := &fdeHookV2DataHandler{}
+	sb.RegisterPlatformKeyDataHandler(legacyFdeHooksPlatformName, v2Handler)
+


nitpick

Suggested change

ZeyadYasser · 2024-11-01T13:53:15Z

secboot/secboot_tpm.go

+type defaultKeyLoader struct {
+	KeyData         *sb.KeyData
+	SealedKeyObject *sb_tpm2.SealedKeyObject
+	FDEHookKeyV1    []byte


nitpick, a small doc comment explaining those fields and what the zero values mean would be helpful.

e.g.

Suggested change

FDEHookKeyV1 []byte

// Non-nil FDEHookKeyV1 indicates that V1 hook key is used

FDEHookKeyV1 []byte

ZeyadYasser · 2024-11-05T11:57:35Z

secboot/secboot_tpm.go

@@ -238,14 +238,40 @@ func newAuthRequestor() (sb.AuthRequestor, error) {
 	)
 }

-// TODO: consider moving this to secboot
-func readKeyFileImpl(keyfile string) (*sb.KeyData, *sb_tpm2.SealedKeyObject, error) {
+type keyLoader interface {


Tiny nitpick, feel free to ignore.

For simplicity, can't this just be a struct that is returned as output from readKeyFileImpl?

I had in mind that in the future we just make callbacks that ignore some input. For instance the sealed key objects for tpm are useless for unlocking, only for resealing. Though in the end I have just used the same type.

I will try it and see if the code looks better. Maybe it can be done in another PR.

Wonder if the interface should be called loadedKey now?

also are we going to have other implementations of this interface in later PRs?

Yes. But I am not sure how it should look at this point. But one thing is sure is that having an interface here seemed a bit useless.

ZeyadYasser · 2024-11-05T11:59:15Z

secboot/secboot_tpm.go

+		// to use a hint we are using FDE hooks.
+		sealedKey, err := os.ReadFile(keyfile)
+		if err != nil {
+			return nil


Is ignoring the error here intentional?

Good catch.

valentindavid · 2024-11-05T13:40:06Z

I had to resolve conflicts, so I squashed and and rebase. I also added some code in the resealing to ignore v1 keys and a test for it. So that will need to be reviewed.

pedronis

some comments

secboot/secboot_hooks.go

pedronis · 2024-11-05T19:09:48Z

secboot/secboot_sb.go

+	if loadedKey.FDEHookKeyV1 != nil {
+		// Special case for hook keys v1. They do not have
+		// primary keys. So we cannot wrap them in KeyData
+		err := unlockDiskWithHookV1Key(mapperName, sourceDevice, loadedKey.FDEHookKeyV1)


they also don't reach here

pedronis · 2024-11-05T19:10:53Z

secboot/secboot_tpm.go

-func readKeyFileImpl(keyfile string) (*sb.KeyData, *sb_tpm2.SealedKeyObject, error) {
+type keyLoader interface {
+	// LoadKeyData keeps track of keys in KeyData format.
+	LoadKeyData(kd *sb.KeyData)


wouldn't Loaded... be clearer? this just keep tracks at the comment says, the loading has already happened

In my mind originally, it was that call that was loading the read content into what ever thing needs to actually interpret it. But I think it makes sense to call it Loaded...

pedronis

thanks, couple naming comments and some more questions/comments

pedronis · 2024-11-06T10:23:24Z

secboot/secboot_tpm.go

@@ -238,14 +238,40 @@ func newAuthRequestor() (sb.AuthRequestor, error) {
 	)
 }

-// TODO: consider moving this to secboot
-func readKeyFileImpl(keyfile string) (*sb.KeyData, *sb_tpm2.SealedKeyObject, error) {
+type keyLoader interface {


Wonder if the interface should be called loadedKey now?

pedronis · 2024-11-06T10:28:19Z

secboot/secboot_sb.go

+		// with key data if there are some on the tokens.
+		// Also the request for recovery key will happen in
+		// ActivateVolumeWithKeyData
+		logger.Noticef("WARNING: attempting opening device %s  with key file %s failed: %v", sourceDevice, sealedEncryptionKeyFile, err)


this fallback path is not reached

I have added `TestUnlockVolumeUsingSealedKeyIfEncryptedFdeRevealKeyV1FallbackKeyData

pedronis · 2024-11-06T10:29:07Z

secboot/secboot_tpm.go

+	LoadedFDEHookKeyV1(sk []byte)
+}
+
+type defaultKeyLoader struct {


then this is also defaultLoadedKey?

I have put a todo to the keyLoader interface to change it.

I am not sure that defaultLoadedKey makes total sense here since we create it empty. So it is not loaded yet. But it will make sense once we get rid of the interface and we return it.

pedronis · 2024-11-06T10:30:29Z

secboot/secboot_tpm.go

@@ -238,14 +238,40 @@ func newAuthRequestor() (sb.AuthRequestor, error) {
 	)
 }

-// TODO: consider moving this to secboot
-func readKeyFileImpl(keyfile string) (*sb.KeyData, *sb_tpm2.SealedKeyObject, error) {
+type keyLoader interface {


also are we going to have other implementations of this interface in later PRs?

pedronis · 2024-11-06T10:33:29Z

secboot/secboot_tpm.go

-			hasNewData = true
-		} else {
-			hasOldObject = true
+		if loadedKey.SealedKeyObject != nil {


we should maybe leave a TODO about refactor some of these ifs into methods on the loadedKey interface, but also I as wrote elsewhere wondering if there will be other implementations?

I have added a TODO to the definition of keyLoader interface.

pedronis

thanks

this has changed enough that it needs a re-review

ZeyadYasser

LGTM, Thank you!

This could use a spread test in a follow up PR.

ZeyadYasser · 2024-11-11T11:54:29Z

secboot/secboot_tpm.go

+		//
+		// It has the same magic header as sealed key object,
+		// but there is no version information. Thus we need
+		// to use a hint we are using FDE hooks.


Suggested change

// to use a hint we are using FDE hooks.

// to use a hint that we are using FDE hooks.

ZeyadYasser · 2024-11-11T11:54:53Z

secboot/secboot_tpm.go

+		// to use a hint we are using FDE hooks.
+		sealedKey, err := os.ReadFile(keyfile)
+		if err != nil {
+			return fmt.Errorf("cannot FDE hook v1 key: %v", err)


Suggested change

return fmt.Errorf("cannot FDE hook v1 key: %v", err)

return fmt.Errorf("cannot read FDE hook v1 key: %v", err)

bboozzoo

Thanks. The cleanups can be done in a followup PR.

chrisccoulson

This looks ok to me, although I've not spent a lot of time looking at the test code.

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

valentindavid force-pushed the valentindavid/fde-hook-v1 branch from b9599c8 to 837045c Compare October 17, 2024 10:45

bboozzoo previously approved these changes Oct 17, 2024

View reviewed changes

secboot/secboot_hooks.go Outdated Show resolved Hide resolved

secboot/secboot_hooks.go Outdated Show resolved Hide resolved

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

valentindavid force-pushed the valentindavid/fde-hook-v1 branch from 837045c to 9955ff3 Compare October 18, 2024 13:16

valentindavid marked this pull request as ready for review October 18, 2024 13:17

valentindavid requested a review from bboozzoo October 18, 2024 13:20

bboozzoo reviewed Oct 18, 2024

View reviewed changes

valentindavid force-pushed the valentindavid/fde-hook-v1 branch from d3a6b29 to e0c0e74 Compare October 22, 2024 11:47

bboozzoo approved these changes Oct 23, 2024

View reviewed changes

bboozzoo requested review from chrisccoulson, pedronis and ZeyadYasser October 23, 2024 12:18

valentindavid force-pushed the valentindavid/fde-hook-v1 branch from e0c0e74 to 3acc10f Compare October 23, 2024 13:33

valentindavid force-pushed the valentindavid/fde-hook-v1 branch from 3acc10f to db273b6 Compare October 23, 2024 14:48

valentindavid force-pushed the valentindavid/fde-hook-v1 branch from db273b6 to 8072bfb Compare October 23, 2024 14:56

valentindavid requested a review from bboozzoo October 23, 2024 14:57

bboozzoo previously approved these changes Nov 4, 2024

View reviewed changes

ZeyadYasser reviewed Nov 5, 2024

View reviewed changes

valentindavid force-pushed the valentindavid/fde-hook-v1 branch from a1ac728 to cbc60bb Compare November 5, 2024 13:39

valentindavid force-pushed the valentindavid/fde-hook-v1 branch from cbc60bb to 81b7f52 Compare November 5, 2024 13:47

valentindavid requested a review from ZeyadYasser November 5, 2024 13:47

pedronis reviewed Nov 5, 2024

View reviewed changes

valentindavid requested a review from pedronis November 6, 2024 10:02

pedronis reviewed Nov 6, 2024

View reviewed changes

valentindavid requested a review from pedronis November 7, 2024 10:08

pedronis approved these changes Nov 7, 2024

View reviewed changes

ZeyadYasser approved these changes Nov 11, 2024

View reviewed changes

valentindavid force-pushed the valentindavid/fde-hook-v1 branch from b8783af to 327e6f6 Compare November 13, 2024 13:29

valentindavid requested a review from bboozzoo November 13, 2024 13:45

bboozzoo approved these changes Nov 14, 2024

View reviewed changes

chrisccoulson approved these changes Nov 15, 2024

View reviewed changes

secboot: re-introduce v1 fde hook format

ac88293

valentindavid force-pushed the valentindavid/fde-hook-v1 branch from 327e6f6 to ac88293 Compare November 19, 2024 09:50

ernestl merged commit 4e36599 into canonical:fde-manager-features Nov 19, 2024
42 of 58 checks passed

valentindavid mentioned this pull request Nov 19, 2024

secboot/secboot_sb_test.go: fix expected value after conflicting merge #14741

Open

	// It has the same magic header has sealed key object,
	// It has the same magic header as sealed key object,

	FDEHookKeyV1 []byte
	// Non-nil FDEHookKeyV1 indicates that V1 hook key is used
	FDEHookKeyV1 []byte

	// to use a hint we are using FDE hooks.
	// to use a hint that we are using FDE hooks.

	return fmt.Errorf("cannot FDE hook v1 key: %v", err)
	return fmt.Errorf("cannot read FDE hook v1 key: %v", err)

secboot: re-introduce v1 fde hook format #14638

secboot: re-introduce v1 fde hook format #14638

Conversation

valentindavid commented Oct 17, 2024

bboozzoo left a comment

Choose a reason for hiding this comment

valentindavid commented Oct 18, 2024

codecov bot commented Oct 18, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

bboozzoo left a comment

Choose a reason for hiding this comment

valentindavid commented Oct 23, 2024

valentindavid commented Oct 23, 2024

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

ZeyadYasser 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

valentindavid commented Nov 5, 2024

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

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

pedronis left a comment

Choose a reason for hiding this comment

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

chrisccoulson left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 18, 2024 •

edited

Loading