many: rework the way we allocate counter handles #14910

valentindavid · 2025-01-09T11:35:40Z

We used 4 handles from 0x01880001-0x01880004 for PCR policies. We always used a pair and would switch to the other pair after factory reset.

No instead we look for free handle in range 0x01880005-0x0188000F. We also use only one handle per primary key.

When we factory reset, we should remove the handles correctly since the old primary key for the save partition should have been the same as the data partition that was removed, thus the handles should be the same.

We also now correctly populate the FDE state with the correct handles.

Thanks for helping us make a better snapd!
Have you signed the license agreement and read the contribution guide?

overlord/fdestate/fdemgr_test.go

codecov · 2025-01-10T11:33:03Z

Codecov Report

Attention: Patch coverage is 55.37849% with 112 lines in your changes missing coverage. Please review.

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

Files with missing lines	Patch %	Lines
secboot/secboot_tpm.go	50.00%	70 Missing and 15 partials ⚠️
overlord/fdestate/fdestate.go	62.50%	8 Missing and 4 partials ⚠️
overlord/devicestate/handlers_install.go	83.78%	4 Missing and 2 partials ⚠️
secboot/secboot_dummy.go	0.00%	6 Missing ⚠️
overlord/fdestate/backend/seal.go	60.00%	1 Missing and 1 partial ⚠️
overlord/devicestate/devicemgr.go	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@                   Coverage Diff                   @@
##             fde-manager-features   #14910   +/-   ##
=======================================================
  Coverage                        ?   78.09%           
=======================================================
  Files                           ?     1170           
  Lines                           ?   155133           
  Branches                        ?        0           
=======================================================
  Hits                            ?   121147           
  Misses                          ?    26449           
  Partials                        ?     7537

Flag	Coverage Δ
unittests	`78.09% <55.37%> (?)`

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.

pedronis

did a first quick pass, some questions

overlord/devicestate/handlers_install.go

pedronis · 2025-01-13T15:45:40Z

overlord/devicestate/handlers_install.go

