diff --git a/client/client.go b/client/client.go index 49d57895ce..ea16ad6e43 100644 --- a/client/client.go +++ b/client/client.go @@ -391,18 +391,19 @@ func (r *NotaryRepository) ListTargets(roles ...string) ([]*TargetWithRole, erro targets := make(map[string]*TargetWithRole) for _, role := range roles { // Define a visitor function to populate the targets map in priority order - listVisitorFunc := func(tgt *data.SignedTargets, roleName string) error { + listVisitorFunc := func(tgt *data.SignedTargets, validRole data.DelegationRole) error { if tgt == nil { return tuf.ErrContinueWalk{} } // We found targets so we should try to add them to our targets map for targetName, targetMeta := range tgt.Signed.Targets { // Follow the priority by not overriding previously set targets - if _, ok := targets[targetName]; ok { + // and check that this path is valid with this role + if _, ok := targets[targetName]; ok || !validRole.CheckPaths(targetName) { continue } targets[targetName] = - &TargetWithRole{Target: Target{Name: targetName, Hashes: targetMeta.Hashes, Length: targetMeta.Length}, Role: roleName} + &TargetWithRole{Target: Target{Name: targetName, Hashes: targetMeta.Hashes, Length: targetMeta.Length}, Role: validRole.Name} } return tuf.ErrContinueWalk{} } @@ -437,15 +438,15 @@ func (r *NotaryRepository) GetTargetByName(name string, roles ...string) (*Targe var resultRoleName string for _, role := range roles { // Define a visitor function to find the specified target - getTargetVisitorFunc := func(tgt *data.SignedTargets, roleName string) error { + getTargetVisitorFunc := func(tgt *data.SignedTargets, validRole data.DelegationRole) error { var ok bool if tgt == nil { return tuf.ErrContinueWalk{} } - // We found the target so we should stop our walk - // and set the resultMeta and resultRoleName variables + // We found the target and validated path compatiblity in our walk, + // so we should stop our walk and set the resultMeta and resultRoleName variables if resultMeta, ok = tgt.Signed.Targets[name]; ok { - resultRoleName = roleName + resultRoleName = validRole.Name return tuf.ErrStopWalk{} } return tuf.ErrContinueWalk{} diff --git a/client/delegations.go b/client/delegations.go index 6e013d94f3..34e93cfa2c 100644 --- a/client/delegations.go +++ b/client/delegations.go @@ -255,11 +255,12 @@ func (r *NotaryRepository) GetDelegationRoles() ([]*data.Role, error) { allDelegations := []*data.Role{} // Define a visitor function to populate the delegations list and translate their key IDs to canonical IDs - delegationCanonicalListVisitor := func(tgt *data.SignedTargets, roleName string) error { + delegationCanonicalListVisitor := func(tgt *data.SignedTargets, validRole data.DelegationRole) error { if tgt == nil { return tuf.ErrContinueWalk{} } // For the return list, update with a copy that includes canonicalKeyIDs + // These aren't validated by the validRole canonicalDelegations, err := translateDelegationsToCanonicalIDs(tgt.Signed.Delegations) if err != nil { return err @@ -267,7 +268,10 @@ func (r *NotaryRepository) GetDelegationRoles() ([]*data.Role, error) { allDelegations = append(allDelegations, canonicalDelegations...) return tuf.ErrContinueWalk{} } - r.tufRepo.WalkTargets("", "", delegationCanonicalListVisitor) + err := r.tufRepo.WalkTargets("", "", delegationCanonicalListVisitor) + if err != nil { + return nil, err + } return allDelegations, nil } diff --git a/client/helpers.go b/client/helpers.go index 7d76f78870..697469c7c2 100644 --- a/client/helpers.go +++ b/client/helpers.go @@ -79,11 +79,13 @@ func changeTargetsDelegation(repo *tuf.Repo, c changelist.Change) error { if err != nil { return err } - r, _, err := repo.GetDelegation(c.Scope()) - if _, ok := err.(data.ErrNoSuchRole); err != nil && !ok { - // error that wasn't ErrNoSuchRole + _, err = repo.GetDelegationRole(c.Scope()) + if _, ok := err.(data.ErrInvalidRole); err != nil && !ok { + // error that wasn't ErrInvalidRole return err } + + r, _, _ := repo.GetDelegation(c.Scope()) if err == nil { // role existed, attempt to merge paths and keys if err := r.AddPaths(td.AddPaths); err != nil { @@ -103,10 +105,11 @@ func changeTargetsDelegation(repo *tuf.Repo, c changelist.Change) error { if err != nil { return err } - r, keys, err := repo.GetDelegation(c.Scope()) + _, err = repo.GetDelegationRole(c.Scope()) if err != nil { return err } + r, keys, _ := repo.GetDelegation(c.Scope()) // We need to translate the keys from canonical ID to TUF ID for compatibility canonicalToTUFID := make(map[string]string) @@ -125,8 +128,7 @@ func changeTargetsDelegation(repo *tuf.Repo, c changelist.Change) error { // If we specify the only keys left delete the role, else just delete specified keys if strings.Join(r.KeyIDs, ";") == strings.Join(removeTUFKeyIDs, ";") && len(td.AddKeys) == 0 { - r := data.Role{Name: c.Scope()} - return repo.DeleteDelegation(r) + return repo.DeleteDelegation(c.Scope()) } // if we aren't deleting and the role exists, merge if err := r.AddPaths(td.AddPaths); err != nil { @@ -142,8 +144,7 @@ func changeTargetsDelegation(repo *tuf.Repo, c changelist.Change) error { r.RemoveKeys(removeTUFKeyIDs) return repo.UpdateDelegations(r, td.AddKeys) case changelist.ActionDelete: - r := data.Role{Name: c.Scope()} - return repo.DeleteDelegation(r) + return repo.DeleteDelegation(c.Scope()) default: return fmt.Errorf("unsupported action against delegations: %s", c.Action()) } diff --git a/client/helpers_test.go b/client/helpers_test.go index fe477c78ed..785a2d7ada 100644 --- a/client/helpers_test.go +++ b/client/helpers_test.go @@ -445,7 +445,7 @@ func TestApplyTargetsDelegationEditNonExisting(t *testing.T) { err = applyTargetsChange(repo, ch) assert.Error(t, err) - assert.IsType(t, data.ErrNoSuchRole{}, err) + assert.IsType(t, data.ErrInvalidRole{}, err) } func TestApplyTargetsDelegationCreateAlreadyExisting(t *testing.T) { diff --git a/tuf/data/targets.go b/tuf/data/targets.go index a1bd9d5b0e..91b8f3ef1a 100644 --- a/tuf/data/targets.go +++ b/tuf/data/targets.go @@ -93,7 +93,8 @@ func (t SignedTargets) GetValidDelegations(parent DelegationRole) []DelegationRo return result } -// helper function to create DelegationRole structures from all delegations in a SignedTargets +// helper function to create DelegationRole structures from all delegations in a SignedTargets, +// these delegations are read directly from the SignedTargets and not modified or validated func (t SignedTargets) buildDelegationRoles() []DelegationRole { var roles []DelegationRole for _, roleData := range t.Signed.Delegations.Roles { diff --git a/tuf/tuf.go b/tuf/tuf.go index 72cac8d6f8..35263a64c7 100644 --- a/tuf/tuf.go +++ b/tuf/tuf.go @@ -230,7 +230,7 @@ func (tr *Repo) GetDelegationRole(name string) (data.DelegationRole, error) { // Start with top level roles in targets. Walk the chain of ancestors // until finding the desired role, or we run out of targets files to search. var foundRole *data.DelegationRole - buildDelegationRoleVisitor := func(tgt *data.SignedTargets, roleName string) error { + buildDelegationRoleVisitor := func(tgt *data.SignedTargets, validRole data.DelegationRole) error { if tgt == nil { return ErrContinueWalk{} } @@ -242,8 +242,8 @@ func (tr *Repo) GetDelegationRole(name string) (data.DelegationRole, error) { for _, keyID := range role.KeyIDs { pubKey, ok := tgt.Signed.Delegations.Keys[keyID] if !ok { - // Couldn't retrieve all keys, so stop walking - return ErrStopWalk{} + // Couldn't retrieve all keys, so stop walking and return invalid role + return data.ErrInvalidRole{Role: name, Reason: "delegation does not exist"} } pubKeys[keyID] = pubKey } @@ -262,7 +262,10 @@ func (tr *Repo) GetDelegationRole(name string) (data.DelegationRole, error) { } // Walk to the parent of this delegation, since that is where its role metadata exists - tr.WalkTargets("", path.Dir(name), buildDelegationRoleVisitor) + err := tr.WalkTargets("", path.Dir(name), buildDelegationRoleVisitor) + if err != nil { + return data.DelegationRole{}, err + } // We never found the delegation. In the context of this repo it is considered // invalid. N.B. it may be that it existed at one point but an ancestor has since @@ -388,22 +391,20 @@ func (tr *Repo) UpdateDelegations(role *data.Role, keys []data.PublicKey) error // DeleteDelegation removes a delegated targets role from its parent // targets object. It also deletes the delegation from the snapshot. // DeleteDelegation will only make use of the role Name field. -func (tr *Repo) DeleteDelegation(role data.Role) error { - if !data.IsDelegation(role.Name) { - return data.ErrInvalidRole{Role: role.Name, Reason: "not a valid delegated role"} +func (tr *Repo) DeleteDelegation(roleName string) error { + if !data.IsDelegation(roleName) { + return data.ErrInvalidRole{Role: roleName, Reason: "not a valid delegated role"} } - // the role variable must not be used past this assignment for safety - name := role.Name - parent := path.Dir(name) + parent := path.Dir(roleName) 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) + delete(tr.Targets, roleName) + tr.Snapshot.DeleteMeta(roleName) p, ok := tr.Targets[parent] if !ok { @@ -412,7 +413,7 @@ func (tr *Repo) DeleteDelegation(role data.Role) error { return nil } - foundAt := utils.FindRoleIndex(p.Signed.Delegations.Roles, name) + foundAt := utils.FindRoleIndex(p.Signed.Delegations.Roles, roleName) if foundAt >= 0 { var roles []*data.Role @@ -601,7 +602,7 @@ func (tr *Repo) VerifyCanSign(roleName string) error { } // used for walking the targets/delegations tree, potentially modifying the underlying SignedTargets for the repo -type walkVisitorFunc func(*data.SignedTargets, string) error +type walkVisitorFunc func(*data.SignedTargets, data.DelegationRole) error // WalkTargets will apply the specified visitor function to iteratively walk the targets/delegation metadata tree, // until receiving a ErrStopWalk. The walk starts from the base "targets" role, and searches for the correct targetPath and/or rolePath @@ -641,7 +642,7 @@ func (tr *Repo) WalkTargets(targetPath, rolePath string, visitTarget walkVisitor // If the paths validate against the specified targetPath and the rolePath is empty or is in the subtree if isValidPath(targetPath, role) && isAncestorRole(role.Name, rolePath) { // If we had matching path or role name, visit this target and determine whether or not to keep walking - err := visitTarget(signedTgt, role.Name) + err := visitTarget(signedTgt, role) switch err.(type) { case ErrStopWalk: // If the visitor function signalled a stop, return nil to finish the walk @@ -691,8 +692,8 @@ func (tr *Repo) AddTargets(role string, targets data.Files) (data.Files, error) } addedTargets := make(data.Files) - addTargetVisitor := func(targetPath string, targetMeta data.FileMeta) func(*data.SignedTargets, string) error { - return func(tgt *data.SignedTargets, roleName string) error { + addTargetVisitor := func(targetPath string, targetMeta data.FileMeta) func(*data.SignedTargets, data.DelegationRole) error { + return func(tgt *data.SignedTargets, validRole data.DelegationRole) error { if tgt == nil { return ErrContinueWalk{} } @@ -722,13 +723,15 @@ func (tr *Repo) RemoveTargets(role string, targets ...string) error { return err } - removeTargetVisitor := func(targetPath string) func(*data.SignedTargets, string) error { - return func(tgt *data.SignedTargets, roleName string) error { + removeTargetVisitor := func(targetPath string) func(*data.SignedTargets, data.DelegationRole) error { + return func(tgt *data.SignedTargets, validRole data.DelegationRole) error { if tgt == nil { return ErrContinueWalk{} } - // We've already validated the role's target path in our walk, so just modify the metadata + // We've already validated the role path in our walk, so just modify the metadata + // We don't check against the target path against the valid role paths because it's + // possible we got into an invalid state and are trying to fix it delete(tgt.Signed.Targets, targetPath) tgt.Dirty = true return ErrStopWalk{} diff --git a/tuf/tuf_test.go b/tuf/tuf_test.go index 0946dc0b35..8c960e642d 100644 --- a/tuf/tuf_test.go +++ b/tuf/tuf_test.go @@ -390,7 +390,7 @@ func TestDeleteDelegations(t *testing.T) { _, ok = repo.Snapshot.Signed.Meta["targets/test"] assert.True(t, ok) - assert.NoError(t, repo.DeleteDelegation(*role)) + assert.NoError(t, repo.DeleteDelegation(role.Name)) assert.Len(t, r.Signed.Delegations.Roles, 0) assert.Len(t, r.Signed.Delegations.Keys, 0) assert.True(t, r.Dirty) @@ -420,7 +420,7 @@ func TestDeleteDelegationsRoleNotExistBecauseNoParentMeta(t *testing.T) { delRole, err := data.NewRole("targets/test/a", 1, []string{testKey.ID()}, []string{"test"}) - err = repo.DeleteDelegation(*delRole) + err = repo.DeleteDelegation(delRole.Name) assert.NoError(t, err) // still no metadata _, ok = repo.Targets["targets/test"] @@ -439,7 +439,7 @@ func TestDeleteDelegationsRoleNotExist(t *testing.T) { role, err := data.NewRole("targets/test", 1, []string{}, []string{""}) assert.NoError(t, err) - err = repo.DeleteDelegation(*role) + err = repo.DeleteDelegation(role.Name) assert.NoError(t, err) r, ok := repo.Targets[data.CanonicalTargetsRole] assert.True(t, ok) @@ -457,7 +457,7 @@ func TestDeleteDelegationsInvalidRole(t *testing.T) { invalidRole, err := data.NewRole("root", 1, []string{}, []string{""}) assert.NoError(t, err) - err = repo.DeleteDelegation(*invalidRole) + err = repo.DeleteDelegation(invalidRole.Name) assert.Error(t, err) assert.IsType(t, data.ErrInvalidRole{}, err) @@ -473,7 +473,7 @@ func TestDeleteDelegationsParentMissing(t *testing.T) { testRole, err := data.NewRole("targets/test/deep", 1, []string{}, []string{""}) assert.NoError(t, err) - err = repo.DeleteDelegation(*testRole) + err = repo.DeleteDelegation(testRole.Name) assert.Error(t, err) assert.IsType(t, data.ErrInvalidRole{}, err) @@ -514,7 +514,7 @@ func TestDeleteDelegationsMissingParentSigningKey(t *testing.T) { // delete all signing keys repo.cryptoService = signed.NewEd25519() - err = repo.DeleteDelegation(*role) + err = repo.DeleteDelegation(role.Name) assert.Error(t, err) assert.IsType(t, signed.ErrNoKeys{}, err) @@ -553,7 +553,7 @@ func TestDeleteDelegationsMidSliceRole(t *testing.T) { err = repo.UpdateDelegations(role3, data.KeyList{testKey}) assert.NoError(t, err) - err = repo.DeleteDelegation(*role2) + err = repo.DeleteDelegation(role2.Name) assert.NoError(t, err) r, ok := repo.Targets[data.CanonicalTargetsRole]