overlord/install/install.go: use plainkey for save key #14706

valentindavid · 2024-11-11T13:41:06Z

No description provided.

codecov · 2024-11-11T15:29:16Z

Codecov Report

Attention: Patch coverage is 45.45455% with 48 lines in your changes missing coverage. Please review.

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

Files with missing lines	Patch %	Lines
overlord/install/install.go	33.33%	20 Missing and 10 partials ⚠️
secboot/keys/plainkey_nosb.go	0.00%	13 Missing ⚠️
secboot/keys/plainkey.go	78.57%	2 Missing and 1 partial ⚠️
boot/seal.go	71.42%	2 Missing ⚠️

Additional details and impacted files

@@                   Coverage Diff                   @@
##             fde-manager-features   #14706   +/-   ##
=======================================================
  Coverage                        ?   78.83%           
=======================================================
  Files                           ?     1096           
  Lines                           ?   148248           
  Branches                        ?        0           
=======================================================
  Hits                            ?   116865           
  Misses                          ?    24082           
  Partials                        ?     7301

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

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 pass

secboot/keys/plainkey.go

pedronis · 2024-12-10T12:39:28Z

cmd/snap-bootstrap/cmd_initramfs_mounts_test.go

@@ -8236,7 +8236,7 @@ echo '{"features":[]}'
 			observeExistingTrustedRecoveryAssetsCalled += 1
 			return nil
 		},
