[1/3] Keydata v3 platform API changes by sespiros · Pull Request #265 · canonical/secboot

sespiros · 2023-10-23T15:53:25Z

Original commit: chrisccoulson@87d9aa3

I will split the commit above in 3 separate cascading PRs and add tests for it:

sespiros · 2023-10-24T09:49:13Z

I don't have permissions to assign reviewers, @chrisccoulson could you add @pedronis?

chrisccoulson

Thanks for working on this - I've left some comments inline.

chrisccoulson · 2024-01-05T15:02:23Z


 func (s *cryptSuite) TestInitializeLUKS2ContainerInvalidKeySize(c *C) {
-	c.Check(InitializeLUKS2Container("/dev/sda1", "data", s.newPrimaryKey()[0:16], nil), ErrorMatches, "expected a key length of at least 256-bits \\(got 128\\)")
+	c.Check(InitializeLUKS2Container("/dev/sda1", "data", ([]byte)(s.keyDataTestBase.newPrimaryKey(c, 32))[0:16], nil), ErrorMatches, "expected a key length of at least 256-bits \\(got 128\\)")


As keyDataTestBase.newPrimaryKey takes a size now, you don't need to create a 32-byte key here and then truncate it to 16 bytes.

Also, is the type conversion to []byte necessary here? That should happen implicitly because []byte is not a named type. This is repeated in a few other places too.

For an idea of when explicit conversions are necessary when converting between types, take a look at this section of the language spec: Assignability (in this case, "V and T have identical underlying types but are not type parameters and at least one of V or T is not a named type").

Removed the truncation, thanks.

Regarding the type conversion, keyDataTestBase's newPrimaryKey returns type PrimaryKey while all the LUKS related APIs expect type DiskUnlockKey so I believe this doesn't fall under the categories above so the type conversion is needed despite them having the same underlying type []byte.

Ah, I think the problem here is that perhaps there should have originally been a newUnlockKey method for returning a key with the correct type.

chrisccoulson · 2024-01-05T15:40:00Z

 	auxKey := testutil.DecodeHexString(c, "8107f1c65c58934f0d59245d1d94d312ea803e69c8599a7bac8c67fe253232f2")
 	j := []byte(
 		`{` +
+			`"version":0,` +


As existing keys won't have this field, we should test that reading works for payloads that don't contain it.

Correct, I got confused here. So I modified the version behavior to use 0 of 1 for the version field if the Version is less than 2 and by marking the Version field as omitempty but I am not sure if that is correct. Can you think of any other legacy scenario that might be weird here?

I also had to modify the tests in keydata_legacy_test.go to use 0 for legacy version. Is keyData with version 1 possible?

I added a comment separately for this - I think the omitempty is fine, but I would still fake version 1 in KeyData.Version so that it returns 1 if the field is 0. I'm not sure if it matters either way to be honest, but it's a bit weird going from v0 -> v2.

Well the main reason I opted for this approach was that I couldn't think of a way to express "omit when 1". I left it as is and faked version 1 as you suggested.

chrisccoulson · 2024-01-05T15:43:42Z

+	EncodedHandle []byte // The JSON encoded platform handle
+	KDFAlg        crypto.Hash
+
+	AuthMode AuthMode


There's nothing in the tests inside mockPlatformKeyDataHandler that checks we pass the expected values for these new fields.

I added some validation for the mockPlatformKeyDataHandler fields. I found a missing field in the legacy tests because of it. Let me know how it looks. I hardcoded crypto.SHA256 in mockProtectKeys for the legacy keyData test bases as there aren't any test cases that test for different KDF algorithms.

Thanks, I added a comment for this separately before responding to this thread: #265 (comment)

chrisccoulson · 2024-01-12T15:04:57Z

 )

 const (
-	KeyDataVersion int = 2


It's probably cleaner to just define KeyDataVersion as a variable here rather than having a mockable function, and then MockKeyDataVersion can just set this instead.

Changed it to a private variable.

chrisccoulson · 2024-01-12T15:45:21Z

+		return xerrors.Errorf("Invalid AuthMode: %d", data.AuthMode)
+	}
+
+	return nil


The validation should check that the supplied fields are the expected values rather than bounds checking them. I'm not sure of the best way to do this though - maybe adding the expected values as fields to mockPlatformKeyDataHandle?

Reverted the last attempt and pushed some changes to the logic if you could take a look. I added Expected* fields to the mockPlatformKeyDataHandle but tbh I find it a bit confusing to parse.

I think this is ok. One other way to implement this would be to make mockKeyDataHandler use GCM and have the extra properties as part of the AAD instead, but maybe we can change that in a follow-up.

chrisccoulson · 2024-01-12T15:47:04Z

+	. "gopkg.in/check.v1"
+)
+
+type keyDataLegacyTestBase struct {


This type is currently only used in one place (keyDataLegacySuite) - do subsequent PRs add other uses of it, or should it just be part of keyDataLegacySuite?

So I had refactored and completely removed that file in the subsequent PR, check 4cea1c7

It looks like this change doesn't exist in the most recently pushed branch.

chrisccoulson · 2024-01-12T15:53:02Z

+}
+
+func (d *KeyData) Version() int {
+	return d.data.Version


I think having this field as omitempty is fine, but I would probably still convert 0 -> 1 here for when the field is not present.

I reverted it to the previous implementation which fakes 1 for version 0.

chrisccoulson · 2024-01-12T15:54:30Z

+	EncodedHandle []byte // The JSON encoded platform handle
+	KDFAlg        crypto.Hash
+
+	AuthMode AuthMode


Thanks, I added a comment for this separately before responding to this thread: #265 (comment)

chrisccoulson

Thanks for the updates - I've left a few more comments

chrisccoulson · 2024-01-12T16:06:40Z

The test failures are occurring because the callback returned from MockKeyDataVersion isn't being called at the end of the test. It should be used something like this:

restore := MockKeyDataVersion(0)
defer restore()

pedronis

did a first quick pass on the top level changes, I have some questions/comments

pedronis · 2024-01-15T13:43:54Z

 	// RecoverKeys attempts to recover the cleartext keys from the supplied key
 	// data using this platform's secure device.


all the doc comments here should match the signature changes, for this one and the other methods

Changed doc comments. @chrisccoulson could you also take a look?

pedronis · 2024-01-15T13:47:18Z

 )

 const (
+	keyDataVersion int = 2


would be good to have a doc comment on this, probably also mentioning some properties for the old 0 value

I added a slightly more complete doc comment in KeyData's Version(). Should we add something here too?

pedronis · 2024-01-15T13:48:06Z

+	})
+}
+
+func MakeDiskUnlockKey(rand io.Reader, alg crypto.Hash, primaryKey PrimaryKey) (unlockKey DiskUnlockKey, cleartextPayload []byte, err error) {


this needs a doc comment

pedronis · 2024-01-15T13:51:32Z

+func (d *KeyData) recoverKeysCommon(data []byte) (DiskUnlockKey, PrimaryKey, error) {
+	switch d.Version() {
+	case 0:
+		unlockKey, primaryKey, err := unmarshalV1KeyPayload(data)


why V1? as Version here is 0? also we have already key own versions, maybe we should use a different naming/termilogy than just version, internal for this level?

This is related to #265 (comment) as well.

So I already changed it and replied to the comment above but summarizing it again to make sure I don't miss something. The following versions are possible:

version 0. Key data already used are missing a version field. Version() called in that keydata will return 1 and unmarshalV1KeyPayload will be called.

version 1 shouldn't exist (?), but even then the behavior would be the same as version 0 keys.

version 2 is the currently introduced format.

When I first saw V1, I assumed there was another V0. Isn't that the case? If not, maybe we could simply call old keys legacy keys?

Maybe "generation"? So that we can refer to cross-platform keydata versions as generations and then each platform can refer to its internal version simply as "version".

i.e TPM platform's key versions 0-2 use cross-platform keys of generation 1 (or 0, this is still a bit confusing) while TPM platform's version 3 keys use generation 2 cross-platform keys.

"generation" would work for me

Yes, generation is ok with me as well.

pedronis

did another pass, a few comments, from a review POV my preference would be for the rename Auxiliary=>Primary to be a separate preparatory PR, it's hard to review the other changes mixed with that mechanical one

pedronis · 2024-01-17T13:42:17Z

+		// This is to support the legacy TPM key data created
+		// via tpm2.NewKeyDataFromSealedKeyObjectFile.
+		return k.Unique


this path is not exercised

I am not sure about this, maybe @chrisccoulson has a better understanding. In my understanding this path is exercised for example by legacy TPM keys (as the comment suggests). For instance platformLegacySuite.TestRecoverKeys is a test in tpm2/platform_legacy_test.go that fails if this code is removed.

but if Version is 1 we don't arrive here, as I said I'm confused by the role of KDFAlg vs Version here and the KeyParams, aynway if this is useful there should be test in keydata_test.go that shows how this is meant to happen

This is exercised by the tests in the tpm2 package - specifically, the ones in tpm2/platform_legacy_test.go that call NewKeyDataFromSealedKeyObjectFile. This creates an ephemeral v2 key data but with 0 for the KDFAlg parameter. This path exists just for that case.

pedronis · 2024-01-17T13:42:33Z

+	return kd, nil
+}
+
+type protectedKeys struct {


this probably could use a doc comment

pedronis · 2024-01-17T13:43:38Z

 	return kd, nil
 }

+func NewKeyDataWithPassphrase(params *KeyWithPassphraseParams, passphrase string, kdf KDF) (*KeyData, error) {


needs a doc comment

Under the new design the disk unlock key will be derived from the primary key and a unique ID. The new key payload will be serialized and unserialized to ASN1.

Make the new API more generic by removing the EncryptedPayload from the PlatformKeyData struct and supply it as an extra argument to RecoverKeys*() functions. Also introduce new fields in the PlatformKeyData struct which will be included as authenticated data for the authenticated encryption.

under the new design a keyData object should always have an encrypted payload therefore: - the EncryptedPayload field of keyData cannot be omitted. - passphraseData is removed in favor of a new passphraseParams field. - AuthMode() will return the type of authentication mode used by this particular keyData object based on the existence of the passphraseParams field.

Under the new design, keyData objects will be differentiated based on whether they are passphrase protected or not. This property of new keyData objects will now be immutable. For that reason passphraseData struct is converted to the new passphraseParams struct which holds the parameters of the passphrase and doesn't contain an EncryptedPayload field anymore. For passphrase protected payloads the key derivation has changed and now it should work as follows: - a strong key will be derived from the passphrase - 3 values will be derived from that strong key given as the IKM input to an hkdf - an encryption key enc with info PASSPHRASE-ENC - an IV with info PASSPHRASE-IV - an auth key with info PASSPHRASE-AUTH The encryption key and IV will be used for the encryption/decryption of keyData's payload which happens on the platform agnostic API. The auth key will then need to be used as a way of key validation using the secure device (i.e the authentication value of the sealed object for the TPM case). To support unmarshalling of keys generated with the old format, the new unmarshalV1KeyPayload() was created under keydata_legacy.go.

under the new design, a keyData object will either have or not have a passphrase therefore there is no use for setting a passphrase in keyData objects that don't have one or clearing.

tests that need to test legacy behavior were copied over to keydata_legacy_test.go and then modified to support the new format in keydata_test.go

and also pass passphrase derivation's kdf hash algorithm as an extra argument.

pedronis · 2024-01-29T16:04:53Z

-	s.checkKeyDataJSONFromReaderAuthModeNone(c, f, protected, 0)
+	var j map[string]interface{}
+
+	d := json.NewDecoder(f)


need to import json

pedronis

thanks, I'm confused by the role of KDFAlg nilHash in a couple of places, maybe it's really a question for Chris

pedronis · 2024-01-29T16:44:52Z

+	// is reserved to support legacy TPM2 key data files, and tells the
+	// KeyData to use the unique key as the unlock key rather than using it
+	// to derive the unlock key.
+	KDFAlg crypto.Hash


I'm confused by the comment about the legacy case, as this used for creating a new KeyData and I don't see any code that special case this KDFAlg when it's null, nor I understand what goes into EncryptedPayload in that case

So I gave it a second look and my guess would be that this is probably forgotten comment/code (maybe from an older implementation?), since we use the version field to differentiate the keys (which now is also marked as omitempty). I removed the unreachable block and the existing tests pass (as expected).

There is some legacy code that reads the original TPM2 keydata files and creates an ephemeral KeyData with 0 for KDFAlg - see tpm2.NewKeyDataFromSealedKeyObjectFile. Removing the special case here should cause the tests in tpm2/platform_legacy_test.go to fail.

those are files without the json wrapper?

Thanks. I would mention tpm2.NewKeyDataFromSealedKeyObjectFile in the comment for clarity.

pedronis · 2024-01-29T16:46:59Z

+		// This is to support the legacy TPM key data created
+		// via tpm2.NewKeyDataFromSealedKeyObjectFile.
+		return k.Unique


but if Version is 1 we don't arrive here, as I said I'm confused by the role of KDFAlg vs Version here and the KeyParams, aynway if this is useful there should be test in keydata_test.go that shows how this is meant to happen

pedronis

this looks reasonable to me, question about testing though. The question about what to do with "version" in the JSON is still open though

pedronis · 2024-02-02T13:34:00Z


-	key, err := kdf.Derive(passphrase, salt[:], params, uint32(keyLen))
+	auth = make([]byte, params.AuthKeySize)
+	r = hkdf.Expand(func() hash.Hash { return kdfAlg.New() }, derived, []byte("PASSPHRASE-AUTH"))


if I change the constant string no tests seem to fail, I wonder if and how this bit of the code is tested

That's expected, as the same code is used to derive keys when both setting and using a passphrase. It would only fail if a key was configured with one string and then unsealed with a different one.

I added a test case for that for good measure. The test hardcodes a keydata generated with the current valid info fields.

The added test looks ok - one note for future reference is that we have testutil.DecodeHexString for embedding bytes into tests which will fail the test if an invalid string is supplied without having to check an error.

use "generation" instead of "version" in the cross-platform APIs keydata struct so that it is easier to parse and differentiate it from the TPM platform's implementation which is also using "version" for its keydata interface implementation.

chrisccoulson · 2024-02-14T17:37:55Z

Thanks - I think the latest changes look good to me

pedronis

thank you, small comment, and a comment/question

pedronis · 2024-02-15T16:13:16Z


 var (
-	keyDataVersion        int = 2
+	keyDataGeneration     int = 2


a short doc comment describing this even it's private might not hurt

I've actually got a follow-up PR that makes this public (so that the version can be bound into the AAD)

oh sorry about that, I thought I had that change in here already, I also make it public in the final PR because I needed it for a test for AAD 33445cf

pedronis · 2024-02-15T16:14:48Z

+	// Generation is a number used to differentiate between different key formats.
+	// i.e Gen1 keys are binary serialized and include a primary and an unlock key while
+	// Gen2 keys are ASN1 serialized and include a primary key and a unique key which is
+	// used to derive the unlock key.


The generations might also support different set of platform data fields?

Keydata testsuite minor refactors and improvements and hashAlg. This addresses: - keyDataLegacyTestBase not used from #265 (comment) - use DecodeHexString from #265 (comment) Also makes hashAlg public as it will be used in a follow-up PR which will slightly rework the mockPlatformKeyDataHandler to use GCM and associated data to check expected values.

sespiros changed the title ~~Keydata v3 platform API changes~~ [1/3] Keydata v3 platform API changes Oct 27, 2023

sespiros mentioned this pull request Oct 27, 2023

[2/3] Keydata v3 scope changes #270

Merged

sespiros mentioned this pull request Nov 17, 2023

[3/3] Keydata v3 TPM platform changes #271

Merged

sespiros force-pushed the keydata-v3-platform branch 2 times, most recently from 515a998 to 80297ae Compare November 23, 2023 10:28

sespiros requested a review from chrisccoulson December 4, 2023 12:52

chrisccoulson suggested changes Jan 5, 2024

View reviewed changes

sespiros requested a review from chrisccoulson January 11, 2024 11:59

chrisccoulson reviewed Jan 12, 2024

View reviewed changes

chrisccoulson requested a review from pedronis January 12, 2024 15:57

pedronis reviewed Jan 15, 2024

View reviewed changes

pedronis self-requested a review January 16, 2024 10:44

pedronis requested changes Jan 17, 2024

View reviewed changes

sespiros requested review from chrisccoulson and pedronis January 22, 2024 19:41

Spyros Seimenis added 9 commits January 29, 2024 11:07

keydata.go: introduce MakeDiskUnlockKey() API and new key format

0dc8c1b

Under the new design the disk unlock key will be derived from the primary key and a unique ID. The new key payload will be serialized and unserialized to ASN1.

keydata.go: allow marshalling hashAlg to ASN1

f6cba7e

keydata.go: remove old API around passphrase protected payloads

98db494

under the new design, a keyData object will either have or not have a passphrase therefore there is no use for setting a passphrase in keyData objects that don't have one or clearing.

keydata*test.go: fix/add tests

fdfa79e

tests that need to test legacy behavior were copied over to keydata_legacy_test.go and then modified to support the new format in keydata_test.go

keydata*test.go: add API tests for keydata v3

6452fb6

crypt_test.go: add API tests for keydata v3

977d320

Spyros Seimenis added 8 commits January 29, 2024 11:07

keydata*.go: refactored the legacy keyData test suite

5af09ed

keydata*.go: modify version behavior and fix legacy test cases

1a9de9b

keydata_test.go: make use of helper in tests

08c2d56

keydata*test.go: validate mock platform data fields

c8cb933

and also pass passphrase derivation's kdf hash algorithm as an extra argument.

keydata.go,platform.go: add doc comments

3cf4c26

keydata.go: add doc comments

759419a

keydata_test.go: add tests for derivePassphraseKeys error handling

ce63a2f

keydata_test.go: test cleanup

25099ff

sespiros force-pushed the keydata-v3-platform branch from ce4c8df to 25099ff Compare January 29, 2024 09:09

pedronis reviewed Jan 29, 2024

View reviewed changes

fixup! keydata*test.go: validate mock platform data fields

c7d0f3b

pedronis reviewed Jan 29, 2024

View reviewed changes

fixup! keydata*test.go: validate mock platform data fields

7f9a3b7

pedronis self-requested a review February 1, 2024 17:32

pedronis reviewed Feb 2, 2024

View reviewed changes

Spyros Seimenis added 4 commits February 7, 2024 13:47

keydata*test.go: add test case for unavailable KDF

8c92177

keydata.go: add doc comment for KDFAlg field

18b89e5

keydata_test.go: add test case for derivation info fields

23c36bc

multiple: rename version field to generation

ce88f13

use "generation" instead of "version" in the cross-platform APIs keydata struct so that it is easier to parse and differentiate it from the TPM platform's implementation which is also using "version" for its keydata interface implementation.

pedronis self-requested a review February 15, 2024 07:59

pedronis approved these changes Feb 15, 2024

View reviewed changes

chrisccoulson approved these changes Feb 16, 2024

View reviewed changes

chrisccoulson merged commit 179584a into canonical:master Feb 19, 2024

This was referenced Feb 20, 2024

Keydata testsuite minor refactors and improvements and hashAlg #284

Merged

Keydata testsuite mockPlatformKeyDataHandler use GCM #285

Draft

		// RecoverKeys attempts to recover the cleartext keys from the supplied key
		// data using this platform's secure device.

Uh oh!

Conversation

sespiros commented Oct 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sespiros commented Oct 24, 2023

Uh oh!

chrisccoulson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chrisccoulson Jan 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sespiros Jan 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisccoulson Jan 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisccoulson left a comment

Choose a reason for hiding this comment

Uh oh!

chrisccoulson commented Jan 12, 2024

Uh oh!

pedronis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pedronis Jan 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

sespiros commented Oct 23, 2023 •

edited

Loading

chrisccoulson Jan 5, 2024 •

edited

Loading

sespiros Jan 10, 2024 •

edited

Loading

chrisccoulson Jan 12, 2024 •

edited

Loading

pedronis Jan 15, 2024 •

edited

Loading