use only canonical IDs for display on delegation CLI commands, translate to TUF key IDs for metadata usage under the hood

Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>

client/client.go

@@ -23,6 +23,7 @@ import (
 	"github.com/docker/notary/tuf/keys"
 	"github.com/docker/notary/tuf/signed"
 	"github.com/docker/notary/tuf/store"
+	"github.com/docker/notary/tuf/utils"
 )
 
 func init() {
@@ -529,6 +530,7 @@ func (r *NotaryRepository) GetChangelist() (changelist.Changelist, error) {
 }
 
 // GetDelegationRoles returns the keys and roles of the repository's delegations
+// Also converts key IDs to canonical key IDs to keep consistent with signing prompts
 func (r *NotaryRepository) GetDelegationRoles() ([]*data.Role, error) {
 	// Update state of the repo to latest
 	if _, err := r.Update(false); err != nil {
@@ -541,7 +543,11 @@ func (r *NotaryRepository) GetDelegationRoles() ([]*data.Role, error) {
 		return nil, store.ErrMetaNotFound{Resource: data.CanonicalTargetsRole}
 	}
 
-	allDelegations := targets.Signed.Delegations.Roles
+	// make a copy of top-level Delegations and only show canonical key IDs
+	allDelegations, err := translateDelegationsToCanonicalIDs(targets.Signed.Delegations)
+	if err != nil {
+		return nil, err
+	}
 
 	// make a copy for traversing nested delegations
 	delegationsList := make([]*data.Role, len(allDelegations))
@@ -560,13 +566,42 @@ func (r *NotaryRepository) GetDelegationRoles() ([]*data.Role, error) {
 			continue
 		}
 
-		// Add nested delegations to return list and exploration list
+		// For the return list, update with a copy that includes canonicalKeyIDs
-		allDelegations = append(allDelegations, delegationMeta.Signed.Delegations.Roles...)
+		canonicalDelegations, err := translateDelegationsToCanonicalIDs(delegationMeta.Signed.Delegations)
+		if err != nil {
+			return nil, err
+		}
+		allDelegations = append(allDelegations, canonicalDelegations...)
+		// Add nested delegations to the exploration list
 		delegationsList = append(delegationsList, delegationMeta.Signed.Delegations.Roles...)
 	}
+
+	// Convert all key IDs to canonical IDs:
 	return allDelegations, nil
 }
 
+func translateDelegationsToCanonicalIDs(delegationInfo data.Delegations) ([]*data.Role, error) {
+	canonicalDelegations := make([]*data.Role, len(delegationInfo.Roles))
+	copy(canonicalDelegations, delegationInfo.Roles)
+	delegationKeys := delegationInfo.Keys
+	for i, delegation := range canonicalDelegations {
+		canonicalKeyIDs := []string{}
+		for _, keyID := range delegation.KeyIDs {
+			pubKey, ok := delegationKeys[keyID]
+			if !ok {
+				return nil, fmt.Errorf("Could not translate canonical key IDs for %s", delegation.Name)
+			}
+			canonicalKeyID, err := utils.CanonicalKeyID(pubKey)
+			if err != nil {
+				return nil, fmt.Errorf("Could not translate canonical key IDs for %s: %v", delegation.Name, err)
+			}
+			canonicalKeyIDs = append(canonicalKeyIDs, canonicalKeyID)
+		}
+		canonicalDelegations[i].KeyIDs = canonicalKeyIDs
+	}
+	return canonicalDelegations, nil
+}
+
 // RoleWithSignatures is a Role with its associated signatures
 type RoleWithSignatures struct {
 	Signatures []data.Signature

client/client_test.go

@@ -2312,7 +2312,9 @@ func TestPublishRemoveDelgation(t *testing.T) {
 	assert.NoError(t, delgRepo.Publish())
 
 	// owner removes delegation
-	assert.NoError(t, ownerRepo.RemoveDelegation("targets/a", []string{aKey.ID()}, []string{}, false))
+	aKeyCanonicalID, err := utils.CanonicalKeyID(aKey)
+	assert.NoError(t, err)
+	assert.NoError(t, ownerRepo.RemoveDelegation("targets/a", []string{aKeyCanonicalID}, []string{}, false))
 	assert.NoError(t, ownerRepo.Publish())
 
 	// delegated repo can now no longer publish to delegated role
@@ -2696,7 +2698,9 @@ func TestRemoveDelegationChangefileApplicable(t *testing.T) {
 	assert.Len(t, targetRole.Signed.Delegations.Keys, 1)
 
 	// now remove it
-	assert.NoError(t, repo.RemoveDelegation("targets/a", []string{rootKeyID}, []string{}, false))
+	rootKeyCanonicalID, err := utils.CanonicalKeyID(rootPubKey)
+	assert.NoError(t, err)
+	assert.NoError(t, repo.RemoveDelegation("targets/a", []string{rootKeyCanonicalID}, []string{}, false))
 	changes = getChanges(t, repo)
 	assert.Len(t, changes, 2)
 	assert.NoError(t, applyTargetsChange(repo.tufRepo, changes[1]))

client/helpers.go

@@ -14,6 +14,7 @@ import (
 	"github.com/docker/notary/tuf/data"
 	"github.com/docker/notary/tuf/keys"
 	"github.com/docker/notary/tuf/store"
+	"github.com/docker/notary/tuf/utils"
 )
 
 // Use this to initialize remote HTTPStores from the config settings
@@ -80,7 +81,7 @@ func changeTargetsDelegation(repo *tuf.Repo, c changelist.Change) error {
 		if err != nil {
 			return err
 		}
-		r, err := repo.GetDelegation(c.Scope())
+		r, _, err := repo.GetDelegation(c.Scope())
 		if _, ok := err.(data.ErrNoSuchRole); err != nil && !ok {
 			// error that wasn't ErrNoSuchRole
 			return err
@@ -104,12 +105,28 @@ func changeTargetsDelegation(repo *tuf.Repo, c changelist.Change) error {
 		if err != nil {
 			return err
 		}
-		r, err := repo.GetDelegation(c.Scope())
+		r, keys, err := repo.GetDelegation(c.Scope())
 		if err != nil {
 			return err
 		}
+
+		// We need to translate the keys from canonical ID to TUF ID for compatibility
+		canonicalToTUFID := make(map[string]string)
+		for tufID, pubKey := range keys {
+			canonicalID, err := utils.CanonicalKeyID(pubKey)
+			if err != nil {
+				return err
+			}
+			canonicalToTUFID[canonicalID] = tufID
+		}
+
+		removeTUFKeyIDs := []string{}
+		for _, canonID := range td.RemoveKeys {
+			removeTUFKeyIDs = append(removeTUFKeyIDs, canonicalToTUFID[canonID])
+		}
+
 		// If we specify the only keys left delete the role, else just delete specified keys
-		if strings.Join(r.KeyIDs, ";") == strings.Join(td.RemoveKeys, ";") && len(td.AddKeys) == 0 {
+		if strings.Join(r.KeyIDs, ";") == strings.Join(removeTUFKeyIDs, ";") && len(td.AddKeys) == 0 {
 			r := data.Role{Name: c.Scope()}
 			return repo.DeleteDelegation(r)
 		}
@@ -120,7 +137,7 @@ func changeTargetsDelegation(repo *tuf.Repo, c changelist.Change) error {
 		if err := r.AddPathHashPrefixes(td.AddPathHashPrefixes); err != nil {
 			return err
 		}
-		r.RemoveKeys(td.RemoveKeys)
+		r.RemoveKeys(removeTUFKeyIDs)
 		r.RemovePaths(td.RemovePaths)
 		r.RemovePathHashPrefixes(td.RemovePathHashPrefixes)
 		return repo.UpdateDelegations(r, td.AddKeys)

client/helpers_test.go

@@ -504,10 +504,14 @@ func TestApplyTargetsDelegationCreateAlreadyExisting(t *testing.T) {
 	// when attempting to create the same role again, check that we added a key
 	err = applyTargetsChange(repo, ch)
 	assert.NoError(t, err)
-	delegation, err := repo.GetDelegation("targets/level1")
+	delegation, keys, err := repo.GetDelegation("targets/level1")
 	assert.NoError(t, err)
 	assert.Contains(t, delegation.Paths, "level1")
 	assert.Equal(t, len(delegation.KeyIDs), 2)
+	for _, keyID := range delegation.KeyIDs {
+		_, ok := keys[keyID]
+		assert.True(t, ok)
+	}
 }
 
 func TestApplyTargetsDelegationAlreadyExistingMergePaths(t *testing.T) {
@@ -559,7 +563,7 @@ func TestApplyTargetsDelegationAlreadyExistingMergePaths(t *testing.T) {
 	// merged with previous details
 	err = applyTargetsChange(repo, ch)
 	assert.NoError(t, err)
-	delegation, err := repo.GetDelegation("targets/level1")
+	delegation, _, err := repo.GetDelegation("targets/level1")
 	assert.NoError(t, err)
 	// Assert we have both paths
 	assert.Contains(t, delegation.Paths, "level2")

cmd/notary/delegations.go

@@ -9,6 +9,7 @@ import (
 	"github.com/docker/notary/passphrase"
 	"github.com/docker/notary/trustmanager"
 	"github.com/docker/notary/tuf/data"
+	"github.com/docker/notary/tuf/utils"
 	"github.com/spf13/cobra"
 	"github.com/spf13/viper"
 )
@@ -206,7 +207,11 @@ func (d *delegationCommander) delegationAdd(cmd *cobra.Command, args []string) e
 	// Make keyID slice for better CLI print
 	pubKeyIDs := []string{}
 	for _, pubKey := range pubKeys {
-		pubKeyIDs = append(pubKeyIDs, pubKey.ID())
+		pubKeyID, err := utils.CanonicalKeyID(pubKey)
+		if err != nil {
+			return err
+		}
+		pubKeyIDs = append(pubKeyIDs, pubKeyID)
 	}
 
 	cmd.Println("")

cmd/notary/integration_test.go

@@ -24,6 +24,7 @@ import (
 	"github.com/docker/notary/server/storage"
 	"github.com/docker/notary/trustmanager"
 	"github.com/docker/notary/tuf/data"
+	"github.com/docker/notary/tuf/utils"
 	"github.com/spf13/cobra"
 	"github.com/spf13/viper"
 	"github.com/stretchr/testify/assert"
@@ -172,7 +173,8 @@ func TestClientDelegationsInteraction(t *testing.T) {
 
 	rawPubBytes, _ := ioutil.ReadFile(tempFile.Name())
 	parsedPubKey, _ := trustmanager.ParsePEMPublicKey(rawPubBytes)
-	keyID := parsedPubKey.ID()
+	keyID, err := utils.CanonicalKeyID(parsedPubKey)
+	assert.NoError(t, err)
 
 	var output string
 
@@ -219,6 +221,7 @@ func TestClientDelegationsInteraction(t *testing.T) {
 	output, err = runCommand(t, tempDir, "-s", server.URL, "delegation", "list", "gun")
 	assert.NoError(t, err)
 	assert.Contains(t, output, "targets/delegation")
+	assert.Contains(t, output, keyID)
 
 	// Setup another certificate
 	tempFile2, err := ioutil.TempFile("/tmp", "pemfile2")
@@ -238,9 +241,10 @@ func TestClientDelegationsInteraction(t *testing.T) {
 
 	rawPubBytes2, _ := ioutil.ReadFile(tempFile2.Name())
 	parsedPubKey2, _ := trustmanager.ParsePEMPublicKey(rawPubBytes2)
-	keyID2 := parsedPubKey2.ID()
+	keyID2, err := utils.CanonicalKeyID(parsedPubKey2)
+	assert.NoError(t, err)
 
-	// add to the delegation by specifying the same role, this time add a path
+	// add to the delegation by specifying the same role, this time add another key and path
 	output, err = runCommand(t, tempDir, "delegation", "add", "gun", "targets/delegation", tempFile2.Name(), "--paths", "path")
 	assert.NoError(t, err)
 	assert.Contains(t, output, "Addition of delegation role")
@@ -254,6 +258,8 @@ func TestClientDelegationsInteraction(t *testing.T) {
 	assert.NoError(t, err)
 	assert.Contains(t, output, ",")
 	assert.Contains(t, output, "path")
+	assert.Contains(t, output, keyID)
+	assert.Contains(t, output, keyID2)
 
 	// remove the delegation's first key
 	output, err = runCommand(t, tempDir, "delegation", "remove", "gun", "targets/delegation", keyID)
@@ -267,7 +273,8 @@ func TestClientDelegationsInteraction(t *testing.T) {
 	// list delegations - we should see the delegation but with only the second key
 	output, err = runCommand(t, tempDir, "-s", server.URL, "delegation", "list", "gun")
 	assert.NoError(t, err)
-	assert.NotContains(t, output, ",")
+	assert.NotContains(t, output, keyID)
+	assert.Contains(t, output, keyID2)
 
 	// remove the delegation's second key
 	output, err = runCommand(t, tempDir, "delegation", "remove", "gun", "targets/delegation", keyID2)
@@ -297,6 +304,8 @@ func TestClientDelegationsInteraction(t *testing.T) {
 	assert.NoError(t, err)
 	assert.Contains(t, output, ",")
 	assert.Contains(t, output, "path1,path2")
+	assert.Contains(t, output, keyID)
+	assert.Contains(t, output, keyID2)
 
 	// add delegation with multiple certs and multiple paths
 	output, err = runCommand(t, tempDir, "delegation", "add", "gun", "targets/delegation", "--paths", "path3")
@@ -329,6 +338,8 @@ func TestClientDelegationsInteraction(t *testing.T) {
 	assert.Contains(t, output, "path1")
 	assert.NotContains(t, output, "path2")
 	assert.NotContains(t, output, "path3")
+	assert.Contains(t, output, keyID)
+	assert.Contains(t, output, keyID2)
 
 	// remove the remaining path, should not remove the delegation entirely
 	output, err = runCommand(t, tempDir, "delegation", "remove", "gun", "targets/delegation", "--paths", "path1")
@@ -346,6 +357,8 @@ func TestClientDelegationsInteraction(t *testing.T) {
 	assert.NotContains(t, output, "path1")
 	assert.NotContains(t, output, "path2")
 	assert.NotContains(t, output, "path3")
+	assert.Contains(t, output, keyID)
+	assert.Contains(t, output, keyID2)
 
 	// remove by force to delete the delegation entirely
 	output, err = runCommand(t, tempDir, "delegation", "remove", "gun", "targets/delegation", "-y")

tuf/tuf.go

@@ -179,31 +179,36 @@ func (tr *Repo) GetAllLoadedRoles() []*data.Role {
 }
 
 // GetDelegation finds the role entry representing the provided
-// role name or ErrInvalidRole
+// role name along with its associated public keys, or ErrInvalidRole
-func (tr *Repo) GetDelegation(role string) (*data.Role, error) {
+func (tr *Repo) GetDelegation(role string) (*data.Role, data.Keys, error) {
 	r := data.Role{Name: role}
 	if !r.IsDelegation() {
-		return nil, data.ErrInvalidRole{Role: role, Reason: "not a valid delegated role"}
+		return nil, nil, data.ErrInvalidRole{Role: role, Reason: "not a valid delegated role"}
 	}
 
 	parent := path.Dir(role)
 
 	// check the parent role
 	if parentRole := tr.keysDB.GetRole(parent); parentRole == nil {
-		return nil, data.ErrInvalidRole{Role: role, Reason: "parent role not found"}
+		return nil, 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}
+		return nil, nil, data.ErrNoSuchRole{Role: role}
 	}
 
 	foundAt := utils.FindRoleIndex(p.Signed.Delegations.Roles, role)
 	if foundAt < 0 {
-		return nil, data.ErrNoSuchRole{Role: role}
+		return nil, nil, data.ErrNoSuchRole{Role: role}
 	}
-	return p.Signed.Delegations.Roles[foundAt], nil
+	delegationRole := p.Signed.Delegations.Roles[foundAt]
+	keys := make(data.Keys)
+	for _, keyID := range delegationRole.KeyIDs {
+		keys[keyID] = p.Signed.Delegations.Keys[keyID]
+	}
+	return delegationRole, keys, nil
 }
 
 // UpdateDelegations updates the appropriate delegations, either adding

tuf/tuf_test.go

@@ -639,9 +639,11 @@ func TestGetDelegationRoleAndMetadataExistDelegationExists(t *testing.T) {
 	assert.NoError(t, err)
 	assert.NoError(t, repo.UpdateDelegations(role, data.KeyList{testKey}))
 
-	gottenRole, err := repo.GetDelegation("targets/level1/level2")
+	gottenRole, gottenKeys, err := repo.GetDelegation("targets/level1/level2")
 	assert.NoError(t, err)
 	assert.Equal(t, role, gottenRole)
+	_, ok := gottenKeys[testKey.ID()]
+	assert.True(t, ok)
 }
 
 // If the parent exists, the metadata exists, and the delegation isn't in it,
@@ -662,7 +664,7 @@ func TestGetDelegationRoleAndMetadataExistDelegationDoesntExists(t *testing.T) {
 	// ensure metadata exists
 	repo.InitTargets("targets/level1")
 
-	_, err = repo.GetDelegation("targets/level1/level2")
+	_, _, err = repo.GetDelegation("targets/level1/level2")
 	assert.Error(t, err)
 	assert.IsType(t, data.ErrNoSuchRole{}, err)
 }
@@ -685,7 +687,7 @@ func TestGetDelegationRoleAndMetadataDoesntExists(t *testing.T) {
 	_, ok := repo.Targets["targets/test"]
 	assert.False(t, ok, "no targets file should be created for empty delegation")
 
-	_, err = repo.GetDelegation("targets/level1/level2")
+	_, _, err = repo.GetDelegation("targets/level1/level2")
 	assert.Error(t, err)
 	assert.IsType(t, data.ErrNoSuchRole{}, err)
 }
@@ -696,7 +698,7 @@ func TestGetDelegationParentMissing(t *testing.T) {
 	keyDB := keys.NewDB()
 	repo := initRepo(t, ed25519, keyDB)
 
-	_, err := repo.GetDelegation("targets/level1/level2")
+	_, _, err := repo.GetDelegation("targets/level1/level2")
 	assert.Error(t, err)
 	assert.IsType(t, data.ErrInvalidRole{}, err)
 }