Add checks to TUFRepo to fail on updating a target if there are no signing keys.

So UpdateDelegation, DeleteDelegation, AddTargets, RemoveTargets now
all check for the role existence, not metadata existence.  And they
also check the role's signing keys - there's no point in adding if
we can't sign.

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

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

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{

tuf/tuf.go

@@ -212,16 +212,18 @@ func (tr *Repo) UpdateDelegations(role *data.Role, keys []data.PublicKey) error
 	}
 	parent := filepath.Dir(role.Name)
 
-	// check the parent role
+	if err := tr.VerifyCanSign(parent); err != nil {
-	if parentRole := tr.keysDB.GetRole(parent); parentRole == nil {
+		return err
-		return data.ErrInvalidRole{Role: role.Name, 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 - if not, then create it
-		p = data.NewTargets()
+		var err error
-		tr.Targets[parent] = p
+		p, err = tr.InitTargets(parent)
+		if err != nil {
+			return err
+		}
 	}
 
 	for _, k := range keys {
@@ -270,9 +272,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)
@@ -289,10 +302,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
@@ -306,7 +315,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 {
@@ -341,18 +350,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
@@ -510,22 +519,48 @@ func (tr Repo) FindTarget(path string) *data.FileMeta {
 	return walkTargets("targets")
 }
 
+// VerifyCanSign returns nil if the role exists and we have at least one
+// signing key for the role, false otherwise.  This does not check that we have
+// enough signing keys to meet the threshold, since we want to support the use
+// case of multiple signers for a role.  It returns an error if the role doesn't
+// exist or if there are no signing keys.
+func (tr *Repo) VerifyCanSign(roleName string) error {
+	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) {
-	// check the role exists
+
-	r := tr.keysDB.GetRole(role)
+	err := tr.VerifyCanSign(role)
-	if r == nil {
+	if err != nil {
-		return nil, data.ErrInvalidRole{Role: role, Reason: "does not exist"}
+		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
-		t = data.NewTargets()
+		var err error
-		tr.Targets[role] = t
+		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 {
@@ -546,15 +581,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"}
 	}
 
+	// if the role exists but metadata does not yet, then our work is done
+	t, ok := tr.Targets[role]
+	if ok {
 		for _, path := range targets {
 			delete(t.Signed.Targets, path)
 		}
 		t.Dirty = true
+	}
+
 	return nil
 }
 

tuf/tuf_test.go

@@ -215,6 +215,31 @@ func TestUpdateDelegationsParentMissing(t *testing.T) {
 	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) {
 	ed25519 := signed.NewEd25519()
 	keyDB := keys.NewDB()
@@ -241,7 +266,9 @@ func TestUpdateDelegationsInvalidRole(t *testing.T) {
 	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)
@@ -403,10 +430,16 @@ func TestDeleteDelegations(t *testing.T) {
 	assert.Len(t, keyIDs, 1)
 	assert.Equal(t, testKey.ID(), keyIDs[0])
 
-	// ensure that the metadata is there
+	// ensure that the metadata is there and snapshot is there
-	repo.InitTargets("targets/test")
+	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)
 
-	err = repo.DeleteDelegation(*role)
+	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)
@@ -414,7 +447,35 @@ func TestDeleteDelegations(t *testing.T) {
 	// 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) {
@@ -475,6 +536,54 @@ func TestDeleteDelegationsParentMissing(t *testing.T) {
 	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()
@@ -675,6 +784,129 @@ func TestAddTargetsRoleDoesntExist(t *testing.T) {
 	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{"test"}, []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 {