remove signatures that are no longer valid during signing, either because the key is no longer a valid signing key for the role, or the signature is invalid.

Signed-off-by: David Lawrence <david.lawrence@docker.com> (github: endophage)

Signed-off-by: David Lawrence <david.lawrence@docker.com> (github: endophage)

certs/certs_test.go

@@ -297,10 +297,7 @@ func testValidateSuccessfulRootRotation(t *testing.T, keyAlg, rootKeyType string
 	signedTestRoot, err := testRoot.ToSigned()
 	assert.NoError(t, err)
 
-	err = signed.Sign(cs, signedTestRoot, replRootKey)
+	err = signed.Sign(cs, signedTestRoot, replRootKey, origRootKey)
-	assert.NoError(t, err)
-
-	err = signed.Sign(cs, signedTestRoot, origRootKey)
 	assert.NoError(t, err)
 
 	// This call to ValidateRoot will succeed since we are using a valid PEM

tuf/signed/sign.go

@@ -22,10 +22,13 @@ import (
 
 // Sign takes a data.Signed and a key, calculated and adds the signature
 // 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, keys ...data.PublicKey) error {
 	logrus.Debugf("sign called with %d keys", len(keys))
 	signatures := make([]data.Signature, 0, len(s.Signatures)+1)
 	signingKeyIDs := make(map[string]struct{})
+	tufIDs := make(map[string]data.PublicKey)
 	ids := make([]string, 0, len(keys))
 
 	privKeys := make(map[string]data.PrivateKey)
@@ -34,6 +37,7 @@ func Sign(service CryptoService, s *data.Signed, keys ...data.PublicKey) error {
 	for _, key := range keys {
 		canonicalID, err := utils.CanonicalKeyID(key)
 		ids = append(ids, canonicalID)
+		tufIDs[key.ID()] = key
 		if err != nil {
 			continue
 		}
@@ -78,6 +82,20 @@ func Sign(service CryptoService, s *data.Signed, keys ...data.PublicKey) error {
 			// 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

tuf/signed/sign_test.go

@@ -21,11 +21,6 @@ type FailingCryptoService struct {
 	testKey data.PrivateKey
 }
 
-func (mts *FailingCryptoService) Sign(keyIDs []string, _ []byte) ([]data.Signature, error) {
-	sigs := make([]data.Signature, 0, len(keyIDs))
-	return sigs, nil
-}
-
 func (mts *FailingCryptoService) Create(_, _ string) (data.PublicKey, error) {
 	return mts.testKey, nil
 }
@@ -36,10 +31,10 @@ func (mts *FailingCryptoService) ListKeys(role string) []string {
 
 func (mts *FailingCryptoService) ListAllKeys() map[string]string {
 	return map[string]string{
-		mts.testKey.ID(): "root",
+		mts.testKey.ID(): data.CanonicalRootRole,
-		mts.testKey.ID(): "targets",
+		mts.testKey.ID(): data.CanonicalTargetsRole,
-		mts.testKey.ID(): "snapshot",
+		mts.testKey.ID(): data.CanonicalSnapshotRole,
-		mts.testKey.ID(): "timestamp",
+		mts.testKey.ID(): data.CanonicalTimestampRole,
 	}
 }
 
@@ -69,14 +64,6 @@ type MockCryptoService struct {
 	testKey data.PrivateKey
 }
 
-func (mts *MockCryptoService) Sign(keyIDs []string, _ []byte) ([]data.Signature, error) {
-	sigs := make([]data.Signature, 0, len(keyIDs))
-	for _, keyID := range keyIDs {
-		sigs = append(sigs, data.Signature{KeyID: keyID})
-	}
-	return sigs, nil
-}
-
 func (mts *MockCryptoService) Create(_ string, _ string) (data.PublicKey, error) {
 	return mts.testKey, nil
 }
@@ -94,10 +81,10 @@ func (mts *MockCryptoService) ListKeys(role string) []string {
 
 func (mts *MockCryptoService) ListAllKeys() map[string]string {
 	return map[string]string{
-		mts.testKey.ID(): "root",
+		mts.testKey.ID(): data.CanonicalRootRole,
-		mts.testKey.ID(): "targets",
+		mts.testKey.ID(): data.CanonicalTargetsRole,
-		mts.testKey.ID(): "snapshot",
+		mts.testKey.ID(): data.CanonicalSnapshotRole,
-		mts.testKey.ID(): "timestamp",
+		mts.testKey.ID(): data.CanonicalTimestampRole,
 	}
 }
 
@@ -119,16 +106,6 @@ type StrictMockCryptoService struct {
 	MockCryptoService
 }
 
-func (mts *StrictMockCryptoService) Sign(keyIDs []string, _ []byte) ([]data.Signature, error) {
-	sigs := make([]data.Signature, 0, len(keyIDs))
-	for _, keyID := range keyIDs {
-		if keyID == mts.testKey.ID() {
-			sigs = append(sigs, data.Signature{KeyID: keyID})
-		}
-	}
-	return sigs, nil
-}
-
 func (mts *StrictMockCryptoService) GetKey(keyID string) data.PublicKey {
 	if keyID == mts.testKey.ID() {
 		return data.PublicKeyFromPrivate(mts.testKey)
@@ -142,10 +119,10 @@ func (mts *StrictMockCryptoService) ListKeys(role string) []string {
 
 func (mts *StrictMockCryptoService) ListAllKeys() map[string]string {
 	return map[string]string{
-		mts.testKey.ID(): "root",
+		mts.testKey.ID(): data.CanonicalRootRole,
-		mts.testKey.ID(): "targets",
+		mts.testKey.ID(): data.CanonicalTargetsRole,
-		mts.testKey.ID(): "snapshot",
+		mts.testKey.ID(): data.CanonicalSnapshotRole,
-		mts.testKey.ID(): "timestamp",
+		mts.testKey.ID(): data.CanonicalTimestampRole,
 	}
 }
 
@@ -156,7 +133,7 @@ func (mts *StrictMockCryptoService) ImportRootKey(r io.Reader) error {
 // Test signing and ensure the expected signature is added
 func TestBasicSign(t *testing.T) {
 	cs := NewEd25519()
-	key, err := cs.Create("root", data.ED25519Key)
+	key, err := cs.Create(data.CanonicalRootRole, data.ED25519Key)
 	require.NoError(t, err)
 	testData := data.Signed{}
 
@@ -176,7 +153,7 @@ func TestBasicSign(t *testing.T) {
 // with the same key ID
 func TestReSign(t *testing.T) {
 	cs := NewEd25519()
-	key, err := cs.Create("root", data.ED25519Key)
+	key, err := cs.Create(data.CanonicalRootRole, data.ED25519Key)
 	require.NoError(t, err)
 	testData := data.Signed{}
 
@@ -198,8 +175,9 @@ func TestMultiSign(t *testing.T) {
 	cs := NewEd25519()
 	testData := data.Signed{}
 
-	key1, err := cs.Create("root", data.ED25519Key)
+	key1, err := cs.Create(data.CanonicalRootRole, data.ED25519Key)
 	require.NoError(t, err)
+
 	Sign(cs, &testData, key1)
 
 	// reinitializing cs means it won't know about key1. We want
@@ -207,8 +185,9 @@ func TestMultiSign(t *testing.T) {
 	// that the signature for key1 is left intact and the signature
 	// for key2 is added
 	cs = NewEd25519()
-	key2, err := cs.Create("root", data.ED25519Key)
+	key2, err := cs.Create(data.CanonicalRootRole, data.ED25519Key)
 	require.NoError(t, err)
+
 	Sign(
 		cs,
 		&testData,
@@ -229,7 +208,6 @@ func TestMultiSign(t *testing.T) {
 		}
 	}
 	require.Equal(t, 2, count)
-
 }
 
 func TestSignReturnsNoSigs(t *testing.T) {
@@ -270,3 +248,59 @@ func TestSignWithX509(t *testing.T) {
 	require.Len(t, testData.Signatures, 1)
 	require.Equal(t, tufRSAx509Key.ID(), testData.Signatures[0].KeyID)
 }
+
+func TestSignRemovesValidSigByInvalidKey(t *testing.T) {
+	cs := NewEd25519()
+	testData := data.Signed{}
+
+	key1, err := cs.Create(data.CanonicalRootRole, data.ED25519Key)
+	require.NoError(t, err)
+
+	Sign(cs, &testData, key1)
+	require.Len(t, testData.Signatures, 1)
+	require.Equal(t, key1.ID(), testData.Signatures[0].KeyID)
+
+	key2, err := cs.Create(data.CanonicalRootRole, data.ED25519Key)
+	require.NoError(t, err)
+
+	// should remove key1 sig even though it's valid. It no longer appears
+	// in the list of signing keys for the role
+	Sign(
+		cs,
+		&testData,
+		key2,
+	)
+
+	require.Len(t, testData.Signatures, 1)
+	require.Equal(t, key2.ID(), testData.Signatures[0].KeyID)
+}
+
+func TestSignRemovesInvalidSig(t *testing.T) {
+	cs := NewEd25519()
+	testData := data.Signed{}
+
+	key1, err := cs.Create(data.CanonicalRootRole, data.ED25519Key)
+	require.NoError(t, err)
+
+	Sign(cs, &testData, key1)
+	require.Len(t, testData.Signatures, 1)
+	require.Equal(t, key1.ID(), testData.Signatures[0].KeyID)
+
+	// we need cs to "forget" key1 so we can't sign with it
+	cs = NewEd25519()
+	key2, err := cs.Create(data.CanonicalRootRole, data.ED25519Key)
+	require.NoError(t, err)
+
+	// modify test data to invalidate key1 sig
+	testData.Signed = []byte{0xff}
+	// should remove key1 sig because it's out of date
+	Sign(
+		cs,
+		&testData,
+		key1,
+		key2,
+	)
+
+	require.Len(t, testData.Signatures, 1)
+	require.Equal(t, key2.ID(), testData.Signatures[0].KeyID)
+}

tuf/signed/verify.go

@@ -2,6 +2,7 @@ package signed
 
 import (
 	"errors"
+	"fmt"
 	"strings"
 	"time"
 
@@ -124,16 +125,8 @@ func VerifySignatures(s *data.Signed, roleData data.BaseRole) error {
 			logrus.Debugf("continuing b/c keyid lookup was nil: %s\n", sig.KeyID)
 			continue
 		}
-		// method lookup is consistent due to Unmarshal JSON doing lower case for us.
+		if err := VerifySignature(msg, sig, key); err != nil {
-		method := sig.Method
+			logrus.Debugf("continuing b/c %s", err.Error())
-		verifier, ok := Verifiers[method]
-		if !ok {
-			logrus.Debugf("continuing b/c signing method is not supported: %s\n", sig.Method)
-			continue
-		}
-
-		if err := verifier.Verify(key, sig.Signature, msg); err != nil {
-			logrus.Debugf("continuing b/c signature was invalid\n")
 			continue
 		}
 		valid[sig.KeyID] = struct{}{}
@@ -145,3 +138,18 @@ func VerifySignatures(s *data.Signed, roleData data.BaseRole) error {
 
 	return nil
 }
+
+// VerifySignature checks a single signature and public key against a payload
+func VerifySignature(msg []byte, sig data.Signature, pk data.PublicKey) error {
+	// method lookup is consistent due to Unmarshal JSON doing lower case for us.
+	method := sig.Method
+	verifier, ok := Verifiers[method]
+	if !ok {
+		return fmt.Errorf("signing method is not supported: %s\n", sig.Method)
+	}
+
+	if err := verifier.Verify(pk, sig.Signature, msg); err != nil {
+		return fmt.Errorf("signature was invalid\n")
+	}
+	return nil
+}

tuf/tuf.go

@@ -919,12 +919,7 @@ func (tr *Repo) SignTimestamp(expires time.Time) (*data.Signed, error) {
 }
 
 func (tr Repo) sign(signedData *data.Signed, role data.BaseRole) (*data.Signed, error) {
-	ks := role.ListKeys()
+	if err := signed.Sign(tr.cryptoService, signedData, role.ListKeys()...); err != nil {
-	if len(ks) < 1 {
-		return nil, signed.ErrNoKeys{}
-	}
-	err := signed.Sign(tr.cryptoService, signedData, ks...)
-	if err != nil {
 		return nil, err
 	}
 	return signedData, nil