Revert "Modify signed.Sign to replace, not add, signatures."

This reverts commit aca1cf6d43ce9aa423a9de21ac5bcdca35cbe811, and modifies a test to expect 2 signatures.

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

tuf/signed/sign.go

@@ -20,14 +20,19 @@ import (
 	"github.com/docker/notary/tuf/utils"
 )
 
-// Sign takes a data.Signed and replaces data.Signature with at least
+// Sign takes a data.Signed and keys, calculates and adds at least
 // minPrimarySignature signatures using primaryKeys and any possible
-// signatures using optionalKeys
+// signatures using optionalKeys to the data.Signed
+// N.B. All public keys for a role should be passed so that this function
+//      can correctly clean up signatures that are no longer valid.
 func Sign(service CryptoService, s *data.Signed, primaryKeys []data.PublicKey,
 	minSignatures int, optionalKeys []data.PublicKey) error {
 
 	logrus.Debugf("sign called with %d/%d required + %d optional keys",
 		minSignatures, len(primaryKeys), len(optionalKeys))
+	signatures := make([]data.Signature, 0, len(s.Signatures)+1)
+	signingKeyIDs := make(map[string]struct{})
+	tufIDs := make(map[string]data.PublicKey)
 
 	privKeys := make(map[string]data.PrivateKey)
 
@@ -35,6 +40,7 @@ func Sign(service CryptoService, s *data.Signed, primaryKeys []data.PublicKey,
 	missingKeyIDs := []string{}
 	for _, key := range primaryKeys {
 		canonicalID, err := utils.CanonicalKeyID(key)
+		tufIDs[key.ID()] = key
 		if err != nil {
 			return err
 		}
@@ -59,6 +65,7 @@ func Sign(service CryptoService, s *data.Signed, primaryKeys []data.PublicKey,
 		if _, ok := privKeys[key.ID()]; ok {
 			continue
 		}
+		tufIDs[key.ID()] = key
 		canonicalID, err := utils.CanonicalKeyID(key)
 		if err != nil {
 			return err
@@ -71,13 +78,13 @@ func Sign(service CryptoService, s *data.Signed, primaryKeys []data.PublicKey,
 	}
 
 	// Do signing and generate list of signatures
-	signatures := make([]data.Signature, 0, len(privKeys))
 	for keyID, pk := range privKeys {
 		sig, err := pk.Sign(rand.Reader, *s.Signed, nil)
 		if err != nil {
 			logrus.Debugf("Failed to sign with key: %s. Reason: %v", keyID, err)
 			return err
 		}
+		signingKeyIDs[keyID] = struct{}{}
 		signatures = append(signatures, data.Signature{
 			KeyID:     keyID,
 			Method:    pk.SignatureAlgorithm(),
@@ -85,6 +92,27 @@ func Sign(service CryptoService, s *data.Signed, primaryKeys []data.PublicKey,
 		})
 	}
 
+	for _, sig := range s.Signatures {
+		if _, ok := signingKeyIDs[sig.KeyID]; ok {
+			// key is in the set of key IDs for which a signature has been created
+			continue
+		}
+		var (
+			k  data.PublicKey
+			ok bool
+		)
+		if k, ok = tufIDs[sig.KeyID]; !ok {
+			// key is no longer a valid signing key
+			continue
+		}
+		if err := VerifySignature(*s.Signed, sig, k); err != nil {
+			// signature is no longer valid
+			continue
+		}
+		// keep any signatures that still represent valid keys and are
+		// themselves valid
+		signatures = append(signatures, sig)
+	}
 	s.Signatures = signatures
 	return nil
 }

tuf/signed/sign_test.go

@@ -154,7 +154,7 @@ func TestReSign(t *testing.T) {
 
 }
 
-// Resigning drops all obsolete signatures
+// Should not remove signatures for valid keys that were not resigned with
 func TestMultiSign(t *testing.T) {
 	cs := NewEd25519()
 	testData := data.Signed{
@@ -166,7 +166,10 @@ func TestMultiSign(t *testing.T) {
 
 	require.NoError(t, Sign(cs, &testData, []data.PublicKey{key1}, 1, nil))
 
-	// reinitializing cs means it won't know about key1.
+	// reinitializing cs means it won't know about key1. We want
+	// to attempt to sign passing both key1 and key2, while expecting
+	// that the signature for key1 is left intact and the signature
+	// for key2 is added
 	cs = NewEd25519()
 	key2, err := cs.Create(data.CanonicalRootRole, "", data.ED25519Key)
 	require.NoError(t, err)
@@ -182,6 +185,7 @@ func TestMultiSign(t *testing.T) {
 
 	require.Len(t, testData.Signatures, 2)
 	require.Equal(t, key2.ID(), testData.Signatures[0].KeyID)
+	require.Equal(t, key1.ID(), testData.Signatures[1].KeyID)
 }
 
 func TestSignReturnsNoSigs(t *testing.T) {

tuf/testutils/swizzler.go

@@ -64,6 +64,10 @@ func getPubKeys(cs signed.CryptoService, s *data.Signed, role string) ([]data.Pu
 // signs the new metadata, replacing whatever signature was there
 func serializeMetadata(cs signed.CryptoService, s *data.Signed, role string,
 	pubKeys ...data.PublicKey) ([]byte, error) {
+
+	// delete the existing signatures
+	s.Signatures = []data.Signature{}
+
 	if len(pubKeys) < 1 {
 		return nil, ErrNoKeyForRole{role}
 	}