addressing comments in PR

Signed-off-by: David Lawrence <david.lawrence@docker.com> (github: endophage)

client/helpers.go

@@ -40,7 +40,7 @@ func applyChangelist(repo *tuf.Repo, cl changelist.Changelist) error {
 		if err != nil {
 			return err
 		}
-		isDel := data.IsDelegation(c.Scope())
+		isDel := data.IsDelegation(c.Scope()) || data.IsWildDelegation(c.Scope())
 		switch {
 		case c.Scope() == changelist.ScopeTargets || isDel:
 			err = applyTargetsChange(repo, c)

cmd/notary/delegations.go

@@ -34,8 +34,8 @@ var cmdDelegationRemoveTemplate = usageTemplate{
 
 var cmdDelegationPurgeKeysTemplate = usageTemplate{
 	Use:   "purge [ GUN ]",
-	Short: "Remove KeyID(s) from all delegations it is found in.",
-	Long:  "Remove KeyID(s) from all delegations it is found in, for which the signing keys are available. Warnings will be printed for delegations that cannot be updated.",
+	Short: "Remove KeyID(s) from all delegation roles in the given GUN.",
+	Long:  "Remove KeyID(s) from all delegation roles in the given GUN, for which the signing keys are available. Warnings will be printed for delegations that cannot be updated.",
 }
 
 var cmdDelegationAddTemplate = usageTemplate{
@@ -59,7 +59,7 @@ func (d *delegationCommander) GetCommand() *cobra.Command {
 	cmd.AddCommand(cmdDelegationListTemplate.ToCommand(d.delegationsList))
 
 	cmdPurgeDelgKeys := cmdDelegationPurgeKeysTemplate.ToCommand(d.delegationPurgeKeys)
-	cmdPurgeDelgKeys.Flags().StringSliceVar(&d.keyIDs, "key", nil, "Delegation keys to be removed from the GUN")
+	cmdPurgeDelgKeys.Flags().StringSliceVar(&d.keyIDs, "key", nil, "Delegation key IDs to be removed from the GUN")
 	cmd.AddCommand(cmdPurgeDelgKeys)
 
 	cmdRemDelg := cmdDelegationRemoveTemplate.ToCommand(d.delegationRemove)
@@ -112,7 +112,7 @@ func (d *delegationCommander) delegationPurgeKeys(cmd *cobra.Command, args []str
 
 	err = nRepo.RemoveDelegationKeys("targets/*", d.keyIDs)
 	if err != nil {
-		return fmt.Errorf("failed to remove delegation: %v", err)
+		return fmt.Errorf("failed to remove keys from delegations: %v", err)
 	}
 	fmt.Printf(
 		"Removal of the following keys from all delegations in %s staged for next publish:\n\t- %s\n",

cmd/notary/integration_test.go

@@ -1292,3 +1292,77 @@ func TestMain(m *testing.M) {
 	}
 	os.Exit(m.Run())
 }
+
+func TestPurge(t *testing.T) {
+	setUp(t)
+
+	tempDir := tempDirWithConfig(t, "{}")
+	defer os.RemoveAll(tempDir)
+
+	server := setupServer()
+	defer server.Close()
+
+	startTime := time.Now()
+	endTime := startTime.AddDate(10, 0, 0)
+
+	// Setup certificates
+	tempFile, err := ioutil.TempFile("", "pemfile")
+	require.NoError(t, err)
+	privKey, err := utils.GenerateECDSAKey(rand.Reader)
+	cert, err := cryptoservice.GenerateCertificate(privKey, "gun", startTime, endTime)
+	require.NoError(t, err)
+	_, err = tempFile.Write(utils.CertToPEM(cert))
+	require.NoError(t, err)
+	tempFile.Close()
+	defer os.Remove(tempFile.Name())
+	rawPubBytes, _ := ioutil.ReadFile(tempFile.Name())
+	parsedPubKey, _ := utils.ParsePEMPublicKey(rawPubBytes)
+	keyID, err := utils.CanonicalKeyID(parsedPubKey)
+	require.NoError(t, err)
+
+	delgName := "targets/delegation"
+
+	_, err = runCommand(t, tempDir, "-s", server.URL, "init", "gun")
+	require.NoError(t, err)
+
+	_, err = runCommand(t, tempDir, "delegation", "add", "gun", delgName, tempFile.Name(), "--all-paths")
+	require.NoError(t, err)
+
+	_, err = runCommand(t, tempDir, "-s", server.URL, "publish", "gun")
+	require.NoError(t, err)
+
+	// remove targets key and ensure we fail to publish
+	out, err := runCommand(t, tempDir, "key", "list")
+	require.NoError(t, err)
+	lines := splitLines(out)
+	if len(lines) == 1 && lines[0] == "No signing keys found." {
+		t.Fail()
+	}
+	var targetsKeyID string
+	for _, line := range lines[2:] {
+		parts := strings.Fields(line)
+		if strings.TrimSpace(parts[0]) == data.CanonicalTargetsRole {
+			targetsKeyID = strings.TrimSpace(parts[2])
+			break
+		}
+	}
+
+	if targetsKeyID == "" {
+		t.Fail()
+	}
+
+	err = os.Remove(filepath.Join(tempDir, notary.PrivDir, notary.NonRootKeysSubdir, "gun", targetsKeyID+".key"))
+	require.NoError(t, err)
+
+	_, err = runCommand(t, tempDir, "delegation", "purge", "gun", "--key", keyID)
+	require.NoError(t, err)
+
+	// publish doesn't error because purge only updates the roles we have signing keys for
+	_, err = runCommand(t, tempDir, "-s", server.URL, "publish", "gun")
+	require.NoError(t, err)
+
+	// check the delegation wasn't removed
+	out, err = runCommand(t, tempDir, "-s", server.URL, "delegation", "list", "gun")
+	require.NoError(t, err)
+	require.Contains(t, out, delgName)
+}

docs/advanced_usage.md

@@ -188,8 +188,8 @@ Forced removal (including all keys and paths) of delegation role targets/release
 
 You can remove individual keys and/or paths by passing keys as arguments, and/or
 paths under the `--paths` flag. Use `--all-paths` to clear all paths for this
-role. If you specify all key IDs currently in the delegation role, you will
-delete the role entirely.
+role. If you specify all key IDs currently in the delegation role, you will be left
+with a role that is unusable as it will not have enough valid signatures.
 
 To add targets to a specified delegation role, we can use the `notary add`
 command with the `--roles` flag.

tuf/data/roles.go

@@ -7,7 +7,6 @@ import (
 	"strings"
 
 	"github.com/Sirupsen/logrus"
-	"path/filepath"
 )
 
 // Canonical base role names
@@ -102,9 +101,14 @@ func IsBaseRole(role string) bool {
 // The wildcard may only appear as the final part of the delegation and must
 // be a whole segment, i.e. targets/foo* is not a valid wildcard delegation.
 func IsWildDelegation(role string) bool {
-	base := filepath.Dir(role)
-	splat := filepath.Base(role)
-	return splat == "*" && (IsDelegation(base) || base == CanonicalTargetsRole)
+	if path.Clean(role) != role {
+		return false
+	}
+	base := path.Dir(role)
+	if !(IsDelegation(base) || base == CanonicalTargetsRole) {
+		return false
+	}
+	return role[len(role)-2:] == "/*"
 }
 
 // BaseRole is an internal representation of a root/targets/snapshot/timestamp role, with its public keys included

tuf/data/roles_test.go

@@ -5,6 +5,7 @@ import (
 	"strings"
 	"testing"
 
+	"fmt"
 	"github.com/stretchr/testify/require"
 )
 
@@ -153,6 +154,9 @@ func TestIsWildDelegation(t *testing.T) {
 		path.Join(CanonicalTimestampRole, "*"):      f,
 		path.Join(CanonicalTargetsRole, "*", "foo"): f,
 		path.Join(CanonicalTargetsRole, "*", "*"):   f,
+		fmt.Sprintf("%s//*", CanonicalTargetsRole):  f,
+		fmt.Sprintf("%s/*//", CanonicalTargetsRole): f,
+		fmt.Sprintf("%s/*/", CanonicalTargetsRole):  f,
 
 		// true checks
 		path.Join(CanonicalTargetsRole, "*"):        tr,

tuf/tuf.go

@@ -325,8 +325,7 @@ func delegationUpdateVisitor(roleName string, addKeys data.KeyList, removeKeys,
 				break
 			}
 		}
-		// We didn't find the role earlier, so create it only if we have keys to add
-		// that are also sufficient to meet the threshold.
+		// We didn't find the role earlier, so create it.
 		if addKeys == nil {
 			addKeys = data.KeyList{} // initialize to empty list if necessary so calling .IDs() below won't panic
 		}
@@ -416,13 +415,9 @@ func (tr *Repo) PurgeDelegationKeys(role string, removeKeys []string) error {
 
 	purgeKeys := func(tgt *data.SignedTargets, validRole data.DelegationRole) interface{} {
 		var (
-			deleteCandidates    []string
-			err                 error
-			canSign             bool
-			needsUpdateCantSign bool
+			deleteCandidates []string
+			err              error
 		)
-		err = tr.VerifyCanSign(validRole.Name)
-		canSign = err == nil
 		for id, key := range tgt.Signed.Delegations.Keys {
 			var (
 				canonID string
@@ -431,33 +426,32 @@ func (tr *Repo) PurgeDelegationKeys(role string, removeKeys []string) error {
 			if canonID, ok = tufIDToCanon[id]; !ok {
 				canonID, err = utils.CanonicalKeyID(key)
 				if err != nil {
-					// TODO: should we return here or just log?
 					return err
 				}
 				tufIDToCanon[id] = canonID
 			}
 			if _, ok := removeIDs[canonID]; ok {
-				if !canSign {
-					needsUpdateCantSign = true
-					break
-				}
-				delete(tgt.Signed.Delegations.Keys, id)
 				deleteCandidates = append(deleteCandidates, id)
 			}
 		}
-		if needsUpdateCantSign {
+		if len(deleteCandidates) == 0 {
+			// none of the interesting keys were present. We're done with this role
+			return nil
+		}
+		// now we know there are changes, check if we'll be able to sign them in
+		if err := tr.VerifyCanSign(validRole.Name); err != nil {
 			logrus.Warnf(
 				"role %s contains keys being purged but you do not have the necessary keys present to sign it; keys will not be purged from %s or its immediate children",
 				validRole.Name,
 				validRole.Name,
 			)
 			return nil
 		}
-		if len(deleteCandidates) == 0 {
-			// none of the interesting keys were present
-			return nil
+		// we know we can sign in the changes, delete the keys
+		for _, id := range deleteCandidates {
+			delete(tgt.Signed.Delegations.Keys, id)
 		}
-		// delete candidate keys from all roles. Keep only roles that still have keys
+		// delete candidate keys from all roles.
 		for _, role := range tgt.Signed.Delegations.Roles {
 			role.RemoveKeys(deleteCandidates)
 			if len(role.KeyIDs) < role.Threshold {

tuf/tuf_test.go

@@ -227,6 +227,10 @@ func TestPurgeDelegationsKeyFromTop(t *testing.T) {
 	role, err = repo.GetDelegationRole(carrot)
 	require.NoError(t, err)
 	require.Len(t, role.Keys, 0)
+
+	// we know id1 was successfully purged, try purging again and make sure it doesn't error
+	err = repo.PurgeDelegationKeys(targetsWild, []string{id1})
+	require.NoError(t, err)
 }
 
 func TestPurgeDelegationsKeyFromDeep(t *testing.T) {