-	// keys.
-
-	// FIXME: maybe there is better if we had a function returning the devname instead.
+func deleteOldKeys(saveMntPnt string) error {


this needs a doc comment about what is doing

boot/seal.go

secboot/secboot_tpm.go

valentindavid · 2025-01-14T08:46:01Z

secboot/secboot_tpm.go

+	if err != nil {
+		if os.IsNotExist(err) {
+			if readKeyDataErr != nil {
+				// FIXME: we need to check for


That fixme is not complete...

pedronis

thanks for the changes, some small comments, please ping people to get more reviews

pedronis · 2025-01-17T08:26:10Z

overlord/devicestate/handlers_install.go

+
+	var oldKeys []string
+	renameKey := false
+	if osutil.FileExists(saveFallbackKeyFactory) {


this little bit of logic probably needs a small comment

pedronis · 2025-01-17T08:27:03Z

overlord/devicestate/handlers_install.go

@@ -1360,7 +1360,18 @@ func createSaveBootstrappedContainer(saveNode string) (secboot.BootstrappedConta
 	return secbootCreateBootstrappedContainer(secboot.DiskUnlockKey(saveEncryptionKey), saveNode), nil
 }

+
+// deleteOldKeys remove old keys that were used in previous installation after successful factory reset.
+//  * Key files ubuntu-save.recovery.sealed-key has to be replaced by key file ubuntu-save.recovery.sealed-key.factory-reset


given this bit, shouldn't the name say something like swap save key as well?

pedronis · 2025-01-17T08:28:07Z

secboot/secboot_tpm.go

@@ -936,6 +936,9 @@ func mockableReadKeyFileImpl(keyFile string, keyLoader *mockableKeyLoader, hintE

 var mockableReadKeyFile = mockableReadKeyFileImpl

+// GetPCRHandle returns the handle used by a key.  The key will be
+// search on as token on node in the keySlot. If keySlot has no


s/search on/searched/ ?

Maybe "searched on the token of the keySlot from the node". I will change that.

bboozzoo

Just some trivial comments. TBH I'm not fond of all the mocking we do right now. On the one hand, it'd be nice to think about it a little, on the other there are things to deliver.

bboozzoo · 2025-01-20T15:16:38Z

overlord/devicestate/handlers_install.go

+//  * Key files ubuntu-save.recovery.sealed-key has to be replaced by key file ubuntu-save.recovery.sealed-key.factory-reset
+//  * Keyslots factory-reset-* have to be removed
+//  * TPM handles used by the removed keys have to be released


Suggested change

// * Key files ubuntu-save.recovery.sealed-key has to be replaced by key file ubuntu-save.recovery.sealed-key.factory-reset

// * Keyslots factory-reset-* have to be removed

// * TPM handles used by the removed keys have to be released

// - rotate ubuntu-save key file, replace ubuntu-save.recovery.sealed-key with

// ubuntu-save.recovery.sealed-key.factory-reset, which was successfully recovered

// - remove factory-reset-* keyslots

// - reelase TPM handles used by the removed keys

bboozzoo · 2025-01-20T15:17:54Z

overlord/devicestate/handlers_install.go

+	// If the fallback save key exists, then it is the new
+	// key. That means the default save key is the old save key
+	// that needs to be removed.


Suggested change

// If the fallback save key exists, then it is the new

// key. That means the default save key is the old save key

// that needs to be removed.

// ubuntu-save key rotation, delete old key and use

// fallback save key if it exists

It is more confusing. If the fallback key does not exist, the default one is not the old key, because the rotation might have happened already.

shouldn't we capture in the comment a bit what are the cases when is missing as well then?

bboozzoo · 2025-01-20T15:18:23Z

overlord/devicestate/handlers_install.go

+	// If the fallback save key exists, then it is the new
+	// key. That means the default save key is the old save key
+	// that needs to be removed.
+	if osutil.FileExists(saveFallbackKeyFactory) {


maybe I'm missing something, but is it possible this key doesn't exist?

I think it is possible if we already swapped it before but we did not finish clean up.

bboozzoo · 2025-01-20T15:47:41Z

secboot/secboot_tpm.go

+// possibleKeyFiles is a list to paths.  hintExpectFDEHook helps
+// reading old key object files.  If not TPM2 key is found, nothing
+// happens.
+func RemoveOldCounterHandles(node string, possibleOldKeys map[string]bool, possibleKeyFiles []string, hintExpectFDEHook bool) error {


unless other code is using node already

Suggested change

func RemoveOldCounterHandles(node string, possibleOldKeys map[string]bool, possibleKeyFiles []string, hintExpectFDEHook bool) error {

func RemoveOldCounterHandles(device string, possibleOldKeys map[string]bool, possibleKeyFiles []string, hintExpectFDEHook bool) error {

bboozzoo · 2025-01-20T15:51:17Z

secboot/secboot_tpm.go

+// RemoveOldCounterHandles tracks and release TPM2 handles used by
+// given keys node and possibleOldKeys point to keyslot tokens. And
+// possibleKeyFiles is a list to paths.  hintExpectFDEHook helps
+// reading old key object files.  If not TPM2 key is found, nothing
+// happens.


Suggested change

// RemoveOldCounterHandles tracks and release TPM2 handles used by

// given keys node and possibleOldKeys point to keyslot tokens. And

// possibleKeyFiles is a list to paths. hintExpectFDEHook helps

// reading old key object files. If not TPM2 key is found, nothing

// happens.

// RemoveOldCounterHandles releases TPM2 handles of keys used on a given

// device, which are either stored in key slots matching possibleOldKeys or in key

// files listed in possibleKeyFiles. If not TPM2 key is found, nothing happens.

bboozzoo · 2025-01-20T16:14:22Z

secboot/secboot_tpm.go

+				readKeyDataErr = err
+				break
+			}
+			keyData, err := mockableReadKeyData(reader)


this is slightly annoying

pedronis

thank you, couple small comments

pedronis · 2025-01-21T15:28:41Z

overlord/devicestate/handlers_install.go

+		renameKey = true
+	}
+
+	// FIXME: we are missing a bit of context here. We should do this only if we are using TPM2.


do we still need this comment, now that we pass hasHook?

pedronis · 2025-01-21T15:30:38Z

overlord/devicestate/handlers_install.go

+	saveFallbackKeyFactory := device.FactoryResetFallbackSaveSealedKeyUnder(boot.InitramfsSeedEncryptionKeyDir)
+
+	var oldKeys []string
+	renameKey := false


maybe this should be called rotateKey to match the doc comment of the function

I have changed it.

bboozzoo · 2025-01-22T14:12:03Z

secboot/secboot_tpm.go

+
+	takenHandles := make(map[uint32]bool)
+	for _, handle := range handles {
+		takenHandles[uint32(handle)] = true


I would log which handles are in use here

bboozzoo · 2025-01-22T14:13:08Z

secboot/secboot_tpm.go

+	for _, tentative := range randutil.Perm(int(PCRPolicyCounterHandleRange)) {
+		handle := PCRPolicyCounterHandleStart + uint32(tentative)
+		if !takenHandles[PCRPolicyCounterHandleStart+uint32(tentative)] {
+			return handle, nil


and here, log the free handle

bboozzoo · 2025-01-22T14:17:07Z

overlord/devicestate/handlers_install.go

+func rotateSaveKeyAndDeleteOldKeys(saveMntPnt string) error {
+	hasHook, err := boot.HasFDESetupHook(nil)
+	if err != nil {
+		logger.Noticef("WARNING: cannot figure out if FDE hooks are in use: %v", err)


Suggested change

logger.Noticef("WARNING: cannot figure out if FDE hooks are in use: %v", err)

logger.Noticef("WARNING: cannot determine whether FDE hooks are in use: %v", err)

bboozzoo · 2025-01-22T14:36:06Z

overlord/managers_test.go

@@ -7482,6 +7482,11 @@ func (s *mgrsSuiteCore) testRemodelUC20WithRecoverySystem(c *C, encrypted bool)
 	})
 	defer restore()

+	restore = fdestate.MockSecbootGetPCRHandle(func(devicePath, keySlot, keyFile string) (uint32, error) {
+		return 42, nil


FWIW, I would return an int which looks like the actual handles we expect, such that when looking at the logs you'd see familiar values, so eg. 0x1880002

We used 4 handles from 0x01880001-0x01880004 for PCR policies. We always used a pair and would switch to the other pair after factory reset. No instead we look for free handle in range 0x01880005-0x0188000F. We also use only one handle per primary key. When we factory reset, we should remove the handles correctly since the old primary key for the save partition should have been the same as the data partition that was removed, thus the handles should be the same. We also now correctly populate the FDE state with the correct handles.

valentindavid added Run nested The PR also runs tests inluded in nested suite FDE Manager Pull requests that target FDE manager branch labels Jan 9, 2025

valentindavid force-pushed the valentindavid/fde-rework-counter-handles branch 8 times, most recently from 3766f88 to 796c572 Compare January 9, 2025 14:50

bboozzoo reviewed Jan 10, 2025

View reviewed changes

overlord/fdestate/fdemgr_test.go Show resolved Hide resolved

valentindavid force-pushed the valentindavid/fde-rework-counter-handles branch from 796c572 to bb2978a Compare January 10, 2025 11:11

valentindavid marked this pull request as ready for review January 10, 2025 11:11

valentindavid requested review from chrisccoulson and pedronis January 10, 2025 11:11

pedronis force-pushed the fde-manager-features branch from 5802840 to 5ec2e43 Compare January 13, 2025 09:50

valentindavid force-pushed the valentindavid/fde-rework-counter-handles branch from bb2978a to 20133bc Compare January 13, 2025 09:55

valentindavid mentioned this pull request Jan 13, 2025

many: use only one counter handle on TPM #14836

Closed

pedronis reviewed Jan 13, 2025

View reviewed changes

valentindavid commented Jan 14, 2025

View reviewed changes

valentindavid force-pushed the valentindavid/fde-rework-counter-handles branch 2 times, most recently from a96331b to 0dc4a83 Compare January 15, 2025 14:15

valentindavid requested review from bboozzoo and pedronis January 16, 2025 15:28

pedronis reviewed Jan 17, 2025

View reviewed changes

valentindavid force-pushed the valentindavid/fde-rework-counter-handles branch from 0dc4a83 to 374fffa Compare January 20, 2025 12:29

valentindavid requested a review from pedronis January 20, 2025 12:29

valentindavid force-pushed the valentindavid/fde-rework-counter-handles branch from 374fffa to 4883247 Compare January 20, 2025 13:09

bboozzoo reviewed Jan 20, 2025

View reviewed changes

valentindavid requested a review from bboozzoo January 21, 2025 09:50

pedronis approved these changes Jan 21, 2025

View reviewed changes

bboozzoo approved these changes Jan 22, 2025

View reviewed changes

valentindavid added 8 commits January 24, 2025 17:37

fixup! many: rework the way we allocate counter handles

549cbc6

fixup! many: rework the way we allocate counter handles

c5108fd

fixup! many: rework the way we allocate counter handles

a43be53

fixup! many: rework the way we allocate counter handles

e15febf

fixup! many: rework the way we allocate counter handles

646281e

fixup! many: rework the way we allocate counter handles

bc19bd3

fixup! many: rework the way we allocate counter handles

2d88bcf

valentindavid force-pushed the valentindavid/fde-rework-counter-handles branch from 24ce51f to 2d88bcf Compare January 24, 2025 16:37

fixup! many: rework the way we allocate counter handles

75fd404

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

many: rework the way we allocate counter handles #14910

many: rework the way we allocate counter handles #14910

valentindavid commented Jan 9, 2025

codecov bot commented Jan 10, 2025 •

edited

Loading

pedronis left a comment

pedronis Jan 13, 2025

valentindavid Jan 14, 2025

pedronis left a comment

pedronis Jan 17, 2025

pedronis Jan 17, 2025

pedronis Jan 17, 2025

valentindavid Jan 17, 2025

bboozzoo left a comment

bboozzoo Jan 20, 2025

bboozzoo Jan 20, 2025

valentindavid Jan 21, 2025

pedronis Jan 21, 2025

bboozzoo Jan 20, 2025

valentindavid Jan 21, 2025

bboozzoo Jan 20, 2025

bboozzoo Jan 20, 2025

bboozzoo Jan 20, 2025

pedronis left a comment

pedronis Jan 21, 2025

valentindavid Jan 21, 2025

pedronis Jan 21, 2025

valentindavid Jan 21, 2025

bboozzoo Jan 22, 2025

bboozzoo Jan 22, 2025

bboozzoo Jan 22, 2025

bboozzoo Jan 22, 2025

-//  * Key files ubuntu-save.recovery.sealed-key has to be replaced by key file ubuntu-save.recovery.sealed-key.factory-reset
-//  * Keyslots factory-reset-* have to be removed
-//  * TPM handles used by the removed keys have to be released
+//  - rotate ubuntu-save key file, replace ubuntu-save.recovery.sealed-key with
+//    ubuntu-save.recovery.sealed-key.factory-reset, which was successfully recovered
+//  - remove factory-reset-* keyslots
+//  - reelase TPM handles used by the removed keys

	func RemoveOldCounterHandles(node string, possibleOldKeys map[string]bool, possibleKeyFiles []string, hintExpectFDEHook bool) error {
	func RemoveOldCounterHandles(device string, possibleOldKeys map[string]bool, possibleKeyFiles []string, hintExpectFDEHook bool) error {

	logger.Noticef("WARNING: cannot figure out if FDE hooks are in use: %v", err)
	logger.Noticef("WARNING: cannot determine whether FDE hooks are in use: %v", err)

many: rework the way we allocate counter handles #14910

Are you sure you want to change the base?

many: rework the way we allocate counter handles #14910

Conversation

valentindavid commented Jan 9, 2025

codecov bot commented Jan 10, 2025 • edited Loading

Codecov Report

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

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

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

codecov bot commented Jan 10, 2025 •

edited

Loading