Sort signatures by keys IDs for deterministic signature order (#155)

ethan-lowman-dd · web-flow · commit 5908a16987fa · 2021-10-18T16:35:38.000-04:00
* Sort signatures by key ID * Improve slice comparison * Fix assert comment * Remove sortStrings test helper * Add test for Signer sort * Sort keys before usage * Revert "Sort keys before usage" This reverts commit 88d9340. * Rename to getSortedSigningKeys * Check sort in a different way
diff --git a/.gitignore b/.gitignore
@@ -1,2 +1,3 @@
+.DS_Store
 cmd/tuf/tuf
 cmd/tuf-client/tuf-client
diff --git a/client/testdata/go-tuf/consistent-snapshot-false/1/repository/2.root.json b/client/testdata/go-tuf/consistent-snapshot-false/1/repository/2.root.json
@@ -1,12 +1,12 @@
 {
 	"signatures": [
-		{
-			"keyid": "ce72db3f938914205461a415c9b7b91267a2079df991fd6283aa8461988c1add",
-			"sig": "65f63745aa8bd39132f827c427ea6d87f620ec805d9f376b6a8400dc3db7eb964e5423d31ed08867916d039661a70d6bf255bca552248021a4e78b5d357c2b0f"
-		},
 		{
 			"keyid": "b2403f96ae9b1089d8cbc15bbc35e9acbacd7984571f951b43aab56aedcfa84f",
 			"sig": "20e91b55c995989b270091b714347b15169285cb636ef05d68d49ed7b7a96ebfda13898e0d9ef928c382873b9dba90dca492dbf705d56a4b293adaaed574340f"
+		},
+		{
+			"keyid": "ce72db3f938914205461a415c9b7b91267a2079df991fd6283aa8461988c1add",
+			"sig": "65f63745aa8bd39132f827c427ea6d87f620ec805d9f376b6a8400dc3db7eb964e5423d31ed08867916d039661a70d6bf255bca552248021a4e78b5d357c2b0f"
 		}
 	],
 	"signed": {
diff --git a/client/testdata/go-tuf/consistent-snapshot-false/1/repository/root.json b/client/testdata/go-tuf/consistent-snapshot-false/1/repository/root.json
@@ -1,12 +1,12 @@
 {
 	"signatures": [
-		{
-			"keyid": "ce72db3f938914205461a415c9b7b91267a2079df991fd6283aa8461988c1add",
-			"sig": "65f63745aa8bd39132f827c427ea6d87f620ec805d9f376b6a8400dc3db7eb964e5423d31ed08867916d039661a70d6bf255bca552248021a4e78b5d357c2b0f"
-		},
 		{
 			"keyid": "b2403f96ae9b1089d8cbc15bbc35e9acbacd7984571f951b43aab56aedcfa84f",
 			"sig": "20e91b55c995989b270091b714347b15169285cb636ef05d68d49ed7b7a96ebfda13898e0d9ef928c382873b9dba90dca492dbf705d56a4b293adaaed574340f"
+		},
+		{
+			"keyid": "ce72db3f938914205461a415c9b7b91267a2079df991fd6283aa8461988c1add",
+			"sig": "65f63745aa8bd39132f827c427ea6d87f620ec805d9f376b6a8400dc3db7eb964e5423d31ed08867916d039661a70d6bf255bca552248021a4e78b5d357c2b0f"
 		}
 	],
 	"signed": {
diff --git a/client/testdata/go-tuf/consistent-snapshot-false/1/repository/snapshot.json b/client/testdata/go-tuf/consistent-snapshot-false/1/repository/snapshot.json
@@ -2,7 +2,7 @@
 	"signatures": [
 		{
 			"keyid": "289e5a9e71afd7909326aa4caea92f7557ee0e2283d8c31f0c3401ce67248a45",
-			"sig": "9a90d837dd4f3f4020777108de0ea9a06ea1d5e4693247a21feecf8ab5855e356dc0ef909086b664b9535cde8c74d9130cff8faa488d97f0cd2922f61d42f309"
+			"sig": "f832b8fd3e3a2ea65e35d8fe11861d9e252bef6c1a1775e39b4453d93314e0fc6c1b81b819af93ecd1433f707d06e026794e1dd3eccb1f5df5ed99dc3f13d30f"
 		}
 	],
 	"signed": {
@@ -11,7 +11,7 @@
 		"meta": {
 			"root.json": {
 				"hashes": {
-					"sha512": "db01435dcd98422dc54771acf9a0091ac2f6e223ad953dc665261b155bf84fdf4a3f306d197185f0a39fbc7f14ef2165c1aa609b2103dff49c473253f449ee02"
+					"sha512": "6b79dd15ae5aed0b96b5eb6226b27ba9b77f33bdac90a8da9b749120d312cd0240bd254cb8569daa777a7668edfee80681ee988f57416586bf1f7b04eefa4dc0"
 				},
 				"length": 2408,
 				"version": 2
diff --git a/client/testdata/go-tuf/consistent-snapshot-false/1/repository/timestamp.json b/client/testdata/go-tuf/consistent-snapshot-false/1/repository/timestamp.json
@@ -2,7 +2,7 @@
 	"signatures": [
 		{
 			"keyid": "aa3255b4e8e17e566d2bdbea0e5842978f9fa1d2fa9ec76ae76b146164acbfc8",
-			"sig": "bee62f48a1383aadad3ff47ef1eeed4f28158c903c5eee2cfc55b2d4d30f6b665f943e2d2b8f5aa81a67a3ad9edb3b4a4b625cda2f7b7c2806efc7e343758b02"
+			"sig": "5543ad32130a134921cf82d44575ad4b72a276297d6f8eafca116476d7e62b397e75e596f485517afb6c0a74e92f8ff96e1d24a1a341f8c68ceff06363386d07"
 		}
 	],
 	"signed": {
@@ -11,7 +11,7 @@
 		"meta": {
 			"snapshot.json": {
 				"hashes": {
-					"sha512": "f8fd7a63c597491397c7b155c39286c5be15127e0570ecefbb4cf3b6a166e0c35e5ef103e48650d44aedf93ab5d26f91465c970b1c0d0aa75aaefb4d38ec38d7"
+					"sha512": "d5ca4e4060b044075d38132891de10ba7ed9024f0a709674bd76b8c6270afabc16da2a449f5fd8740aebcca7c9828eb7fb28c917aa290a0cf1fc9c0daed879e7"
 				},
 				"length": 847,
 				"version": 2
diff --git a/client/testdata/go-tuf/consistent-snapshot-true/1/repository/2.root.json b/client/testdata/go-tuf/consistent-snapshot-true/1/repository/2.root.json
@@ -1,12 +1,12 @@
 {
 	"signatures": [
-		{
-			"keyid": "ce72db3f938914205461a415c9b7b91267a2079df991fd6283aa8461988c1add",
-			"sig": "e560f386c0c270bb44cedda965ad21526e65162b41a2c30d84fb3a80f58913432e8dc74e5e7b0635ccdf51a2b951dae3b0ba28b4aac4088641a2b2cd2933dd04"
-		},
 		{
 			"keyid": "b2403f96ae9b1089d8cbc15bbc35e9acbacd7984571f951b43aab56aedcfa84f",
 			"sig": "7ac71619b21fe3a076fbbf3e17f92177c5374f005c32f1818e7c92eee107e3c726ccf906e800878035a9caf7679610147d72cb515cd76164b08b5c8bf93c0f0e"
+		},
+		{
+			"keyid": "ce72db3f938914205461a415c9b7b91267a2079df991fd6283aa8461988c1add",
+			"sig": "e560f386c0c270bb44cedda965ad21526e65162b41a2c30d84fb3a80f58913432e8dc74e5e7b0635ccdf51a2b951dae3b0ba28b4aac4088641a2b2cd2933dd04"
 		}
 	],
 	"signed": {
diff --git a/client/testdata/go-tuf/consistent-snapshot-true/1/repository/2.snapshot.json b/client/testdata/go-tuf/consistent-snapshot-true/1/repository/2.snapshot.json
@@ -2,7 +2,7 @@
 	"signatures": [
 		{
 			"keyid": "289e5a9e71afd7909326aa4caea92f7557ee0e2283d8c31f0c3401ce67248a45",
-			"sig": "25c7977a662d4cd511eb348284a0420eb62e654f90e10338e0653bc2f7428af8033818ba2901734956112bdcf95ebfd9f86070c6ed75ae4c3a21ebf7098b1f04"
+			"sig": "a9aa74e87f6ee1719d0c2cb3a37c2ff63bee3648bff572377b9169833012ed50b6d5a4604e668e200600ed6ef7c622e998e44580d4358aa638ab767e5de90509"
 		}
 	],
 	"signed": {
@@ -11,7 +11,7 @@
 		"meta": {
 			"root.json": {
 				"hashes": {
-					"sha512": "645840e3191d531617ebab3de88e77461f1ff673cd6c857f23f87860e61ab6d80880933bb2b4f353dd89894c84b3497e3e02d791eaaab3750f4beabbba763c7a"
+					"sha512": "51cd60db1c77e80b7d4553bff37eba869eb7ff9e3eb2726f6e550f4209ca8bde123a74f04cd42b0ac3a34af92f32e313ff8d5b64488f3b46c4c4fc4b836fc003"
 				},
 				"length": 2407,
 				"version": 2
diff --git a/client/testdata/go-tuf/consistent-snapshot-true/1/repository/root.json b/client/testdata/go-tuf/consistent-snapshot-true/1/repository/root.json
@@ -1,12 +1,12 @@
 {
 	"signatures": [
-		{
-			"keyid": "ce72db3f938914205461a415c9b7b91267a2079df991fd6283aa8461988c1add",
-			"sig": "e560f386c0c270bb44cedda965ad21526e65162b41a2c30d84fb3a80f58913432e8dc74e5e7b0635ccdf51a2b951dae3b0ba28b4aac4088641a2b2cd2933dd04"
-		},
 		{
 			"keyid": "b2403f96ae9b1089d8cbc15bbc35e9acbacd7984571f951b43aab56aedcfa84f",
 			"sig": "7ac71619b21fe3a076fbbf3e17f92177c5374f005c32f1818e7c92eee107e3c726ccf906e800878035a9caf7679610147d72cb515cd76164b08b5c8bf93c0f0e"
+		},
+		{
+			"keyid": "ce72db3f938914205461a415c9b7b91267a2079df991fd6283aa8461988c1add",
+			"sig": "e560f386c0c270bb44cedda965ad21526e65162b41a2c30d84fb3a80f58913432e8dc74e5e7b0635ccdf51a2b951dae3b0ba28b4aac4088641a2b2cd2933dd04"
 		}
 	],
 	"signed": {
diff --git a/client/testdata/go-tuf/consistent-snapshot-true/1/repository/snapshot.json b/client/testdata/go-tuf/consistent-snapshot-true/1/repository/snapshot.json
@@ -2,7 +2,7 @@
 	"signatures": [
 		{
 			"keyid": "289e5a9e71afd7909326aa4caea92f7557ee0e2283d8c31f0c3401ce67248a45",
-			"sig": "25c7977a662d4cd511eb348284a0420eb62e654f90e10338e0653bc2f7428af8033818ba2901734956112bdcf95ebfd9f86070c6ed75ae4c3a21ebf7098b1f04"
+			"sig": "a9aa74e87f6ee1719d0c2cb3a37c2ff63bee3648bff572377b9169833012ed50b6d5a4604e668e200600ed6ef7c622e998e44580d4358aa638ab767e5de90509"
 		}
 	],
 	"signed": {
@@ -11,7 +11,7 @@
 		"meta": {
 			"root.json": {
 				"hashes": {
-					"sha512": "645840e3191d531617ebab3de88e77461f1ff673cd6c857f23f87860e61ab6d80880933bb2b4f353dd89894c84b3497e3e02d791eaaab3750f4beabbba763c7a"
+					"sha512": "51cd60db1c77e80b7d4553bff37eba869eb7ff9e3eb2726f6e550f4209ca8bde123a74f04cd42b0ac3a34af92f32e313ff8d5b64488f3b46c4c4fc4b836fc003"
 				},
 				"length": 2407,
 				"version": 2
diff --git a/client/testdata/go-tuf/consistent-snapshot-true/1/repository/timestamp.json b/client/testdata/go-tuf/consistent-snapshot-true/1/repository/timestamp.json
@@ -2,7 +2,7 @@
 	"signatures": [
 		{
 			"keyid": "aa3255b4e8e17e566d2bdbea0e5842978f9fa1d2fa9ec76ae76b146164acbfc8",
-			"sig": "d2674f7afdfd013c9a8bdaf716db22521ce3929cb35917893ebae679a789d30ce875a4657c39906883766b5b80efe3ff96835fcbe704e08fda0e8887c936650f"
+			"sig": "ace63f55631ff695d7ad960b4f40cf6013732f76edcb945e89798c21304df43c03c0f0e6e322dba35d488c517de5681318600813ccedb3f166f6ceb4f462930f"
 		}
 	],
 	"signed": {
@@ -11,7 +11,7 @@
 		"meta": {
 			"snapshot.json": {
 				"hashes": {
-					"sha512": "e03715cdd4fc212599d320b3392052e1c452b3426d5658c9faee72b31df65e80823f42cb8649fae4d950f73736492a501e5f4569741d102386a4cb5194512b91"
+					"sha512": "08a4fe01b9da9d07909d8ec02d2ea870b67fcc816751e56c61a8aebc587f051003f4b8a77c268f2a97b79997f7cbf836855e8638fc1df6c5df54ab7b49b2c671"
 				},
 				"length": 847,
 				"version": 2
diff --git a/internal/signer/sort.go b/internal/signer/sort.go
@@ -0,0 +1,49 @@
+package signer
+
+import (
+	"sort"
+
+	"github.com/theupdateframework/go-tuf/pkg/keys"
+)
+
+// ByIDs implements sort.Interface for []keys.Signer based on
+// the sorted public IDs() for each Signer. This facilitates
+// deterministic order of signatures, which prevents tests
+// that use fixtures from being flaky.
+type ByIDs []keys.Signer
+
+func (b ByIDs) Len() int {
+	return len(b)
+}
+
+func (b ByIDs) Swap(i, j int) {
+	b[i], b[j] = b[j], b[i]
+}
+
+func (b ByIDs) Less(i, j int) bool {
+	ids := b[i].PublicData().IDs()
+	iIDs := make([]string, len(ids))
+	copy(iIDs, ids)
+	sort.Strings(iIDs)
+
+	ids = b[j].PublicData().IDs()
+	jIDs := make([]string, len(ids))
+	copy(jIDs, ids)
+	sort.Strings(jIDs)
+
+	minLen := len(iIDs)
+	if len(jIDs) < minLen {
+		minLen = len(jIDs)
+	}
+
+	// Compare iIDs[:minLen] to jIDs[:minLen] element-wise.
+	for c := 0; c < minLen; c++ {
+		if iIDs[c] == jIDs[c] {
+			continue
+		}
+		return iIDs[c] < jIDs[c]
+	}
+
+	// iIDs[:minLen] is equal to jIDs[:minLen], so sort based on length.
+	return len(iIDs) < len(jIDs)
+}
diff --git a/internal/signer/sort_test.go b/internal/signer/sort_test.go
@@ -0,0 +1,76 @@
+package signer_test
+
+import (
+	"encoding/json"
+	"sort"
+	"testing"
+
+	"github.com/theupdateframework/go-tuf/data"
+	"github.com/theupdateframework/go-tuf/internal/signer"
+	"github.com/theupdateframework/go-tuf/pkg/keys"
+)
+
+type mockSigner struct {
+	value json.RawMessage
+}
+
+func (s *mockSigner) MarshalPrivateKey() (*data.PrivateKey, error) {
+	panic("not implemented")
+	return nil, nil
+}
+
+func (s *mockSigner) UnmarshalPrivateKey(key *data.PrivateKey) error {
+	panic("not implemented")
+	return nil
+}
+
+func (s *mockSigner) PublicData() *data.PublicKey {
+	return &data.PublicKey{
+		Type:       "mock",
+		Scheme:     "mock",
+		Algorithms: []string{"mock"},
+		Value:      s.value,
+	}
+}
+func (s *mockSigner) SignMessage(message []byte) ([]byte, error) {
+	panic("not implemented")
+	return nil, nil
+}
+
+func TestSignerSortByIDs(t *testing.T) {
+	s1 := &mockSigner{
+		value: json.RawMessage(`{"mock": 1}`),
+	}
+	s2 := &mockSigner{
+		value: json.RawMessage(`{"mock": 2}`),
+	}
+	s3 := &mockSigner{
+		value: json.RawMessage(`{"mock": 3}`),
+	}
+	s4 := &mockSigner{
+		value: json.RawMessage(`{"mock": 4}`),
+	}
+	s5 := &mockSigner{
+		value: json.RawMessage(`{"mock": 5}`),
+	}
+
+	s := []keys.Signer{
+		s1, s2, s3, s4, s5,
+	}
+
+	sort.Sort(signer.ByIDs(s))
+
+	signerIDs := []string{}
+
+	for i, signer := range s {
+		ids := signer.PublicData().IDs()
+		if len(ids) != 1 {
+			t.Errorf("Signer %v IDs %v should have length 1", i, ids)
+		}
+		signerIDs = append(signerIDs, ids[0])
+	}
+
+	if !sort.StringsAreSorted(signerIDs) {
+		t.Errorf("Signers incorrectly sorted: %+v", signerIDs)
+	}
+}
diff --git a/repo.go b/repo.go
@@ -6,11 +6,13 @@ import (
 	"fmt"
 	"io"
 	"path"
+	"sort"
 	"strings"
 	"time"
 
 	cjson "github.com/tent/canonical-json-go"
 	"github.com/theupdateframework/go-tuf/data"
+	"github.com/theupdateframework/go-tuf/internal/signer"
 	"github.com/theupdateframework/go-tuf/pkg/keys"
 	"github.com/theupdateframework/go-tuf/sign"
 	"github.com/theupdateframework/go-tuf/util"
@@ -498,7 +500,7 @@ func (r *Repo) jsonMarshal(v interface{}) ([]byte, error) {
 }
 
 func (r *Repo) setMeta(roleFilename string, meta interface{}) error {
-	keys, err := r.getSigningKeys(strings.TrimSuffix(roleFilename, ".json"))
+	keys, err := r.getSortedSigningKeys(strings.TrimSuffix(roleFilename, ".json"))
 	if err != nil {
 		return err
 	}
@@ -525,7 +527,7 @@ func (r *Repo) Sign(roleFilename string) error {
 		return err
 	}
 
-	keys, err := r.getSigningKeys(role)
+	keys, err := r.getSortedSigningKeys(role)
 	if err != nil {
 		return err
 	}
@@ -597,19 +599,22 @@ func (r *Repo) AddOrUpdateSignature(roleFilename string, signature data.Signatur
 	return r.local.SetMeta(roleFilename, b)
 }
 
-// getSigningKeys returns available signing keys.
+// getSortedSigningKeys returns available signing keys, sorted by key ID.
 //
 // Only keys contained in the keys db are returned (i.e. local keys which have
 // been revoked are omitted), except for the root role in which case all local
 // keys are returned (revoked root keys still need to sign new root metadata so
 // clients can verify the new root.json and update their keys db accordingly).
-func (r *Repo) getSigningKeys(name string) ([]keys.Signer, error) {
+func (r *Repo) getSortedSigningKeys(name string) ([]keys.Signer, error) {
 	signingKeys, err := r.local.GetSigners(name)
 	if err != nil {
 		return nil, err
 	}
 	if name == "root" {
-		return signingKeys, nil
+		sorted := make([]keys.Signer, len(signingKeys))
+		copy(sorted, signingKeys)
+		sort.Sort(signer.ByIDs(sorted))
+		return sorted, nil
 	}
 	db, err := r.db()
 	if err != nil {
@@ -630,6 +635,9 @@ func (r *Repo) getSigningKeys(name string) ([]keys.Signer, error) {
 			}
 		}
 	}
+
+	sort.Sort(signer.ByIDs(keys))
+
 	return keys, nil
 }
 
diff --git a/repo_test.go b/repo_test.go

Original file line number	Diff line number	Diff line change
`@@ -1,2 +1,3 @@`
	`1`	`+.DS_Store`
`1`	`2`	`cmd/tuf/tuf`
`2`	`3`	`cmd/tuf-client/tuf-client`
Original file line number	Diff line number	Diff line change
`@@ -1,12 +1,12 @@`
`1`	`1`	`{`
`2`	`2`	`"signatures": [`
`3`		`- {`
`4`		`- "keyid": "ce72db3f938914205461a415c9b7b91267a2079df991fd6283aa8461988c1add",`
`5`		`- "sig": "65f63745aa8bd39132f827c427ea6d87f620ec805d9f376b6a8400dc3db7eb964e5423d31ed08867916d039661a70d6bf255bca552248021a4e78b5d357c2b0f"`
`6`		`- },`
`7`	`3`	`{`
`8`	`4`	`"keyid": "b2403f96ae9b1089d8cbc15bbc35e9acbacd7984571f951b43aab56aedcfa84f",`
`9`	`5`	`"sig": "20e91b55c995989b270091b714347b15169285cb636ef05d68d49ed7b7a96ebfda13898e0d9ef928c382873b9dba90dca492dbf705d56a4b293adaaed574340f"`
	`6`	`+ },`
	`7`	`+ {`
	`8`	`+ "keyid": "ce72db3f938914205461a415c9b7b91267a2079df991fd6283aa8461988c1add",`
	`9`	`+ "sig": "65f63745aa8bd39132f827c427ea6d87f620ec805d9f376b6a8400dc3db7eb964e5423d31ed08867916d039661a70d6bf255bca552248021a4e78b5d357c2b0f"`
`10`	`10`	`}`
`11`	`11`	`],`
`12`	`12`	`"signed": {`