overlord/fdestate: keep FDE state up to date #14516

valentindavid · 2024-09-17T12:15:02Z

Ensure() initializes the empty profiles, and reseal updates them.

This is on top of #14400

valentindavid · 2024-09-20T14:42:16Z

overlord/fdestate/fdestate.go

+	}
+}
+
+type KeyDigest struct {


This might be a type that should be handled in secboot. And we just use json.RawMessage for it.

valentindavid · 2024-09-20T14:43:08Z

overlord/fdestate/fdemgr.go

+		if !locked {
+			m.state.Lock()
+			defer m.state.Unlock()
+		}


This is not elegant. But I am not sure how to handle it correctly. We do resealing sometimes locked, sometimes not.

do we know where is it called with the state really already unlocked?

overlord/fdestate/fdestate.go

pedronis · 2024-09-24T08:59:26Z

overlord/fdestate/fdemgr.go

+	m.state.Lock()
+	defer m.state.Unlock()
+	return ensureState(m.state)


it's a bit unclear if this will really run before any reseal op? maybe it would be better to use StartUp for this?

secboot/secboot.go

valentindavid · 2024-10-02T12:25:32Z

overlord/fdestate/fdemgr.go

+func (m *FDEManager) resealKeyForBootChains(unlocker boot.Unlocker, method device.SealingMethod, rootdir string, params *boot.ResealKeyForBootChainsParams, expectReseal bool) error {
+	doUpdate := func(role string, containerRole string, bootModes []string, models []secboot.ModelForSealing, tpmPCRProfile []byte) error {
+		if unlocker != nil {
+			m.state.Lock()
+			defer m.state.Unlock()
+		}
+		return updateParameters(m.state, role, containerRole, bootModes, models, tpmPCRProfile)
+	}
+	if unlocker != nil {
+		locker := unlocker()
+		defer locker()
+	}
+	return backend.ResealKeyForBootChains(doUpdate, method, rootdir, params, expectReseal)
 }


This function is not covered by tests in this package, but still covered by tests from overlord/managers_test.go. I wonder if we should add tests in the current package. Or move them.

ideally it should be tested in the package directly too

Done. It was easier than expected.

pedronis

drive-by comment

secboot/secboot_tpm.go

bboozzoo · 2024-10-03T13:28:04Z

secboot/secboot.go

 type ResealKeysParams struct {
 	// The snap model parameters
-	ModelParams []*SealKeyModelParams
+	PCRProfile SerializedPCRProfile


could this be []byte ?

Do you want to convert a json into base64 to put it in another json?

I have added marshalling/unmarshalling methods here.

bboozzoo · 2024-10-03T14:33:15Z

secboot/secboot_sb.go

@@ -291,3 +295,32 @@ func DeleteKeys(node string, matches map[string]bool) error {

 	return nil
 }
+
+func GetPrimaryKeyHash(devicePath string, alg crypto.Hash) ([]byte, []byte, error) {


Suggested change

func GetPrimaryKeyHash(devicePath string, alg crypto.Hash) ([]byte, []byte, error) {

func GetPrimaryKeyHash(devicePath string, alg crypto.Hash) (salt []byte, digest []byte, err error) {

bboozzoo · 2024-10-03T14:35:11Z

secboot/secboot_sb.go

+		return nil, nil, err
+	}
+
+	h := hmac.New(func() hash.Hash { return alg.New() }, salt[:])


this should work:

Suggested change

h := hmac.New(func() hash.Hash { return alg.New() }, salt[:])

h := hmac.New(alg.New, salt[:])

bboozzoo · 2024-10-03T14:35:42Z

secboot/secboot_sb.go

+		return false, err
+	}
+
+	h := hmac.New(func() hash.Hash { return alg.New() }, salt[:])


Suggested change

h := hmac.New(func() hash.Hash { return alg.New() }, salt[:])

h := hmac.New(alg.New, salt[:])

bboozzoo · 2024-10-04T07:23:29Z

osutil/disks/disks.go

+
+// ErrMountPointNotFound is return by DMCryptUUIDFromMountPoint a path
+// is not a mount point
+type ErrMountPointNotFound struct {


to keep uses or errors.Is() simple, you could have:

var ErrMountPointNotFound = errors.New("cannot find mountpoint") type errMountPointNotFound struct{ Path string } func (e *errMountPointNotFound) Error() string { return fmt.Sprintf("cannot find mountpoint %q", e.Path) } func (e *errMountPointNotFound) Unwrap() { return ErrMountPintNotFound }

bboozzoo · 2024-10-04T07:34:21Z

overlord/fdestate/backend/reseal.go

+	var models []secboot.ModelForSealing
+	for _, m := range modelParams {
+		models = append(models, m.Model)
+	}


Suggested change

var models []secboot.ModelForSealing

for _, m := range modelParams {

models = append(models, m.Model)

}

func modelsFromModelParams() []secboot.ModelForSealing {

var models []secboot.ModelForSealing

for _, m := range modelParams {

models = append(models, m.Model)

}

return models

}

Not sure I understand. Github is probably not showing me the right thing. Do you want me to factor the similar code from resealFallbackObjectKeys and resealRunObjectKeys?

yes, not a big thing though, but both functions are close to each other and maybe it would be a bit nicer

bboozzoo · 2024-10-04T07:37:41Z

overlord/fdestate/fdestate.go

+	}
+
+	if errors.Is(dataErr, disks.ErrNoDmUUID) && errors.Is(saveErr, disks.ErrNoDmUUID) {
+		return nil


this could use a comment

bboozzoo · 2024-10-04T07:38:54Z

overlord/fdestate/fdestate.go

+		return err
+	}
+	if !sameDigest {
+		return fmt.Errorf("Primary key for data and save partition are not the same")


Suggested change

return fmt.Errorf("Primary key for data and save partition are not the same")

return fmt.Errorf("primary key for data and save partition are not the same")

bboozzoo · 2024-10-04T07:40:20Z

overlord/fdestate/fdestate.go

+
+	// Note that Parameters will be updated on first update
+	s.KeyslotRoles = map[string]KeyslotRoleInfo{
+		"run+recover": {


feels like this should be a const somewhere to rule out use of say recover+run

The problem here is that we need that value both in overlord/fdestate and overlord/fdestate/backend. But overlord/fdestate/backend can not import overlord/fdestate. So it has to be in the backend. Unless we create new package. @pedronis what do you think?

let's leave TODOs for now, I wonder which other places will need these string constants

bboozzoo · 2024-10-04T07:42:32Z

overlord/fdestate/backend/reseal.go

 		KeyFiles:             keyFiles,
 		TPMPolicyAuthKeyFile: authKeyFile,
 	}
 	if err := secbootResealKeys(resealKeyParams); err != nil {
 		return fmt.Errorf("cannot reseal the encryption key: %v", err)
 	}

+	if err := updateState("run+recover", "all", []string{"run", "recover"}, models, pcrProfile); err != nil {


perhaps same thing about bare "all", what if we simply have type for it?

Let's leave a TODO about maybe. We'll try to figure it out later when we have a better feeling of what's needed.

bboozzoo

LGTM

bboozzoo · 2024-10-07T08:33:01Z

overlord/fdestate/fdestate.go

+	// BootModes are the optional list of approved modes (run, recover, ...)
+	BootModes []string `json:"boot-modes,omitempty"`
+	// Tpm2PCRProfile is an optional serialized PCR profile
+	Tpm2PCRProfile secboot.SerializedPCRProfile `json:"tpm2-pcr-profile,omitempty"`


Suggested change

Tpm2PCRProfile secboot.SerializedPCRProfile `json:"tpm2-pcr-profile,omitempty"`

TPM2PCRProfile secboot.SerializedPCRProfile `json:"tpm2-pcr-profile,omitempty"`

bboozzoo · 2024-10-07T08:33:16Z

overlord/fdestate/fdestate.go

+	// Tpm2PCRPolicyRevocationCounter is the handle for the TPM
+	// policy revocation counter.  A value of 0 means it is not
+	// set.
+	Tpm2PCRPolicyRevocationCounter uint32 `json:"tpm2-pcr-policy-revocation-counter,omitempty"`


Suggested change

Tpm2PCRPolicyRevocationCounter uint32 `json:"tpm2-pcr-policy-revocation-counter,omitempty"`

TPM2PCRPolicyRevocationCounter uint32 `json:"tpm2-pcr-policy-revocation-counter,omitempty"`

bboozzoo · 2024-10-07T08:33:23Z

overlord/fdestate/fdestate.go

+type KeyslotRoleInfo struct {
+	// PrimaryKeyId is the ID for the primary key found in
+	// PrimaryKeys field of FdeState
+	PrimaryKeyId int `json:"primary-key-id"`


Suggested change

PrimaryKeyId int `json:"primary-key-id"`

PrimaryKeyID int `json:"primary-key-id"`

bboozzoo · 2024-10-07T08:41:50Z

overlord/fdestate/fdemgr_test.go

+		&mockModel{},
+	}
+
+	fdestate.UpdateParameters(st, "run+recover", "container-role", []string{"run"}, models, secboot.SerializedPCRProfile(`"serialized-profile"`))


Suggested change

fdestate.UpdateParameters(st, "run+recover", "container-role", []string{"run"}, models, secboot.SerializedPCRProfile(`"serialized-profile"`))

fdestate.UpdateParameters(st, "run+recover", "container-role", []string{"run"}, models, secboot.SerializedPCRProfile(`{"something":"serialized-profile"}`))

Any reason why? It was valid json.

I suppose just to make a bit more realistic in terms of the kind of object we expect here, "aaa" is valid JSON but is also valid many other things

bboozzoo · 2024-10-07T08:45:12Z

overlord/fdestate/fdemgr_test.go

+	fdestate.UpdateParameters(st, "run+recover", "container-role", []string{"run"}, models, secboot.SerializedPCRProfile(`"serialized-profile"`))
+
+	var fdeSt fdestate.FdeState
+	err := st.Get("fde", &fdeSt)


and to make sure serialization for SerialziedPCRProfile is working correctly I think I'd add something like:

var raw map[string]any c.Assert(t.Get("fde", &raw), Nil) c.Check(raw, DeepEquals, map[string]any{ ... "tpm2-prc-profile": map[string]any { "something": "serialized-profile", } })

I do not think that would work. The TPM2PCRProfile field is kept serialized.

Ah I get it.

I have put a test in secboot because this is where we define that type.

StartUp() initializes the empty profiles, and reseal updates them.

pedronis

thank you, bunch of small comments

pedronis · 2024-10-07T16:37:24Z

osutil/disks/disks.go

@@ -237,3 +238,23 @@ func RegisterDeviceMapperBackResolver(name string, f func(dmUUID, dmName []byte)
 func unregisterDeviceMapperBackResolver(name string) {
 	delete(deviceMapperBackResolvers, name)
 }
+
+// ErrNoDmUUID is return by DMCryptUUIDFromMountPoint when the device


typo: returned

pedronis · 2024-10-07T16:37:34Z

osutil/disks/disks.go

@@ -237,3 +238,23 @@ func RegisterDeviceMapperBackResolver(name string, f func(dmUUID, dmName []byte)
 func unregisterDeviceMapperBackResolver(name string) {
 	delete(deviceMapperBackResolvers, name)
 }
+
+// ErrNoDmUUID is return by DMCryptUUIDFromMountPoint when the device
+// at the mount point is not a device mapper device


missing final .

pedronis · 2024-10-07T16:37:43Z

osutil/disks/disks.go

+var ErrNoDmUUID = errors.New("device has no DM_UUID")
+
+// ErrMountPointNotFound is return by DMCryptUUIDFromMountPoint a path
+// is not a mount point


same issues

pedronis · 2024-10-07T16:38:51Z

secboot/secboot_tpm.go

@@ -628,6 +625,16 @@ func buildPCRProtectionProfile(modelParams []*SealKeyModelParams) (*sb_tpm2.PCRP
 	return pcrProfile, nil
 }

+// BuildPCRProtectionProfile builds and serializes a PCR profile from
+// a list of SealKeyModelParams


missing final .

pedronis · 2024-10-07T16:47:31Z

overlord/fdestate/fdestate.go

+
+	roleInfo, hasRole := s.KeyslotRoles[role]
+	if !hasRole {
+		return fmt.Errorf("Cannot find role %s", role)


cannot find key role ?

pedronis · 2024-10-07T16:48:04Z

overlord/fdestate/fdestate.go

+	case "sha256":
+		*h = hashAlg(crypto.SHA256)
+	default:
+		return fmt.Errorf("Unknown algorithm %s", s)


lowercase: unknown ...

pedronis · 2024-10-07T16:48:18Z

overlord/fdestate/fdestate.go

+	case crypto.SHA256:
+		return json.Marshal("sha256")
+	default:
+		return nil, fmt.Errorf("Unknown algorithm %v", h)


lowercase: unknown ...

pedronis · 2024-10-07T16:48:50Z

overlord/fdestate/fdemgr.go

+func (m *FDEManager) resealKeyForBootChains(unlocker boot.Unlocker, method device.SealingMethod, rootdir string, params *boot.ResealKeyForBootChainsParams, expectReseal bool) error {
+	doUpdate := func(role string, containerRole string, bootModes []string, models []secboot.ModelForSealing, tpmPCRProfile []byte) error {
+		if unlocker != nil {
+			m.state.Lock()
+			defer m.state.Unlock()
+		}
+		return updateParameters(m.state, role, containerRole, bootModes, models, tpmPCRProfile)
+	}
+	if unlocker != nil {
+		locker := unlocker()
+		defer locker()
+	}
+	return backend.ResealKeyForBootChains(doUpdate, method, rootdir, params, expectReseal)
 }


ideally it should be tested in the package directly too

valentindavid added the FDE Manager Pull requests that target FDE manager branch label Sep 17, 2024

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

valentindavid changed the title ~~overlord/fdestate: Keep FDE state up to date~~ overlord/fdestate: keep FDE state up to date Sep 17, 2024

valentindavid force-pushed the valentindavid/sync-fde-state branch 12 times, most recently from 728ff63 to 14d22b2 Compare September 20, 2024 14:23

valentindavid commented Sep 20, 2024

View reviewed changes

overlord/fdestate/fdestate.go Outdated Show resolved Hide resolved

valentindavid marked this pull request as ready for review September 20, 2024 14:46

valentindavid requested a review from pedronis September 20, 2024 14:46

pedronis reviewed Sep 24, 2024

View reviewed changes

secboot/secboot.go Outdated Show resolved Hide resolved

valentindavid added the Run nested The PR also runs tests inluded in nested suite label Oct 1, 2024

valentindavid closed this Oct 1, 2024

valentindavid reopened this Oct 1, 2024

valentindavid force-pushed the valentindavid/sync-fde-state branch 2 times, most recently from 425117e to 5d69be1 Compare October 2, 2024 11:35

valentindavid requested a review from pedronis October 2, 2024 12:05

valentindavid commented Oct 2, 2024

View reviewed changes

pedronis reviewed Oct 2, 2024

View reviewed changes

secboot/secboot_tpm.go Show resolved Hide resolved

valentindavid requested review from bboozzoo, ZeyadYasser and chrisccoulson October 3, 2024 08:59

bboozzoo reviewed Oct 3, 2024

View reviewed changes

bboozzoo reviewed Oct 4, 2024

View reviewed changes

valentindavid requested review from pedronis and bboozzoo October 7, 2024 08:21

bboozzoo approved these changes Oct 7, 2024

View reviewed changes

valentindavid force-pushed the valentindavid/sync-fde-state branch 2 times, most recently from 284c647 to 00abb2d Compare October 7, 2024 09:59

overlord/fdestate: keep FDE state up to date

46eb39e

StartUp() initializes the empty profiles, and reseal updates them.

valentindavid force-pushed the valentindavid/sync-fde-state branch from 00abb2d to 46eb39e Compare October 7, 2024 11:39

pedronis reviewed Oct 7, 2024

View reviewed changes

valentindavid added 3 commits October 8, 2024 12:25

fixup! overlord/fdestate: keep FDE state up to date

320d014

fixup! overlord/fdestate: keep FDE state up to date

27e7e14

fixup! overlord/fdestate: keep FDE state up to date

805db8c

	func GetPrimaryKeyHash(devicePath string, alg crypto.Hash) ([]byte, []byte, error) {
	func GetPrimaryKeyHash(devicePath string, alg crypto.Hash) (salt []byte, digest []byte, err error) {

	h := hmac.New(func() hash.Hash { return alg.New() }, salt[:])
	h := hmac.New(alg.New, salt[:])

	return fmt.Errorf("Primary key for data and save partition are not the same")
	return fmt.Errorf("primary key for data and save partition are not the same")

	Tpm2PCRProfile secboot.SerializedPCRProfile `json:"tpm2-pcr-profile,omitempty"`
	TPM2PCRProfile secboot.SerializedPCRProfile `json:"tpm2-pcr-profile,omitempty"`

	Tpm2PCRPolicyRevocationCounter uint32 `json:"tpm2-pcr-policy-revocation-counter,omitempty"`
	TPM2PCRPolicyRevocationCounter uint32 `json:"tpm2-pcr-policy-revocation-counter,omitempty"`

	PrimaryKeyId int `json:"primary-key-id"`
	PrimaryKeyID int `json:"primary-key-id"`

	fdestate.UpdateParameters(st, "run+recover", "container-role", []string{"run"}, models, secboot.SerializedPCRProfile(`"serialized-profile"`))
	fdestate.UpdateParameters(st, "run+recover", "container-role", []string{"run"}, models, secboot.SerializedPCRProfile(`{"something":"serialized-profile"}`))

overlord/fdestate: keep FDE state up to date #14516

Are you sure you want to change the base?

overlord/fdestate: keep FDE state up to date #14516

Conversation

valentindavid commented Sep 17, 2024 • edited Loading

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

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

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

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

valentindavid commented Sep 17, 2024 •

edited

Loading