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

valentindavid · 2024-10-25T13:38:31Z

Instead of splitting completely FDE hook and TPM resealing, we split the calculation of role parameters, udpate the state, then do the resealing based on that state.

This depends on #14305

codecov · 2024-10-25T14:04:15Z

Codecov Report

Attention: Patch coverage is 65.95745% with 64 lines in your changes missing coverage. Please review.

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

Files with missing lines	Patch %	Lines
overlord/fdestate/backend/reseal.go	66.39%	25 Missing and 16 partials ⚠️
overlord/fdestate/fdestate.go	60.00%	8 Missing and 4 partials ⚠️
overlord/fdestate/fdemgr.go	69.44%	8 Missing and 3 partials ⚠️

Additional details and impacted files

@@                   Coverage Diff                   @@
##             fde-manager-features   #14674   +/-   ##
=======================================================
  Coverage                        ?   78.81%           
=======================================================
  Files                           ?     1093           
  Lines                           ?   147878           
  Branches                        ?        0           
=======================================================
  Hits                            ?   116556           
  Misses                          ?    24037           
  Partials                        ?     7285

Flag	Coverage Δ
unittests	`78.81% <65.95%> (?)`

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

suggestion

pedronis · 2024-11-06T12:55:34Z

overlord/fdestate/backend/reseal.go

 type StateUpdater func(role string, containerRole string, bootModes []string, models []secboot.ModelForSealing, tpmPCRProfile []byte) error
+type StateGetter func(role string, containerRole string) (bootModes []string, models []secboot.ModelForSealing, tpmPCRProfile []byte, err error)


wouldn't it make sense to combine these two into an interface and have FDEManager implement it?

Yes. I will do that.

Though, I will need to wrap it since we need to handle the unlocker.

bboozzoo

LGTM

bboozzoo · 2024-11-07T10:45:08Z

overlord/fdestate/backend/reseal.go

+type FDEState interface {
+	UpdateState(role string, containerRole string, bootModes []string, models []secboot.ModelForSealing, tpmPCRProfile []byte) error
+	GetState(role string, containerRole string) (bootModes []string, models []secboot.ModelForSealing, tpmPCRProfile []byte, err error)
+}


Suggested change

type FDEState interface {

UpdateState(role string, containerRole string, bootModes []string, models []secboot.ModelForSealing, tpmPCRProfile []byte) error

GetState(role string, containerRole string) (bootModes []string, models []secboot.ModelForSealing, tpmPCRProfile []byte, err error)

}

type FDEStateManager interface {

Update(role string, containerRole string, bootModes []string, models []secboot.ModelForSealing, tpmPCRProfile []byte) error

Get(role string, containerRole string) (bootModes []string, models []secboot.ModelForSealing, tpmPCRProfile []byte, err error)

}

depending on how useful it would be for mocking in the tests the interface can be split into *Getter (eg. doReseal only uses Get(), and *Updater and then have one interface combining the two. Although looking at test changes it's probably not worth it.

bboozzoo

LGTM

pedronis

did a pass, bunch of comments and questions

pedronis · 2024-11-08T09:02:38Z

overlord/fdestate/backend/reseal.go

+		return err
+	}
+
+	_, recoveryModels, recoveryPCRProfile, err := manager.Get("recover", "system-save")


gadget has constants for things like "system-save", I even wonder if we should give them a type

Yes we should. I think we have FIXMEs on the Set calls.

pedronis · 2024-11-08T09:03:04Z

overlord/fdestate/backend/reseal.go

