feat: parameterize deprecated key formats in init and add testing (part 3/4) #376

asraa · 2022-09-13T18:26:00Z

Signed-off-by: Asra Ali asraa@google.com

PART 3 OF #329 (comment)

I know this makes a lot of changes in the summary, but they are all fairly minor.

Summary

feat: Add parameter to InitCmd to use deprecated key formats when adding HSM/online keys
fix: Fixes bug in InitCmd that adds pre-entries for allRootKeys (old + new) to targets role instead of root. E2E testing verifies this.
cleanup: KeyAndAttestations only requires the ecdsa public key, format agnostic.
fix: Fixes bug (?) where we store canonical JSON in the metadata store. We don't need this, and it breaks with the PEM newlines.
feat: Explicit error when we SignCmd but no signature was added to the metadata, useful for catching if we sign with PEM and do not expect PEM keys.
feat: Factors out the verify CLI command and uses it in e2e testing.
test: Extensive testing for verifying the migration process. Start with a root with deprecated format, and then flip the deprecated format bool and expect a key migration.

We still have a TODO to make the SignCmd CLI command allow multiple key IDs for the same sig. Awaiting part 4.

Added TODO for also unit testing the pre-entries logic. It is fragile and worth testing.

Release Note

Documentation

Signed-off-by: Asra Ali <asraa@google.com> lint Signed-off-by: Asra Ali <asraa@google.com>

asraa · 2022-09-14T17:44:08Z

A review would be great! Would love to wrap this up with part 4 so I can send out keyholder instructions!

cmd/tuf/app/init.go

kommendorkapten · 2022-09-15T11:50:21Z

cmd/tuf/app/init.go