-		SetBootstrappedContainersFunc: func(key, saveKey secboot.BootstrappedContainer) {
+		SetBootstrappedContainersFunc: func(key, saveKey secboot.BootstrappedContainer, primaryKey []byte) {


not new to this PR but shouldn't these check the arguments they get?

boot/makebootable.go

pedronis · 2024-12-10T15:53:13Z

boot/assets.go

@@ -256,7 +256,7 @@ func isAssetHashTrackedInMap(bam bootAssetsMap, assetName, assetHash string) boo
 type TrustedAssetsInstallObserver interface {
 	BootLoaderSupportsEfiVariables() bool
 	ObserveExistingTrustedRecoveryAssets(recoveryRootDir string) error
-	SetBootstrappedContainers(key, saveKey secboot.BootstrappedContainer)
+	SetBootstrappedContainers(key, saveKey secboot.BootstrappedContainer, primaryKey []byte)


... AndPrimarKey should be added to the name I suppose now

pedronis · 2024-12-10T15:54:01Z

boot/makebootable_test.go

@@ -1102,7 +1102,7 @@ version: 5.0
 	// set encryption key
 	myKey := secboot.CreateMockBootstrappedContainer()
 	myKey2 := secboot.CreateMockBootstrappedContainer()
-	obs.SetBootstrappedContainers(myKey, myKey2)
+	obs.SetBootstrappedContainers(myKey, myKey2, nil)


are there tests in this package where primaryKey is not set to nil?

boot/seal.go

pedronis · 2024-12-10T15:56:33Z

overlord/fdestate/backend/seal.go

@@ -112,12 +112,12 @@ func sealRunObjectKeys(key secboot.BootstrappedContainer, pbc boot.PredictableBo
 	// path only unseals one object because unsealing is expensive.
 	// Furthermore, the run object key is stored on ubuntu-boot so that we do not
 	// need to continually write/read keys from ubuntu-seed.
-	primaryKey, err := secbootSealKeys(runKeySealRequests(key, useTokens), sealKeyParams)
+	createdPrimaryKey, err := secbootSealKeys(runKeySealRequests(key, useTokens), sealKeyParams)


I'm confused, does it make sense to call it created if it was passed in or it depends whether is passed in or not? even in that case something more generic would be better?

Agreed, according to sbNewProtectedKey, it will get created only if nil was passed as the primary key

It might be created, or the same if not nil. If we are in the case where use tokens, the primary key will be already picked because of the plainkey, and the parameter primaryKey will contain that key. And createdPrimaryKey will be the same key. If we do not use a tokens, then we did not chose a primary key yet. Then primaryKey is nil. And then createdPrimaryKey is a new key.

I could name them "primaryKeyInput" and "primaryKeyOutput". Or I could name same "primaryKey", "chosenPrimaryKey". Or "maybePrimaryKey" and "primaryKey".

maybePrimaryKey and primaryKey seem fine here

ZeyadYasser

Thank you! small nitpicks

secboot/keys/plainkey_nosb.go

secboot/keys/plainkey.go

ZeyadYasser · 2024-12-11T09:03:49Z

secboot/keys/plainkey_test.go

+
+	protectorKey, err := keys.NewProtectorKey()
+	c.Assert(err, IsNil)
+	protectedKey, primaryKeyOut, unlockKey, err := protectorKey.CreateProtectedKey([]byte("primary-in"))


maybe another variant where nil is passed, as this mimics what is done in install.PrepareEncryptedSystemData

Good point. Done.

ZeyadYasser · 2024-12-11T09:05:06Z

overlord/install/install_test.go

+	c.Check(marker, HasLen, 32)
+	c.Check(filepath.Join(boot.InstallHostFDESaveDir, "marker"), testutil.FileEquals, marker)
+
+	// the assets cache was written to


Suggested change

// the assets cache was written to

// Check that assets cache was written

ZeyadYasser · 2024-12-11T09:07:36Z

overlord/fdestate/backend/seal.go

@@ -112,12 +112,12 @@ func sealRunObjectKeys(key secboot.BootstrappedContainer, pbc boot.PredictableBo
 	// path only unseals one object because unsealing is expensive.
 	// Furthermore, the run object key is stored on ubuntu-boot so that we do not
 	// need to continually write/read keys from ubuntu-seed.
-	primaryKey, err := secbootSealKeys(runKeySealRequests(key, useTokens), sealKeyParams)
+	createdPrimaryKey, err := secbootSealKeys(runKeySealRequests(key, useTokens), sealKeyParams)


Agreed, according to sbNewProtectedKey, it will get created only if nil was passed as the primary key

ZeyadYasser

LGTM!

small nitpick on test coverage https://github.com/canonical/snapd/pull/14706/files#r1879654817

pedronis

thanks, style comment

pedronis · 2024-12-12T12:42:24Z

secboot/keys/plainkey.go

+	sbNewProtectedKey = sb_plainkey.NewProtectedKey
+)
+
+// ProtectorKey is a key that can be used to protect "plainkey" keys


style: all the doc comments should be sentences/paragraphs ending with "." thx

valentindavid · 2024-12-12T13:29:33Z

I have cleaned up the history.

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

valentindavid force-pushed the valentindavid/save-plainkeys branch 4 times, most recently from 4bc0793 to 5c615ca Compare November 11, 2024 15:07

valentindavid force-pushed the valentindavid/save-plainkeys branch from 5c615ca to 3be47e8 Compare November 20, 2024 11:17

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

valentindavid force-pushed the valentindavid/save-plainkeys branch from 3be47e8 to 76c9119 Compare November 20, 2024 11:29

valentindavid force-pushed the valentindavid/save-plainkeys branch 5 times, most recently from aa56f86 to 2853766 Compare December 6, 2024 12:14

valentindavid marked this pull request as ready for review December 6, 2024 14:01

valentindavid requested review from bboozzoo, pedronis and ZeyadYasser December 6, 2024 14:02

pedronis reviewed Dec 10, 2024

View reviewed changes

ZeyadYasser reviewed Dec 11, 2024

View reviewed changes

valentindavid requested review from ZeyadYasser and pedronis December 11, 2024 16:13

ZeyadYasser approved these changes Dec 12, 2024

View reviewed changes

pedronis approved these changes Dec 12, 2024

View reviewed changes

valentindavid force-pushed the valentindavid/save-plainkeys branch from 6b5d83e to d778338 Compare December 12, 2024 13:29

valentindavid added 3 commits December 12, 2024 14:33

secboot/keys: add wrapper secboot plainkey

24695ad

many: propagate the primary key from when we set encrypted containers

9fa4e2a

boot: make useTokens public

15bf1d4

valentindavid added 2 commits December 12, 2024 14:33

overlord/install/install.go: use plainkey for save key

28a38ec

tests/nested: check for plainkey used when tokens are in use

941da67

valentindavid force-pushed the valentindavid/save-plainkeys branch from d778338 to 941da67 Compare December 12, 2024 13:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

overlord/install/install.go: use plainkey for save key #14706

overlord/install/install.go: use plainkey for save key #14706

valentindavid commented Nov 11, 2024

codecov bot commented Nov 11, 2024 •

edited

Loading

pedronis left a comment

pedronis Dec 10, 2024

pedronis Dec 10, 2024

pedronis Dec 10, 2024

pedronis Dec 10, 2024

ZeyadYasser Dec 11, 2024

valentindavid Dec 11, 2024

pedronis Dec 11, 2024

valentindavid Dec 12, 2024

ZeyadYasser left a comment

ZeyadYasser Dec 11, 2024

valentindavid Dec 12, 2024

ZeyadYasser Dec 11, 2024

ZeyadYasser Dec 11, 2024

ZeyadYasser left a comment

pedronis left a comment

pedronis Dec 12, 2024

valentindavid commented Dec 12, 2024

	// the assets cache was written to
	// Check that assets cache was written

overlord/install/install.go: use plainkey for save key #14706

Are you sure you want to change the base?

overlord/install/install.go: use plainkey for save key #14706

Conversation

valentindavid commented Nov 11, 2024

codecov bot commented Nov 11, 2024 • 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

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

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

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

valentindavid commented Dec 12, 2024

codecov bot commented Nov 11, 2024 •

edited

Loading