-type StateUpdater func(role string, containerRole string, bootModes []string, models []secboot.ModelForSealing, tpmPCRProfile []byte) error
+type FDEStateManager interface {
+	Update(role string, containerRole string, bootModes []string, models []secboot.ModelForSealing, tpmPCRProfile []byte) error
+	Get(role string, containerRole string) (bootModes []string, models []secboot.ModelForSealing, tpmPCRProfile []byte, err error)


not something we can do much about but is indeed not great that both things are called role, we need to be very careful

We should use types at least. Again, this is related to the FIXMEs on calls to Update.

pedronis · 2024-11-08T09:12:06Z

overlord/fdestate/backend/reseal.go

+	}
+}
+
+func resealKeyForBootChainsFDEHook(manager FDEStateManager, method device.SealingMethod, rootdir string, inputs resealInputs, opts resealOptions) error {


this functions needs a new name no? is not really doing the reseal anymore?

pedronis · 2024-11-08T09:12:15Z

overlord/fdestate/backend/reseal.go

-	inputs resealInputs,
-	opts resealOptions,
-) error {
+func resealKeyForBootChainsTPM(manager FDEStateManager, method device.SealingMethod, rootdir string, inputs resealInputs, opts resealOptions) error {


pedronis · 2024-11-08T09:15:37Z

overlord/fdestate/fdemgr.go

+	unlocker boot.Unlocker
+	m        *FDEManager


this could be simplified to:

{
FDEManager
unlock bool
}

as we just check if unlocker is nil

pedronis · 2024-11-08T09:16:12Z

overlord/fdestate/fdemgr.go

+
+func (m *unlockedStateManager) Update(role string, containerRole string, bootModes []string, models []secboot.ModelForSealing, tpmPCRProfile []byte) error {
+	if m.unlocker != nil {
+		m.m.state.Lock()


that would avoid the m.m here

pedronis · 2024-11-08T09:16:52Z

overlord/fdestate/fdemgr.go

+	return updateParameters(m.m.state, role, containerRole, bootModes, models, tpmPCRProfile)
+}
+
+func (m *unlockedStateManager) Get(role string, containerRole string) (bootModes []string, models []secboot.ModelForSealing, tpmPCRProfile []byte, err error) {


this needs testing

pedronis · 2024-11-08T09:18:06Z

overlord/fdestate/fdestate.go

+	var s FdeState
+	err = st.Get(fdeStateKey, &s)
+	if err != nil {
+		return nil, nil, nil, err
+	}


as this is not tested

pedronis · 2024-11-08T09:19:35Z

overlord/fdestate/fdestate.go

 	return nil
 }

+func getParameters(st *state.State, role string, containerRole string) (bootModes []string, models []secboot.ModelForSealing, tpmPCRProfile []byte, err error) {


we have the manager now where get/updateParameters are called? can they become simply method on it?

pedronis · 2024-11-08T09:21:46Z

overlord/fdestate/fdemgr.go

+}
+
+func (m *unlockedStateManager) Update(role string, containerRole string, bootModes []string, models []secboot.ModelForSealing, tpmPCRProfile []byte) error {
+	if m.unlocker != nil {


so now we unlock to run reseal below, then lock/unlock to update, then lock/unlock again to get, then relock? can we avoid or coalesce some of these?

We would have to bring the unlocker down. I think what we need to do use unlock while we calculate the profile. This is what takes time. Then we do not have to relock to Get/Update

is it better to save the state after the Update or not? unlock does that too

otherwise we could give FDEStateManger interface a Unlock() (relock func()) method and indeed use it in the backend only when needed

That sounds good. I will try that.

I have changed to do the unlocking only when calculating the profile. And put the unlocker into the FDEStateManager abstraction.

Instead of splitting completely FDE hook and TPM resealing, we split the calculation of role parameters, udpate the state, then do the resealing based on that state.

…esealing

pedronis

thanks, a few more cleanup comments

pedronis · 2024-11-11T15:41:11Z

overlord/fdestate/fdemgr.go

+	return m.GetParameters(role, containerRole)
+}
+
+func (m *unlockedStateManager) Unlock() {


it feels like returning the relock function or a nop function would be a bit simpler that needing to have a relock member?

pedronis · 2024-11-11T15:45:37Z

overlord/fdestate/backend/reseal.go

@@ -200,7 +191,7 @@ func resealKeys(
 		pbcJSON, _ := json.Marshal(pbc)
 		logger.Debugf("resealing (%d) to boot chains: %s", nextCount, pbcJSON)

-		err := resealRunObjectKeys(updateState, runOnlyPbc, pbc, inputs.signatureDBUpdate, authKeyFile, params.RoleToBlName)
+		err := resealRunObjectKeys(manager, runOnlyPbc, pbc, inputs.signatureDBUpdate, params.RoleToBlName)


it seems these function also needs a new name, like updateRunObjectKeysProtectionProfile or something like that?

pedronis · 2024-11-11T15:45:44Z

overlord/fdestate/backend/reseal.go

@@ -227,7 +218,7 @@ func resealKeys(
 		rpbcJSON, _ := json.Marshal(rpbc)
 		logger.Debugf("resealing (%d) to recovery boot chains: %s", nextFallbackCount, rpbcJSON)

-		err := resealFallbackObjectKeys(updateState, rpbc, inputs.signatureDBUpdate, authKeyFile, params.RoleToBlName)
+		err := resealFallbackObjectKeys(manager, rpbc, inputs.signatureDBUpdate, params.RoleToBlName)


same for this

…esealing

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

valentindavid mentioned this pull request Oct 25, 2024

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

Merged

valentindavid force-pushed the valentindavid/factorize-hook-tpm-resealing branch from a305470 to 75bd290 Compare October 25, 2024 13:42

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

valentindavid force-pushed the valentindavid/factorize-hook-tpm-resealing branch from 75bd290 to 9ffe2f3 Compare November 5, 2024 10:05

valentindavid marked this pull request as ready for review November 5, 2024 10:05

valentindavid requested review from bboozzoo and pedronis November 5, 2024 10:05

valentindavid mentioned this pull request Nov 6, 2024

overlord/fdestate/backend/reseal.go: save run only boot chain in state #14668

Merged

pedronis reviewed Nov 6, 2024

View reviewed changes

bboozzoo approved these changes Nov 7, 2024

View reviewed changes

valentindavid requested a review from pedronis November 7, 2024 12:09

valentindavid force-pushed the valentindavid/factorize-hook-tpm-resealing branch from 4fab00f to b467e2d Compare November 7, 2024 15:19

bboozzoo approved these changes Nov 7, 2024

View reviewed changes

pedronis requested changes Nov 8, 2024

View reviewed changes

valentindavid added 3 commits November 8, 2024 15:49

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

b31b0e6

Instead of splitting completely FDE hook and TPM resealing, we split the calculation of role parameters, udpate the state, then do the resealing based on that state.

fixup! overlord/fdestate/backend/reseal.go: factor FDE hook and TPM r…

1885186

…esealing

fixup! overlord/fdestate/backend/reseal.go: factor FDE hook and TPM r…

7daac54

…esealing

valentindavid force-pushed the valentindavid/factorize-hook-tpm-resealing branch from 1a50feb to f981410 Compare November 8, 2024 14:49

fixup! overlord/fdestate/backend/reseal.go: factor FDE hook and TPM r…

b753897

…esealing

valentindavid force-pushed the valentindavid/factorize-hook-tpm-resealing branch from f981410 to b753897 Compare November 8, 2024 15:01

fixup! overlord/fdestate/backend/reseal.go: factor FDE hook and TPM r…

d7b7ad0

…esealing

valentindavid requested a review from pedronis November 8, 2024 16:42

pedronis reviewed Nov 11, 2024

View reviewed changes

valentindavid added 2 commits November 12, 2024 17:20

fixup! overlord/fdestate/backend/reseal.go: factor FDE hook and TPM r…

5f7117d

…esealing

fixup! overlord/fdestate/backend/reseal.go: factor FDE hook and TPM r…

18e63e7

…esealing

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

valentindavid commented Oct 25, 2024

codecov bot commented Oct 25, 2024 •

edited

Loading

pedronis left a comment

pedronis Nov 6, 2024

valentindavid Nov 6, 2024

valentindavid Nov 6, 2024

bboozzoo left a comment

bboozzoo Nov 7, 2024

bboozzoo left a comment

pedronis left a comment

pedronis Nov 8, 2024

valentindavid Nov 8, 2024

pedronis Nov 8, 2024

valentindavid Nov 8, 2024

pedronis Nov 8, 2024

pedronis Nov 8, 2024

pedronis Nov 8, 2024

pedronis Nov 8, 2024

pedronis Nov 8, 2024

pedronis Nov 8, 2024

pedronis Nov 8, 2024

pedronis Nov 8, 2024

valentindavid Nov 8, 2024

pedronis Nov 8, 2024

valentindavid Nov 8, 2024

valentindavid Nov 8, 2024

pedronis left a comment

pedronis Nov 11, 2024

pedronis Nov 11, 2024

pedronis Nov 11, 2024

		type StateUpdater func(role string, containerRole string, bootModes []string, models []secboot.ModelForSealing, tpmPCRProfile []byte) error
		type StateGetter func(role string, containerRole string) (bootModes []string, models []secboot.ModelForSealing, tpmPCRProfile []byte, err error)

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

Are you sure you want to change the base?

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

Conversation

valentindavid commented Oct 25, 2024

codecov bot commented Oct 25, 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

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

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

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

codecov bot commented Oct 25, 2024 •

edited

Loading