Require signing with all previous roles, instead of just the immediately previous role

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

client/client.go

@@ -514,9 +514,8 @@ func (r *NotaryRepository) ListRoles() ([]RoleWithSignatures, error) {
 		case data.CanonicalTimestampRole:
 			roleWithSig.Signatures = r.tufRepo.Timestamp.Signatures
 		default:
-			// If the role isn't a delegation, we should error -- this is only possible if we have invalid state
 			if !data.IsDelegation(role.Name) {
-				return nil, data.ErrInvalidRole{Role: role.Name, Reason: "invalid role name"}
+				continue
 			}
 			if _, ok := r.tufRepo.Targets[role.Name]; ok {
 				// We'll only find a signature if we've published any targets with this delegation

client/client_test.go

@@ -494,7 +494,7 @@ func requireRepoHasExpectedMetadata(t *testing.T, repo *NotaryRepository,
 		// each of root, targets, snapshot, timestamp
 		require.Len(t, decodedRoot.Keys, len(data.BaseRoles),
 			"wrong number of keys in root.json")
-		require.Len(t, decodedRoot.Roles, len(data.BaseRoles),
+		require.True(t, len(decodedRoot.Roles) >= len(data.BaseRoles),
 			"wrong number of roles in root.json")
 
 		for _, role := range data.BaseRoles {

tuf/data/roles.go

@@ -116,6 +116,22 @@ func (b BaseRole) ListKeyIDs() []string {
 	return listKeyIDs(b.Keys)
 }
 
+// Equals returns whether this BaseRole equals another BaseRole
+func (b BaseRole) Equals(o BaseRole) bool {
+	if b.Threshold != o.Threshold || b.Name != o.Name || len(b.Keys) != len(o.Keys) {
+		return false
+	}
+
+	for keyID, key := range b.Keys {
+		oKey, ok := o.Keys[keyID]
+		if !ok || key.ID() != oKey.ID() {
+			return false
+		}
+	}
+
+	return true
+}
+
 // DelegationRole is an internal representation of a delegation role, with its public keys included
 type DelegationRole struct {
 	BaseRole

tuf/data/roles_test.go

@@ -164,3 +164,25 @@ func TestValidRoleFunction(t *testing.T) {
 
 	require.False(t, ValidRole(path.Join("role")))
 }
+
+func TestBaseRoleEquals(t *testing.T) {
+	fakeKeyHello := NewRSAPublicKey([]byte("hello"))
+	fakeKeyThere := NewRSAPublicKey([]byte("there"))
+
+	keys := map[string]PublicKey{"hello": fakeKeyHello, "there": fakeKeyThere}
+	baseRole := BaseRole{Name: "name", Threshold: 1, Keys: keys}
+
+	require.True(t, BaseRole{}.Equals(BaseRole{}))
+	require.True(t, baseRole.Equals(BaseRole{Name: "name", Threshold: 1, Keys: keys}))
+	require.False(t, baseRole.Equals(BaseRole{}))
+	require.False(t, baseRole.Equals(BaseRole{Name: "notName", Threshold: 1, Keys: keys}))
+	require.False(t, baseRole.Equals(BaseRole{Name: "name", Threshold: 2, Keys: keys}))
+	require.False(t, baseRole.Equals(BaseRole{Name: "name", Threshold: 1,
+		Keys: map[string]PublicKey{"hello": fakeKeyThere, "there": fakeKeyHello}}))
+	require.False(t, baseRole.Equals(BaseRole{Name: "name", Threshold: 1,
+		Keys: map[string]PublicKey{"hello": fakeKeyHello, "there": fakeKeyHello}}))
+	require.False(t, baseRole.Equals(BaseRole{Name: "name", Threshold: 1,
+		Keys: map[string]PublicKey{"hello": fakeKeyHello}}))
+	require.False(t, baseRole.Equals(BaseRole{Name: "name", Threshold: 1,
+		Keys: map[string]PublicKey{"hello": fakeKeyHello, "there": fakeKeyThere, "again": fakeKeyHello}}))
+}

tuf/testutils/swizzler_test.go

@@ -599,8 +599,8 @@ func TestSwizzlerMutateRoot(t *testing.T) {
 			origSigned, newSigned := &data.SignedRoot{}, &data.SignedRoot{}
 			require.NoError(t, json.Unmarshal(metaBytes, origSigned))
 			require.NoError(t, json.Unmarshal(newMeta, newSigned))
-			require.Len(t, origSigned.Signed.Roles, 4)
-			require.Len(t, newSigned.Signed.Roles, 5)
+			require.Len(t, origSigned.Signed.Roles, 5)
+			require.Len(t, newSigned.Signed.Roles, 6)
 		}
 	}
 }

tuf/tuf.go

@@ -6,6 +6,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"path"
+	"sort"
 	"strconv"
 	"strings"
 	"time"
@@ -70,7 +71,6 @@ type Repo struct {
 	// If we know what the original was, we'll if and how to handle root
 	// rotations.
 	originalRootRole data.BaseRole
-	rootRoleDirty    bool
 }
 
 // NewRepo initializes a Repo instance with a CryptoService.
@@ -99,12 +99,8 @@ func (tr *Repo) AddBaseKeys(role string, keys ...data.PublicKey) error {
 	tr.Root.Dirty = true
 
 	// also, whichever role was added to out needs to be re-signed
-	// root has already been marked dirty.  If the root keys themselves were
-	// changed, we want to mark the root role as dirty because we might have to
-	// do a root rotation
+	// root has already been marked dirty.
 	switch role {
-	case data.CanonicalRootRole:
-		tr.rootRoleDirty = true
 	case data.CanonicalSnapshotRole:
 		if tr.Snapshot != nil {
 			tr.Snapshot.Dirty = true
@@ -157,12 +153,8 @@ func (tr *Repo) RemoveBaseKeys(role string, keyIDs ...string) error {
 	tr.Root.Signed.Roles[role].KeyIDs = keep
 
 	// also, whichever role had keys removed needs to be re-signed
-	// root has already been marked dirty.  If the root keys themselves were
-	// changed, we want to mark the root role as dirty because we might have to
-	// do a root rotation
+	// root has already been marked dirty.
 	switch role {
-	case data.CanonicalRootRole:
-		tr.rootRoleDirty = true
 	case data.CanonicalSnapshotRole:
 		if tr.Snapshot != nil {
 			tr.Snapshot.Dirty = true
@@ -499,7 +491,9 @@ func (tr *Repo) InitRoot(root, timestamp, snapshot, targets data.BaseRole, consi
 	if err != nil {
 		return err
 	}
-	return tr.SetRoot(r)
+	tr.Root = r
+	tr.originalRootRole = root
+	return nil
 }
 
 // InitTargets initializes an empty targets, and returns the new empty target
@@ -511,7 +505,7 @@ func (tr *Repo) InitTargets(role string) (*data.SignedTargets, error) {
 		}
 	}
 	targets := data.NewTargets()
-	tr.SetTargets(role, targets)
+	tr.Targets[role] = targets
 	return targets, nil
 }
 
@@ -536,7 +530,8 @@ func (tr *Repo) InitSnapshot() error {
 	if err != nil {
 		return err
 	}
-	return tr.SetSnapshot(snapshot)
+	tr.Snapshot = snapshot
+	return nil
 }
 
 // InitTimestamp initializes a timestamp based on the current snapshot
@@ -550,7 +545,8 @@ func (tr *Repo) InitTimestamp() error {
 		return err
 	}
 
-	return tr.SetTimestamp(timestamp)
+	tr.Timestamp = timestamp
+	return nil
 }
 
 // SetRoot sets the Repo.Root field to the SignedRoot object.
@@ -828,72 +824,93 @@ func (tr *Repo) UpdateTimestamp(s *data.Signed) error {
 	return nil
 }
 
+type versionedRootRole struct {
+	data.BaseRole
+	version int
+}
+
+type versionedRootRoles []versionedRootRole
+
+func (v versionedRootRoles) Len() int           { return len(v) }
+func (v versionedRootRoles) Swap(i, j int)      { v[i], v[j] = v[j], v[i] }
+func (v versionedRootRoles) Less(i, j int) bool { return v[i].version < v[j].version }
+
 // SignRoot signs the root, using all keys from the "root" role (i.e. currently trusted)
 // as well as available keys used to sign the previous version, if the public part is
 // carried in tr.Root.Keys and the private key is available (i.e. probably previously
 // trusted keys, to allow rollover).
 func (tr *Repo) SignRoot(expires time.Time) (*data.Signed, error) {
 	logrus.Debug("signing root...")
-
-	tr.Root.Signed.Expires = expires
-	tr.Root.Signed.Version++
-
-	root, err := tr.GetBaseRole(data.CanonicalRootRole)
+	currRoot, err := tr.GetBaseRole(data.CanonicalRootRole)
 	if err != nil {
 		return nil, err
 	}
 
-	rolesToSignWith := []data.BaseRole{root}
+	oldRootRoles, origRoles := tr.getOldRootRoles()
 
-	optionalKeys := tr.getOldRootKeys(root)
-	// if the root role has changed, save this version's root role as a new
-	// versioned root role.  Also exclude the previous root role's keys
-	// from the map of optional keys, because the previous root role's keys are
-	// not optional
-	if tr.rootRoleDirty {
-		tr.saveRootRole()
-		for keyID := range tr.originalRootRole.Keys {
-			delete(optionalKeys, keyID)
+	var latestSavedRole data.BaseRole
+	rolesToSignWith := make([]data.BaseRole, 0, len(oldRootRoles))
+
+	if len(oldRootRoles) > 0 {
+		sort.Sort(oldRootRoles)
+		for _, vRole := range oldRootRoles {
+			rolesToSignWith = append(rolesToSignWith, vRole.BaseRole)
+		}
+		latest := rolesToSignWith[len(rolesToSignWith)-1]
+		latestSavedRole = data.BaseRole{
+			Name:      data.CanonicalRootRole,
+			Threshold: latest.Threshold,
+			Keys:      latest.Keys,
 		}
-		rolesToSignWith = append(rolesToSignWith, tr.originalRootRole)
 	}
 
-	var optionalKeysList []data.PublicKey
-	for _, key := range optionalKeys {
-		optionalKeysList = append(optionalKeysList, key)
+	// if the root role has changed and original role had not been saved as a previous role, save it now
+	if !tr.originalRootRole.Equals(currRoot) && !tr.originalRootRole.Equals(latestSavedRole) {
+		tr.saveOldRootRole(tr.originalRootRole, tr.Root.Signed.Version)
+		rolesToSignWith = append(rolesToSignWith, tr.originalRootRole)
+		latestSavedRole = tr.originalRootRole
+	}
+
+	origVersion := tr.Root.Signed.Expires
+	tr.Root.Signed.Expires = expires
+	tr.Root.Signed.Version++
+
+	// if the current role doesn't match with the latest saved role, save it
+	if !currRoot.Equals(latestSavedRole) {
+		tr.saveOldRootRole(currRoot, tr.Root.Signed.Version)
+		rolesToSignWith = append(rolesToSignWith, currRoot)
 	}
 
 	signed, err := tr.Root.ToSigned()
 	if err != nil {
+		tr.Root.Signed.Expires = origVersion
+		tr.Root.Signed.Version--
+		tr.Root.Signed.Roles = origRoles
 		return nil, err
 	}
-	signed, err = tr.sign(signed, rolesToSignWith, optionalKeysList)
+	signed, err = tr.sign(signed, rolesToSignWith, tr.getOptionalRootKeys(rolesToSignWith))
 	if err != nil {
+		tr.Root.Signed.Expires = origVersion
+		tr.Root.Signed.Version--
+		tr.Root.Signed.Roles = origRoles
 		return nil, err
 	}
 
 	tr.Root.Signatures = signed.Signatures
+	tr.originalRootRole = currRoot
 	return signed, nil
 }
 
-// build a map containing the old root keys, excluding all current root keys
-// We get these from (1) existing root.json signatures, because older
-// repositories that have already done root rotation may not necessarily
-// have older root roles, and (2) from saved older root roles
-func (tr *Repo) getOldRootKeys(currentRootRole data.BaseRole) map[string]data.PublicKey {
-	oldKeysMap := make(map[string]data.PublicKey)
-	for _, oldSig := range tr.Root.Signatures {
-		if _, ok := currentRootRole.Keys[oldSig.KeyID]; ok {
-			continue
-		}
+// get all the saved previous roles <= the current root version
+func (tr *Repo) getOldRootRoles() (versionedRootRoles, map[string]*data.RootRole) {
+	oldRootRoles := make(versionedRootRoles, 0, len(tr.Root.Signed.Roles))
+	oldRoles := make(map[string]*data.RootRole)
 
-		if k, ok := tr.Root.Signed.Keys[oldSig.KeyID]; ok {
-			oldKeysMap[k.ID()] = k
-		}
-	}
 	// now go through the old roles
-	for roleName, rootRole := range tr.Root.Signed.Roles {
-		// ensure that the rolename matches our format
+	for roleName, r := range tr.Root.Signed.Roles {
+		oldRoles[roleName] = r
+		// ensure that the rolename matches our format and that the version is
+		// not too high
 		if data.ValidRole(roleName) {
 			continue
 		}
@@ -901,25 +918,51 @@ func (tr *Repo) getOldRootKeys(currentRootRole data.BaseRole) map[string]data.Pu
 		if len(nameTokens) != 2 || nameTokens[0] != data.CanonicalRootRole {
 			continue
 		}
-		_, err := strconv.Atoi(nameTokens[1])
+		version, err := strconv.Atoi(nameTokens[1])
+		if err != nil || version > tr.Root.Signed.Version {
+			continue
+		}
+
+		// ignore invalid roles, which shouldn't happen
+		oldRole, err := tr.Root.BuildBaseRole(roleName)
 		if err != nil {
 			continue
 		}
-		for _, keyID := range rootRole.KeyIDs {
-			if _, ok := currentRootRole.Keys[keyID]; ok {
-				continue
-			}
-			if k, ok := tr.Root.Signed.Keys[keyID]; ok {
-				oldKeysMap[k.ID()] = k
-			}
-		}
+
+		oldRootRoles = append(oldRootRoles, versionedRootRole{BaseRole: oldRole, version: version})
 	}
-	return oldKeysMap
+
+	return oldRootRoles, oldRoles
 }
 
-func (tr *Repo) saveRootRole() {
-	versionedRolename := fmt.Sprintf("%s.%v", data.CanonicalRootRole, tr.Root.Signed.Version)
-	tr.Root.Signed.Roles[versionedRolename] = tr.Root.Signed.Roles[data.CanonicalRootRole]
+// gets any extra optional root keys from the existing root.json signatures
+// (because older repositories that have already done root rotation may not
+// necessarily have older root roles)
+func (tr *Repo) getOptionalRootKeys(signingRoles []data.BaseRole) []data.PublicKey {
+	oldKeysMap := make(map[string]data.PublicKey)
+	for _, oldSig := range tr.Root.Signatures {
+		if k, ok := tr.Root.Signed.Keys[oldSig.KeyID]; ok {
+			oldKeysMap[k.ID()] = k
+		}
+	}
+	for _, role := range signingRoles {
+		for keyID := range role.Keys {
+			delete(oldKeysMap, keyID)
+		}
+	}
+
+	oldKeys := make([]data.PublicKey, 0, len(oldKeysMap))
+	for _, key := range oldKeysMap {
+		oldKeys = append(oldKeys, key)
+	}
+
+	return oldKeys
+}
+
+func (tr *Repo) saveOldRootRole(role data.BaseRole, version int) {
+	versionName := fmt.Sprintf("%s.%v", data.CanonicalRootRole, version)
+	tr.Root.Signed.Roles[versionName] = &data.RootRole{
+		KeyIDs: role.ListKeyIDs(), Threshold: role.Threshold}
 }
 
 // SignTargets signs the targets file for the given top level or delegated targets role

tuf/tuf_test.go

@@ -111,6 +111,17 @@ func TestInitRepo(t *testing.T) {
 	ed25519 := signed.NewEd25519()
 	repo := initRepo(t, ed25519)
 	writeRepo(t, "/tmp/tufrepo", repo)
+	// after signing a new repo, the first version's root is saved
+	currRoot, err := repo.GetBaseRole(data.CanonicalRootRole)
+	require.NoError(t, err)
+	// can't use getBaseRole bcause it's not a valid real role
+	savedRoot, err := repo.Root.BuildBaseRole("root.1")
+	require.Equal(t, "root.1", savedRoot.Name)
+
+	// we can't compare the roots if the names are different
+	savedRoot.Name = data.CanonicalRootRole
+	require.NoError(t, err)
+	require.True(t, currRoot.Equals(savedRoot))
 }
 
 func TestUpdateDelegations(t *testing.T) {
@@ -798,7 +809,7 @@ func TestAddBaseKeysToRoot(t *testing.T) {
 		case data.CanonicalTimestampRole:
 			require.True(t, repo.Timestamp.Dirty)
 		case data.CanonicalRootRole:
-			require.True(t, repo.rootRoleDirty)
+			require.NoError(t, err)
 			require.Len(t, repo.originalRootRole.Keys, 1)
 			require.Contains(t, repo.originalRootRole.ListKeyIDs(), origKeyIDs[0])
 		}
@@ -829,7 +840,6 @@ func TestRemoveBaseKeysFromRoot(t *testing.T) {
 		case data.CanonicalTimestampRole:
 			require.True(t, repo.Timestamp.Dirty)
 		case data.CanonicalRootRole:
-			require.True(t, repo.rootRoleDirty)
 			require.Len(t, repo.originalRootRole.Keys, 1)
 			require.Contains(t, repo.originalRootRole.ListKeyIDs(), origKeyIDs[0])
 		}
@@ -865,7 +875,6 @@ func TestReplaceBaseKeysInRoot(t *testing.T) {
 		case data.CanonicalTimestampRole:
 			require.True(t, repo.Timestamp.Dirty)
 		case data.CanonicalRootRole:
-			require.True(t, repo.rootRoleDirty)
 			require.Len(t, repo.originalRootRole.Keys, 1)
 			require.Contains(t, repo.originalRootRole.ListKeyIDs(), origKeyIDs[0])
 		}
@@ -1134,6 +1143,7 @@ func verifySignatureList(t *testing.T, signed *data.Signed, expectedKeys ...data
 	for _, key := range expectedKeys {
 		_, ok := usedKeys[key.ID()]
 		require.True(t, ok)
+		verifyRootSignatureAgainstKey(t, signed, key)
 	}
 }
 
@@ -1253,15 +1263,20 @@ func TestSignRootOldKeyCertMissing(t *testing.T) {
 	require.Error(t, err)
 }
 
-func TestSignRootGetsOldKeysFromOldRootRoles(t *testing.T) {
+// SignRoot signs with all old roles with valid keys, and also optionally any old
+// signatures we have keys for even if they aren't in an old root.  It ignores any
+// root role whose version is higher than the current version.  If signing fails,
+// it reverts back.
+func TestSignRootOldRootRolesAndOldSigs(t *testing.T) {
 	gun := "docker/test-sign-root"
 	referenceTime := time.Now()
 
 	cs := cryptoservice.NewCryptoService(trustmanager.NewKeyMemoryStore(
 		passphrase.ConstantRetriever("password")))
 
-	rootCertKeys := make([]data.PublicKey, 6)
-	for i := 0; i < 6; i++ {
+	rootCertKeys := make([]data.PublicKey, 9)
+	rootPrivKeys := make([]data.PrivateKey, cap(rootCertKeys))
+	for i := 0; i < cap(rootCertKeys); i++ {
 		rootPublicKey, err := cs.Create(data.CanonicalRootRole, gun, data.ECDSAKey)
 		require.NoError(t, err)
 		rootPrivateKey, _, err := cs.GetPrivateKey(rootPublicKey.ID())
@@ -1270,47 +1285,134 @@ func TestSignRootGetsOldKeysFromOldRootRoles(t *testing.T) {
 			referenceTime.AddDate(1, 0, 0))
 		require.NoError(t, err)
 		rootCertKeys[i] = trustmanager.CertToKey(rootCert)
+		rootPrivKeys[i] = rootPrivateKey
 	}
 
-	repo := initRepoWithRoot(t, cs, rootCertKeys[5])
+	repo := initRepoWithRoot(t, cs, rootCertKeys[6])
+	// sign with key 0, which represents the key for the a version of the root we
+	// no longer have a record of
+	signedObj, err := repo.Root.ToSigned()
+	require.NoError(t, err)
+	signedObj, err = repo.sign(signedObj, nil, []data.PublicKey{rootCertKeys[0]})
+	require.NoError(t, err)
+	// should be signed with key 0
+	verifySignatureList(t, signedObj, rootCertKeys[0])
+	repo.Root.Signatures = signedObj.Signatures
 
 	// bump root version and also add the above keys and extra roles to root
-	repo.Root.Signed.Version = 5
-	// valid root version, but don't include the key in the key map
-	repo.Root.Signed.Roles["root.0"] = &data.RootRole{KeyIDs: []string{rootCertKeys[0].ID()}, Threshold: 1}
-	// invalid root versions
-	repo.Root.Signed.Roles["1.root"] = &data.RootRole{KeyIDs: []string{rootCertKeys[1].ID()}, Threshold: 1}
-	repo.Root.Signed.Roles["root2"] = &data.RootRole{KeyIDs: []string{rootCertKeys[2].ID()}, Threshold: 1}
-	repo.Root.Signed.Roles["root.3a"] = &data.RootRole{KeyIDs: []string{rootCertKeys[3].ID()}, Threshold: 1}
-	// valid old root version
-	repo.Root.Signed.Roles["root.4"] = &data.RootRole{KeyIDs: []string{rootCertKeys[4].ID()}, Threshold: 1}
-	// add every key except key 0 (because we want to leave it out)
+	repo.Root.Signed.Version = 6
+	// add every key to the root's key list except 1
 	for i, key := range rootCertKeys {
-		if i != 0 {
+		if i != 1 {
 			repo.Root.Signed.Keys[key.ID()] = key
 		}
 	}
+	// invalid root role because key not included in the key map - valid root version name
+	repo.Root.Signed.Roles["root.1"] = &data.RootRole{KeyIDs: []string{rootCertKeys[1].ID()}, Threshold: 1}
+	// invalid root versions names, but valid roles
+	repo.Root.Signed.Roles["2.root"] = &data.RootRole{KeyIDs: []string{rootCertKeys[2].ID()}, Threshold: 1}
+	repo.Root.Signed.Roles["root3"] = &data.RootRole{KeyIDs: []string{rootCertKeys[3].ID()}, Threshold: 1}
+	repo.Root.Signed.Roles["root.4a"] = &data.RootRole{KeyIDs: []string{rootCertKeys[4].ID()}, Threshold: 1}
+	// valid old root role and version
+	repo.Root.Signed.Roles["root.5"] = &data.RootRole{KeyIDs: []string{rootCertKeys[5].ID()}, Threshold: 1}
+	// higher than current root version, so invalid name, but valid root role
+	repo.Root.Signed.Roles["root.7"] = &data.RootRole{KeyIDs: []string{rootCertKeys[7].ID()}, Threshold: 1}
 
-	// SignRoot will sign with all the old keys based on old root roles, but doesn't require them
-	signedRoot, err := repo.SignRoot(data.DefaultExpires(data.CanonicalRootRole))
+	lenRootRoles := len(repo.Root.Signed.Roles)
+
+	// rotate the current key to key 8
+	require.NoError(t, repo.ReplaceBaseKeys(data.CanonicalRootRole, rootCertKeys[8]))
+
+	requiredKeys := []data.PrivateKey{
+		rootPrivKeys[5], // we need an old valid root role - this was specified in root5
+		rootPrivKeys[6], // we need the previous valid key prior to root rotation
+		rootPrivKeys[8], // we need the new root key we've rotated to
+	}
+
+	for _, privKey := range requiredKeys {
+		// if we can't sign with a previous root, we fail
+		require.NoError(t, cs.RemoveKey(privKey.ID()))
+		_, err = repo.SignRoot(data.DefaultExpires(data.CanonicalRootRole))
+		require.Error(t, err)
+		require.IsType(t, signed.ErrInsufficientSignatures{}, err)
+		require.Contains(t, err.Error(), "signing keys not available")
+
+		// add back for next test
+		require.NoError(t, cs.AddKey(data.CanonicalRootRole, gun, privKey))
+	}
+	// we haven't saved any unsaved roles because there was an error signing,
+	// nor have we bumped the version
+	require.Equal(t, 6, repo.Root.Signed.Version)
+	require.Len(t, repo.Root.Signed.Roles, lenRootRoles)
+
+	// remove all the keys we don't need and demonstrate we can still sign
+	for _, index := range []int{1, 2, 3, 4, 7} {
+		require.NoError(t, cs.RemoveKey(rootPrivKeys[index].ID()))
+	}
+
+	// SignRoot will sign with all the old keys based on old root roles as well
+	// as any old signatures
+	signedObj, err = repo.SignRoot(data.DefaultExpires(data.CanonicalRootRole))
 	require.NoError(t, err)
-	expectedSigningKeys := rootCertKeys[4:]
-	verifySignatureList(t, signedRoot, expectedSigningKeys...)
-	for _, key := range expectedSigningKeys {
-		require.NoError(t, verifyRootSignatureAgainstKey(t, signedRoot, key))
+	expectedSigningKeys := []data.PublicKey{
+		rootCertKeys[0], // old signature key, not in any role
+		rootCertKeys[5], // root.5 key which is valid
+		rootCertKeys[6], // previous key before rotation,
+		rootCertKeys[8], //  newly rotated key
 	}
+	verifySignatureList(t, signedObj, expectedSigningKeys...)
+	// verify that we saved the previous root, since it wasn't in the list of old roots,
+	// and the new root (which overwrote an invalid root)
+	require.NotNil(t, repo.Root.Signed.Roles["root.6"])
+	require.Equal(t, data.RootRole{KeyIDs: []string{rootCertKeys[6].ID()}, Threshold: 1},
+		*repo.Root.Signed.Roles["root.6"])
+	require.NotNil(t, repo.Root.Signed.Roles["root.7"])
+	require.Equal(t, data.RootRole{KeyIDs: []string{rootCertKeys[8].ID()}, Threshold: 1},
+		*repo.Root.Signed.Roles["root.7"])
+
+	// bumped version, 2 new roles, but one overwrote the previous root.7, so one additional role
+	require.Equal(t, 7, repo.Root.Signed.Version)
+	require.Len(t, repo.Root.Signed.Roles, lenRootRoles+1)
+	lenRootRoles = len(repo.Root.Signed.Roles)
+
+	// remove the optional key
+	require.NoError(t, cs.RemoveKey(rootPrivKeys[0].ID()))
+
+	// SignRoot will still succeed even if the key that wasn't in a root isn't
+	// available
+	signedObj, err = repo.SignRoot(data.DefaultExpires(data.CanonicalRootRole))
+	require.NoError(t, err)
+	verifySignatureList(t, signedObj, expectedSigningKeys[1:]...)
+
+	// no additional roles were added
+	require.Len(t, repo.Root.Signed.Roles, lenRootRoles)
+	require.Equal(t, 8, repo.Root.Signed.Version) // bumped version
+
+	// now rotate a non-root key
+	newTargetsKey, err := cs.Create(data.CanonicalTargetsRole, gun, data.ECDSAKey)
+	require.NoError(t, err)
+	require.NoError(t, repo.ReplaceBaseKeys(data.CanonicalTargetsRole, newTargetsKey))
+
+	// we still sign with all old roles no additional roles were added
+	signedObj, err = repo.SignRoot(data.DefaultExpires(data.CanonicalRootRole))
+	require.NoError(t, err)
+	verifySignatureList(t, signedObj, expectedSigningKeys[1:]...)
+	require.Len(t, repo.Root.Signed.Roles, lenRootRoles)
+	require.Equal(t, 9, repo.Root.Signed.Version) // bumped version
+
+	// rotating a targets key again, if we are missing the previous root's keys, signing will fail
+	newTargetsKey, err = cs.Create(data.CanonicalTargetsRole, gun, data.ECDSAKey)
+	require.NoError(t, err)
+	require.NoError(t, repo.ReplaceBaseKeys(data.CanonicalTargetsRole, newTargetsKey))
+
+	require.NoError(t, cs.RemoveKey(rootPrivKeys[6].ID()))
 
-	// if the previous root role is set, and the keys have been rotated, then not being able to
-	// sign that previous root role means signing fails entirely
-	signedRoot.Signatures = []data.Signature{}
-	repo.rootRoleDirty = true
-	repo.originalRootRole = data.BaseRole{
-		Name:      "root.0",
-		Threshold: 1,
-	}
-	// SignRoot will fail because it can't sign root.0
 	_, err = repo.SignRoot(data.DefaultExpires(data.CanonicalRootRole))
 	require.Error(t, err)
 	require.IsType(t, signed.ErrInsufficientSignatures{}, err)
 	require.Contains(t, err.Error(), "signing keys not available")
+
+	// no additional roles were saved, version has not changed
+	require.Len(t, repo.Root.Signed.Roles, lenRootRoles)
+	require.Equal(t, 9, repo.Root.Signed.Version) // version has not changed
 }