Add support for thresholds, which means signed.VerifyRoot needs to be have just like

signed.VerifySignatures. So remove signed.VerifyRoot and just use signed.VerifySignatures
instead.  Also, to fix some tests, add an additional check for version when validating
metadata since versions can't be negative.

Signed-off-by: Ying Li <ying.li@docker.com>

client/client_update_test.go

@@ -800,7 +800,7 @@ var waysToMessUpServer = []swizzleExpectations{
 		swizzle: (*testutils.MetadataSwizzler).ExpireMetadata},
 
 	{desc: "lower metadata version", expectErrs: []interface{}{
-		&trustpinning.ErrValidationFail{}, signed.ErrLowVersion{}},
+		&trustpinning.ErrValidationFail{}, signed.ErrLowVersion{}, data.ErrInvalidMetadata{}},
 		swizzle: func(s *testutils.MetadataSwizzler, role string) error {
 			return s.OffsetMetadataVersion(role, -3)
 		}},
@@ -851,13 +851,6 @@ func TestUpdateRootRemoteCorruptedNoLocalCache(t *testing.T) {
 	}
 
 	for _, testData := range waysToMessUpServerRoot() {
-		if testData.desc == "insufficient signatures" {
-			// Currently if we download the root during the bootstrap phase,
-			// we don't check for enough signatures to meet the threshold.  We
-			// are also not entirely sure if we want to support threshold.
-			continue
-		}
-
 		testUpdateRemoteCorruptValidChecksum(t, updateOpts{
 			forWrite: false,
 			role:     data.CanonicalRootRole,
@@ -1209,12 +1202,6 @@ func TestUpdateLocalAndRemoteRootCorrupt(t *testing.T) {
 				// against the previous if we can
 				continue
 			}
-			if serverExpt.desc == "insufficient signatures" {
-				// Currently if we download the root during the bootstrap phase,
-				// we don't check for enough signatures to meet the threshold.
-				// We are also not sure if we want to support thresholds.
-				continue
-			}
 			testUpdateLocalAndRemoteRootCorrupt(t, true, localExpt, serverExpt)
 			testUpdateLocalAndRemoteRootCorrupt(t, false, localExpt, serverExpt)
 		}

server/handlers/validation.go

@@ -417,7 +417,7 @@ func validateRoot(gun string, oldRoot, newRoot []byte) (
 		}
 	}
 
-	if err := signed.VerifyRoot(parsedNewSigned, newRootRole.Threshold, newRootRole.Keys); err != nil {
+	if err := signed.VerifySignatures(parsedNewSigned, newRootRole); err != nil {
 		return nil, validation.ErrBadRoot{Msg: err.Error()}
 	}
 
@@ -440,7 +440,7 @@ func checkAgainstOldRoot(oldRoot []byte, newRootRole data.BaseRole, newSigned *d
 	}
 
 	// Always verify the new root against the old root
-	if err := signed.VerifyRoot(newSigned, oldRootRole.Threshold, oldRootRole.Keys); err != nil {
+	if err := signed.VerifySignatures(newSigned, oldRootRole); err != nil {
 		return validation.ErrBadRoot{Msg: fmt.Sprintf(
 			"rotation detected and new root was not signed with at least %d old keys",
 			oldRootRole.Threshold)}

trustpinning/certs.go

@@ -93,6 +93,11 @@ func ValidateRoot(certStore trustmanager.X509Store, root *data.Signed, gun strin
 		return err
 	}
 
+	rootRole, err := signedRoot.BuildBaseRole(data.CanonicalRootRole)
+	if err != nil {
+		return err
+	}
+
 	// Retrieve all the leaf and intermediate certificates in root for which the CN matches the GUN
 	allLeafCerts, allIntCerts := parseAllCerts(signedRoot)
 	certsFromRoot, err := validRootLeafCerts(allLeafCerts, gun)
@@ -117,7 +122,8 @@ func ValidateRoot(certStore trustmanager.X509Store, root *data.Signed, gun strin
 	if len(trustedCerts) != 0 {
 		logrus.Debugf("found %d valid root certificates for %s: %s", len(trustedCerts), gun,
 			prettyFormatCertIDs(trustedCerts))
-		err = signed.VerifyRoot(root, 0, trustmanager.CertsToKeys(trustedCerts, allIntCerts))
+		err = signed.VerifySignatures(
+			root, data.BaseRole{Keys: trustmanager.CertsToKeys(trustedCerts, allIntCerts), Threshold: 1})
 		if err != nil {
 			logrus.Debugf("failed to verify TUF data for: %s, %v", gun, err)
 			return &ErrValidationFail{Reason: "failed to validate data with current trusted certificates"}
@@ -149,7 +155,8 @@ func ValidateRoot(certStore trustmanager.X509Store, root *data.Signed, gun strin
 	// Validate the integrity of the new root (does it have valid signatures)
 	// Note that certsFromRoot is guaranteed to be unchanged only if we had prior cert data for this GUN or enabled TOFUS
 	// If we attempted to pin a certain certificate or CA, certsFromRoot could have been pruned accordingly
-	err = signed.VerifyRoot(root, 0, trustmanager.CertsToKeys(certsFromRoot, allIntCerts))
+	err = signed.VerifySignatures(root, data.BaseRole{
+		Keys: trustmanager.CertsToKeys(certsFromRoot, allIntCerts), Threshold: rootRole.Threshold})
 	if err != nil {
 		logrus.Debugf("failed to verify TUF data for: %s, %v", gun, err)
 		return &ErrValidationFail{Reason: "failed to validate integrity of roots"}

tuf/data/root.go

@@ -31,6 +31,11 @@ func isValidRootStructure(r Root) error {
 			role: CanonicalRootRole, msg: fmt.Sprintf("expected type %s, not %s", expectedType, r.Type)}
 	}
 
+	if r.Version < 0 {
+		return ErrInvalidMetadata{
+			role: CanonicalRootRole, msg: "version cannot be negative"}
+	}
+
 	// all the base roles MUST appear in the root.json - other roles are allowed,
 	// but other than the mirror role (not currently supported) are out of spec
 	for _, roleName := range BaseRoles {

tuf/data/root_test.go

@@ -218,3 +218,11 @@ func TestRootFromSignedValidatesRoleType(t *testing.T) {
 	require.NoError(t, err)
 	require.Equal(t, "Root", sRoot.Signed.Type)
 }
+
+// The version cannot be negative
+func TestRootFromSignedValidatesVersion(t *testing.T) {
+	root := validRootTemplate()
+	root.Signed.Version = -1
+	_, err := rootToSignedAndBack(t, root)
+	require.IsType(t, ErrInvalidMetadata{}, err)
+}

tuf/data/snapshot.go

@@ -32,6 +32,11 @@ func isValidSnapshotStructure(s Snapshot) error {
 			role: CanonicalSnapshotRole, msg: fmt.Sprintf("expected type %s, not %s", expectedType, s.Type)}
 	}
 
+	if s.Version < 0 {
+		return ErrInvalidMetadata{
+			role: CanonicalSnapshotRole, msg: "version cannot be negative"}
+	}
+
 	for _, role := range []string{CanonicalRootRole, CanonicalTargetsRole} {
 		// Meta is a map of FileMeta, so if the role isn't in the map it returns
 		// an empty FileMeta, which has an empty map, and you can check on keys

tuf/data/snapshot_test.go

@@ -194,6 +194,14 @@ func TestSnapshotFromSignedValidatesRoleType(t *testing.T) {
 	require.Equal(t, TUFTypes[CanonicalSnapshotRole], sSnapshot.Signed.Type)
 }
 
+// The version cannot be negative
+func TestSnapshotFromSignedValidatesVersion(t *testing.T) {
+	sn := validSnapshotTemplate()
+	sn.Signed.Version = -1
+	_, err := snapshotToSignedAndBack(t, sn)
+	require.IsType(t, ErrInvalidMetadata{}, err)
+}
+
 // GetMeta returns the checksum, or an error if it is missing.
 func TestSnapshotGetMeta(t *testing.T) {
 	ts := validSnapshotTemplate()

tuf/data/targets.go

@@ -38,6 +38,10 @@ func isValidTargetsStructure(t Targets, roleName string) error {
 			role: roleName, msg: fmt.Sprintf("expected type %s, not %s", expectedType, t.Type)}
 	}
 
+	if t.Version < 0 {
+		return ErrInvalidMetadata{role: roleName, msg: "version cannot be negative"}
+	}
+
 	for _, roleObj := range t.Delegations.Roles {
 		if !IsDelegation(roleObj.Name) || path.Dir(roleObj.Name) != roleName {
 			return ErrInvalidMetadata{

tuf/data/targets_test.go

@@ -227,3 +227,13 @@ func TestTargetsFromSignedValidatesRoleName(t *testing.T) {
 		require.IsType(t, ErrInvalidRole{}, err)
 	}
 }
+
+// The version cannot be negative
+func TestTargetsFromSignedValidatesVersion(t *testing.T) {
+	tg := validTargetsTemplate()
+	tg.Signed.Version = -1
+	s, err := tg.ToSigned()
+	require.NoError(t, err)
+	_, err = TargetsFromSigned(s, "targets/a")
+	require.IsType(t, ErrInvalidMetadata{}, err)
+}

tuf/data/timestamp.go

@@ -31,6 +31,11 @@ func isValidTimestampStructure(t Timestamp) error {
 			role: CanonicalTimestampRole, msg: fmt.Sprintf("expected type %s, not %s", expectedType, t.Type)}
 	}
 
+	if t.Version < 0 {
+		return ErrInvalidMetadata{
+			role: CanonicalTimestampRole, msg: "version cannot be negative"}
+	}
+
 	// Meta is a map of FileMeta, so if the role isn't in the map it returns
 	// an empty FileMeta, which has an empty map, and you can check on keys
 	// from an empty map.

tuf/data/timestamp_test.go

@@ -194,6 +194,14 @@ func TestTimestampFromSignedValidatesRoleType(t *testing.T) {
 	require.Equal(t, tsType, sTimestamp.Signed.Type)
 }
 
+// The version cannot be negative
+func TestTimestampFromSignedValidatesVersion(t *testing.T) {
+	ts := validTimestampTemplate()
+	ts.Signed.Version = -1
+	_, err := timestampToSignedAndBack(t, ts)
+	require.IsType(t, ErrInvalidMetadata{}, err)
+}
+
 // GetSnapshot returns the snapshot checksum, or an error if it is missing.
 func TestTimestampGetSnapshot(t *testing.T) {
 	ts := validTimestampTemplate()

tuf/signed/verify.go

@@ -21,45 +21,6 @@ var (
 	ErrWrongType    = errors.New("tuf: meta file has wrong type")
 )
 
-// VerifyRoot checks if a given root file is valid against a known set of keys.
-// Threshold is always assumed to be 1
-func VerifyRoot(s *data.Signed, minVersion int, keys map[string]data.PublicKey) error {
-	if len(s.Signatures) == 0 {
-		return ErrNoSignatures
-	}
-
-	var decoded map[string]interface{}
-	if err := json.Unmarshal(*s.Signed, &decoded); err != nil {
-		return err
-	}
-	msg, err := json.MarshalCanonical(decoded)
-	if err != nil {
-		return err
-	}
-	for _, sig := range s.Signatures {
-		// method lookup is consistent due to Unmarshal JSON doing lower case for us.
-		method := sig.Method
-		verifier, ok := Verifiers[method]
-		if !ok {
-			logrus.Debugf("continuing b/c signing method is not supported for verify root: %s\n", sig.Method)
-			continue
-		}
-		key, ok := keys[sig.KeyID]
-		if !ok {
-			logrus.Debugf("continuing b/c signing key isn't present in keys: %s\n", sig.KeyID)
-			continue
-		}
-
-		if err := verifier.Verify(key, sig.Signature, msg); err != nil {
-			logrus.Debugf("continuing b/c signature was invalid\n")
-			continue
-		}
-		// threshold of 1 so return on first success
-		return verifyMeta(s, data.CanonicalRootRole, minVersion)
-	}
-	return ErrRoleThreshold{}
-}
-
 // Verify checks the signatures and metadata (expiry, version) for the signed role
 // data
 func Verify(s *data.Signed, role data.BaseRole, minVersion int) error {