Merge pull request #1104 from cyli/do-not-sign-if-no-targets-change

Do not require signing if no there are no targets changes

tuf/data/types.go

@@ -1,6 +1,7 @@
 package data
 
 import (
+	"bytes"
 	"crypto/sha256"
 	"crypto/sha512"
 	"crypto/subtle"
@@ -181,6 +182,34 @@ type FileMeta struct {
 	Custom *json.RawMessage `json:"custom,omitempty"`
 }
 
+// Equals returns true if the other FileMeta object is equivalent to this one
+func (f FileMeta) Equals(o FileMeta) bool {
+	if o.Length != f.Length || len(f.Hashes) != len(f.Hashes) {
+		return false
+	}
+	if f.Custom == nil && o.Custom != nil || f.Custom != nil && o.Custom == nil {
+		return false
+	}
+	// we don't care if these are valid hashes, just that they are equal
+	for key, val := range f.Hashes {
+		if !bytes.Equal(val, o.Hashes[key]) {
+			return false
+		}
+	}
+	if f.Custom == nil && o.Custom == nil {
+		return true
+	}
+	fBytes, err := f.Custom.MarshalJSON()
+	if err != nil {
+		return false
+	}
+	oBytes, err := o.Custom.MarshalJSON()
+	if err != nil {
+		return false
+	}
+	return bytes.Equal(fBytes, oBytes)
+}
+
 // CheckHashes verifies all the checksums specified by the "hashes" of the payload.
 func CheckHashes(payload []byte, name string, hashes Hashes) error {
 	cnt := 0

tuf/data/types_test.go

@@ -235,3 +235,66 @@ func TestCompareMultiHashes(t *testing.T) {
 	err = CompareMultiHashes(hashes1, hashes2)
 	require.Error(t, err)
 }
+
+func TestFileMetaEquals(t *testing.T) {
+	var err error
+
+	hashes1 := make(Hashes)
+	hashes2 := make(Hashes)
+	hashes1["sha384"], err = hex.DecodeString("64becc3c23843942b1040ffd4743d1368d988ddf046d17d448a6e199c02c3044b425a680112b399d4dbe9b35b7ccc989")
+	require.NoError(t, err)
+	hashes2[notary.SHA256], err = hex.DecodeString("766af0ef090a4f2307e49160fa242db6fb95f071ad81a198eeb7d770e61cd6d8")
+	require.NoError(t, err)
+
+	rawMessage1, rawMessage2 := json.RawMessage{}, json.RawMessage{}
+	require.NoError(t, rawMessage1.UnmarshalJSON([]byte("hello")))
+	require.NoError(t, rawMessage2.UnmarshalJSON([]byte("there")))
+
+	f1 := []FileMeta{
+		{
+			Length: 1,
+			Hashes: hashes1,
+		},
+		{
+			Length: 2,
+			Hashes: hashes1,
+		},
+		{
+			Length: 1,
+			Hashes: hashes2,
+		},
+		{
+			Length: 1,
+			Hashes: hashes1,
+			Custom: &rawMessage1,
+		},
+		{},
+	}
+
+	f2 := []FileMeta{
+		{
+			Length: 1,
+			Hashes: hashes1,
+			Custom: &rawMessage1,
+		},
+		{
+			Length: 1,
+			Hashes: hashes1,
+			Custom: &rawMessage2,
+		},
+		{
+			Length: 1,
+			Hashes: hashes1,
+		},
+	}
+
+	require.False(t, FileMeta{}.Equals(f1[0]))
+	require.True(t, f1[0].Equals(f1[0]))
+	for _, meta := range f1[1:] {
+		require.False(t, f1[0].Equals(meta))
+	}
+	require.True(t, f2[0].Equals(f2[0]))
+	for _, meta := range f2[1:] {
+		require.False(t, f2[0].Equals(meta))
+	}
+}

tuf/tuf.go

@@ -779,10 +779,11 @@ func isValidPath(candidatePath string, delgRole data.DelegationRole) bool {
 // the directed role. If the metadata for the role doesn't exist yet,
 // AddTargets will create one.
 func (tr *Repo) AddTargets(role data.RoleName, targets data.Files) (data.Files, error) {
-	err := tr.VerifyCanSign(role)
-	if err != nil {
-		return nil, err
+	cantSignErr := tr.VerifyCanSign(role)
+	if _, ok := cantSignErr.(data.ErrInvalidRole); ok {
+		return nil, cantSignErr
 	}
+	var needSign bool
 
 	// check existence of the role's metadata
 	_, ok := tr.Targets[role]
@@ -798,17 +799,28 @@ func (tr *Repo) AddTargets(role data.RoleName, targets data.Files) (data.Files,
 	addTargetVisitor := func(targetPath string, targetMeta data.FileMeta) func(*data.SignedTargets, data.DelegationRole) interface{} {
 		return func(tgt *data.SignedTargets, validRole data.DelegationRole) interface{} {
 			// We've already validated the role's target path in our walk, so just modify the metadata
-			tgt.Signed.Targets[targetPath] = targetMeta
-			tgt.Dirty = true
-			// Also add to our new addedTargets map to keep track of every target we've added successfully
-			addedTargets[targetPath] = targetMeta
+			if targetMeta.Equals(tgt.Signed.Targets[targetPath]) {
+				// Also add to our new addedTargets map because this target was "added" successfully
+				addedTargets[targetPath] = targetMeta
+				return StopWalk{}
+			}
+			needSign = true
+			if cantSignErr == nil {
+				tgt.Signed.Targets[targetPath] = targetMeta
+				tgt.Dirty = true
+				// Also add to our new addedTargets map to keep track of every target we've added successfully
+				addedTargets[targetPath] = targetMeta
+			}
 			return StopWalk{}
 		}
 	}
 
 	// Walk the role tree while validating the target paths, and add all of our targets
 	for path, target := range targets {
 		tr.WalkTargets(path, role, addTargetVisitor(path, target))
+		if needSign && cantSignErr != nil {
+			return nil, cantSignErr
+		}
 	}
 	if len(addedTargets) != len(targets) {
 		return nil, fmt.Errorf("Could not add all targets")
@@ -818,17 +830,20 @@ func (tr *Repo) AddTargets(role data.RoleName, targets data.Files) (data.Files,
 
 // RemoveTargets removes the given target (paths) from the given target role (delegation)
 func (tr *Repo) RemoveTargets(role data.RoleName, targets ...string) error {
-	if err := tr.VerifyCanSign(role); err != nil {
-		return err
+	cantSignErr := tr.VerifyCanSign(role)
+	if _, ok := cantSignErr.(data.ErrInvalidRole); ok {
+		return cantSignErr
 	}
-
+	var needSign bool
 	removeTargetVisitor := func(targetPath string) func(*data.SignedTargets, data.DelegationRole) interface{} {
 		return func(tgt *data.SignedTargets, validRole data.DelegationRole) interface{} {
 			// We've already validated the role path in our walk, so just modify the metadata
 			// We don't check against the target path against the valid role paths because it's
 			// possible we got into an invalid state and are trying to fix it
-			delete(tgt.Signed.Targets, targetPath)
-			tgt.Dirty = true
+			if _, needSign = tgt.Signed.Targets[targetPath]; needSign && cantSignErr == nil {
+				delete(tgt.Signed.Targets, targetPath)
+				tgt.Dirty = true
+			}
 			return StopWalk{}
 		}
 	}
@@ -838,6 +853,9 @@ func (tr *Repo) RemoveTargets(role data.RoleName, targets ...string) error {
 	if ok {
 		for _, path := range targets {
 			tr.WalkTargets("", role, removeTargetVisitor(path))
+			if needSign && cantSignErr != nil {
+				return cantSignErr
+			}
 		}
 	}
 

tuf/tuf_test.go

@@ -791,9 +791,23 @@ func TestAddTargetsRoleExistsAndMetadataDoesntExist(t *testing.T) {
 	targetsF, ok := r.Signed.Targets["f"]
 	require.True(t, ok)
 	require.Equal(t, f, targetsF)
+	require.True(t, r.Dirty)
+
+	// set it to not dirty so we can assert that if we add the exact same data, it won't be dirty
+	r.Dirty = false
+	_, err = repo.AddTargets("targets/test", data.Files{"f": f})
+	require.NoError(t, err)
+	require.False(t, r.Dirty)
+
+	// If we add the same target but with different metadata, it's dirty again
+	f2 := f
+	f2.Length = 2
+	_, err = repo.AddTargets("targets/test", data.Files{"f": f2})
+	require.NoError(t, err)
+	require.True(t, r.Dirty)
 }
 
-// Adding targets to a role that doesn't exist fails
+// Adding targets to a role that doesn't exist fails only if a target was actually added or updated
 func TestAddTargetsRoleDoesntExist(t *testing.T) {
 	hash := sha256.Sum256([]byte{})
 	f := data.FileMeta{
@@ -828,14 +842,21 @@ func TestAddTargetsNoSigningKeys(t *testing.T) {
 	require.NoError(t, err)
 	err = repo.UpdateDelegationKeys("targets/test", []data.PublicKey{testKey}, []string{}, 1)
 	require.NoError(t, err)
-	err = repo.UpdateDelegationPaths("targets/test", []string{"test"}, []string{}, false)
+	err = repo.UpdateDelegationPaths("targets/test", []string{""}, []string{}, false)
+	require.NoError(t, err)
+
+	_, err = repo.AddTargets("targets/test", data.Files{"f": f})
 	require.NoError(t, err)
 
 	// now delete the signing key (all keys)
 	repo.cryptoService = signed.NewEd25519()
 
-	// adding the targets to the role should create the metadata though
+	// adding the same exact target to the role should succeed even though the key is missing
 	_, err = repo.AddTargets("targets/test", data.Files{"f": f})
+	require.NoError(t, err)
+
+	// adding a different target to the role should fail because the keys is missing
+	_, err = repo.AddTargets("targets/test", data.Files{"t": f})
 	require.Error(t, err)
 	require.IsType(t, signed.ErrNoKeys{}, err)
 }
@@ -863,16 +884,29 @@ func TestRemoveExistingAndNonexistingTargets(t *testing.T) {
 	// still no metadata
 	_, ok = repo.Targets["targets/test"]
 	require.False(t, ok)
-}
 
-// Removing targets from a role that exists but without metadata succeeds.
-func TestRemoveTargetsNonexistentMetadata(t *testing.T) {
-	ed25519 := signed.NewEd25519()
-	repo := initRepo(t, ed25519)
+	// add a target to remove
+	hash := sha256.Sum256([]byte{})
+	_, err = repo.AddTargets("targets/test", data.Files{"test": data.FileMeta{
+		Length: 1,
+		Hashes: map[string][]byte{
+			"sha256": hash[:],
+		},
+	}})
+	require.NoError(t, err)
+	tgt, ok := repo.Targets["targets/test"]
+	require.True(t, ok)
+	require.True(t, tgt.Dirty)
+	// set this to false so we can prove that removing a non-existing target does not mark as dirty
+	tgt.Dirty = false
 
-	err := repo.RemoveTargets("targets/test", "f")
-	require.Error(t, err)
-	require.IsType(t, data.ErrInvalidRole{}, err)
+	require.NoError(t, repo.RemoveTargets("targets/test", "not_real"))
+	require.False(t, tgt.Dirty)
+	require.NotEmpty(t, tgt.Signed.Targets)
+
+	require.NoError(t, repo.RemoveTargets("targets/test", "test"))
+	require.True(t, tgt.Dirty)
+	require.Empty(t, tgt.Signed.Targets)
 }
 
 // Removing targets from a role that doesn't exist fails
@@ -885,7 +919,8 @@ func TestRemoveTargetsRoleDoesntExist(t *testing.T) {
 	require.IsType(t, data.ErrInvalidRole{}, err)
 }
 
-// Removing targets from a role that we don't have signing keys for fails
+// Removing targets from a role that we don't have signing keys for fails only if
+// a target was actually removed
 func TestRemoveTargetsNoSigningKeys(t *testing.T) {
 	hash := sha256.Sum256([]byte{})
 	f := data.FileMeta{
@@ -917,8 +952,12 @@ func TestRemoveTargetsNoSigningKeys(t *testing.T) {
 	// now delete the signing key (all keys)
 	repo.cryptoService = signed.NewEd25519()
 
-	// now remove the target - it should fail
-	err = repo.RemoveTargets("targets/test", "f")
+	// remove a nonexistent target - it should not fail
+	err = repo.RemoveTargets("targets/test", "t")
+	require.NoError(t, err)
+
+	// now remove a target that does exist - it should fail
+	err = repo.RemoveTargets("targets/test", "t", "f", "g")
 	require.Error(t, err)
 	require.IsType(t, signed.ErrNoKeys{}, err)
 }