diff --git a/client/client.go b/client/client.go index de18a49a06..6379d193d7 100644 --- a/client/client.go +++ b/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 - allDelegations = append(allDelegations, delegationMeta.Signed.Delegations.Roles...) + // For the return list, update with a copy that includes canonicalKeyIDs + 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 diff --git a/client/client_test.go b/client/client_test.go index 428915f110..33d86b3ebd 100644 --- a/client/client_test.go +++ b/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])) diff --git a/client/helpers.go b/client/helpers.go index a9fd590a9f..fe8f94266a 100644 --- a/client/helpers.go +++ b/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) diff --git a/client/helpers_test.go b/client/helpers_test.go index 04a5e7f13a..98da70cfeb 100644 --- a/client/helpers_test.go +++ b/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") diff --git a/cmd/notary/delegations.go b/cmd/notary/delegations.go index 174340f181..565b1d8baf 100644 --- a/cmd/notary/delegations.go +++ b/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("") diff --git a/cmd/notary/integration_test.go b/cmd/notary/integration_test.go index 392b2243f4..82ab50ba3f 100644 --- a/cmd/notary/integration_test.go +++ b/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") diff --git a/tuf/tuf.go b/tuf/tuf.go index 96ab7da1d6..8001c45dec 100644 --- a/tuf/tuf.go +++ b/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 -func (tr *Repo) GetDelegation(role string) (*data.Role, error) { +// role name along with its associated public keys, or ErrInvalidRole +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 diff --git a/tuf/tuf_test.go b/tuf/tuf_test.go index 9178d257da..d04ca6c110 100644 --- a/tuf/tuf_test.go +++ b/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) }