@@ -321,7 +324,8 @@ func ClearEmptySignatures(store tuf.LocalStore, role string) error {
 }

 func jsonMarshal(v interface{}) ([]byte, error) {
-	b, err := cjson.Marshal(v)
+	// We don't need to canonically encode the payload in the store.
+	b, err := json.Marshal(v)


When changing from cjson to encoding/json for Marshalling, there is no risk that the checksum/signature for the metadata file will differ from what's written in the snapshot file? Or is this only verified prior to writing to the metadata cache, my memory is failing me here :)

Good question!
On get: we verify the checksum/length based on the retrieved metadata from the store. (no canonicalization)
On write: we write the snapshot file metadata (checksum/length) based on the retrieved metadata from the store (no canonicalization)

Admittedly, I feel like this is generally a little fragile. It requires that the implementations for GetMeta on the repo and client's store implementation and the repository writers SetMeta implementation return equivalent bytes.

It only works out this way because they both do not canonicalize. So snapshot checksums and client's verification of the checksum will still be correct, since they checksum exactly what we write to the store.

Does that make sense? I guess the tldr is no: snapshot checksums are generated based on what we write to the store, regardless of canonicalization or not.

Does that make sense? I guess the tldr is no: snapshot checksums are generated based on what we write to the store, regardless of canonicalization or not.

I think it does! But, this feels like it may break for sigstore tuf client v2, where multiple languages can share cache, as JSON serialization may differ? Although out of scope for this PR, we should keep track of that, right?

But, this feels like it may break for sigstore tuf client v2, where multiple languages can share cache, as JSON serialization may differ?

I think things will work by accident, as long as clients SetMeta with just the raw bytes, and don't store in a different representation.

we should keep track of that, right?

I think so. We should be able to catch issues with this with the presubmits, but I would like to ask on theupdateframework/specification for x-lang guidance here. Creating an issue

kommendorkapten

It looks good, and the test cases are convincing. Admittedly I'm still new to this code-base :)

joshuagl

This is awesome. Few minor typos and personally I would have preferred separate commits for some of the fixes, but really nice set of changes!

joshuagl · 2022-09-15T09:27:41Z

cmd/tuf/app/init.go

@@ -41,6 +40,7 @@ var DefaultThreshold = 3
 var ConsistentSnapshot = true

 // Use deprecated hex-encoded ECDSA keys.
+// TODO(asraa): Flip this to false or v5 when migration code complete.


Suggested change

// TODO(asraa): Flip this to false or v5 when migration code complete.

// TODO(asraa): Flip this to false for v5 when migration code complete.

?

joshuagl · 2022-09-15T09:29:23Z

cmd/tuf/app/init.go

+func InitCmd(ctx context.Context, directory, previous string,
+	threshold int, targetsConfig map[string]json.RawMessage,
+	snapshotRef string, timestampRef string,
+	deprecatdKeyFormat bool) error {


Suggested change

deprecatdKeyFormat bool) error {

deprecatedKeyFormat bool) error {

joshuagl · 2022-09-15T09:29:38Z

cmd/tuf/app/init.go

@@ -150,7 +153,7 @@ func InitCmd(ctx context.Context, directory, previous string, threshold int, tar
 	if err != nil {
 		return err
 	}
-	keys, err := getKeysFromDir(directory + "/keys")
+	keys, err := getKeysFromDir(directory+"/keys", deprecatdKeyFormat)


Suggested change

keys, err := getKeysFromDir(directory+"/keys", deprecatdKeyFormat)

keys, err := getKeysFromDir(directory+"/keys", deprecatedKeyFormat)

joshuagl · 2022-09-15T09:29:57Z

cmd/tuf/app/init.go

@@ -191,7 +194,7 @@ func InitCmd(ctx context.Context, directory, previous string, threshold int, tar

 	// Add keys used for snapshot and timestamp roles.
 	for role, keyRef := range map[string]string{"snapshot": snapshotRef, "timestamp": timestampRef} {
-		signerKey, err := pkeys.GetSigningKey(ctx, keyRef, DeprecatedEcdsaFormat)
+		signerKey, err := pkeys.GetSigningKey(ctx, keyRef, deprecatdKeyFormat)


Suggested change

signerKey, err := pkeys.GetSigningKey(ctx, keyRef, deprecatdKeyFormat)

signerKey, err := pkeys.GetSigningKey(ctx, keyRef, deprecatedKeyFormat)

joshuagl · 2022-09-15T09:32:55Z

cmd/tuf/app/init.go

@@ -321,7 +324,8 @@ func ClearEmptySignatures(store tuf.LocalStore, role string) error {
 }

 func jsonMarshal(v interface{}) ([]byte, error) {
-	b, err := cjson.Marshal(v)
+	// We don't need to canonically encode the payload in the store.


This is a great catch. We should store the raw data that we retrieve from the repository, not the canonicalised form.

joshuagl · 2022-09-15T09:33:23Z

cmd/tuf/app/init.go

@@ -334,7 +338,7 @@ func jsonMarshal(v interface{}) ([]byte, error) {
 	return out.Bytes(), nil
 }

-func getKeysFromDir(dir string) ([]*data.PublicKey, error) {
+func getKeysFromDir(dir string, deprecatdKeyFormat bool) ([]*data.PublicKey, error) {


Suggested change

func getKeysFromDir(dir string, deprecatdKeyFormat bool) ([]*data.PublicKey, error) {

func getKeysFromDir(dir string, deprecatedKeyFormat bool) ([]*data.PublicKey, error) {

joshuagl · 2022-09-15T09:33:35Z

cmd/tuf/app/init.go

@@ -346,7 +350,7 @@ func getKeysFromDir(dir string) ([]*data.PublicKey, error) {
 			if err != nil {
 				return nil, err
 			}
-			tufKey, err := pkeys.ToTufKey(*key, DeprecatedEcdsaFormat)
+			tufKey, err := pkeys.ToTufKey(*key, deprecatdKeyFormat)


Suggested change

tufKey, err := pkeys.ToTufKey(*key, deprecatdKeyFormat)

tufKey, err := pkeys.ToTufKey(*key, deprecatedKeyFormat)

joshuagl · 2022-09-16T08:51:13Z

Oh, this merged before I finished my review 🤦
Let me open a PR for the typo fixes.

Signed-off-by: Joshua Lock <jlock@vmware.com>

Signed-off-by: Joshua Lock <jlock@vmware.com> Signed-off-by: Joshua Lock <jlock@vmware.com>

feat: parametrize deprecated key formats in init and add testing

94777ab

Signed-off-by: Asra Ali <asraa@google.com> lint Signed-off-by: Asra Ali <asraa@google.com>

asraa force-pushed the part-3-init-param branch from f57718c to 94777ab Compare September 13, 2022 18:28

asraa requested review from haydentherapper, dlorenc, joshuagl and kommendorkapten September 13, 2022 18:31

asraa changed the title ~~feat: parameterize deprecated key formats in init and add testing~~ feat: parameterize deprecated key formats in init and add testing (part 3/4) Sep 13, 2022

kommendorkapten reviewed Sep 15, 2022

View reviewed changes

cmd/tuf/app/init.go Show resolved Hide resolved

kommendorkapten reviewed Sep 15, 2022

View reviewed changes

asraa mentioned this pull request Sep 15, 2022

FileMeta verification guidance with client store representations theupdateframework/specification#249

Open

kommendorkapten approved these changes Sep 15, 2022

View reviewed changes

haydentherapper approved these changes Sep 15, 2022

View reviewed changes

asraa merged commit dcd366a into sigstore:main Sep 15, 2022

joshuagl approved these changes Sep 16, 2022

View reviewed changes

joshuagl added a commit to joshuagl/root-signing that referenced this pull request Sep 16, 2022

Minor typo fixes for sigstore#376

12707de

Signed-off-by: Joshua Lock <jlock@vmware.com>

asraa pushed a commit that referenced this pull request Sep 16, 2022

Minor typo fixes for #376 (#379)

d87c091

Signed-off-by: Joshua Lock <jlock@vmware.com> Signed-off-by: Joshua Lock <jlock@vmware.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: parameterize deprecated key formats in init and add testing (part 3/4) #376

feat: parameterize deprecated key formats in init and add testing (part 3/4) #376

asraa commented Sep 13, 2022 •

edited

asraa commented Sep 14, 2022

kommendorkapten Sep 15, 2022

asraa Sep 15, 2022 •

edited

kommendorkapten Sep 15, 2022

asraa Sep 15, 2022

kommendorkapten left a comment

joshuagl left a comment

joshuagl Sep 15, 2022

joshuagl Sep 15, 2022

joshuagl Sep 15, 2022

joshuagl Sep 15, 2022

joshuagl Sep 15, 2022

joshuagl Sep 15, 2022

joshuagl Sep 15, 2022

joshuagl commented Sep 16, 2022

	// TODO(asraa): Flip this to false or v5 when migration code complete.
	// TODO(asraa): Flip this to false for v5 when migration code complete.

	deprecatdKeyFormat bool) error {
	deprecatedKeyFormat bool) error {

	keys, err := getKeysFromDir(directory+"/keys", deprecatdKeyFormat)
	keys, err := getKeysFromDir(directory+"/keys", deprecatedKeyFormat)

	signerKey, err := pkeys.GetSigningKey(ctx, keyRef, deprecatdKeyFormat)
	signerKey, err := pkeys.GetSigningKey(ctx, keyRef, deprecatedKeyFormat)

	func getKeysFromDir(dir string, deprecatdKeyFormat bool) ([]*data.PublicKey, error) {
	func getKeysFromDir(dir string, deprecatedKeyFormat bool) ([]*data.PublicKey, error) {

	tufKey, err := pkeys.ToTufKey(*key, deprecatdKeyFormat)
	tufKey, err := pkeys.ToTufKey(*key, deprecatedKeyFormat)

feat: parameterize deprecated key formats in init and add testing (part 3/4) #376

feat: parameterize deprecated key formats in init and add testing (part 3/4) #376

Conversation

asraa commented Sep 13, 2022 • edited

Summary

Release Note

Documentation

asraa commented Sep 14, 2022

Choose a reason for hiding this comment

asraa Sep 15, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kommendorkapten left a comment

Choose a reason for hiding this comment

joshuagl 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

joshuagl commented Sep 16, 2022

asraa commented Sep 13, 2022 •

edited

asraa Sep 15, 2022 •

edited