Merge pull request #864 from docker/fix-deep-delegation-listing

Ensure that we do not clobber in-memory key id data on delegation list

client/client_test.go

@@ -2325,6 +2325,25 @@ func TestPublishTargetsDelegationSuccessNeedsToDownloadRoles(t *testing.T) {
 	// delegation parents all get signed
 	ownerRec.requireAsked(t, []string{data.CanonicalTargetsRole, "targets/a"})
 
+	// assert both delegation roles appear to the other repo in a call to GetDelegationRoles
+	delgRoleList, err := delgRepo.GetDelegationRoles()
+	require.NoError(t, err)
+	require.Len(t, delgRoleList, 2)
+	// The walk is a pre-order so we can enforce ordering.  Also check that canonical key IDs are reported from this walk
+	require.Equal(t, delgRoleList[0].Name, "targets/a")
+	require.NotContains(t, delgRoleList[0].KeyIDs, ownerRepo.tufRepo.Targets[data.CanonicalTargetsRole].Signed.Delegations.Roles[0].KeyIDs)
+	canonicalAKeyID, err := utils.CanonicalKeyID(aKey)
+	require.NoError(t, err)
+	require.Contains(t, delgRoleList[0].KeyIDs, canonicalAKeyID)
+	require.Equal(t, delgRoleList[1].Name, "targets/a/b")
+	require.NotContains(t, delgRoleList[1].KeyIDs, ownerRepo.tufRepo.Targets["targets/a"].Signed.Delegations.Roles[0].KeyIDs)
+	canonicalBKeyID, err := utils.CanonicalKeyID(bKey)
+	require.NoError(t, err)
+	require.Contains(t, delgRoleList[1].KeyIDs, canonicalBKeyID)
+	// assert that the key ID data didn't somehow change between the two repos, since we translated to canonical key IDs
+	require.Equal(t, ownerRepo.tufRepo.Targets[data.CanonicalTargetsRole].Signed.Delegations.Roles[0].KeyIDs, delgRepo.tufRepo.Targets[data.CanonicalTargetsRole].Signed.Delegations.Roles[0].KeyIDs)
+	require.Equal(t, ownerRepo.tufRepo.Targets["targets/a"].Signed.Delegations.Roles[0].KeyIDs, delgRepo.tufRepo.Targets["targets/a"].Signed.Delegations.Roles[0].KeyIDs)
+
 	// delegated repo now publishes to delegated roles, but it will need
 	// to download those roles first, since it doesn't know about them
 	requirePublishToRolesSucceeds(t, delgRepo, []string{"targets/a/b"},

client/delegations.go

@@ -238,7 +238,7 @@ func newDeleteDelegationChange(name string, content []byte) *changelist.TUFChang
 
 // 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) {
+func (r *NotaryRepository) GetDelegationRoles() ([]data.Role, error) {
 	// Update state of the repo to latest
 	if err := r.Update(false); err != nil {
 		return nil, err
@@ -251,7 +251,7 @@ func (r *NotaryRepository) GetDelegationRoles() ([]*data.Role, error) {
 	}
 
 	// make a copy for traversing nested delegations
-	allDelegations := []*data.Role{}
+	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, validRole data.DelegationRole) interface{} {
@@ -271,20 +271,23 @@ func (r *NotaryRepository) GetDelegationRoles() ([]*data.Role, error) {
 	return allDelegations, nil
 }
 
-func translateDelegationsToCanonicalIDs(delegationInfo data.Delegations) ([]*data.Role, error) {
-	canonicalDelegations := make([]*data.Role, len(delegationInfo.Roles))
-	copy(canonicalDelegations, delegationInfo.Roles)
+func translateDelegationsToCanonicalIDs(delegationInfo data.Delegations) ([]data.Role, error) {
+	canonicalDelegations := make([]data.Role, len(delegationInfo.Roles))
+	// Do a copy by value to ensure local delegation metadata is untouched
+	for idx, origRole := range delegationInfo.Roles {
+		canonicalDelegations[idx] = *origRole
+	}
 	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)
+				return []data.Role{}, 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)
+				return []data.Role{}, fmt.Errorf("Could not translate canonical key IDs for %s: %v", delegation.Name, err)
 			}
 			canonicalKeyIDs = append(canonicalKeyIDs, canonicalKeyID)
 		}

cmd/notary/integration_test.go

@@ -946,6 +946,58 @@ func TestClientDelegationsPublishing(t *testing.T) {
 	output, err = runCommand(t, tempDir, "-s", server.URL, "list", "gun", "--roles", "targets/releases")
 	require.NoError(t, err)
 	require.Contains(t, output, "targets/releases")
+
+	// Setup another certificate
+	tempFile2, err := ioutil.TempFile("", "pemfile2")
+	require.NoError(t, err)
+
+	privKey, err = utils.GenerateECDSAKey(rand.Reader)
+	startTime = time.Now()
+	endTime = startTime.AddDate(10, 0, 0)
+	cert, err = cryptoservice.GenerateCertificate(privKey, "gun", startTime, endTime)
+	require.NoError(t, err)
+
+	_, err = tempFile2.Write(utils.CertToPEM(cert))
+	require.NoError(t, err)
+	require.NoError(t, err)
+	tempFile2.Close()
+	defer os.Remove(tempFile2.Name())
+
+	rawPubBytes2, _ := ioutil.ReadFile(tempFile2.Name())
+	parsedPubKey2, _ := utils.ParsePEMPublicKey(rawPubBytes2)
+	keyID2, err := utils.CanonicalKeyID(parsedPubKey2)
+	require.NoError(t, err)
+
+	// add a nested delegation under this releases role
+	output, err = runCommand(t, tempDir, "delegation", "add", "gun", "targets/releases/nested", tempFile2.Name(), "--paths", "nested/path")
+	require.NoError(t, err)
+	require.Contains(t, output, "Addition of delegation role")
+	require.Contains(t, output, keyID2)
+
+	// publish repo
+	_, err = runCommand(t, tempDir, "-s", server.URL, "publish", "gun")
+	require.NoError(t, err)
+
+	// list delegations - we should see two roles
+	output, err = runCommand(t, tempDir, "-s", server.URL, "delegation", "list", "gun")
+	require.NoError(t, err)
+	require.Contains(t, output, "targets/releases")
+	require.Contains(t, output, "targets/releases/nested")
+	require.Contains(t, output, canonicalKeyID)
+	require.Contains(t, output, keyID2)
+	require.Contains(t, output, "nested/path")
+	require.Contains(t, output, "\"\"")
+	require.Contains(t, output, "<all paths>")
+
+	// remove by force to delete the nested delegation entirely
+	output, err = runCommand(t, tempDir, "delegation", "remove", "gun", "targets/releases/nested", "-y")
+	require.NoError(t, err)
+	require.Contains(t, output, "Forced removal (including all keys and paths) of delegation role")
+
+	// publish repo
+	_, err = runCommand(t, tempDir, "-s", server.URL, "publish", "gun")
+	require.NoError(t, err)
+
 }
 
 // Splits a string into lines, and returns any lines that are not empty (

cmd/notary/prettyprint.go

@@ -140,7 +140,7 @@ func (t targetsSorter) Less(i, j int) bool {
 
 // --- pretty printing roles ---
 
-type roleSorter []*data.Role
+type roleSorter []data.Role
 
 func (r roleSorter) Len() int      { return len(r) }
 func (r roleSorter) Swap(i, j int) { r[i], r[j] = r[j], r[i] }
@@ -173,7 +173,7 @@ func prettyPrintTargets(ts []*client.TargetWithRole, writer io.Writer) {
 }
 
 // Pretty-prints the list of provided Roles
-func prettyPrintRoles(rs []*data.Role, writer io.Writer, roleType string) {
+func prettyPrintRoles(rs []data.Role, writer io.Writer, roleType string) {
 	if len(rs) == 0 {
 		writer.Write([]byte(fmt.Sprintf("\nNo %s present in this repository.\n\n", roleType)))
 		return

cmd/notary/prettyprint_test.go

@@ -205,7 +205,7 @@ func TestPrettyPrintSortedTargets(t *testing.T) {
 // are no roles.
 func TestPrettyPrintZeroRoles(t *testing.T) {
 	var b bytes.Buffer
-	prettyPrintRoles([]*data.Role{}, &b, "delegations")
+	prettyPrintRoles([]data.Role{}, &b, "delegations")
 	text, err := ioutil.ReadAll(&b)
 	require.NoError(t, err)
 
@@ -218,7 +218,7 @@ func TestPrettyPrintZeroRoles(t *testing.T) {
 func TestPrettyPrintSortedRoles(t *testing.T) {
 	var err error
 
-	unsorted := []*data.Role{
+	unsorted := []data.Role{
 		{Name: "targets/zebra", Paths: []string{"stripes", "black", "white"}, RootRole: data.RootRole{KeyIDs: []string{"101"}, Threshold: 1}},
 		{Name: "targets/aardvark/unicorn/pony", Paths: []string{"rainbows"}, RootRole: data.RootRole{KeyIDs: []string{"135"}, Threshold: 1}},
 		{Name: "targets/bee", Paths: []string{"honey"}, RootRole: data.RootRole{KeyIDs: []string{"246"}, Threshold: 1}},