clean up root rotation

Signed-off-by: Evan Cordell <[email protected]>

buildscripts/testclient.py

@@ -248,9 +248,9 @@ def root_rotation_test(self, tempfile, tempdir):
         self.client.run(["list", self.repo_name], tempdir)
         with open(os.path.join(tempdir, "tuf", self.repo_name, "metadata", "root.json")) as root:
             root_json = json.load(root)
-            assert len(root_json["signed"]["keys"]) == old_root_num_keys, (
+            assert len(root_json["signed"]["keys"]) == old_root_num_keys + 1, (
                 "expected {0} base keys, but got {1}".format(
-                    old_root_num_keys, len(root_json["signed"]["keys"])))
+                    old_root_num_keys + 1, len(root_json["signed"]["keys"])))
 
             root_certs = root_json["signed"]["roles"]["root"]["keyids"]
 

client/client.go

@@ -10,8 +10,6 @@ import (
 	"os"
 	"path/filepath"
 	"regexp"
-	"strconv"
-	"strings"
 	"time"
 
 	"github.com/Sirupsen/logrus"
@@ -782,40 +780,11 @@ func (r *NotaryRepository) oldKeysForLegacyClientSupport(legacyVersions int, ini
 
 // get all the saved previous roles keys < the current root version
 func getOldRootPublicKeys(root *data.SignedRoot) data.KeyList {
-	oldRootKeys := make(data.KeyList, 0, len(root.Signed.Roles))
-	// now go through the old roles
-	for roleName := range root.Signed.Roles {
-		// must be either `root` or `x.root`
-		isVersionedRoot := false
-
-		nameTokens := strings.Split(roleName, ".")
-		if len(nameTokens) == 2 && nameTokens[0] == data.CanonicalRootRole {
-			isVersionedRoot = true
-		}
-
-		if isVersionedRoot {
-			version, err := strconv.Atoi(nameTokens[1])
-			if err != nil || version >= root.Signed.Version {
-				continue
-			}
-		} else {
-			if roleName != data.CanonicalRootRole {
-				continue
-			}
-		}
-
-		// ignore invalid roles, which shouldn't happen
-		oldRole, err := root.BuildBaseRole(roleName)
-		if err != nil {
-			continue
-		}
-
-		for _, key := range oldRole.ListKeys() {
-			oldRootKeys = append(oldRootKeys, key)
-			logrus.Debugf("found old key %s for supporting legacy clients", key.ID())
-		}
+	rootRole, err := root.BuildBaseRole(data.CanonicalRootRole)
+	if err != nil {
+		return nil
 	}
-	return oldRootKeys
+	return rootRole.ListKeys()
 }
 
 func signTargets(updates map[string][]byte, repo *tuf.Repo, initialPublish bool) error {
@@ -1114,16 +1083,12 @@ func (r *NotaryRepository) pubKeysToCerts(role string, pubKeyList data.KeyList)
 	}
 
 	for i, pubKey := range pubKeyList {
-		keyInfo, err := r.CryptoService.GetKeyInfo(pubKey.ID())
+		privKey, loadedRole, err := r.CryptoService.GetPrivateKey(pubKey.ID())
 		if err != nil {
 			return nil, err
 		}
-		if keyInfo.Role != role {
-			return nil, fmt.Errorf("attempted to load root key but given %s key instead", keyInfo.Role)
-		}
-		privKey, _, err := r.CryptoService.GetPrivateKey(pubKey.ID())
-		if err != nil {
-			return nil, err
+		if loadedRole != role {
+			return nil, fmt.Errorf("attempted to load root key but given %s key instead", loadedRole)
 		}
 		pubKey, err = rootCertKey(r.gun, privKey)
 		if err != nil {

client/client_test.go

@@ -3047,6 +3047,7 @@ func TestRotateRootKeyProvided(t *testing.T) {
 	newCanonicalKeyID, err := utils.CanonicalKeyID(newRootRole.Keys[newRootCertID])
 	require.NoError(t, err)
 	require.NotEqual(t, oldCanonicalKeyID, newCanonicalKeyID)
+	require.Equal(t, rootPrivateKey.ID(), newCanonicalKeyID)
 
 	// Set up a target to verify the repo is actually usable.
 	_, err = userRepo.GetTargetByName("current")
@@ -3145,6 +3146,10 @@ func TestRotateRootKeyLegacySupport(t *testing.T) {
 	require.NoError(t, err)
 	logRepoTrustRoot(t, "client", userRepo)
 
+	// Verify that the user's rotated root is signed with all available old keys
+	require.NoError(t, err)
+	require.Equal(t, 3, len(userRepo.tufRepo.Root.Signatures))
+
 	// Verify that clients initialized post-rotation can use the repo, and use
 	// the new certificate immediately.
 	freshUserRepo, _ := newRepoToTestRepo(t, authorRepo, true)

client/tufclient.go

@@ -101,6 +101,17 @@ func (c *TUFClient) updateRoot() error {
 		return nil
 	}
 
+	// Load current version into newBuilder
+	currentRaw, err := c.cache.GetSized(data.CanonicalRootRole, -1)
+	if err != nil {
+		logrus.Debugf("error loading %d.%s: %s", currentVersion, data.CanonicalRootRole, err)
+		return err
+	}
+	if err := c.newBuilder.LoadRootForUpdate(currentRaw, currentVersion, false); err != nil {
+		logrus.Debugf("%d.%s is invalid: %s", currentVersion, data.CanonicalRootRole, err)
+		return err
+	}
+
 	// Extract newest version number
 	signedRoot := &data.Signed{}
 	if err := json.Unmarshal(raw, signedRoot); err != nil {
@@ -112,10 +123,22 @@ func (c *TUFClient) updateRoot() error {
 	}
 	newestVersion := newestRoot.Signed.SignedCommon.Version
 
-	// Update from current to newest
-	if err := c.updateRootVersions(currentVersion, newestVersion); err != nil {
+	// Update from current + 1 (current already loaded) to newest - 1 (newest loaded below)
+	if err := c.updateRootVersions(currentVersion+1, newestVersion-1); err != nil {
 		return err
 	}
+
+	// Already downloaded newest, verify it against newest - 1
+	if err := c.newBuilder.LoadRootForUpdate(raw, newestVersion, true); err != nil {
+		logrus.Debugf("downloaded %d.%s is invalid: %s", newestVersion, data.CanonicalRootRole, err)
+		return err
+	}
+	logrus.Debugf("successfully verified downloaded %d.%s", newestVersion, data.CanonicalRootRole)
+
+	// Write newest to cache
+	if err := c.cache.Set(data.CanonicalRootRole, raw); err != nil {
+		logrus.Debugf("unable to write %s to cache: %d.%s", newestVersion, data.CanonicalRootRole, err)
+	}
 	logrus.Debugf("finished updating root files")
 	return nil
 }
@@ -127,15 +150,13 @@ func (c *TUFClient) updateRootVersions(fromVersion, toVersion int) error {
 		logrus.Debugf("updating root from version %d to version %d, currently fetching %d", fromVersion, toVersion, v)
 
 		versionedRole := fmt.Sprintf("%d.%s", v, data.CanonicalRootRole)
-		isFinal := v == toVersion
 
 		raw, err := c.remote.GetSized(versionedRole, -1)
 		if err != nil {
 			logrus.Debugf("error downloading %s: %s", versionedRole, err)
 			return err
 		}
-		// If loading the most recent version, check expiry
-		if err := c.newBuilder.LoadRootForUpdate(raw, v, isFinal); err != nil {
+		if err := c.newBuilder.LoadRootForUpdate(raw, v, false); err != nil {
 			logrus.Debugf("downloaded %s is invalid: %s", versionedRole, err)
 			return err
 		}

cmd/notary/keys.go

@@ -16,8 +16,6 @@ import (
 	"github.com/docker/notary/tuf/data"
 	"github.com/docker/notary/utils"
 
-	"github.com/docker/notary"
-	"github.com/docker/notary/tuf/data"
 	"github.com/spf13/cobra"
 	"github.com/spf13/viper"
 )

server/handlers/validation.go

@@ -43,8 +43,13 @@ func validateUpdate(cs signed.CryptoService, gun string, updates []storage.MetaU
 	}
 
 	if rootUpdate, ok := roles[data.CanonicalRootRole]; ok {
+		currentRootVersion := builder.GetLoadedVersion(data.CanonicalRootRole)
+		if rootUpdate.Version != currentRootVersion && rootUpdate.Version != currentRootVersion+1 {
+			msg := fmt.Sprintf("Root modifications must increment the version. Current %d, new %d", currentRootVersion, rootUpdate.Version)
+			return nil, validation.ErrBadRoot{Msg: msg}
+		}
 		builder = builder.BootstrapNewBuilder()
-		if err := builder.Load(data.CanonicalRootRole, rootUpdate.Data, 1, false); err != nil {
+		if err := builder.Load(data.CanonicalRootRole, rootUpdate.Data, currentRootVersion, false); err != nil {
 			return nil, validation.ErrBadRoot{Msg: err.Error()}
 		}
 

server/storage/memory.go

@@ -5,6 +5,7 @@ import (
 	"encoding/hex"
 	"fmt"
 	"sort"
+	"strconv"
 	"strings"
 	"sync"
 	"time"

server/storage/rethinkdb.go

@@ -216,7 +216,7 @@ func (rdb RethinkDB) GetCurrent(gun, role string) (created *time.Time, data []by
 func (rdb RethinkDB) GetChecksum(gun, role, checksum string) (created *time.Time, data []byte, err error) {
 	var file RDBTUFFile
 	res, err := gorethink.DB(rdb.dbName).Table(file.TableName(), gorethink.TableOpts{ReadMode: "majority"}).GetAllByIndex(
-		rdbGunRoleSHA256Idx, []string{gun, role, checksum},
+		rdbGunRoleSha256Idx, []string{gun, role, checksum},
 	).Run(rdb.sess)
 	if err != nil {
 		return nil, nil, err

server/storage/rethinkdb_models.go

@@ -18,7 +18,7 @@ var (
 		Name:       RDBTUFFile{}.TableName(),
 		PrimaryKey: "gun_role_version",
 		SecondaryIndexes: map[string][]string{
-			rdbSHA256Idx:         nil,
+			rdbSha256Idx:         nil,
 			"gun":                nil,
 			"timestamp_checksum": nil,
 			rdbGunRoleIdx:        {"gun", "role"},

server/storage/sqldb.go

@@ -4,6 +4,7 @@ import (
 	"crypto/sha256"
 	"encoding/hex"
 	"fmt"
+	"strconv"
 	"time"
 
 	"github.com/Sirupsen/logrus"

signer/keydbstore/rethink_keydbstore.go

@@ -219,7 +219,7 @@ func (rdb *RethinkDBKeyStore) GetKey(keyID string) data.PublicKey {
 func (rdb *RethinkDBKeyStore) GetKeyInfo(keyID string) (trustmanager.KeyInfo, error) {
 	dbPrivateKey, _, err := rdb.getKey(keyID)
 	if err != nil {
-		return trustmanager.KeyInfo{}, nil
+		return trustmanager.KeyInfo{}, err
 	}
 
 	return trustmanager.KeyInfo{Gun: dbPrivateKey.Gun, Role: dbPrivateKey.Role}, nil

tuf/tuf.go

@@ -165,10 +165,18 @@ func (tr *Repo) RemoveBaseKeys(role string, keyIDs ...string) error {
 		}
 	}
 
-	// Remove keys no longer in use by any roles
-	for k := range toDelete {
-		delete(tr.Root.Signed.Keys, k)
-		tr.cryptoService.RemoveKey(k)
+	// Remove keys no longer in use by any roles, except for root keys.
+	// Root private keys must be kept in tr.cryptoService to be able to sign
+	// for rotation, and root certificates must be kept in tr.Root.SignedKeys
+	// because we are not necessarily storing them elsewhere (tuf.Repo does not
+	// depend on certs.Manager, that is an upper layer), and without storing
+	// the certificates in their x509 form we are not able to do the
+	// util.CanonicalKeyID conversion.
+	if role != data.CanonicalRootRole {
+		for k := range toDelete {
+			delete(tr.Root.Signed.Keys, k)
+			tr.cryptoService.RemoveKey(k)
+		}
 	}
 
 	tr.Root.Dirty = true
@@ -908,14 +916,7 @@ func (tr *Repo) SignRoot(expires time.Time, extraSigningKeys data.KeyList) (*dat
 		return nil, err
 	}
 
-	optionalRootKeys := tr.getOptionalRootKeys(rolesToSignWith)
-
-	// If we're supporting legacy clients with additional root keys, sign with those
-	for _, extraKey := range extraSigningKeys {
-		optionalRootKeys = append(optionalRootKeys, extraKey)
-	}
-
-	signed, err = tr.sign(signed, rolesToSignWith, optionalRootKeys)
+	signed, err = tr.sign(signed, rolesToSignWith, extraSigningKeys)
 	if err != nil {
 		return nil, err
 	}
@@ -926,30 +927,6 @@ func (tr *Repo) SignRoot(expires time.Time, extraSigningKeys data.KeyList) (*dat
 	return signed, nil
 }
 
-// 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 oldRootVersionName(version int) string {
 	return fmt.Sprintf("%s.%v", data.CanonicalRootRole, version)
 }

tuf/tuf_test.go

@@ -989,8 +989,7 @@ func TestRemoveBaseKeysFromRoot(t *testing.T) {
 	}
 }
 
-// replacing keys in a root should fail, root rotations must be signed by the previous
-// root role keys and threshold
+// replacing keys in a role marks root as dirty as well as the role
 func TestReplaceBaseKeysInRoot(t *testing.T) {
 	for _, role := range data.BaseRoles {
 		ed25519 := signed.NewEd25519()
@@ -1024,15 +1023,9 @@ func TestReplaceBaseKeysInRoot(t *testing.T) {
 		}
 
 		origNumRoles := len(repo.Root.Signed.Roles)
+		// sign the root and assert the number of roles after
 		_, err = repo.SignRoot(data.DefaultExpires(data.CanonicalRootRole), nil)
-		switch role {
-		case data.CanonicalRootRole:
-			// root role can't rotate without signing with previous keys
-			require.Error(t, err, "Root shouldn't be able to able to replace all keys at once.")
-		default:
-			require.NoError(t, err)
-		}
-
+		require.NoError(t, err)
 		// number of roles should not have changed
 		require.Len(t, repo.Root.Signed.Roles, origNumRoles)
 	}