From 0892ebb13f1ddbd25a737a4ed81c6c3b24664868 Mon Sep 17 00:00:00 2001 From: Ying Li Date: Fri, 18 Dec 2015 15:40:00 -0800 Subject: [PATCH] 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 --- client/client.go | 2 +- client/helpers_test.go | 29 ++--- tuf/tuf.go | 93 +++++++++++----- tuf/tuf_test.go | 240 ++++++++++++++++++++++++++++++++++++++++- 4 files changed, 309 insertions(+), 55 deletions(-) diff --git a/client/client.go b/client/client.go index 4d3a757d2e..537673a49e 100644 --- a/client/client.go +++ b/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 diff --git a/client/helpers_test.go b/client/helpers_test.go index ecb28bb2bf..c7b84baf2a 100644 --- a/client/helpers_test.go +++ b/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() - role, err := data.NewRole("targets", 1, nil, nil, nil) - assert.NoError(t, err) - kdb.AddRole(role) - - repo := tuf.NewRepo(kdb, nil) - err = repo.InitTargets(data.CanonicalTargetsRole) + _, repo, _ := testutils.EmptyRepo() + _, 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() - role, err := data.NewRole("targets", 1, nil, nil, nil) - assert.NoError(t, err) - kdb.AddRole(role) - - repo := tuf.NewRepo(kdb, nil) - err = repo.InitTargets(data.CanonicalTargetsRole) + _, repo, _ := testutils.EmptyRepo() + _, 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() - role, err := data.NewRole("targets", 1, nil, nil, nil) - assert.NoError(t, err) - kdb.AddRole(role) - - repo := tuf.NewRepo(kdb, nil) - err = repo.InitTargets(data.CanonicalTargetsRole) + _, repo, _ := testutils.EmptyRepo() + _, err := repo.InitTargets(data.CanonicalTargetsRole) assert.NoError(t, err) hash := sha256.Sum256([]byte{}) f := &data.FileMeta{ diff --git a/tuf/tuf.go b/tuf/tuf.go index d6cb4e032a..5ae2776daa 100644 --- a/tuf/tuf.go +++ b/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 parentRole := tr.keysDB.GetRole(parent); parentRole == nil { - return data.ErrInvalidRole{Role: role.Name, Reason: "parent role not found"} + if err := tr.VerifyCanSign(parent); err != nil { + 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 - p = data.NewTargets() - tr.Targets[parent] = p + var err error + 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 -func (tr *Repo) InitTargets(role string) error { +// InitTargets initializes an empty targets, and returns the new empty target +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,23 +519,49 @@ 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) - if r == nil { - return nil, data.ErrInvalidRole{Role: role, Reason: "does not exist"} + + 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 - t = data.NewTargets() - tr.Targets[role] = t + 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)) @@ -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 !ok { - return data.ErrInvalidRole{Role: role, Reason: "does not exist"} + if err := tr.VerifyCanSign(role); err != nil { + return err } - for _, path := range targets { - delete(t.Signed.Targets, path) + // 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 } - t.Dirty = true + return nil } diff --git a/tuf/tuf_test.go b/tuf/tuf_test.go index 9e42149962..4b210d70f3 100644 --- a/tuf/tuf_test.go +++ b/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 - repo.InitTargets("targets/test") + // 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) - 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 {