Merge pull request #378 from cyli/publish-delegations

Publishing delegation changes, and targets to delegations

client/client.go

@@ -245,7 +245,7 @@ func (r *NotaryRepository) Initialize(rootKeyID string, serverManagedRoles ...st
 		logrus.Debug("Error on InitRoot: ", err.Error())
 		return err
 	}
-	err = r.tufRepo.InitTargets(data.CanonicalTargetsRole)
+	_, err = r.tufRepo.InitTargets(data.CanonicalTargetsRole)
 	if err != nil {
 		logrus.Debug("Error on InitTargets: ", err.Error())
 		return err
@@ -522,7 +522,7 @@ func (r *NotaryRepository) GetChangelist() (changelist.Changelist, error) {
 // Publish pushes the local changes in signed material to the remote notary-server
 // Conceptually it performs an operation similar to a `git rebase`
 func (r *NotaryRepository) Publish() error {
-	var updateRoot bool
+	var initialPublish bool
 	// attempt to initialize the repo from the remote store
 	c, err := r.bootstrapClient()
 	if err != nil {
@@ -538,10 +538,11 @@ func (r *NotaryRepository) Publish() error {
 				return err
 			}
 			// We had local data but the server doesn't know about the repo yet,
-			// ensure we will push the initial root file.  The root may not
+			// ensure we will push the initial root and targets file.  Either or
-			// be marked as Dirty, since there may not be any changes that
+			// both of the root and targets may not be marked as Dirty, since
-			// update it, so use a different boolean.
+			// there may not be any changes that update them, so use a
-			updateRoot = true
+			// different boolean.
+			initialPublish = true
 		} else {
 			// The remote store returned an error other than 404. We're
 			// unable to determine if the repo has been initialized or not.
@@ -576,7 +577,7 @@ func (r *NotaryRepository) Publish() error {
 	updatedFiles := make(map[string][]byte)
 
 	// check if our root file is nearing expiry. Resign if it is.
-	if nearExpiry(r.tufRepo.Root) || r.tufRepo.Root.Dirty || updateRoot {
+	if nearExpiry(r.tufRepo.Root) || r.tufRepo.Root.Dirty || initialPublish {
 		rootJSON, err := serializeCanonicalRole(r.tufRepo, data.CanonicalRootRole)
 		if err != nil {
 			return err
@@ -584,12 +585,16 @@ func (r *NotaryRepository) Publish() error {
 		updatedFiles[data.CanonicalRootRole] = rootJSON
 	}
 
-	// we will always re-sign targets
+	// iterate through all the targets files - if they are dirty, sign and update
-	targetsJSON, err := serializeCanonicalRole(r.tufRepo, data.CanonicalTargetsRole)
+	for roleName, roleObj := range r.tufRepo.Targets {
-	if err != nil {
+		if roleObj.Dirty || (roleName == data.CanonicalTargetsRole && initialPublish) {
-		return err
+			targetsJSON, err := serializeCanonicalRole(r.tufRepo, roleName)
+			if err != nil {
+				return err
+			}
+			updatedFiles[roleName] = targetsJSON
+		}
 	}
-	updatedFiles[data.CanonicalTargetsRole] = targetsJSON
 
 	// if we initialized the repo while designating the server as the snapshot
 	// signer, then there won't be a snapshots file.  However, we might now

client/client_test.go

@@ -6,6 +6,7 @@ import (
 	regJson "encoding/json"
 	"fmt"
 	"io/ioutil"
+	"math"
 	"net/http"
 	"net/http/httptest"
 	"os"
@@ -1139,28 +1140,74 @@ func testGetChangelist(t *testing.T, rootType string) {
 	assert.Equal(t, "latest", latestChange.Path())
 }
 
-// Create a repo, instantiate a notary server, and publish the repo to the
+// Create a repo, instantiate a notary server, and publish the bare repo to the
-// server, signing all the non-timestamp metadata.
+// server, signing all the non-timestamp metadata.  Root, targets, and snapshots
+// (if locally signing) should be sent.
+func TestPublishBareRepo(t *testing.T) {
+	testPublishNoData(t, data.ECDSAKey, true)
+	testPublishNoData(t, data.ECDSAKey, false)
+	if !testing.Short() {
+		testPublishNoData(t, data.RSAKey, true)
+		testPublishNoData(t, data.RSAKey, false)
+	}
+}
+
+func testPublishNoData(t *testing.T, rootType string, serverManagesSnapshot bool) {
+	var tempDirs [2]string
+	for i := 0; i < 2; i++ {
+		tempBaseDir, err := ioutil.TempDir("", "notary-test-")
+		assert.NoError(t, err, "failed to create a temporary directory: %s", err)
+		defer os.RemoveAll(tempBaseDir)
+		tempDirs[i] = tempBaseDir
+	}
+
+	gun := "docker.com/notary"
+	ts := fullTestServer(t)
+	defer ts.Close()
+
+	repo1, _ := initializeRepo(t, rootType, tempDirs[0], gun, ts.URL,
+		serverManagesSnapshot)
+	assert.NoError(t, repo1.Publish())
+
+	// use another repo to check metadata
+	repo2, err := NewNotaryRepository(tempDirs[1], gun, ts.URL,
+		http.DefaultTransport, passphraseRetriever)
+	assert.NoError(t, err, "error creating repository: %s", err)
+
+	targets, err := repo2.ListTargets()
+	assert.NoError(t, err)
+	assert.Empty(t, targets)
+
+	for role := range data.ValidRoles {
+		// we don't cache timstamp metadata
+		if role != data.CanonicalTimestampRole {
+			assertRepoHasExpectedMetadata(t, repo2, role, true)
+		}
+	}
+}
+
+// Create a repo, instantiate a notary server, and publish the repo with
+// some targets to the server, signing all the non-timestamp metadata.
 // We test this with both an RSA and ECDSA root key
 func TestPublishClientHasSnapshotKey(t *testing.T) {
-	testPublish(t, data.ECDSAKey, false)
+	testPublishWithData(t, data.ECDSAKey, false)
 	if !testing.Short() {
-		testPublish(t, data.RSAKey, false)
+		testPublishWithData(t, data.RSAKey, false)
 	}
 }
 
 // Create a repo, instantiate a notary server (designating the server as the
-// snapshot signer) , and publish the repo to the server, signing the root and
+// snapshot signer) , and publish the repo with some targets to the server,
-// targets metadata only.  The server should sign just fine.
+// signing the root and targets metadata only.  The server should sign just fine.
 // We test this with both an RSA and ECDSA root key
 func TestPublishAfterInitServerHasSnapshotKey(t *testing.T) {
-	testPublish(t, data.ECDSAKey, true)
+	testPublishWithData(t, data.ECDSAKey, true)
 	if !testing.Short() {
-		testPublish(t, data.RSAKey, true)
+		testPublishWithData(t, data.RSAKey, true)
 	}
 }
 
-func testPublish(t *testing.T, rootType string, serverManagesSnapshot bool) {
+func testPublishWithData(t *testing.T, rootType string, serverManagesSnapshot bool) {
 	// Temporary directory where test files will be created
 	tempBaseDir, err := ioutil.TempDir("/tmp", "notary-test-")
 	defer os.RemoveAll(tempBaseDir)
@@ -1175,11 +1222,31 @@ func testPublish(t *testing.T, rootType string, serverManagesSnapshot bool) {
 	assertPublishSucceeds(t, repo)
 }
 
+// asserts that publish succeeds by adding to the default only and publishing;
+// the targets should appear in targets
 func assertPublishSucceeds(t *testing.T, repo1 *NotaryRepository) {
-	// Create 2 targets
+	assertPublishToRolesSucceeds(t, repo1, nil, []string{data.CanonicalTargetsRole})
-	latestTarget := addTarget(t, repo1, "latest", "../fixtures/intermediate-ca.crt")
+}
-	currentTarget := addTarget(t, repo1, "current", "../fixtures/intermediate-ca.crt")
+
-	assert.Len(t, getChanges(t, repo1), 2, "wrong number of changelist files found")
+// asserts that adding to the given roles results in the targets actually
+func assertPublishToRolesSucceeds(t *testing.T, repo1 *NotaryRepository,
+	publishToRoles []string, expectedPublishedRoles []string) {
+
+	// were there unpublished changes before?
+	changesOffset := len(getChanges(t, repo1))
+
+	// Create 2 targets - (actually 3, but we delete 1)
+	addTarget(t, repo1, "toDelete", "../fixtures/intermediate-ca.crt", publishToRoles...)
+	latestTarget := addTarget(
+		t, repo1, "latest", "../fixtures/intermediate-ca.crt", publishToRoles...)
+	currentTarget := addTarget(
+		t, repo1, "current", "../fixtures/intermediate-ca.crt", publishToRoles...)
+	repo1.RemoveTarget("toDelete", publishToRoles...)
+
+	// if no roles are provided, the default role is target
+	numRoles := int(math.Max(1, float64(len(publishToRoles))))
+	assert.Len(t, getChanges(t, repo1), changesOffset+4*numRoles,
+		"wrong number of changelist files found")
 
 	// Now test Publish
 	err := repo1.Publish()
@@ -1187,7 +1254,7 @@ func assertPublishSucceeds(t *testing.T, repo1 *NotaryRepository) {
 	assert.Len(t, getChanges(t, repo1), 0, "wrong number of changelist files found")
 
 	// Create a new repo and pull from the server
-	tempBaseDir, err := ioutil.TempDir("/tmp", "notary-test-")
+	tempBaseDir, err := ioutil.TempDir("", "notary-test-")
 	defer os.RemoveAll(tempBaseDir)
 
 	assert.NoError(t, err, "failed to create a temporary directory: %s", err)
@@ -1196,26 +1263,31 @@ func assertPublishSucceeds(t *testing.T, repo1 *NotaryRepository) {
 		http.DefaultTransport, passphraseRetriever)
 	assert.NoError(t, err, "error creating repository: %s", err)
 
-	// Should be two targets
+	// Should be two targets per role
-	for _, repo := range []*NotaryRepository{repo1, repo2} {
+	for _, role := range expectedPublishedRoles {
-		targets, err := repo.ListTargets(data.CanonicalTargetsRole)
+		for _, repo := range []*NotaryRepository{repo1, repo2} {
-		assert.NoError(t, err)
+			targets, err := repo.ListTargets(role)
+			assert.NoError(t, err)
 
-		assert.Len(t, targets, 2, "unexpected number of targets returned by ListTargets")
+			assert.Len(t, targets, 2,
+				"unexpected number of targets returned by ListTargets(%s)", role)
 
-		sort.Stable(targetSorter(targets))
+			sort.Stable(targetSorter(targets))
 
-		assert.Equal(t, currentTarget, targets[0], "current target does not match")
+			assert.Equal(t, currentTarget, targets[0], "current target does not match")
-		assert.Equal(t, latestTarget, targets[1], "latest target does not match")
+			assert.Equal(t, latestTarget, targets[1], "latest target does not match")
 
-		// Also test GetTargetByName
+			// Also test GetTargetByName
-		newLatestTarget, err := repo.GetTargetByName("latest")
+			if role == data.CanonicalTargetsRole {
-		assert.NoError(t, err)
+				newLatestTarget, err := repo.GetTargetByName("latest")
-		assert.Equal(t, latestTarget, newLatestTarget, "latest target does not match")
+				assert.NoError(t, err)
+				assert.Equal(t, latestTarget, newLatestTarget, "latest target does not match")
 
-		newCurrentTarget, err := repo.GetTargetByName("current")
+				newCurrentTarget, err := repo.GetTargetByName("current")
-		assert.NoError(t, err)
+				assert.NoError(t, err)
-		assert.Equal(t, currentTarget, newCurrentTarget, "current target does not match")
+				assert.Equal(t, currentTarget, newCurrentTarget, "current target does not match")
+			}
+		}
 	}
 }
 
@@ -1398,6 +1470,222 @@ func TestPublishSnapshotLocalKeysCreatedFirst(t *testing.T) {
 	assert.False(t, requestMade)
 }
 
+// Publishing delegations works so long as the delegation parent exists by the
+// time that delegation addition change is applied.  Most of the tests for
+// applying delegation changes in in helpers_test.go (applyTargets tests), so
+// this is just a sanity test to make sure Publish calls it correctly and
+// no fallback happens.
+func TestPublishDelegations(t *testing.T) {
+	var tempDirs [2]string
+	for i := 0; i < 2; i++ {
+		tempBaseDir, err := ioutil.TempDir("", "notary-test-")
+		assert.NoError(t, err, "failed to create a temporary directory: %s", err)
+		defer os.RemoveAll(tempBaseDir)
+		tempDirs[i] = tempBaseDir
+	}
+
+	gun := "docker.com/notary"
+	ts := fullTestServer(t)
+	defer ts.Close()
+
+	repo1, _ := initializeRepo(t, data.ECDSAKey, tempDirs[0], gun, ts.URL, false)
+	delgKey, err := repo1.CryptoService.Create("targets/a", data.ECDSAKey)
+	assert.NoError(t, err, "error creating delegation key")
+
+	// This should publish fine, even though targets/a/b is dependent upon
+	// targets/a, because these should execute in order
+	for _, delgName := range []string{"targets/a", "targets/a/b", "targets/c"} {
+		assert.NoError(t,
+			repo1.AddDelegation(delgName, 1, []data.PublicKey{delgKey}, []string{""}),
+			"error creating delegation")
+	}
+	assert.Len(t, getChanges(t, repo1), 3, "wrong number of changelist files found")
+	assert.NoError(t, repo1.Publish())
+	assert.Len(t, getChanges(t, repo1), 0, "wrong number of changelist files found")
+
+	// this should not publish, because targets/z doesn't exist
+	assert.NoError(t,
+		repo1.AddDelegation("targets/z/y", 1, []data.PublicKey{delgKey}, []string{""}),
+		"error creating delegation")
+	assert.Len(t, getChanges(t, repo1), 1, "wrong number of changelist files found")
+	assert.Error(t, repo1.Publish())
+	assert.Len(t, getChanges(t, repo1), 1, "wrong number of changelist files found")
+
+	// Create a new repo and pull from the server
+	repo2, err := NewNotaryRepository(tempDirs[1], gun, ts.URL,
+		http.DefaultTransport, passphraseRetriever)
+	assert.NoError(t, err, "error creating repository: %s", err)
+
+	// pull
+	_, err = repo2.ListTargets()
+	assert.NoError(t, err, "unable to pull repo")
+
+	for _, repo := range []*NotaryRepository{repo1, repo2} {
+		// targets should have delegations targets/a and targets/c
+		targets := repo.tufRepo.Targets[data.CanonicalTargetsRole]
+		assert.Len(t, targets.Signed.Delegations.Roles, 2)
+		assert.Len(t, targets.Signed.Delegations.Keys, 1)
+
+		_, ok := targets.Signed.Delegations.Keys[delgKey.ID()]
+		assert.True(t, ok)
+
+		foundRoleNames := make(map[string]bool)
+		for _, r := range targets.Signed.Delegations.Roles {
+			foundRoleNames[r.Name] = true
+		}
+		assert.True(t, foundRoleNames["targets/a"])
+		assert.True(t, foundRoleNames["targets/c"])
+
+		// targets/a should have delegation targets/a/b only
+		a := repo.tufRepo.Targets["targets/a"]
+		assert.Len(t, a.Signed.Delegations.Roles, 1)
+		assert.Len(t, a.Signed.Delegations.Keys, 1)
+
+		_, ok = a.Signed.Delegations.Keys[delgKey.ID()]
+		assert.True(t, ok)
+
+		assert.Equal(t, "targets/a/b", a.Signed.Delegations.Roles[0].Name)
+	}
+}
+
+// If a changelist specifies a particular role to push targets to, and there
+// is no such role, publish will try to publish to its parent.  If the parent
+// doesn't work, it falls back on its parent, and so forth, and eventually
+// falls back on publishing to "target".  This *only* falls back if the role
+// doesn't exist, not if the user doesn't have a key.  (different test)
+func TestPublishTargetsDelgationScopeFallback(t *testing.T) {
+	tempBaseDir, err := ioutil.TempDir("", "notary-test-")
+	assert.NoError(t, err, "failed to create a temporary directory: %s", err)
+	defer os.RemoveAll(tempBaseDir)
+
+	gun := "docker.com/notary"
+	ts := fullTestServer(t)
+	defer ts.Close()
+
+	repo, _ := initializeRepo(t, data.ECDSAKey, tempBaseDir, gun, ts.URL, false)
+	assertPublishToRolesSucceeds(t, repo, []string{"targets/a/b", "targets/b/c"},
+		[]string{data.CanonicalTargetsRole})
+}
+
+// If a changelist specifies a particular role to push targets to, and there
+// is a role but no key, publish not fall back and just fail.
+func TestPublishTargetsDelgationScopeNoFallbackIfNoKeys(t *testing.T) {
+	tempBaseDir, err := ioutil.TempDir("", "notary-test-")
+	assert.NoError(t, err, "failed to create a temporary directory: %s", err)
+	defer os.RemoveAll(tempBaseDir)
+
+	gun := "docker.com/notary"
+	ts := fullTestServer(t)
+	defer ts.Close()
+
+	repo, _ := initializeRepo(t, data.ECDSAKey, tempBaseDir, gun, ts.URL, false)
+
+	// generate a key that isn't in the cryptoservice, so we can't sign this
+	// one
+	aPrivKey, err := trustmanager.GenerateECDSAKey(rand.Reader)
+	assert.NoError(t, err, "error generating key that is not in our cryptoservice")
+	aPubKey := data.PublicKeyFromPrivate(aPrivKey)
+
+	// ensure that the role exists
+	assert.NoError(t, repo.AddDelegation("targets/a", 1, []data.PublicKey{aPubKey}, []string{""}))
+	assert.NoError(t, repo.Publish())
+
+	// add a target to targets/a/b - no role b, so it falls back on a, which
+	// exists but there is no signing key for
+	addTarget(t, repo, "latest", "../fixtures/intermediate-ca.crt", "targets/a/b")
+	assert.Len(t, getChanges(t, repo), 1, "wrong number of changelist files found")
+
+	// Now Publish should fail
+	assert.Error(t, repo.Publish())
+	assert.Len(t, getChanges(t, repo), 1, "wrong number of changelist files found")
+
+	targets, err := repo.ListTargets("targets", "targets/a", "targets/a/b")
+	assert.NoError(t, err)
+	assert.Empty(t, targets)
+}
+
+// If a changelist specifies a particular role to push targets to, and is such
+// a role and the keys are present, publish will write to that role only, and
+// not its parents.  This tests the case where the local machine knows about
+// all the roles (in fact, the role creations will be applied before the
+// targets)
+func TestPublishTargetsDelgationSuccessLocallyHasRoles(t *testing.T) {
+	tempBaseDir, err := ioutil.TempDir("", "notary-test-")
+	assert.NoError(t, err, "failed to create a temporary directory: %s", err)
+	defer os.RemoveAll(tempBaseDir)
+
+	gun := "docker.com/notary"
+	ts := fullTestServer(t)
+	defer ts.Close()
+
+	repo, _ := initializeRepo(t, data.ECDSAKey, tempBaseDir, gun, ts.URL, false)
+	delgKey, err := repo.CryptoService.Create("targets/a", data.ECDSAKey)
+	assert.NoError(t, err, "error creating delegation key")
+
+	for _, delgName := range []string{"targets/a", "targets/a/b"} {
+		assert.NoError(t,
+			repo.AddDelegation(delgName, 1, []data.PublicKey{delgKey}, []string{""}),
+			"error creating delegation")
+	}
+
+	assertPublishToRolesSucceeds(t, repo, []string{"targets/a/b"},
+		[]string{"targets/a/b"})
+}
+
+// If a changelist specifies a particular role to push targets to, and is such
+// a role and the keys are present, publish will write to that role only, and
+// not its parents.  Tests:
+// - case where the local doesn't know about all the roles, and has to download
+//   them before publish.
+// - owner of a repo may not have the delegated keys, so can't sign a delegated
+//   role
+func TestPublishTargetsDelgationSuccessNeedsToDownloadRoles(t *testing.T) {
+	var tempDirs [2]string
+	for i := 0; i < 2; i++ {
+		tempBaseDir, err := ioutil.TempDir("", "notary-test-")
+		assert.NoError(t, err, "failed to create a temporary directory: %s", err)
+		defer os.RemoveAll(tempBaseDir)
+		tempDirs[i] = tempBaseDir
+	}
+
+	gun := "docker.com/notary"
+	ts := fullTestServer(t)
+	defer ts.Close()
+
+	// this is the original repo - it owns the root/targets keys and creates
+	// the delegation to which it doesn't have the key (so server snapshot
+	// signing would be required)
+	ownerRepo, _ := initializeRepo(t, data.ECDSAKey, tempDirs[0], gun, ts.URL, true)
+	// this is a user, or otherwise a repo that only has access to the delegation
+	// key so it can publish targets to the delegated role
+	delgRepo, err := NewNotaryRepository(tempDirs[1], gun, ts.URL,
+		http.DefaultTransport, passphraseRetriever)
+	assert.NoError(t, err, "error creating repository: %s", err)
+
+	// create a key on the owner repo
+	aKey, err := ownerRepo.CryptoService.Create("targets/a", data.ECDSAKey)
+	assert.NoError(t, err, "error creating delegation key")
+
+	// create a key on the delegated repo
+	bKey, err := delgRepo.CryptoService.Create("targets/a/b", data.ECDSAKey)
+	assert.NoError(t, err, "error creating delegation key")
+
+	// owner creates delegations, adds the delegated key to them, and publishes them
+	assert.NoError(t,
+		ownerRepo.AddDelegation("targets/a", 1, []data.PublicKey{aKey}, []string{""}),
+		"error creating delegation")
+	assert.NoError(t,
+		ownerRepo.AddDelegation("targets/a/b", 1, []data.PublicKey{bKey}, []string{""}),
+		"error creating delegation")
+
+	assert.NoError(t, ownerRepo.Publish())
+
+	// delegated repo now publishes to delegated roles, but it will need
+	// to download those roles first, since it doesn't know about them
+	assertPublishToRolesSucceeds(t, delgRepo, []string{"targets/a/b"},
+		[]string{"targets/a/b"})
+}
+
 // Rotate invalid roles, or attempt to delegate target signing to the server
 func TestRotateKeyInvalidRole(t *testing.T) {
 	tempBaseDir, err := ioutil.TempDir("/tmp", "notary-test-")

client/helpers.go

@@ -4,6 +4,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"net/http"
+	"path/filepath"
 	"time"
 
 	"github.com/Sirupsen/logrus"
@@ -122,6 +123,22 @@ func changeTargetsDelegation(repo *tuf.Repo, c changelist.Change) error {
 
 }
 
+// applies a function repeatedly, falling back on the parent role, until it no
+// longer can
+func doWithRoleFallback(role string, doFunc func(string) error) error {
+	for role == data.CanonicalTargetsRole || data.IsDelegation(role) {
+		err := doFunc(role)
+		if err == nil {
+			return nil
+		}
+		if _, ok := err.(data.ErrInvalidRole); !ok {
+			return err
+		}
+		role = filepath.Dir(role)
+	}
+	return data.ErrInvalidRole{Role: role}
+}
+
 func changeTargetMeta(repo *tuf.Repo, c changelist.Change) error {
 	var err error
 	switch c.Action() {
@@ -133,13 +150,25 @@ func changeTargetMeta(repo *tuf.Repo, c changelist.Change) error {
 			return err
 		}
 		files := data.Files{c.Path(): *meta}
-		_, err = repo.AddTargets(c.Scope(), files)
+
+		err = doWithRoleFallback(c.Scope(), func(role string) error {
+			_, e := repo.AddTargets(role, files)
+			return e
+		})
 		if err != nil {
 			logrus.Errorf("couldn't add target to %s: %s", c.Scope(), err.Error())
 		}
+
 	case changelist.ActionDelete:
 		logrus.Debug("changelist remove: ", c.Path())
-		err = repo.RemoveTargets(c.Scope(), c.Path())
+
+		err = doWithRoleFallback(c.Scope(), func(role string) error {
+			return repo.RemoveTargets(role, c.Path())
+		})
+		if err != nil {
+			logrus.Errorf("couldn't remove target from %s: %s", c.Scope(), err.Error())
+		}
+
 	default:
 		logrus.Debug("action not yet supported: ", c.Action())
 	}
@@ -216,13 +245,14 @@ func addKeyForRole(kdb *keys.KeyDB, role string, key data.PublicKey) error {
 // signs and serializes the metadata for a canonical role in a tuf repo to JSON
 func serializeCanonicalRole(tufRepo *tuf.Repo, role string) (out []byte, err error) {
 	var s *data.Signed
-	switch role {
+	switch {
-	case data.CanonicalRootRole:
+	case role == data.CanonicalRootRole:
 		s, err = tufRepo.SignRoot(data.DefaultExpires(role))
-	case data.CanonicalSnapshotRole:
+	case role == data.CanonicalSnapshotRole:
 		s, err = tufRepo.SignSnapshot(data.DefaultExpires(role))
-	case data.CanonicalTargetsRole:
+	case tufRepo.Targets[role] != nil:
-		s, err = tufRepo.SignTargets(role, data.DefaultExpires(role))
+		s, err = tufRepo.SignTargets(
+			role, data.DefaultExpires(data.CanonicalTargetsRole))
 	default:
 		err = fmt.Errorf("%s not supported role to sign on the client", role)
 	}

client/helpers_test.go

@@ -6,21 +6,14 @@ import (
 	"testing"
 
 	"github.com/docker/notary/client/changelist"
-	tuf "github.com/docker/notary/tuf"
 	"github.com/docker/notary/tuf/data"
-	"github.com/docker/notary/tuf/keys"
 	"github.com/docker/notary/tuf/testutils"
 	"github.com/stretchr/testify/assert"
 )
 
 func TestApplyTargetsChange(t *testing.T) {
-	kdb := keys.NewDB()
+	_, repo, _ := testutils.EmptyRepo()
-	role, err := data.NewRole("targets", 1, nil, nil, nil)
+	_, err := repo.InitTargets(data.CanonicalTargetsRole)
-	assert.NoError(t, err)
-	kdb.AddRole(role)
-
-	repo := tuf.NewRepo(kdb, nil)
-	err = repo.InitTargets(data.CanonicalTargetsRole)
 	assert.NoError(t, err)
 	hash := sha256.Sum256([]byte{})
 	f := &data.FileMeta{
@@ -57,13 +50,8 @@ func TestApplyTargetsChange(t *testing.T) {
 }
 
 func TestApplyChangelist(t *testing.T) {
-	kdb := keys.NewDB()
+	_, repo, _ := testutils.EmptyRepo()
-	role, err := data.NewRole("targets", 1, nil, nil, nil)
+	_, err := repo.InitTargets(data.CanonicalTargetsRole)
-	assert.NoError(t, err)
-	kdb.AddRole(role)
-
-	repo := tuf.NewRepo(kdb, nil)
-	err = repo.InitTargets(data.CanonicalTargetsRole)
 	assert.NoError(t, err)
 	hash := sha256.Sum256([]byte{})
 	f := &data.FileMeta{
@@ -105,13 +93,8 @@ func TestApplyChangelist(t *testing.T) {
 }
 
 func TestApplyChangelistMulti(t *testing.T) {
-	kdb := keys.NewDB()
+	_, repo, _ := testutils.EmptyRepo()
-	role, err := data.NewRole("targets", 1, nil, nil, nil)
+	_, err := repo.InitTargets(data.CanonicalTargetsRole)
-	assert.NoError(t, err)
-	kdb.AddRole(role)
-
-	repo := tuf.NewRepo(kdb, nil)
-	err = repo.InitTargets(data.CanonicalTargetsRole)
 	assert.NoError(t, err)
 	hash := sha256.Sum256([]byte{})
 	f := &data.FileMeta{
@@ -763,3 +746,192 @@ func TestApplyTargetsDelegationParentDoesntExist(t *testing.T) {
 	assert.Error(t, err)
 	assert.IsType(t, data.ErrInvalidRole{}, err)
 }
+
+// If there is no delegation target, ApplyTargets creates it
+func TestApplyChangelistCreatesDelegation(t *testing.T) {
+	_, repo, cs := testutils.EmptyRepo()
+
+	newKey, err := cs.Create("targets/level1", data.ED25519Key)
+	assert.NoError(t, err)
+
+	r, err := data.NewRole("targets/level1", 1, []string{newKey.ID()}, []string{""}, nil)
+	assert.NoError(t, err)
+	repo.UpdateDelegations(r, []data.PublicKey{newKey})
+	delete(repo.Targets, "targets/level1")
+
+	hash := sha256.Sum256([]byte{})
+	f := &data.FileMeta{
+		Length: 1,
+		Hashes: map[string][]byte{
+			"sha256": hash[:],
+		},
+	}
+	fjson, err := json.Marshal(f)
+	assert.NoError(t, err)
+
+	cl := changelist.NewMemChangelist()
+	assert.NoError(t, cl.Add(&changelist.TufChange{
+		Actn:       changelist.ActionCreate,
+		Role:       "targets/level1",
+		ChangeType: "target",
+		ChangePath: "latest",
+		Data:       fjson,
+	}))
+
+	assert.NoError(t, applyChangelist(repo, cl))
+	_, ok := repo.Targets["targets/level1"]
+	assert.True(t, ok, "Failed to create the delegation target")
+	_, ok = repo.Targets["targets/level1"].Signed.Targets["latest"]
+	assert.True(t, ok, "Failed to write change to delegation target")
+}
+
+// Each change applies only to the role specified
+func TestApplyChangelistTargetsToMultipleRoles(t *testing.T) {
+	_, repo, cs := testutils.EmptyRepo()
+
+	newKey, err := cs.Create("targets/level1", data.ED25519Key)
+	assert.NoError(t, err)
+
+	r, err := data.NewRole("targets/level1", 1, []string{newKey.ID()}, []string{""}, nil)
+	assert.NoError(t, err)
+	repo.UpdateDelegations(r, []data.PublicKey{newKey})
+
+	r, err = data.NewRole("targets/level2", 1, []string{newKey.ID()}, []string{""}, nil)
+	assert.NoError(t, err)
+	repo.UpdateDelegations(r, []data.PublicKey{newKey})
+
+	hash := sha256.Sum256([]byte{})
+	f := &data.FileMeta{
+		Length: 1,
+		Hashes: map[string][]byte{
+			"sha256": hash[:],
+		},
+	}
+	fjson, err := json.Marshal(f)
+	assert.NoError(t, err)
+
+	cl := changelist.NewMemChangelist()
+	assert.NoError(t, cl.Add(&changelist.TufChange{
+		Actn:       changelist.ActionCreate,
+		Role:       "targets/level1",
+		ChangeType: "target",
+		ChangePath: "latest",
+		Data:       fjson,
+	}))
+	assert.NoError(t, cl.Add(&changelist.TufChange{
+		Actn:       changelist.ActionDelete,
+		Role:       "targets/level2",
+		ChangeType: "target",
+		ChangePath: "latest",
+		Data:       nil,
+	}))
+
+	assert.NoError(t, applyChangelist(repo, cl))
+	_, ok := repo.Targets["targets/level1"].Signed.Targets["latest"]
+	assert.True(t, ok)
+	_, ok = repo.Targets["targets/level2"]
+	assert.False(t, ok, "no change to targets/level2, so metadata not created")
+}
+
+// ApplyTargets falls back to role that exists when adding or deleting a change
+func TestApplyChangelistTargetsFallbackRoles(t *testing.T) {
+	_, repo, _ := testutils.EmptyRepo()
+
+	hash := sha256.Sum256([]byte{})
+	f := &data.FileMeta{
+		Length: 1,
+		Hashes: map[string][]byte{
+			"sha256": hash[:],
+		},
+	}
+	fjson, err := json.Marshal(f)
+	assert.NoError(t, err)
+
+	cl := changelist.NewMemChangelist()
+	assert.NoError(t, cl.Add(&changelist.TufChange{
+		Actn:       changelist.ActionCreate,
+		Role:       "targets/level1/level2/level3/level4",
+		ChangeType: "target",
+		ChangePath: "latest",
+		Data:       fjson,
+	}))
+
+	assert.NoError(t, applyChangelist(repo, cl))
+	_, ok := repo.Targets[data.CanonicalTargetsRole].Signed.Targets["latest"]
+	assert.True(t, ok)
+
+	// now delete and assert it applies to
+	cl = changelist.NewMemChangelist()
+	assert.NoError(t, cl.Add(&changelist.TufChange{
+		Actn:       changelist.ActionDelete,
+		Role:       "targets/level1/level2/level3/level4",
+		ChangeType: "target",
+		ChangePath: "latest",
+		Data:       nil,
+	}))
+
+	assert.NoError(t, applyChangelist(repo, cl))
+	assert.Empty(t, repo.Targets[data.CanonicalTargetsRole].Signed.Targets)
+}
+
+// changeTargetMeta fallback fails with ErrInvalidRole if role is invalid
+func TestChangeTargetMetaFallbackFailsInvalidRole(t *testing.T) {
+	_, repo, _ := testutils.EmptyRepo()
+
+	hash := sha256.Sum256([]byte{})
+	f := &data.FileMeta{
+		Length: 1,
+		Hashes: map[string][]byte{
+			"sha256": hash[:],
+		},
+	}
+	fjson, err := json.Marshal(f)
+	assert.NoError(t, err)
+
+	err = changeTargetMeta(repo, &changelist.TufChange{
+		Actn:       changelist.ActionCreate,
+		Role:       "ruhroh",
+		ChangeType: "target",
+		ChangePath: "latest",
+		Data:       fjson,
+	})
+	assert.Error(t, err)
+	assert.IsType(t, data.ErrInvalidRole{}, err)
+}
+
+// If applying a change fails due to a prefix error, it does not fall back
+// on the parent.
+func TestChangeTargetMetaDoesntFallbackIfPrefixError(t *testing.T) {
+	_, repo, cs := testutils.EmptyRepo()
+
+	newKey, err := cs.Create("targets/level1", data.ED25519Key)
+	assert.NoError(t, err)
+
+	r, err := data.NewRole("targets/level1", 1, []string{newKey.ID()},
+		[]string{"pathprefix"}, nil)
+	assert.NoError(t, err)
+	repo.UpdateDelegations(r, []data.PublicKey{newKey})
+
+	hash := sha256.Sum256([]byte{})
+	f := &data.FileMeta{
+		Length: 1,
+		Hashes: map[string][]byte{
+			"sha256": hash[:],
+		},
+	}
+	fjson, err := json.Marshal(f)
+	assert.NoError(t, err)
+
+	err = changeTargetMeta(repo, &changelist.TufChange{
+		Actn:       changelist.ActionCreate,
+		Role:       "targets/level1",
+		ChangeType: "target",
+		ChangePath: "notPathPrefix",
+		Data:       fjson,
+	})
+	assert.Error(t, err)
+
+	// no target in targets or targets/latest
+	assert.Empty(t, repo.Targets[data.CanonicalTargetsRole].Signed.Targets)
+	assert.Empty(t, repo.Targets["targets/level1"].Signed.Targets)
+}

server/handlers/validation_test.go

@@ -836,7 +836,11 @@ func TestValidateTargetsLoadParent(t *testing.T) {
 	r, err := data.NewRole("targets/level1", 1, []string{k.ID()}, []string{""}, nil)
 	assert.NoError(t, err)
 
-	baseRepo.UpdateDelegations(r, []data.PublicKey{k})
+	err = baseRepo.UpdateDelegations(r, []data.PublicKey{k})
+	assert.NoError(t, err)
+
+	// no targets file is created for the new delegations, so force one
+	baseRepo.InitTargets("targets/level1")
 
 	// we're not going to validate things loaded from storage, so no need
 	// to sign the base targets, just Marshal it and set it into storage
@@ -885,6 +889,9 @@ func TestValidateTargetsParentInUpdate(t *testing.T) {
 
 	baseRepo.UpdateDelegations(r, []data.PublicKey{k})
 
+	// no targets file is created for the new delegations, so force one
+	baseRepo.InitTargets("targets/level1")
+
 	targets, err := baseRepo.SignTargets("targets", data.DefaultExpires(data.CanonicalTargetsRole))
 
 	tgtsJSON, err := json.Marshal(targets)
@@ -939,6 +946,9 @@ func TestValidateTargetsParentNotFound(t *testing.T) {
 
 	baseRepo.UpdateDelegations(r, []data.PublicKey{k})
 
+	// no targets file is created for the new delegations, so force one
+	baseRepo.InitTargets("targets/level1")
+
 	// generate the update object we're doing to use to call loadAndValidateTargets
 	del, err := baseRepo.SignTargets("targets/level1", data.DefaultExpires(data.CanonicalTargetsRole))
 	assert.NoError(t, err)

tuf/client/client.go

@@ -500,7 +500,7 @@ func (c Client) getTargetsFile(role string, keyIDs []string, snapshotMeta data.F
 		// if we error when setting meta, we should continue.
 		err = c.cache.SetMeta(role, raw)
 		if err != nil {
-			logrus.Errorf("Failed to write snapshot to local cache: %s", err.Error())
+			logrus.Errorf("Failed to write %s to local cache: %s", role, err.Error())
 		}
 	}
 	return s, nil

tuf/tuf.go

@@ -99,8 +99,24 @@ func (tr *Repo) AddBaseKeys(role string, keys ...data.PublicKey) error {
 	}
 	tr.keysDB.AddRole(r)
 	tr.Root.Dirty = true
-	return nil
 
+	// also, whichever role was switched out needs to be re-signed
+	// root has already been marked dirty
+	switch role {
+	case data.CanonicalSnapshotRole:
+		if tr.Snapshot != nil {
+			tr.Snapshot.Dirty = true
+		}
+	case data.CanonicalTargetsRole:
+		if target, ok := tr.Targets[data.CanonicalTargetsRole]; ok {
+			target.Dirty = true
+		}
+	case data.CanonicalTimestampRole:
+		if tr.Timestamp != nil {
+			tr.Timestamp.Dirty = true
+		}
+	}
+	return nil
 }
 
 // ReplaceBaseKeys is used to replace all keys for the given role with the new keys
@@ -164,11 +180,20 @@ func (tr *Repo) GetDelegation(role string) (*data.Role, error) {
 	if !r.IsDelegation() {
 		return nil, data.ErrInvalidRole{Role: role, Reason: "not a valid delegated role"}
 	}
+
 	parent := filepath.Dir(role)
-	p, ok := tr.Targets[parent]
+
-	if !ok {
+	// check the parent role
+	if parentRole := tr.keysDB.GetRole(parent); parentRole == nil {
 		return nil, data.ErrInvalidRole{Role: role, Reason: "parent role not found"}
 	}
+
+	// check the parent role's metadata
+	p, ok := tr.Targets[parent]
+	if !ok { // the parent targetfile may not exist yet, so it can't be in the list
+		return nil, data.ErrNoSuchRole{Role: role}
+	}
+
 	foundAt := utils.FindRoleIndex(p.Signed.Delegations.Roles, role)
 	if foundAt < 0 {
 		return nil, data.ErrNoSuchRole{Role: role}
@@ -185,10 +210,21 @@ func (tr *Repo) UpdateDelegations(role *data.Role, keys []data.PublicKey) error
 		return data.ErrInvalidRole{Role: role.Name, Reason: "not a valid delegated role"}
 	}
 	parent := filepath.Dir(role.Name)
-	p, ok := tr.Targets[parent]
+
-	if !ok {
+	if err := tr.VerifyCanSign(parent); err != nil {
-		return data.ErrInvalidRole{Role: role.Name, Reason: "parent role not found"}
+		return err
 	}
+
+	// check the parent role's metadata
+	p, ok := tr.Targets[parent]
+	if !ok { // the parent targetfile may not exist yet - if not, then create it
+		var err error
+		p, err = tr.InitTargets(parent)
+		if err != nil {
+			return err
+		}
+	}
+
 	for _, k := range keys {
 		if !utils.StrSliceContains(role.KeyIDs, k.ID()) {
 			role.KeyIDs = append(role.KeyIDs, k.ID())
@@ -214,11 +250,11 @@ func (tr *Repo) UpdateDelegations(role *data.Role, keys []data.PublicKey) error
 	// We've made a change to parent. Set it to dirty
 	p.Dirty = true
 
-	roleTargets := data.NewTargets() // NewTargets always marked Dirty
+	// We don't actually want to create the new delegation metadata yet.
-	tr.Targets[role.Name] = roleTargets
+	// When we add a delegation, it may only be signable by a key we don't have
+	// (hence we are delegating signing).
 
 	tr.keysDB.AddRole(role)
-
 	utils.RemoveUnusedKeys(p)
 
 	return nil
@@ -235,9 +271,20 @@ func (tr *Repo) DeleteDelegation(role data.Role) error {
 	name := role.Name
 
 	parent := filepath.Dir(name)
+	if err := tr.VerifyCanSign(parent); err != nil {
+		return err
+	}
+
+	// delete delegated data from Targets map and Snapshot - if they don't
+	// exist, these are no-op
+	delete(tr.Targets, name)
+	tr.Snapshot.DeleteMeta(name)
+
 	p, ok := tr.Targets[parent]
 	if !ok {
-		return data.ErrInvalidRole{Role: name, Reason: "parent role not found"}
+		// if there is no parent metadata (the role exists though), then this
+		// is as good as done.
+		return nil
 	}
 
 	foundAt := utils.FindRoleIndex(p.Signed.Delegations.Roles, name)
@@ -254,10 +301,6 @@ func (tr *Repo) DeleteDelegation(role data.Role) error {
 		utils.RemoveUnusedKeys(p)
 
 		p.Dirty = true
-
-		// delete delegated data from Targets map and Snapshot
-		delete(tr.Targets, name)
-		tr.Snapshot.DeleteMeta(name)
 	} // if the role wasn't found, it's a good as deleted
 
 	return nil
@@ -271,7 +314,7 @@ func (tr *Repo) InitRepo(consistent bool) error {
 	if err := tr.InitRoot(consistent); err != nil {
 		return err
 	}
-	if err := tr.InitTargets(data.CanonicalTargetsRole); err != nil {
+	if _, err := tr.InitTargets(data.CanonicalTargetsRole); err != nil {
 		return err
 	}
 	if err := tr.InitSnapshot(); err != nil {
@@ -306,18 +349,18 @@ func (tr *Repo) InitRoot(consistent bool) error {
 	return nil
 }
 
-// InitTargets initializes an empty targets
+// InitTargets initializes an empty targets, and returns the new empty target
-func (tr *Repo) InitTargets(role string) error {
+func (tr *Repo) InitTargets(role string) (*data.SignedTargets, error) {
 	r := data.Role{Name: role}
 	if !r.IsDelegation() && !(data.CanonicalRole(role) == data.CanonicalTargetsRole) {
-		return data.ErrInvalidRole{
+		return nil, data.ErrInvalidRole{
 			Role:   role,
 			Reason: fmt.Sprintf("role is not a valid targets role name: %s", role),
 		}
 	}
 	targets := data.NewTargets()
 	tr.Targets[data.RoleName(role)] = targets
-	return nil
+	return targets, nil
 }
 
 // InitSnapshot initializes a snapshot based on the current root and targets
@@ -475,19 +518,53 @@ func (tr Repo) FindTarget(path string) *data.FileMeta {
 	return walkTargets("targets")
 }
 
-// AddTargets will attempt to add the given targets specifically to
+// VerifyCanSign returns nil if the role exists and we have at least one
-// the directed role. If the user does not have the signing keys for the role
+// signing key for the role, false otherwise.  This does not check that we have
-// the function will return an error and the full slice of targets.
+// enough signing keys to meet the threshold, since we want to support the use
-func (tr *Repo) AddTargets(role string, targets data.Files) (data.Files, error) {
+// case of multiple signers for a role.  It returns an error if the role doesn't
-	t, ok := tr.Targets[role]
+// exist or if there are no signing keys.
-	if !ok {
+func (tr *Repo) VerifyCanSign(roleName string) error {
-		return targets, data.ErrInvalidRole{Role: role, Reason: "does not exist"}
+	role := tr.keysDB.GetRole(roleName)
+	if role == nil {
+		return data.ErrInvalidRole{Role: roleName, Reason: "does not exist"}
 	}
+
+	for _, keyID := range role.KeyIDs {
+		p, _, err := tr.cryptoService.GetPrivateKey(keyID)
+		if err == nil && p != nil {
+			return nil
+		}
+	}
+	return signed.ErrNoKeys{KeyIDs: role.KeyIDs}
+}
+
+// AddTargets will attempt to add the given targets specifically to
+// the directed role. If the metadata for the role doesn't exist yet,
+// AddTargets will create one.
+func (tr *Repo) AddTargets(role string, targets data.Files) (data.Files, error) {
+
+	err := tr.VerifyCanSign(role)
+	if err != nil {
+		return nil, err
+	}
+
+	// check the role's metadata
+	t, ok := tr.Targets[role]
+	if !ok { // the targetfile may not exist yet - if not, then create it
+		var err error
+		t, err = tr.InitTargets(role)
+		if err != nil {
+			return nil, err
+		}
+	}
+
+	// VerifyCanSign already makes sure this is not nil
+	r := tr.keysDB.GetRole(role)
+
 	invalid := make(data.Files)
 	for path, target := range targets {
 		pathDigest := sha256.Sum256([]byte(path))
 		pathHex := hex.EncodeToString(pathDigest[:])
-		r := tr.keysDB.GetRole(role)
 		if role == data.ValidRoles["targets"] || (r.CheckPaths(path) || r.CheckPrefixes(pathHex)) {
 			t.Signed.Targets[path] = target
 		} else {
@@ -503,15 +580,19 @@ func (tr *Repo) AddTargets(role string, targets data.Files) (data.Files, error)
 
 // RemoveTargets removes the given target (paths) from the given target role (delegation)
 func (tr *Repo) RemoveTargets(role string, targets ...string) error {
-	t, ok := tr.Targets[role]
+	if err := tr.VerifyCanSign(role); err != nil {
-	if !ok {
+		return err
-		return data.ErrInvalidRole{Role: role, Reason: "does not exist"}
 	}
 
-	for _, path := range targets {
+	// if the role exists but metadata does not yet, then our work is done
-		delete(t.Signed.Targets, path)
+	t, ok := tr.Targets[role]
+	if ok {
+		for _, path := range targets {
+			delete(t.Signed.Targets, path)
+		}
+		t.Dirty = true
 	}
-	t.Dirty = true
+
 	return nil
 }
 

tuf/tuf_test.go

@@ -1,6 +1,7 @@
 package tuf
 
 import (
+	"crypto/sha256"
 	"encoding/json"
 	"io/ioutil"
 	"os"
@@ -155,7 +156,12 @@ func TestUpdateDelegations(t *testing.T) {
 	err = repo.UpdateDelegations(role, data.KeyList{testKey})
 	assert.NoError(t, err)
 
-	r := repo.Targets[data.CanonicalTargetsRole]
+	// no empty metadata is created for this role
+	_, ok := repo.Targets["targets/test"]
+	assert.False(t, ok, "no empty targets file should be created for deepest delegation")
+
+	r, ok := repo.Targets[data.CanonicalTargetsRole]
+	assert.True(t, ok)
 	assert.Len(t, r.Signed.Delegations.Roles, 1)
 	assert.Len(t, r.Signed.Delegations.Keys, 1)
 	keyIDs := r.Signed.Delegations.Roles[0].KeyIDs
@@ -170,13 +176,20 @@ func TestUpdateDelegations(t *testing.T) {
 	err = repo.UpdateDelegations(roleDeep, data.KeyList{testDeepKey})
 	assert.NoError(t, err)
 
-	r = repo.Targets["targets/test"]
+	// this metadata didn't exist before, but creating targets/test/deep created
+	// the targets/test metadata
+	r, ok = repo.Targets["targets/test"]
+	assert.True(t, ok)
 	assert.Len(t, r.Signed.Delegations.Roles, 1)
 	assert.Len(t, r.Signed.Delegations.Keys, 1)
 	keyIDs = r.Signed.Delegations.Roles[0].KeyIDs
 	assert.Len(t, keyIDs, 1)
 	assert.Equal(t, testDeepKey.ID(), keyIDs[0])
 	assert.True(t, r.Dirty)
+
+	// no empty delegation metadata is created for targets/test/deep
+	_, ok = repo.Targets["targets/test/deep"]
+	assert.False(t, ok, "no empty targets file should be created for deepest delegation")
 }
 
 func TestUpdateDelegationsParentMissing(t *testing.T) {
@@ -193,8 +206,38 @@ func TestUpdateDelegationsParentMissing(t *testing.T) {
 	assert.Error(t, err)
 	assert.IsType(t, data.ErrInvalidRole{}, err)
 
-	r := repo.Targets[data.CanonicalTargetsRole]
+	r, ok := repo.Targets[data.CanonicalTargetsRole]
+	assert.True(t, ok)
 	assert.Len(t, r.Signed.Delegations.Roles, 0)
+
+	// no delegation metadata created for non-existent parent
+	_, ok = repo.Targets["targets/test"]
+	assert.False(t, ok, "no targets file should be created for nonexistent parent delegation")
+}
+
+// Updating delegations needs to modify the parent of the role being updated.
+// If there is no signing key for that parent, the delegation cannot be added.
+func TestUpdateDelegationsMissingParentKey(t *testing.T) {
+	ed25519 := signed.NewEd25519()
+	keyDB := keys.NewDB()
+	repo := initRepo(t, ed25519, keyDB)
+
+	// remove the target key (all keys)
+	repo.cryptoService = signed.NewEd25519()
+
+	roleKey, err := ed25519.Create("Invalid Role", data.ED25519Key)
+	assert.NoError(t, err)
+
+	role, err := data.NewRole("targets/role", 1, []string{}, []string{""}, []string{})
+	assert.NoError(t, err)
+
+	err = repo.UpdateDelegations(role, data.KeyList{roleKey})
+	assert.Error(t, err)
+	assert.IsType(t, signed.ErrNoKeys{}, err)
+
+	// no empty delegation metadata created for new delegation
+	_, ok := repo.Targets["targets/role"]
+	assert.False(t, ok, "no targets file should be created for empty delegation")
 }
 
 func TestUpdateDelegationsInvalidRole(t *testing.T) {
@@ -214,11 +257,18 @@ func TestUpdateDelegationsInvalidRole(t *testing.T) {
 	assert.Error(t, err)
 	assert.IsType(t, data.ErrInvalidRole{}, err)
 
-	r := repo.Targets[data.CanonicalTargetsRole]
+	r, ok := repo.Targets[data.CanonicalTargetsRole]
+	assert.True(t, ok)
 	assert.Len(t, r.Signed.Delegations.Roles, 0)
+
+	// no delegation metadata created for invalid delgation
+	_, ok = repo.Targets["root"]
+	assert.False(t, ok, "no targets file should be created since delegation failed")
 }
 
-func TestUpdateDelegationsRoleMissingKey(t *testing.T) {
+// A delegation can be created with a role that is missing a signing key, so
+// long as UpdateDelegations is called with the key
+func TestUpdateDelegationsRoleThatIsMissingDelegationKey(t *testing.T) {
 	ed25519 := signed.NewEd25519()
 	keyDB := keys.NewDB()
 	repo := initRepo(t, ed25519, keyDB)
@@ -233,13 +283,18 @@ func TestUpdateDelegationsRoleMissingKey(t *testing.T) {
 	err = repo.UpdateDelegations(role, data.KeyList{roleKey})
 	assert.NoError(t, err)
 
-	r := repo.Targets[data.CanonicalTargetsRole]
+	r, ok := repo.Targets[data.CanonicalTargetsRole]
+	assert.True(t, ok)
 	assert.Len(t, r.Signed.Delegations.Roles, 1)
 	assert.Len(t, r.Signed.Delegations.Keys, 1)
 	keyIDs := r.Signed.Delegations.Roles[0].KeyIDs
 	assert.Len(t, keyIDs, 1)
 	assert.Equal(t, roleKey.ID(), keyIDs[0])
 	assert.True(t, r.Dirty)
+
+	// no empty delegation metadata created for new delegation
+	_, ok = repo.Targets["targets/role"]
+	assert.False(t, ok, "no targets file should be created for empty delegation")
 }
 
 func TestUpdateDelegationsNotEnoughKeys(t *testing.T) {
@@ -253,10 +308,13 @@ func TestUpdateDelegationsNotEnoughKeys(t *testing.T) {
 	role, err := data.NewRole("targets/role", 2, []string{}, []string{""}, []string{})
 	assert.NoError(t, err)
 
-	// key should get added to role as part of updating the delegation
 	err = repo.UpdateDelegations(role, data.KeyList{roleKey})
 	assert.Error(t, err)
 	assert.IsType(t, data.ErrInvalidRole{}, err)
+
+	// no delegation metadata created for failed delegation
+	_, ok := repo.Targets["targets/role"]
+	assert.False(t, ok, "no targets file should be created since delegation failed")
 }
 
 func TestUpdateDelegationsReplaceRole(t *testing.T) {
@@ -272,13 +330,22 @@ func TestUpdateDelegationsReplaceRole(t *testing.T) {
 	err = repo.UpdateDelegations(role, data.KeyList{testKey})
 	assert.NoError(t, err)
 
-	r := repo.Targets[data.CanonicalTargetsRole]
+	r, ok := repo.Targets[data.CanonicalTargetsRole]
+	assert.True(t, ok)
 	assert.Len(t, r.Signed.Delegations.Roles, 1)
 	assert.Len(t, r.Signed.Delegations.Keys, 1)
 	keyIDs := r.Signed.Delegations.Roles[0].KeyIDs
 	assert.Len(t, keyIDs, 1)
 	assert.Equal(t, testKey.ID(), keyIDs[0])
 
+	// no empty delegation metadata created for new delegation
+	_, ok = repo.Targets["targets/test"]
+	assert.False(t, ok, "no targets file should be created for empty delegation")
+
+	// create one now to assert that replacing the delegation doesn't delete the
+	// metadata
+	repo.InitTargets("targets/test")
+
 	// create another role with the same name and ensure it replaces the
 	// previous role
 	testKey2, err := ed25519.Create("targets/test", data.ED25519Key)
@@ -289,13 +356,18 @@ func TestUpdateDelegationsReplaceRole(t *testing.T) {
 	err = repo.UpdateDelegations(role2, data.KeyList{testKey2})
 	assert.NoError(t, err)
 
-	r = repo.Targets["targets"]
+	r, ok = repo.Targets["targets"]
+	assert.True(t, ok)
 	assert.Len(t, r.Signed.Delegations.Roles, 1)
 	assert.Len(t, r.Signed.Delegations.Keys, 1)
 	keyIDs = r.Signed.Delegations.Roles[0].KeyIDs
 	assert.Len(t, keyIDs, 1)
 	assert.Equal(t, testKey2.ID(), keyIDs[0])
 	assert.True(t, r.Dirty)
+
+	// delegation was not deleted
+	_, ok = repo.Targets["targets/test"]
+	assert.True(t, ok, "targets file should still be here")
 }
 
 func TestUpdateDelegationsAddKeyToRole(t *testing.T) {
@@ -311,7 +383,8 @@ func TestUpdateDelegationsAddKeyToRole(t *testing.T) {
 	err = repo.UpdateDelegations(role, data.KeyList{testKey})
 	assert.NoError(t, err)
 
-	r := repo.Targets[data.CanonicalTargetsRole]
+	r, ok := repo.Targets[data.CanonicalTargetsRole]
+	assert.True(t, ok)
 	assert.Len(t, r.Signed.Delegations.Roles, 1)
 	assert.Len(t, r.Signed.Delegations.Keys, 1)
 	keyIDs := r.Signed.Delegations.Roles[0].KeyIDs
@@ -324,7 +397,8 @@ func TestUpdateDelegationsAddKeyToRole(t *testing.T) {
 	err = repo.UpdateDelegations(role, data.KeyList{testKey2})
 	assert.NoError(t, err)
 
-	r = repo.Targets["targets"]
+	r, ok = repo.Targets["targets"]
+	assert.True(t, ok)
 	assert.Len(t, r.Signed.Delegations.Roles, 1)
 	assert.Len(t, r.Signed.Delegations.Keys, 2)
 	keyIDs = r.Signed.Delegations.Roles[0].KeyIDs
@@ -348,17 +422,60 @@ func TestDeleteDelegations(t *testing.T) {
 	err = repo.UpdateDelegations(role, data.KeyList{testKey})
 	assert.NoError(t, err)
 
-	r := repo.Targets[data.CanonicalTargetsRole]
+	r, ok := repo.Targets[data.CanonicalTargetsRole]
+	assert.True(t, ok)
 	assert.Len(t, r.Signed.Delegations.Roles, 1)
 	assert.Len(t, r.Signed.Delegations.Keys, 1)
 	keyIDs := r.Signed.Delegations.Roles[0].KeyIDs
 	assert.Len(t, keyIDs, 1)
 	assert.Equal(t, testKey.ID(), keyIDs[0])
 
-	err = repo.DeleteDelegation(*role)
+	// ensure that the metadata is there and snapshot is there
+	targets, err := repo.InitTargets("targets/test")
+	assert.NoError(t, err)
+	targetsSigned, err := targets.ToSigned()
+	assert.NoError(t, err)
+	assert.NoError(t, repo.UpdateSnapshot("targets/test", targetsSigned))
+	_, ok = repo.Snapshot.Signed.Meta["targets/test"]
+	assert.True(t, ok)
+
+	assert.NoError(t, repo.DeleteDelegation(*role))
 	assert.Len(t, r.Signed.Delegations.Roles, 0)
 	assert.Len(t, r.Signed.Delegations.Keys, 0)
 	assert.True(t, r.Dirty)
+
+	// metadata should be deleted
+	_, ok = repo.Targets["targets/test"]
+	assert.False(t, ok)
+	_, ok = repo.Snapshot.Signed.Meta["targets/test"]
+	assert.False(t, ok)
+}
+
+func TestDeleteDelegationsRoleNotExistBecauseNoParentMeta(t *testing.T) {
+	ed25519 := signed.NewEd25519()
+	keyDB := keys.NewDB()
+	repo := initRepo(t, ed25519, keyDB)
+
+	testKey, err := ed25519.Create("targets/test", data.ED25519Key)
+	assert.NoError(t, err)
+	role, err := data.NewRole("targets/test", 1, []string{testKey.ID()}, []string{"test"}, []string{})
+	assert.NoError(t, err)
+
+	err = repo.UpdateDelegations(role, data.KeyList{testKey})
+	assert.NoError(t, err)
+
+	// no empty delegation metadata created for new delegation
+	_, ok := repo.Targets["targets/test"]
+	assert.False(t, ok, "no targets file should be created for empty delegation")
+
+	delRole, err := data.NewRole(
+		"targets/test/a", 1, []string{testKey.ID()}, []string{"test"}, []string{})
+
+	err = repo.DeleteDelegation(*delRole)
+	assert.NoError(t, err)
+	// still no metadata
+	_, ok = repo.Targets["targets/test"]
+	assert.False(t, ok)
 }
 
 func TestDeleteDelegationsRoleNotExist(t *testing.T) {
@@ -376,7 +493,8 @@ func TestDeleteDelegationsRoleNotExist(t *testing.T) {
 
 	err = repo.DeleteDelegation(*role)
 	assert.NoError(t, err)
-	r := repo.Targets[data.CanonicalTargetsRole]
+	r, ok := repo.Targets[data.CanonicalTargetsRole]
+	assert.True(t, ok)
 	assert.Len(t, r.Signed.Delegations.Roles, 0)
 	assert.Len(t, r.Signed.Delegations.Keys, 0)
 	assert.False(t, r.Dirty)
@@ -396,7 +514,8 @@ func TestDeleteDelegationsInvalidRole(t *testing.T) {
 	assert.Error(t, err)
 	assert.IsType(t, data.ErrInvalidRole{}, err)
 
-	r := repo.Targets[data.CanonicalTargetsRole]
+	r, ok := repo.Targets[data.CanonicalTargetsRole]
+	assert.True(t, ok)
 	assert.Len(t, r.Signed.Delegations.Roles, 0)
 }
 
@@ -412,10 +531,59 @@ func TestDeleteDelegationsParentMissing(t *testing.T) {
 	assert.Error(t, err)
 	assert.IsType(t, data.ErrInvalidRole{}, err)
 
-	r := repo.Targets[data.CanonicalTargetsRole]
+	r, ok := repo.Targets[data.CanonicalTargetsRole]
+	assert.True(t, ok)
 	assert.Len(t, r.Signed.Delegations.Roles, 0)
 }
 
+// Can't delete a delegation if we don't have the parent's signing key
+func TestDeleteDelegationsMissingParentSigningKey(t *testing.T) {
+	ed25519 := signed.NewEd25519()
+	keyDB := keys.NewDB()
+	repo := initRepo(t, ed25519, keyDB)
+
+	testKey, err := ed25519.Create("targets/test", data.ED25519Key)
+	assert.NoError(t, err)
+	role, err := data.NewRole("targets/test", 1, []string{testKey.ID()}, []string{"test"}, []string{})
+	assert.NoError(t, err)
+
+	err = repo.UpdateDelegations(role, data.KeyList{testKey})
+	assert.NoError(t, err)
+
+	r, ok := repo.Targets[data.CanonicalTargetsRole]
+	assert.True(t, ok)
+	assert.Len(t, r.Signed.Delegations.Roles, 1)
+	assert.Len(t, r.Signed.Delegations.Keys, 1)
+	keyIDs := r.Signed.Delegations.Roles[0].KeyIDs
+	assert.Len(t, keyIDs, 1)
+	assert.Equal(t, testKey.ID(), keyIDs[0])
+
+	// ensure that the metadata is there and snapshot is there
+	targets, err := repo.InitTargets("targets/test")
+	assert.NoError(t, err)
+	targetsSigned, err := targets.ToSigned()
+	assert.NoError(t, err)
+	assert.NoError(t, repo.UpdateSnapshot("targets/test", targetsSigned))
+	_, ok = repo.Snapshot.Signed.Meta["targets/test"]
+	assert.True(t, ok)
+
+	// delete all signing keys
+	repo.cryptoService = signed.NewEd25519()
+	err = repo.DeleteDelegation(*role)
+	assert.Error(t, err)
+	assert.IsType(t, signed.ErrNoKeys{}, err)
+
+	assert.Len(t, r.Signed.Delegations.Roles, 1)
+	assert.Len(t, r.Signed.Delegations.Keys, 1)
+	assert.True(t, r.Dirty)
+
+	// metadata should be here still
+	_, ok = repo.Targets["targets/test"]
+	assert.True(t, ok)
+	_, ok = repo.Snapshot.Signed.Meta["targets/test"]
+	assert.True(t, ok)
+}
+
 func TestDeleteDelegationsMidSliceRole(t *testing.T) {
 	ed25519 := signed.NewEd25519()
 	keyDB := keys.NewDB()
@@ -444,12 +612,85 @@ func TestDeleteDelegationsMidSliceRole(t *testing.T) {
 	err = repo.DeleteDelegation(*role2)
 	assert.NoError(t, err)
 
-	r := repo.Targets[data.CanonicalTargetsRole]
+	r, ok := repo.Targets[data.CanonicalTargetsRole]
+	assert.True(t, ok)
 	assert.Len(t, r.Signed.Delegations.Roles, 2)
 	assert.Len(t, r.Signed.Delegations.Keys, 1)
 	assert.True(t, r.Dirty)
 }
 
+// If the parent exists, the metadata exists, and the delegation is in it,
+// returns the role that was found
+func TestGetDelegationRoleAndMetadataExistDelegationExists(t *testing.T) {
+	ed25519 := signed.NewEd25519()
+	keyDB := keys.NewDB()
+	repo := initRepo(t, ed25519, keyDB)
+
+	testKey, err := ed25519.Create("meh", data.ED25519Key)
+	assert.NoError(t, err)
+
+	role, err := data.NewRole(
+		"targets/level1", 1, []string{testKey.ID()}, []string{""}, []string{})
+	assert.NoError(t, err)
+	assert.NoError(t, repo.UpdateDelegations(role, data.KeyList{testKey}))
+
+	role, err = data.NewRole(
+		"targets/level1/level2", 1, []string{testKey.ID()}, []string{""}, []string{})
+	assert.NoError(t, err)
+	assert.NoError(t, repo.UpdateDelegations(role, data.KeyList{testKey}))
+
+	gottenRole, err := repo.GetDelegation("targets/level1/level2")
+	assert.NoError(t, err)
+	assert.Equal(t, role, gottenRole)
+}
+
+// If the parent exists, the metadata exists, and the delegation isn't in it,
+// returns an ErrNoSuchRole
+func TestGetDelegationRoleAndMetadataExistDelegationDoesntExists(t *testing.T) {
+	ed25519 := signed.NewEd25519()
+	keyDB := keys.NewDB()
+	repo := initRepo(t, ed25519, keyDB)
+
+	testKey, err := ed25519.Create("meh", data.ED25519Key)
+	assert.NoError(t, err)
+
+	role, err := data.NewRole(
+		"targets/level1", 1, []string{testKey.ID()}, []string{""}, []string{})
+	assert.NoError(t, err)
+	assert.NoError(t, repo.UpdateDelegations(role, data.KeyList{testKey}))
+
+	// ensure metadata exists
+	repo.InitTargets("targets/level1")
+
+	_, err = repo.GetDelegation("targets/level1/level2")
+	assert.Error(t, err)
+	assert.IsType(t, data.ErrNoSuchRole{}, err)
+}
+
+// If the parent exists but the metadata doesn't exist, returns an ErrNoSuchRole
+func TestGetDelegationRoleAndMetadataDoesntExists(t *testing.T) {
+	ed25519 := signed.NewEd25519()
+	keyDB := keys.NewDB()
+	repo := initRepo(t, ed25519, keyDB)
+
+	testKey, err := ed25519.Create("meh", data.ED25519Key)
+	assert.NoError(t, err)
+
+	role, err := data.NewRole(
+		"targets/level1", 1, []string{testKey.ID()}, []string{""}, []string{})
+	assert.NoError(t, err)
+	assert.NoError(t, repo.UpdateDelegations(role, data.KeyList{testKey}))
+
+	// no empty delegation metadata created for new delegation
+	_, ok := repo.Targets["targets/test"]
+	assert.False(t, ok, "no targets file should be created for empty delegation")
+
+	_, err = repo.GetDelegation("targets/level1/level2")
+	assert.Error(t, err)
+	assert.IsType(t, data.ErrNoSuchRole{}, err)
+}
+
+// If the parent role doesn't exist, GetDelegation fails with an ErrInvalidRole
 func TestGetDelegationParentMissing(t *testing.T) {
 	ed25519 := signed.NewEd25519()
 	keyDB := keys.NewDB()
@@ -459,3 +700,239 @@ func TestGetDelegationParentMissing(t *testing.T) {
 	assert.Error(t, err)
 	assert.IsType(t, data.ErrInvalidRole{}, err)
 }
+
+// Adding targets to a role that exists and has metadata (like targets)
+// correctly adds the target
+func TestAddTargetsRoleAndMetadataExist(t *testing.T) {
+	ed25519 := signed.NewEd25519()
+	keyDB := keys.NewDB()
+	repo := initRepo(t, ed25519, keyDB)
+
+	hash := sha256.Sum256([]byte{})
+	f := data.FileMeta{
+		Length: 1,
+		Hashes: map[string][]byte{
+			"sha256": hash[:],
+		},
+	}
+
+	_, err := repo.AddTargets(data.CanonicalTargetsRole, data.Files{"f": f})
+	assert.NoError(t, err)
+
+	r, ok := repo.Targets[data.CanonicalTargetsRole]
+	assert.True(t, ok)
+	targetsF, ok := r.Signed.Targets["f"]
+	assert.True(t, ok)
+	assert.Equal(t, f, targetsF)
+}
+
+// Adding targets to a role that exists and has not metadata first creates the
+// metadata and then correctly adds the target
+func TestAddTargetsRoleExistsAndMetadataDoesntExist(t *testing.T) {
+	hash := sha256.Sum256([]byte{})
+	f := data.FileMeta{
+		Length: 1,
+		Hashes: map[string][]byte{
+			"sha256": hash[:],
+		},
+	}
+
+	ed25519 := signed.NewEd25519()
+	keyDB := keys.NewDB()
+	repo := initRepo(t, ed25519, keyDB)
+
+	testKey, err := ed25519.Create("targets/test", data.ED25519Key)
+	assert.NoError(t, err)
+	role, err := data.NewRole(
+		"targets/test", 1, []string{testKey.ID()}, []string{""}, []string{})
+	assert.NoError(t, err)
+
+	err = repo.UpdateDelegations(role, data.KeyList{testKey})
+	assert.NoError(t, err)
+
+	// no empty metadata is created for this role
+	_, ok := repo.Targets["targets/test"]
+	assert.False(t, ok, "no empty targets file should be created")
+
+	// adding the targets to the role should create the metadata though
+	_, err = repo.AddTargets("targets/test", data.Files{"f": f})
+	assert.NoError(t, err)
+
+	r, ok := repo.Targets["targets/test"]
+	assert.True(t, ok)
+	targetsF, ok := r.Signed.Targets["f"]
+	assert.True(t, ok)
+	assert.Equal(t, f, targetsF)
+}
+
+// Adding targets to a role that doesn't exist fails
+func TestAddTargetsRoleDoesntExist(t *testing.T) {
+	hash := sha256.Sum256([]byte{})
+	f := data.FileMeta{
+		Length: 1,
+		Hashes: map[string][]byte{
+			"sha256": hash[:],
+		},
+	}
+
+	ed25519 := signed.NewEd25519()
+	keyDB := keys.NewDB()
+	repo := initRepo(t, ed25519, keyDB)
+
+	_, err := repo.AddTargets("targets/test", data.Files{"f": f})
+	assert.Error(t, err)
+	assert.IsType(t, data.ErrInvalidRole{}, err)
+}
+
+// Adding targets to a role that we don't have signing keys for fails
+func TestAddTargetsNoSigningKeys(t *testing.T) {
+	hash := sha256.Sum256([]byte{})
+	f := data.FileMeta{
+		Length: 1,
+		Hashes: map[string][]byte{
+			"sha256": hash[:],
+		},
+	}
+
+	ed25519 := signed.NewEd25519()
+	keyDB := keys.NewDB()
+	repo := initRepo(t, ed25519, keyDB)
+
+	testKey, err := ed25519.Create("targets/test", data.ED25519Key)
+	assert.NoError(t, err)
+	role, err := data.NewRole(
+		"targets/test", 1, []string{testKey.ID()}, []string{"test"}, []string{})
+	assert.NoError(t, err)
+
+	err = repo.UpdateDelegations(role, data.KeyList{testKey})
+	assert.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
+	_, err = repo.AddTargets("targets/test", data.Files{"f": f})
+	assert.Error(t, err)
+	assert.IsType(t, signed.ErrNoKeys{}, err)
+}
+
+// Removing targets from a role that exists, has targets, and is signable
+// should succeed, even if we also want to remove targets that don't exist.
+func TestRemoveExistingAndNonexistingTargets(t *testing.T) {
+	ed25519 := signed.NewEd25519()
+	keyDB := keys.NewDB()
+	repo := initRepo(t, ed25519, keyDB)
+
+	testKey, err := ed25519.Create("targets/test", data.ED25519Key)
+	assert.NoError(t, err)
+	role, err := data.NewRole(
+		"targets/test", 1, []string{testKey.ID()}, []string{"test"}, []string{})
+	assert.NoError(t, err)
+
+	err = repo.UpdateDelegations(role, data.KeyList{testKey})
+	assert.NoError(t, err)
+
+	// no empty metadata is created for this role
+	_, ok := repo.Targets["targets/test"]
+	assert.False(t, ok, "no empty targets file should be created")
+
+	// now remove a target
+	assert.NoError(t, repo.RemoveTargets("targets/test", "f"))
+
+	// still no metadata
+	_, ok = repo.Targets["targets/test"]
+	assert.False(t, ok)
+}
+
+// Removing targets from a role that exists but without metadata succeeds.
+func TestRemoveTargetsNonexistentMetadata(t *testing.T) {
+	ed25519 := signed.NewEd25519()
+	keyDB := keys.NewDB()
+	repo := initRepo(t, ed25519, keyDB)
+
+	err := repo.RemoveTargets("targets/test", "f")
+	assert.Error(t, err)
+	assert.IsType(t, data.ErrInvalidRole{}, err)
+}
+
+// Removing targets from a role that doesn't exist fails
+func TestRemoveTargetsRoleDoesntExist(t *testing.T) {
+	ed25519 := signed.NewEd25519()
+	keyDB := keys.NewDB()
+	repo := initRepo(t, ed25519, keyDB)
+
+	err := repo.RemoveTargets("targets/test", "f")
+	assert.Error(t, err)
+	assert.IsType(t, data.ErrInvalidRole{}, err)
+}
+
+// Removing targets from a role that we don't have signing keys for fails
+func TestRemoveTargetsNoSigningKeys(t *testing.T) {
+	hash := sha256.Sum256([]byte{})
+	f := data.FileMeta{
+		Length: 1,
+		Hashes: map[string][]byte{
+			"sha256": hash[:],
+		},
+	}
+
+	ed25519 := signed.NewEd25519()
+	keyDB := keys.NewDB()
+	repo := initRepo(t, ed25519, keyDB)
+
+	testKey, err := ed25519.Create("targets/test", data.ED25519Key)
+	assert.NoError(t, err)
+	role, err := data.NewRole(
+		"targets/test", 1, []string{testKey.ID()}, []string{""}, []string{})
+	assert.NoError(t, err)
+
+	err = repo.UpdateDelegations(role, data.KeyList{testKey})
+	assert.NoError(t, err)
+
+	// adding the targets to the role should create the metadata though
+	_, err = repo.AddTargets("targets/test", data.Files{"f": f})
+	assert.NoError(t, err)
+
+	r, ok := repo.Targets["targets/test"]
+	assert.True(t, ok)
+	_, ok = r.Signed.Targets["f"]
+	assert.True(t, ok)
+
+	// now delete the signing key (all keys)
+	repo.cryptoService = signed.NewEd25519()
+
+	// now remove the target - it should fail
+	err = repo.RemoveTargets("targets/test", "f")
+	assert.Error(t, err)
+	assert.IsType(t, signed.ErrNoKeys{}, err)
+}
+
+// adding a key to a role marks root as dirty as well as the role
+func TestAddBaseKeysToRoot(t *testing.T) {
+	for role := range data.ValidRoles {
+		ed25519 := signed.NewEd25519()
+		keyDB := keys.NewDB()
+		repo := initRepo(t, ed25519, keyDB)
+
+		key, err := ed25519.Create(role, data.ED25519Key)
+		assert.NoError(t, err)
+
+		assert.Len(t, repo.Root.Signed.Roles[role].KeyIDs, 1)
+
+		assert.NoError(t, repo.AddBaseKeys(role, key))
+
+		_, ok := repo.Root.Signed.Keys[key.ID()]
+		assert.True(t, ok)
+		assert.Len(t, repo.Root.Signed.Roles[role].KeyIDs, 2)
+		assert.True(t, repo.Root.Dirty)
+
+		switch role {
+		case data.CanonicalSnapshotRole:
+			assert.True(t, repo.Snapshot.Dirty)
+		case data.CanonicalTargetsRole:
+			assert.True(t, repo.Targets[data.CanonicalTargetsRole].Dirty)
+		case data.CanonicalTimestampRole:
+			assert.True(t, repo.Timestamp.Dirty)
+		}
+	}
+}